Make Rc and Arc #[fundamental]

[cramertj]

Fixes #24317.

r? @alexcrichton
and @rust-lang/libs

[alexcrichton]

As a baseline I'd want to be sure to run crater to see what happen, hopefully that breakage is just limited to specialization!

[SimonSapin]

If I understand https://github.com/rust-lang/rfcs/blob/master/text/1023-rebalancing-coherence.md#errors-from-cargo-and-the-fundamental-attribute correctly, this means that we can never extend the set of traits implemented for Rc<T> or Arc<T> generically? (Unless that impl is added for a new trait in the same release cycle as the trait is stabilized?)

• Hash
• DispatchFromDyn<Rc<U>>
• Eq
• Ord
• Borrow<T>
• CoerceUnsized<Rc<U>>
• Display
• From<T>
• From<Box<T>>
• AsRef<T>
• PartialOrd
• Clone
• Debug
• PartialEq
• Deref
• Default
• Drop
• fmt::Pointer
• Unpin
• UnwindSafe

(The above is for Rc, I’ll assume Arc is similar enough for the purpose of this discussion.)

Does this also affect more specific impls, like for Rc<[T]> or Rc<str>?

[nikomatsakis]

This means that we can never extend the set of traits implemented for Rc<T> or Arc<T> generically

I believe that is correct. Someone may have a impl OldTrait for Rc<MyType> already and you can't be sure (or they may have something where the negative reasoning is more subtle -- e.g., impl NewTrait for Rc<MyType> and impl<T: OldTrait> NewTrait for T.

I wonder: @cramertj, do you have specific examples of impls you wanted to write but couldn't? Maybe there is a more limited formulation of #[fundamental] we might consider? (Probably not, but it'd be interesting to see said examples regardless)

[nikomatsakis]

I do tend to agree with @cramertj that it is hard to justify a discrepancy beween Box and Rc/Arc here, apart from "why commit to more than we have to".

[cramertj]

do you have specific examples of impls you wanted to write but couldn't?

They're basically all of the form impl othercrate::Trait for Rc<MyType> { ... }.

[alexcrichton]

@bors: try

[bors]

⌛ Trying commit 69e465a with merge e70472f...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[alexcrichton]

@craterbot run start=master#d99a320cba42f661aebfa1293b7b2ec3603dda75 end=try#e70472fc501b855c9080f3772432c10fe20b8866 mode=check-only

[craterbot]

👌 Experiment pr-56955 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-56955 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-56955 is completed!
📊 1650 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[alexcrichton]

Spot checking a few of those they look legitimate, unless that's a compiler but I don't think we can do this change :(

[cramertj]

Ugh. Previously, we allowed negative reasoning that relied on #[fundamental] SomeUpstreamTrait not being implemented by Arc<T>. Making Arc<T> #[fundamental] makes it possible to implement a fundamental upstream trait for Arc<MyType>, making overlap now possible (interestingly, this also means that we can't ever implement Fn<..> for Arc<T> where T: Fn<...>, which seems extremely unfortunate).

example of error:

[INFO] [stderr] error[E0119]: conflicting implementations of trait `NewService` for type `std::sync::Arc<_>`:
[INFO] [stderr]    --> /opt/crater/cargo-home/registry/src/jackfan.us.kg-1ecc6299db9ec823/tower-service-0.1.0/src/lib.rs:279:1
[INFO] [stderr]     |
[INFO] [stderr] 262 | / impl<F, R, E, S> NewService for F
[INFO] [stderr] 263 | |     where F: Fn() -> R,
[INFO] [stderr] 264 | |           R: IntoFuture<Item = S, Error = E>,
[INFO] [stderr] 265 | |           S: Service,
[INFO] [stderr] ...   |
[INFO] [stderr] 276 | |     }
[INFO] [stderr] 277 | | }
[INFO] [stderr]     | |_- first implementation here
[INFO] [stderr] 278 | 
[INFO] [stderr] 279 |   impl<S: NewService + ?Sized> NewService for Arc<S> {
[INFO] [stderr]     |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::sync::Arc<_>`
[INFO] [stderr]     |
[INFO] [stderr]     = note: downstream crates may implement trait `std::ops::FnOnce<()>` for type `std::sync::Arc<_>`
[INFO] [stderr]     = note: downstream crates may implement trait `std::ops::Fn<()>` for type `std::sync::Arc<_>`
[INFO] [stderr] 

[withoutboats]

I vaguely remember this situation in tokio-service, but I don't remember the exact details. Maybe there's an alternative fundamental-like rule that makes this work?

[cramertj]

It's actually incoherent if a user can write impl FnOnce<...> for Arc<MyType>, esp. if MyType already implements FnOnce<...> with the appropriate requirements. In that case, without even referencing NewService trait it's possible to have conflicting impls of NewService.

[withoutboats]

@cramertj correct, but what about a variant of fundamental that doesnt include impls of fundamental traits? so users just cant impl the fn traits for arcs and rcs, which is probably fine since you cant implement those traits on stable anyway and the established pattern is to write these blanket impls instead

since I don't think any fundamental traits are stable to implement its possible we could just change the meaning of the fundamental attribute in this way without breaking any stable code..

[cramertj]

Yeah, we could introduce #[fundamental_but_not_for_fundamental] or something and add that to Rc and Arc, which would get you all the custom impls you want for #[fundamental] types that aren't for #[fundamental] traits.

[withoutboats]

That would make it a breaking change to add the fundamental attribute to any existing trait, though!

Not sure what the next step in the process should be, since some change along these lines would be necessary before moving forward with this PR.

[cramertj]

Adding #[fundamental] to a trait being breaking doesn't seem unreasonable to me, but it is unfortunate.

[nikomatsakis]

I feel like maybe the answer is that we should think about designing a "better solution" to the fundamental problem -- or perhaps it's best to broaden the scope a bit to coherence more generally. (I would also not be surprised to learn that adding fundamental to existing traits may be breaking, at least in the absence of new features.)

In general, I feel like there is a need to be able to "publicly affirm" that certain impls will never be added. For example, I'd like to be able to declare definitely that Box<T> will never be Copy. =) (Right now, external crates cannot rely on that.) In this case, one could imagine saying that (e.g.) Arc<T> will never be Fn for any type T.

(But of course this leaves open the question of how to say things like "Arc<T> will never implement any other existing trait" and so forth, which is basically what #[fundamental] is saying.)

I feel like as we move towards Chalk, we have the tools in place to talk carefully about these questions (e.g., we now have a model of coherence in Chalk and it was kind of illuminating). But it's not a small design challenge :( and maybe it is not sufficient priority to think about.

[Dylan-DPC-zz]

ping from triage @cramertj @alexcrichton @nikomatsakis any updates on this?

[withoutboats]

This can't make progress unless we also change how fundamental types work in some way. With the current fundamental semantics, this PR is a breaking change.

[Dylan-DPC-zz]

@withoutboats thanks. Marking this as blocked.

[nikomatsakis]

I'm going to just close this PR. Feel free to re-open @cramertj if you think there's value in having it open.

[cramertj]

Nah, closing is fine-- I'll just continue to sulk, but more quietly now, knowing that there's a reason we can't have nice things ;)