Implement Fn traits for Rc/Arc

[Stebalien]

This allows one to, e.g., use an Rc wrapped closure in Iterator::map. I would have implemented Fn and FnMut for Box as well but ran into coherence problems (see #33811).

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[Stebalien]

Make check pending. I guess you now do this for me using travis. My poor laptop thanks you.

/cc @bluss

[aturon]

Nice! cc @rust-lang/libs, I feel like we considered something like this before but ran into trouble; any concerns?

[alexcrichton]

Note that in the strictest sense this is a breaking change because these traits are #[fundamental] (I think that's what that means, right?). In that sense we probably want to crater this before landing, but otherwise this seems fine to me!

[brson]

I want to object to this feature, but can't think of any technical reasons.

What are the parallels to Box<Fn> that need to be considered? e.g. I don't see any equivalent Fn<I> for Box<F> impls.

[Stebalien]

@brson there are coherence issues with implementing this for Box<Fn> (due to the FnOnce stuff) that I couldn't work around (see #33811). I believe this should eventually be possible to implement (I hope).

My technical reason for wanting this is that it provides a way to create immutable (owned) closures that can be cloned (currently impossible, see #23501).

[bluss]

It makes sense, cloning closures is a good reason. I haven't needed it since you can clone &F where F: Fn instead, but this liberates it to more than stack scoped use cases.

[alexcrichton]

The libs team was able to discuss this PR during triage today, but unfortunately we were unable to reach a conclusion. The stickler point was that these traits are #[fundamental] so it's a breaking change to add more impls of them to existing types. I believe @sfackler will update with more info as well.

[aturon]

@alexcrichton We should consider a crater run, just to gauge how much impact there actually is. While fundamental in principle means new impls are breaking, this is only the case if code is relying on the lack of those impls for coherence reasons, which is somewhat rare. It'd be worth talking through the overall semver policy here at some point.

[Stebalien]

Also note, unless I'm mistaken, this can't break stable code (Fn etc. are unstable).

[sfackler]

Won't this break anything that involves a blanket impl on Fn and an impl on Rc/Arc?

[aturon]

@Stebalien It affects stable code because downstream code gets the effect of fundamental even if it doesn't use it.

@sfackler Yes, that's roughly correct:

traitMyTrait{}impl<T:FnOnce()>MyTraitforT{}impl<T>MyTraitforRc<T>{}

This code compiles today, and would break if we provided the proposed impls. What I'm trying to find out is how common this situation is. (I imagine in most cases the breakage would be handled by essentially deleting the Rc impl, but there may be more "interesting" cases.)

[Stebalien]

@aturon, I see. I misunderstood fundamental.

[sfackler]

@aturon but that solution would then break its use by older versions of rustc, right?

This seems pretty explicitly like something that we were very aware would be a breaking change when introducing #[fundamental]. It is why we haven't moved Error into core, and why I believe we're unwilling to stabilize #[fundamental] at all.

I honestly cringe a bit every time someone sees a breaking change and says "let's run crater". Crater can compile some fraction of the Rust code that is uploaded to crates.io, which is some fraction of Rust code that is publicly available, which is itself some fraction of Rust code that actually exists. It is incredibly useful to ease the transition for changes we have explicitly outlined as causing acceptable breakage (new trait impls etc), but if we keep relying to crater to find all of the breakage that exists when making arbitrary changes, we are going to get bit at some point. I'm worried our stability story is slowly drifting from what was outlined a year ago to "if a tree falls in the woods and I don't personally see anyone standing nearby, did it really make a sound?".

[alexcrichton]

Looks like this causes breakage on crates.io, breaking both iron and rustful.

[Stebalien]

😿

However, iron actually does something that this PR would have made impossible.