Supertrait item shadowing v2

[Amanieu]

This is an alternative to #2845 which aims to resolve the issues surrounding the stabilization of Iterator::intersperse.

Summary

When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error.

Rendered

[fbstj]

nit:

Suggested change

	When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select that method instead of reporting an ambiguity error.
	When name resolution encounters an ambiguity between 2 trait methods, if one trait is a sub-trait of the other then select the sub-trait method instead of reporting an ambiguity error.

[lcdr]

I'd like to know how this would interact with the previous RFC #2845 . Is this meant as a complete replacement or as an extension? I deliberately did not address use declarations in that RFC as to keep the RFC simple, so in principle the RFCs are independent.

It reads as a replacement, but fundamentally the same issue also occurs with generic trait bounds, which I was trying to address in the previous RFC. What do you propose in the case where someone has written a generic bound for itertools::Itertools like fn foo<I: itertools::Itertools>(i: &I) ?

[Amanieu]

This is intended as a replacement for #2845 since it is more general: this RFC effectively proposes one of the alternatives listed in #2845 where we always prefer the sub-trait no matter what generic bounds are in use (and also applying to the more general case of use).

What do you propose in the case where someone has written a generic bound for itertools::Itertools like fn foo<I: itertools::Itertools>(i: &I) ?

This RFC would always resolve I::intersperse to Itertools::intersperse since it comes from a sub-trait.

[senekor]

Why is picking a different name not a reasonable path? The unavoidable bikeshedding around a new name is annoying, but it seems to me like a small, one-time cost compared to the permanent additional language complexity of this feature.

I also wonder how many times we anticipate to run into this problem in the future. Are there more examples aside from Itertools::intersperse? If we only ran into this problem once within 9 years of Rust being stable, the benefit of this feature seems very limited.

[ripytide]

Picking a new name also has disadvantages other than than the time taken to pick it, users then have to learn the new method name and that it is semantically identical to the Itertools::intersperse() yet has a different name. This is not a one-time cost as it will effect all future users when, for example, searching docs for this method. This would be the case for all methods stabilized from itertools to std which might be quite a few if we had a reliable way to do such a migration.

[senekor]

users then have to learn the new method name

I don't see the problem. People who are using intersperse from itertools shouldn't expect the standard library to use the exact same name. And I assume few people rely on the unstable version in std for production code, it would be unwise to opt into breaking changes for what amounts to a small ergonomics improvement (which a library can also provide).

For regular Rust users, the function doesn't exist right now. In the future it may - its name is an open question.

This would be the case for all methods stabilized from itertools to std which might be quite a few

I agree that this is something to consider, we don't want to have to pick the "second best" name for many functions in std. But first we should think concretely about which methods from itertools might actually make the jump into std. itertools has been around for a long time and I'm not aware of a big push to upstream many of its methods. Which indicates to me, there isn't that much need. But I'm happy to be convinced otherwise. Examples from other libraries besides itertools count as well.

[digama0]

The issue is that because itertools is used widely, std cannot upstream methods from itertools without causing lots of breakage in all crates currently using itertools. This is a perverse incentive which forces the "second best" issue you mentioned, and it's not limited to itertools, it happens whenever std lacks a function, someone implements an extension trait to add it in a crate (as they should), and everyone picks it up because it is really useful (as they should). The exact sequence of events which leads to strong evidence that something should be in std is also the sequence of events that blocks it from being added to std.

[senekor]

I understand that. The reason I'm pushing back is that to me, the feature doesn't seem to align with Rust's design principles. This new implicit default doesn't seem obviously correct to me. If I call a method that has two implementations, I would generally prefer the compiler yell at me rather than pick one without telling me. That's why I think we should be certain that we'll make good use of this feature before adding it.

But I'll admit this is a theoretical objection. In practice, the problem may never show up and then it's fine to add the feature. Pragmatism comes first. I guess I just agree with this comment. We should think about edge cases where this could go wrong.

[cynecx]

I would generally prefer the compiler yell at me rather than pick one without telling me

The rfc mentions this:

This behavior can be surprising: adding a method to a sub-trait can change which function is called in unrelated code. A lint could be emitted to warn users about the potential ambiguity.

