search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.35k stars 12.72k forks source link

Tracking Issue for RFC: Supertrait Item Shadowing #89151

Open pnkfelix opened 3 years ago

pnkfelix commented 3 years ago

This is a tracking issue for:

The feature gate for the issue is #![feature(supertrait_item_shadowing)].

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

TODO.

Related

dtolnay commented 3 years ago

@rust-lang/libs-api — this change is relevant to our ability to add things like Iterator::intersperse which used to introduce ambiguities against Itertools::intersperse. After this language change it would no longer be ambiguous because trait Itertools: Iterator {...}. Thus downstream uses of .intersperse(...) would resolve to the Itertools one since that's the subtrait.

the8472 commented 3 years ago

For the Iterator/Itertools case it would be necessary to shadow based on the explicit import of Itertools vs. the implicit import via the prelude.

Currently the RFC only concerns itself with traits being brought into scope via generics or trait objects, not via the prelude.

To resolve this issue, this RFC proposes the following: If the user does not explicitly bring the supertrait into scope themselves, the subtrait should "shadow" the supertrait, resolving the current ambiguity in favor of the subtrait. [...] When using a trait as a bound in generics, or using a trait object, a trait with a supertrait will only bring the supertrait's items into scope if it does not define an item with the same name itself.

So the scope of this issue would have to be widened a bit.

lcdr commented 3 years ago

This is correct, the RFC explicitly only concerns itself with generics and trait objects. Note however that glob imports have long been known to have the potential to induce breakage: the Semver and API Evolution RFC points out that glob imports can effectively make any addition of public items a breaking change. It then argues that since a glob import could always have been written as a list of explicit imports, it is considered an acceptable minor change to add new items. Interestingly enough, the RFC suggests glob shadowing as a solution to this issue. However as far as I'm aware there hasn't been a glob shadowing RFC actually submitted.

Similar concerns to what the RFC covers however also apply to the case where both supertrait and subtrait are used, without glob imports being involved at all. Unfortunately in this particular case it's a much more difficult decision whether to shadow or not to shadow - this RFC argued that in the case of generics and trait objects, Rust's implicit bringing into scope of supertrait items was counterintuitive to a user who just imported the subtrait, without even possibly knowing of the supertrait. However, in this case, the user would have consciously imported both traits. It could be argued that the better choice would be to refuse to automatically shadow and force the user to disambiguate, or, on the other hand, that it's better to keep behavior consistent with the subtrait-shadows-supertrait rule in the RFC. It'd be interesting to hear T-lang's opinions on this issue.

the8472 commented 3 years ago

Well, the std prelude (which is what causes #88967) isn't an explicit glob import either, it's fully implicit.

lcdr commented 3 years ago

Ah, I thought you were talking about a prelude in Itertools.

petrochenkov commented 3 years ago

I remember looking at the method resolution logic (excluding auto(de)ref) in the compiler a couple of years ago, and not liking what I've seen.

Treating methods from generic parameter bounds as inherent (bad idea, IMO), treating Trait methods on dyn Trait as inherent (bad idea, IMO), not considering Trait methods on impl Trait at all (bad idea, IMO, #41221), filtering of private methods (needs to have some priority relative to supertrait item shadowing), filtering of unstable methods (needs to have some priority relative to supertrait item shadowing).

I wish that area got some principled common vision (and corresponding bugfixing) before new features are added.

tgross35 commented 1 year ago

@petrochenkov did any of that resolution restructuring happen? I think there has gotten some interest recently

Amanieu commented 6 months ago

I re-read the RFC with the Itertools situation in mind and it doesn't seem to be the right thing that we need. Here is a summary of the current situation:

// core
trait Iterator {
    #[unstable]
    fn intersperse(self, element: Self::Item) -> Intersperse<Self>;
}

// itertools
trait Itertools: Iterator {
    fn intersperse(self, element: Self::Item) -> Intersperse<Self>;
}

Today, Iterator::intersperse is unstable and therefore de-prioritized in method resolution. However we are blocked from stabilizing it because it will cause ambiguity errors in any crates currently using the itertools version of the method. This is pretty widely used, with 59 root regressions in crater (#88967) which makes it impractical to individually update each crate.

Most uses of this method look like this:

// Implicit prelude import
use core::iter::Iterator;

// Explicit trait import
use itertools::Itertools;

// Actual use which causes an ambiguity error.
let text = lines.intersperse("\n").collect();

This would not work with the RFC as proposed since no generic bounds are in use. Instead we would need one of the proposed alternatives, specifically "Always resolving in favor of the subtrait". Essentially, if there is ambiguity between a supertrait and subtrait method, we should always resolve the ambiguity by selecting the subtrait method.

@rust-lang/types What are your thoughts on this? @rust-lang/libs-api would like to know if this is likely to move forward since this affects future development of the Iterator trait in the standard library.

lcnr commented 6 months ago

My personal preference would be to always resolve in favor of the sub trait, linting when defining a sub trait if the trait has an item with the same name as the super trait. I believe always shadowing super traits is fairly straightforward to implement and linting the definition of the sub trait is as well.

I consider this behavior to be part of the area where the responsibilities of t-types and t-lang overlap. T-lang is responsible for the vibe while T-types has to sign off that it works well with the general design of the type system and its implementation in rustc. Nominating this for t-lang to decide whether "always prefering methods from subtraits" is acceptable to them and to decide whether this needs any additional FCP/RFC to be implemented.

nikomatsakis commented 6 months ago

We discussed this in the @rust-lang/lang triage meeting and we felt we didn't have sufficient context to evaluate precisely what is being proposed, so we'd love to see a short RFC about this. What I think would be really useful is to highlight the examples of where this "does the right thing" but also any edge cases.

Basically what are the arguments against this? Off the top of my head...

Amanieu commented 6 months ago

I wrote an RFC: https://github.com/rust-lang/rfcs/pull/3624