[senekor]

Yeah, a lint would make sense if the migration-situation with intersperse is the only use case we expect this feature to be used in practice. But maybe some users will use the shadowing to implement a form of specialization? Or any other unrelated use case I can't think of right now. If that happens, it might lead to a discussion about whether the lint should be enabled by default or not. And if it ends up allow-by-default, it loses much of its value.

[ripytide]

How about an alternative approach for stabilization where itertools renames any methods that the standard library promises to stabilize, like Itertools::intersperse() to Itertools::it_intersperse(). Then after this is released for a reasonable amount of time such as 6/12/18/24 months we hope that a majority of the ecosystem has migrated to this new name. At which time we can stabilize that method on Iterator with mitigated ecosystem churn.

This way we don't have to add any language complexity, while still being able to migrate any methods we like, at the expense that it takes quite a while to do so.

[ripytide]

Then we could then add clippy lints for these renamed methods to encourage people to switch back to the stabilized std versions.

[ChayimFriedman2]

Instead of relying on trait dependence, maybe we can rely on crate dependence? That is, if one crate crate is a dependency of the other use its method.

[jhpratt]

(not directed at anyone in particular)

Keep in mind that this RFC is generally applicable, not specific to itertools. While it is the current impetus, it is a near certainty that this will cause issues in the future. Relying on the current "technically not breaking" is obviously insufficient, as otherwise intersperse would've already been stabilized.

[senekor]

It seems to me like we're adding a feature to the language not because we think it's a good feature on its own, but because it happens to have a convenient side effect for the work of the project. It feels wrong, but I'll always agree to go the pragmatic route.

I'll throw in another idea for an alternative: hard-code the method resolution for those exact methods we want to uplift into std. See a call to intersperse with Itertools in scope? Resolve to one or the other silently, because we know the semantics are identical. All other ambiguities still cause an error as is the status quo. Ugly? Yes. Pragmatic? Maybe.

The advantage I see would be that there is no seemingly useless "feature" in the langauge we have to support forever. There is no user-facing change. We may at some point run crater again and if use of Itertools::intersperse has dropped low enough (there could also be an active effort to migrate the ecosystem) we may finally remove the hard-coded method resolution. Smooth migration, no renaming necessary, no additional language complexity.

I have no idea how complex this would be in terms of the implementation. This is certainly not worth paying for with much complexity.

[burdges]

We should clarify the problematic scenario here:

We've a crate victim that imports say rand::Rng and attack_vactor::prelude::* for some other crate attack_vactor, so now adding a subtrait of Rand to attack_vactor::prelude::* changes beahvior of rand::Rng in victim. That's pretty bad.

There are several safer alternatives:

1. Add some #![auditable] attribute that disables all shadowing, except ideally you want shadowing disabled in both dependents and dependencies too.
2. Always choose the supertrait method, never the subtrait method, in this case meaning core::Iterator::intersperse not Itertools::intersperse. It's still problematic, but works assuming supertrait crates behave more carefully than subtrait traits.
3. Always choose the method from the trait defined under the later edition setting. At this point, itertools simply removes their intersperse method, so then an earlier edition itertools used with a later edition core results in core overriding itertools.
4. Always choose the supertrait method, but return an error unless the supertrait method has a shadow attribute that declares the editions being shadowed. Aka roughly 2+3+attribute.

trait Iterator<..> {
    ...
   #[shadow(edition < 2024)]
    fn intersperse(self, element: Self::Item) -> Intersperse<Self>;
}

I'd think 4 or similar provides the safest solution. We'd stabalize the shadow attribute so that ecosystem crates could benefit too, but only under edition upgrades.

[archer-321]

I'll throw in another idea for an alternative: hard-code the method resolution for those exact methods we want to uplift into std. See a call to intersperse with Itertools in scope? Resolve to one or the other silently, because we know the semantics are identical. All other ambiguities still cause an error as is the status quo. Ugly? Yes. Pragmatic? Maybe.

@senekor, I wonder whether the project would be OK with the implications this has for other (non-rustc) Rust compilers. If this hardcoded list defines method resolution, it would likely have to be mentioned in the reference, and the way I understand your suggestion, removing the hardcoded list would be a breaking change (e.g., because code that was working with the workaround while never being updated would stop working).

I agree that complex features like this must be considered carefully, especially if they can introduce unexpected results. However, hardcoding a list of workarounds for method resolution could easily be "just as confusing", IMHO.

[senekor]

the implications this has for other (non-rustc) Rust compilers. If this hardcoded list defines method resolution, it would likely have to be mentioned in the reference

Yes, that's a good point. It's not an impossible problem to solve, but I'm not fond of the idea myself.

adding a subtrait of Rand to attack_vactor::prelude::* changes beahvior of rand::Rng in victim

Thank you, that is the kind of situation I was worried about. Although this seems hard to exploit in practice, I think it shows that the language feature isn't very desirable on its own.

I like the idea of picking the supertrait method iff the supertrait opted into this shadowing behavior. It makes it clear that this feature is meant for our exact migration use case only. I'm not sure if gating on a specific edition and stabilization are necessary, it seems like those things could be added later as well, if they turn out to be needed.

[kennytm]

@burdges

"A lint could be emitted to warn users about the potential ambiguity."

So #![auditable] for the current crate is just #![deny(rfc_3624_lint)].

Perhaps there should also be another lint, that detects when the sub-trait defined that shared the exact same name with the super-trait, to address the "dependents" case.

---

BTW I'm very confused why some comments here keep saying this RFC is a "complex feature". The whole feature is literally described in 4 sentences. The implementation is most likely localized to the probe phase. There isn't much complexity added compared with many other RFCs out there.

[archer-321]

BTW I'm very confused why some comments here keep saying this RFC is a "complex feature". The whole feature is literally described in 4 sentences. The implementation is most likely localized to the probe phase. There isn't much complexity added compared with many other RFCs out there.

At least for my comment, "complex feature" means a feature introducing language complexity. Many RFCs have more complex implementation details, but this RFC can still be complex language-wise if it makes the code harder to understand. In this case, writing new code using the standard library implementation while old code continues to use the itertools implementation could make the same line of code lead to two different method implementations depending on a use statement in the module.

Some programming languages are centred around keeping language complexity to a minimum, even if it comes at the cost of convenience.

Now, Rust isn't Zig or Go, and the language complexity introduced by this feature is likely manageable, as this form of method resolution "feels natural" in most cases. As such, I personally don't think the complexity should block this RFC.
However, the potential increase in cognitive load should be discussed, IMO.

[senekor]

@kennytm I am also thinking about langauge complexity, specifically for people learning Rust. I recently taught a Rust workshop and traits were a difficult topic for the participants. I'm wondering if in the future, I'll have to teach people about which trait method will be implicitly selected under which circumstances.

Now, it's likely that the average Rust dev will never come into contact with this feature in the first place (meaning I don't have to teach it), because it only ever comes up in such migration situations. In that case, I'm perfectly fine with it. I'm just not sure of it yet. I'm trying to think of ways this feature could be (ab)used in ways that newcomers will have to know about. No success so far, which is a good sign.

What would be the downside of always resolving the implementation to the supertrait? For the intended use case, it doesn't matter, because they are the same. It's also closer to what we're trying to achieve in the first place: Move away from the subtrait toward the supertrait. It seems like less potential for spooky-action-at-a-distance to me. Adding or removing a use statement cannot modify the bahavior of the program. (Or can it already, for different reasons? I'm not sure.)

[programmerjake]

Adding or removing a use statement cannot modify the bahavior of the program. (Or can it already, for different reasons? I'm not sure.)

adding use statements can already change method resolution, e.g. here calling v.len() returns a String where v: Vec<u8>:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=549b192cbf93758ce85965bb442fc68d

[compiler-errors]

I know it's implicit in the statement of "encounters an ambiguity" (because having an ambiguity is dependent on actually having two methods in scope), but I would appreciate if the RFC mentions that we won't automatically select a subtrait method if it's not in scope.

In practice, for the Itertools example, this RFC's new rules only apply if we actually use itertools::Itertools so it's actually in scope (and therefore it's able to create an ambiguity at all).

[burdges]

I think "complexity" does not capture the concerns here, really neither does "intuitiveness". We do not need a single word that captures the concerns though.

Wow @programmerjake that's really nasty. Any idea why that happens? I'd thought inherent methods always won out in resolution? Can we lint against this behavior? Or does std require it somewhere?

Although this seems hard to exploit in practice, I think it shows that the language feature isn't very desirable on its own.

It's not "hard to exploit" in the same sense in which secureity people say "hard to exploit". It's quite easy. An exploit requires work of course, but that's common.

Accedental changes cause some concern too here, like when using traits at VM boundaries.

@kennytm Cool. Yeah, I figured some lint would appear eventually, but..

#![deny(rfc_3624_lint)] would protect your crate against dependencies, which makes it more useful than #![deny(unsafe_code)]. So #![deny(rfc_3624_lint)] would hopefully become more commonly used than #![deny(unsafe_code)].

As denying should become common, we should ask what solution really makes the most sense here?

Around my 3. Afaik, there is no good reason to favor the subtrait method over the supertrait here. It's clear either direction could break agreement between static methods and trait object methods, but we'd break agreement less if we favor supertraits over subtraits. Again, this does not solve the problem I mentioned either, but again it's less bad this way.

Around my 4. We want shadowing to remain rare, so op-in attributes should create less work than any op out solution. Yes, op-in is delicate too, but several designs make sense, so they should be seriously discussed. I thought up op-ins that exploit edition boundaries, but maybe other flavors make sense.

[cynecx]

Any idea why that happens? I'd thought inherent methods always won out in resolution? Can we lint against this behavior? Or does std require it somewhere?

That's probably because the method candidate fn len(self) doesn't require additional autoref-ing.

[burdges]

I'll single out the simplest seeming solution:

Add a #[subshadow] attribute which causes a supertrait item to shadow a subtrait iterm, but otherwise the name conflict causes an error like now. core::Iterator::intersperse would gain this attribute, thereby replacing Itertools::intersperse.

Implementation should be similar to this RFC, except the shadowing would be reversed, and turnned off by default.

Any errors must originate from the rare excplicit references to itertools::structs::Intersperse. We'd solve those by only stabalizing core::Iterator::intersperse in the next edition. If you use itertools struct explicitly, then you must upgrade itertools when you upgrade rust editions, so that your itertools reexports this from core.

There are no impacts on how you read or audit code here: We only have supertraits shadowing subtraits, which sounds safer, for both trait objects and supply chain attacks, but even this shadowing only occurs when explicitly required by the supertrait.

Am I missing anything?

[jhpratt]

Consider this situation:

• library A has trait Foo
• crate B, depending on A, has trait FooExt with Foo as a supertrait
• A adds a new method to Foo, but it has a default implementation so it's not breaking. B has a pre-existing method with the same name.

In this general case, the reason this cannot be resolved in favor of the supertrait is that the method signatures are not necessarily compatible.

In code:

#![allow(unused)]

mod a {
    pub trait Int {
        // fn call(&self) -> u32 {
        //     0
        // }
    }
    impl Int for () {}
}

mod b {
    pub trait Int: super::a::Int {
        fn call(&self) -> u8 {
            0
        }
    }
    impl Int for () {}
}

use a::Int as _;
use b::Int as _;

fn main() {
    let val = ().call();
    println!("{}", std::any::type_name_of_val(&val));
}

Resolving in favor of a is a breaking change; in favor of b is not. The only other option is the status quo: not compiling. a simply cannot happen lest we violate backwards compatibility and the status quo is not ideal. Personally I view b as the only sensible solution, and that is this RFC.

@burdges Nothing about this RFC is specific to the standard library, so I don't see how you envision an edition fitting into this.

[davidhewitt]

As far as I can tell, regardless of the outcome of this RFC, we still have to view introduction of trait methods as introducing ambiguity, e.g. for the intersperse case there might be some other trait method Bar::intersperse which does not have a relationship to either Iterator or Itertools. This RFC does not help with ambiguity in that case (and I don't think it should try to).

The main benefit I can see of this RFC is that it allows me as a library maintainer to refactor methods into supertaits without breaking explicit usage of SubTrait::method by UFCS. I can see that being useful from time to time.

I also think this RFC proposes an idea that feels consistent with both Deref shadowing and also #3245, so the additional complexity doesn't seem high.

[kennytm]

@archer-321 @senekor However, item shadowing is already happening with inherent vs trait methods, so that "complexity" already existed. (Not to mention the auto-ref based method resolution mentioned #3624 (comment) which was exploited for like Generalized Autoref-Based Specialization. This is the emergent complexity.).

Explaining this RFC is as simple as

For each step of auto-deref/auto-ref,
 1. Pick the inherent item `S::name` if it existed
 2. Find all traits `Tn` that `S` implements, and check every trait that `Tn::name` existed.
    If there is a single unique trait `T0` defining `T0::name`
-   and no other traits do,
+   which is a subtrait of every other traits that do,
    pick `T0::name`, otherwise (if there are >1 such candidates) fail for ambiguity error.

[burdges]

Nothing about this RFC is specific to the standard library,

Also in my proposal.

so I don't see how you envision an edition fitting into this

An edition change provides a flag that downstream code upgraded. There is no reason other code cannot use this flag independently of the standard library, but major version upgrades provide a similar flag.

In this general case, the reason this cannot be resolved in favor of the supertrait is that the method signatures are not necessarily compatible.

Sure.

We'd exploit the edition boundary in my proposal precisely because itertools has an incompatible method signature, since itertools' methods are not -> impl Iterator. It's mostly invisible of course since everyone infers the type usually. A transition not involving core could ignore such an invisible signature break, or wait for their own major version upgrade.

In general, subtraits being favored brings a similar problem too. We could build examples that this RFC breaks too: Initially a crate has both use core::iter::Intersperse and use foo::prelude::* which then breaks when foo add itertools into its prelude. We'd need itertools to gate their intersperse upon an edition boundary.

At a high level, all this seems extremely rare, so it's much worse to make auditing hard across the whole ecosystem.

If you ship this RFC as proposed, then supply chain attacks should eventually make #![deny(rfc_3624_lint)] commonplace. That'd completely break the feature this RFC proposes. At that point, you're then stuck favoring subtraits, so you cannot adopt more workable alternative like I'm proposing.

In fact, we'll definitely have future legislation about supply chain attacks in many jurisdictions, especially for government contractors. It's likely legislation cares about upgrades, but does not care about build breakage, which could make #![deny(rfc_3624_lint)] very appealing.

[senekor]

Thanks for the explanations all, my opposition to this is dwindling.

I agree with @burdges proposal to make this behavior opt-in from the supertrait side. So far, there hasn't been a use case discussed where it would be desirable for this shadowing to happen without the knowledge / consent of the supertrait author. So I don't see a reason to extend this implicit behavior beyond its intended use case.

[Amanieu]

• Always choose the supertrait method, never the subtrait method, in this case meaning core::Iterator::intersperse not Itertools::intersperse. It's still problematic, but works assuming supertrait crates behave more carefully than subtrait traits.

In the Iterator case we actually want to prefer the subtrait method because it may have a different signature than the one in the supertrait. Consider the Iterator case, if we prefer supertrait methods then it forces the standard library version to always have the exact same signature as the Itertools version. We want to keep the flexibility of making API changes when uplifting functions to the standard library.

• Always choose the method from the trait defined under the later edition setting. At this point, itertools simply removes their intersperse method, so then an earlier edition itertools used with a later edition core results in core overriding itertools.

This would mean that Iterator can only be changed on edition boundaries every 3 years. This would severely slow down the development of the standard library, which is unacceptable.

• Always choose the supertrait method, but return an error unless the supertrait method has a shadow attribute that declares the editions being shadowed. Aka roughly 2+3+attribute.

An attribute could be an interesting idea, but in a different way: we could explicitly mark specific methods on the Iterator trait so that the logic proposed in this RFC would only apply to these methods. However if this is the case then it is unlikely that this attribute will ever be stabilized: it will remain as a hack just to support the evolution of the Iterator trait in the standard library.

[Amanieu]

As asked above, how do the specilization-like conflicts resolve? In particular if the subtrait were restricted by lifetime.

I think that's mostly explained by the example I added? Name resolution happens before trait solving so it will ignore trait bounds. This means that in your example Bad2::bad will always be selected.

[tmandry]

We'll be creating a slightly bad experience for users of itertools with the lint, and that will only be resolved by the crate releasing a new semver-incompatible version or a new trait (or perhaps doing rustc version hacks, if the signatures are compatible). That said, disambiguating with UFCS isn't so bad. On a related note: I'd like to see iter.Itertools::intersperse(a), which would be a nicer way of disambiguating, work in the future.

Anyway, it clearly seems worth it to stabilize intersperse in std and to mitigate supply chain attacks. I do see a future where we mitigate supply chain attacks in some other way and move the lint to the definition site of the subtrait. Or, less likely, decide silent shadowing isn't so bad after all and remove it entirely.

@rfcbot reviewed

[traviscross]

// @rfcbot reviewed (no-op here)

I agree with everything @tmandry said.

[compiler-errors]

Name resolution is not where method selection is performed. Method resolution is performed during typechecking.

I think the conclusion is still correct, though, and I would expect after implementing supertrait method shadowing that the code sample would give a Copy trait error, but that has more to do with the order dependence of method selection and type inference.

For reference, notice that this code works today:

trait Foo { fn method(&self) {} }
trait Bar: Foo { fn method(&self) {} }
impl<T> Foo for Vec<T> { }
impl<T: Copy> Bar for Vec<T> { }

fn main() {
    let mut x: Vec<Box<i32>> = vec![];
    // We disqualify `Bar`'s method since we know at this point
    // that `Vec<Box<i32>>` doesn't implement `Bar`, since
    // `Box<T>: !Copy`. Thus, there is no ambiguity.
    x.method();
    x.push(Box::new(22));
}

[compiler-errors]

I would prefer if we call this "method selection" or "method resolution", here and all other instances below. There's a pretty big difference between name resolution (which is performed pre-type-check) and method resolution (which is performed during typeck).

[nikomatsakis]

@rfcbot resolve discuss-ambiguity-and-inference

Notes: The RFC does not do the behavior I proposed and, as described and implemented by @compiler-errors, can prematurely commit to using a subtrait that winds up being inapplicable. I'm not crazy about this, but it's already true of method probing, and it seems potentially to be erring on the side of "caution", since we believe that subtrait methods are the ones that users are more likely to want (that is indeed the whole premise of this RFC). Also, @compiler-errors convinced me that ambiguity could arise very frequently in practice due to T: Sized bounds in particular.

To be quite honest, this kind of fine-grained interactions with inference are not (to me) things that RFC should specify. I consider that more the purview of the types team and something to be resolved at implementation-specification-and-stabilization time.

[nikomatsakis]

@rfcbot resolve lint

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[jswrenn]

We'll be creating a slightly bad experience for users of itertools with the lint, and that will only be resolved by the crate releasing a new semver-incompatible version or a new trait (or perhaps doing rustc version hacks, if the signatures are compatible).

This is going to create a slightly bad experience for the maintainers of itertools, too. We will get issues filed by users who would like us to remove now-shadowing methods for their toolchain because of the lint. (If we're very unlucky, those users will have also discovered that the rationale for the lint is to avoid supply chain attacks.) We won't be able to readily remove these methods, because that conflicts with our longstanding policy of maintaining a conservative MSRV. The only way we'll be able to reconcile these interests is by using build.rs-rustc-version-detection hacks, something we've intentionally avoided because build.rs is a vector for supply chain attacks.

Please consider making this lint allow-by-default.

[traviscross]

@rustbot labels +I-lang-nominated

Let's nominate to discuss the useful feedback from @jswrenn above.

(We can of course handle the setting of the lint level during stabilization, but we might as well discuss this while it's fresh.)

[kennytm]

We won't be able to readily remove these methods, because that conflicts with our longstanding policy of maintaining a conservative MSRV. The only way we'll be able to reconcile these interests is by using build.rs-rustc-version-detection hacks, something we've intentionally avoided because build.rs is a vector for supply chain attacks.

I think in the distant future you could use #[cfg(not(version("1.999")))] or #[cfg(not(accessible(::std::iter::Iterator::intersperse)))] to remove these methods, no need to invoke build.rs.

However I don't think rust-lang/rust#64796 nor rust-lang/rust#64797 is going to stabilize before this RFC or Iterator::intersperse, and a conservative MSRV policy probably means cfg_version/cfg_accessible can't be used at all.

[traviscross]

@rfcbot concern consider-feedback-on-lint

This RFC normatively specifies that a warning lint be emitted. It suggests that we might relax this, but only if we find there are use cases other than backward compatibility.

Above, @jswrenn is suggesting a use case that is about backward compatibility but that still calls for reconsideration of this.

We weren't quite able to get to this in the meeting today, so let's file a concern to leave space for discussing that.

[joshtriplett]

We won't be able to readily remove these methods, because that conflicts with our longstanding policy of maintaining a conservative MSRV. The only way we'll be able to reconcile these interests is by using build.rs-rustc-version-detection hacks, something we've intentionally avoided because build.rs is a vector for supply chain attacks.

I think in the distant future you could use #[cfg(not(version("1.999")))] or #[cfg(not(accessible(::std::iter::Iterator::intersperse)))] to remove these methods, no need to invoke build.rs.

However I don't think rust-lang/rust#64796 nor rust-lang/rust#64797 is going to stabilize before this RFC or Iterator::intersperse, and a conservative MSRV policy probably means cfg_version/cfg_accessible can't be used at all.

I can think of other possibilities. For instance, in theory you could add enabled-by-default feature flags that people can disable, or (as a pattern I've seen some crates use) msrv-NNN feature flags that allow you to assume Rust is at least that new.

None of those are particularly palatable options, nor is build.rs-based version detection.

Once we ship the MSRV-aware resolver, there should be better options available for crates to provide a version with conservative MSRV and a version with an up to date MSRV, and have one or the other automatically selected. @epage has done some experiments with polyfills using that technique.

I do think it'd be possible to get accessible and version over the finish line; the former (in the limited incarnation that only works for std/alloc/core) seems relatively close to completion, as far as I can tell.

[matthieu-m]

I have not seen any discussion of accidental overrides, though I may have missed it.

Unlike Java & C++ where inheritance is prevalent, the use of super-traits in Rust is not as widespread, so there's that. Still, it seems beneficial to nudge the developer of a sub-trait to avoid accidentally shadowing the name of a super-trait item.

One of the benefits of requiring override is specifically that it makes overrides opt-in, and thus a developer cannot accidentally stumble upon it.

Of course, it won't help with the Iterator/Itertools situation, as the conservative MSRW policy of Itertools means it couldn't easily use override for new method.

[jswrenn]

Still, it seems beneficial to nudge the developer of a sub-trait to avoid accidentally shadowing the name of a super-trait item.

Why is super-trait item shadowing such a unique threat that it warrants a warn-by-default lint, but Rust's other shadowing mechanisms do not? For example, how does the threat of super-trait item shadowing compare to the resolution preference given to inherent methods? Or to that of shadowing from glob imports?

[epage]

Once we ship the MSRV-aware resolver, there should be better options available for crates to provide a version with conservative MSRV and a version with an up to date MSRV, and have one or the other automatically selected. @epage has done some experiments with polyfills using that technique.

e.g. https://crates.io/crates/is_terminal_polyfill/versions

This approach is very tricky because callers can't set minimum version bounds for each minor version (one minor version per Rust version supported), so if new functionality gets added or a blocking bug gets fixed, you can't require the use of those new versions.

[matthieu-m]

Still, it seems beneficial to nudge the developer of a sub-trait to avoid accidentally shadowing the name of a super-trait item.

Why is super-trait item shadowing such a unique threat that it warrants a warn-by-default lint, but Rust's other shadowing mechanisms do not? For example, how does the threat of super-trait item shadowing compare to the resolution preference given to inherent methods?

I also wish that override was mandated for inherent overrides, to be honest. Just to make it clear.

With warnings for the absence of override annotation, one get the best of both worlds:

1. The overridder explicitly knows they're overriding. A good prompt to add a reasoning in the method documentation as to why it overrides, and in which circumstances to use the override or the "base" instead.
2. The base trait can still add methods which become overridden: it's just a warning, so it doesn't break compilation.

Or to that of shadowing from glob imports?

I personally never invoke "bare" functions, and instead import their immediate parent module and call them through that -- such as cmp::min(...) -- specifically to see immediately where the function comes from and avoid accidental shadowing.

[kpreid]

I'd like to raise a concern (but I am not a team member, so this is not an rfcbot formal concern) with a different perspective on something @zachs18 already mentioned:

One difference between this and OOP-overriding (as I understand it), is that with this, the supertrait method still "exists" and is callable, either by just not importing the subtrait, or by using UFCS.

One of the common misunderstandings that beginners may come to Rust with is that traits are essentially abstract classes or interfaces, offering inheritance and overriding. But in fact the trait system does not work that way; a key difference between typical OOP inheritance and traits is that every trait is its own namespace of methods (or more generally, of associated items) and Trait1::foo() is always a different function than Trait2::foo().

Implicitly allowing shadowing of a supertrait method will make Rust appear to support conventional inheritance more than it already appears to, as long as one is writing code involving concrete types and the subtrait, and then when the supertrait is actually used in a generic fashion, or in a place where the subtrait is not in scope, it will mysteriously start invoking the “overridden” supertrait method (supposing that both traits have provided bodies for the methods in question). In OOP languages I am familiar with, it is either impossible to do this (Java), possible but generally considered a hazard rather than a feature (C++ “object slicing”) or possible but primarily used for the class to implement calling its superclass implementation (Python and JavaScript), not as part of the object’s API surface.

I am concerned that this will make Rust harder to learn by adding an obstacle to understanding the essence of the trait paradigm, and create an opportunity for people to write code that looks elegant and correct (to the reader not deeply familiar with the axioms of Rust) but is in fact incorrect.

---

In order to enable the itertools-uplift use case without harming Rust’s comprehensibility as a whole, I would suggest that this shadowing should only be permitted when explicitly requested (such as by an attribute on the specific method), rather than automatically permitted for all trait methods. The RFC does mention this alternative:

This could be done by using a perma-unstable #[shadowable] attribute specifically on methods like Iterator::intersperse. … While it allows most Rust users to avoid having to think about this issue for most traits, it does make the Iterator trait more "magical" in that it doesn't follow the same rules as the rest of the language. Having a consistent rule for how name resolution works is easier to teach people.

However, I disagree with this analysis, in that I believe that allowing shadowing everywhere, automatically is the “more magical” option; it's adding DWIM, by silently resolving ambiguity in a hopefully-wanted direction. It would be better to reserve that implicit resolution for where it is known to be needed. (Method dispatch is already rather DWIM and operating at the limits of comprehensibility; see e.g. the trap where if foo is a reference to a type that does not implement Clone, then foo.clone() will give you a clone of the reference rather than the referent. One could argue that given that, more DWIM is fine; I believe that this may be more abstractly consistent but will not be to users’ benefit in practice.)

[tmandry]

Thanks for the feedback on the lint. I now think we should do the following:

• Move the lint at the call site to allow-by-default.
• Add a new warn-by-default lint at the definition site of the subtrait.

This has the effect of not annoying users of libraries like itertools, while discouraging the use of this feature or leaving the impression that it's a "first class override feature" when that's not what it is. In particular, the definition-site lint gives us a chance to inform the user about the difference between shadowing and overriding and avoiding the potential confusion that @kpreid talks about.

Finally, it leaves us more room to allow for actual overriding – and refinement (#3245) – of supertrait items, by making the user clarify their intent to shadow today. Given the constraints that led to this RFC, we would need an attribute like #[override] that opts into this, but we shouldn't silently accept shadowing when overriding is another possibility, whether only in the user's mind (for now) or in the actual language someday.

[traviscross]

We talked about this last week in our meeting, and then we touched on it briefly today.

In talking it through last week (ahead of tmandry's comment above), we had agreed that we needed a better story here for the kind of cases raised by @jswrenn before doing a warn-by-default lint at the use site. The plan was to update this RFC to remove the normative language to the effect of doing a warn-by-default lint here.

Given tmandry's proposal above, it would make sense to additionally add an unresolved question about whether we should add a warn-by-default def-site lint before stabilization.

I'm planning to make these changes directly to the RFC, if nobody gets to it first, and then resolve the concern here.