Stabilize the supertrait_item_shadowing feature

[Amanieu]

Stabilization report

Summary

When name resolution encounters an ambiguity between two trait item when both traits are in scope, if one trait is a subtrait of the other then select the item from the subtrait instead of reporting an ambiguity error.

The motivation for this comes from several attempts to add methods to the Iterator trait (#79524, #145733, #141994) in the standard library which already exists in the itertools crate. Stabilizing these Iterator methods leads to the following code failing to compile with an ambiguity error:

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

use itertools::Itertools as _;

fn foo() -> impl Iterator<Item = &'static str> {
    "1,2,3".split(",").intersperse("|")
    // ^ This is ambiguous: it could refer to Iterator::intersperse or Itertools::intersperse
}

Tracking: #89151

Reference PRs:

cc @rust-lang/lang @rust-lang/lang-advisors

What is stabilized

• When name resolution encounters an ambiguity between two trait items when both traits are in scope, if one trait is a subtrait of the other then select the item from the subtrait instead of reporting an ambiguity error.
• A warn-by-default lint when a subtrait defines an item that has the same name as an item in a supertrait.
• An allow-by-default lint when calling a method which is available is both a supertrait and a subtrait but is resolved to the subtrait method.

What isn't stabilized

N/A

Design

Reference

There isn't currently a reference section on name resolution, but there is work towards adding such a section in rust-lang/reference#2055.

RFC history

• RFC: Supertrait item shadowing rfcs#2845
• Supertrait item shadowing v2 rfcs#3624

Answers to unresolved questions

As per the lang team decision, this also sets the default lint levels as follows:

• supertrait_item_shadowing_definition warns by default.
• supertrait_item_shadowing_usage is allowed by default.

Post-RFC changes

The lint levels were decided after the RFC was accepted.

Key points

N/A (already covered above)

Nightly extensions

N/A

Doors closed

This feature could limit the ways in which we resolve ambiguity errors for associated items in the future. This was extensively discussed in the RFC thread and the conclusion was that the proposed behavior is the only sensible one.

Feedback

Call for testing

No call for testing has been done.

Nightly use

There are no current nightly users of this feature.

Implementation

Major parts

This was implemented in

Coverage

There are UI tests for both the shadowing functionality and the lints.

Outstanding bugs

None

Outstanding FIXMEs

None

Tool changes

None

Breaking changes

This is not a breaking change because it only allows code to compile in situations where an ambiguity error would have previously been raised.

Type system, opsem

Compile-time checks

N/A

Type system rules

This feature only provides a way to resolve ambiguities in name resolution and doesn't interact with the type system otherwise.

Sound by default?

Yes

Breaks the AM?

No

Common interactions

Temporaries

N/A

Drop order

N/A

Pre-expansion / post-expansion

N/A

Edition hygiene

N/A

SemVer implications

This doesn't directly introduce a new SemVer hazard. It has always been possible for a sub-trait to accidentally use the same method name as a supertrait. However with this feature this will result in silently calling the subtrait method instead of causing an ambiguity error. The warn-by-default lint at the sub-trait definition site will help crate authors avoid such a situation unless it is intentional.

Exposing other features

N/A

History

• RFC: Supertrait item shadowing rfcs#2845
• Supertrait item shadowing v2 rfcs#3624
• Tracking Issue for RFC: Supertrait Item Shadowing #89151
• Implement RFC 3624 supertrait_item_shadowing (v2) #125782

Acknowledgments

Thanks to @compiler-errors for implementing this and @lcdr for the original RFC.

Open items

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[traviscross]

Thanks @Amanieu for the stabilization PR. As you fill out the stabilization report for us, have a look at our stabilization report template:

https://rustc-dev-guide.rust-lang.org/stabilization_report_template.html#stabilization-report

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[bors]

☔ The latest upstream changes (presumably #139558) made this pull request unmergeable. Please resolve the merge conflicts.

[fee1-dead]

Stabilization looks fine. r=me after FCP.

[Amanieu]

I've updated the summary to match the stabilization template.

[traviscross]

These don't lint. Should they?

#![feature(supertrait_item_shadowing)]
#![allow(unused)]
#![deny(supertrait_item_shadowing_usage)]

trait Super {
    fn f(&self) {}
}
trait Sub: Super {
    fn f(&self) {}
}

fn f<T: Sub>(x: T) {
    let _x = T::f; // Should lint?
    let _ = T::f(&x); // Should lint?
}

[traviscross]

Regarding the lints:

• supertrait_item_shadowing_definition warns by default.
• supertrait_item_shadowing_usage is allowed by default.

Some suggestions on these names:

Def-site lint

Lints on subtrait items that shadow supertrait items.

The name for this seems to call for using a gerund. It came up in a recent meeting whether we already use gerunds for lint names. We do, though it is the exception:

• dropping-copy-types
• dropping-references

If we're willing to lean into using gerunds when convenient, I'd suggest to name this shadowing_supertrait_items. If not, I'd suggest items_shadowing_supertrait_items.

Use-site lint

Let's assume we expand (or might expand) the lint to cover all three cases here:

trait Super {
    fn f(&self) {}
}
trait Sub: Super {
    fn f(&self) {}
}

fn f<T: Sub>(x: T) {
    let _x = T::f; //~ Lints.
    let _ = T::f(&x); //~ Lints.
    let _ = x.f(); //~ Lints.
    // But not:
    let _x = <T as Sub>::f; //~ Does not lint.
    let _ = <T as Sub>::f(&x); //~ Does not lint.
}

This one is a bit tougher. We're linting on when we need to infer a trait to resolve an item and the resolved item is from a subtrait that shadows an item in one of its (transitive) supertraits.

Again, if we're willing to lean into a gerund, it makes this a bit easier, and I'd suggest resolving_to_items_shadowing_supertrait_items. Otherwise, I'd suggest maybe expressions_resolving_to_items_shadowing_supertrait_items.

(This doesn't exactly capture the notion that we don't lint on the fully-qualified form that is, certainly, still a name resolution. Maybe that's OK.)

The gerund question

For my part, I think I'm OK with leaning into gerunds where convenient.

[joshtriplett]

@Amanieu I think we want the use-site lint to trigger in all four cases. That's a two-way door, though, since it's a lint.

I think we want the associated type case to pick the subtrait associated type, rather than erroring. But that doesn't need to be a blocker here, as we can always make that change compatibly in the future. That means we can take our time to check with @rust-lang/types on that one and make sure there isn't an issue with handling associated types as well.

[traviscross]

We talked about this in the lang call today without specific conclusion.

[traviscross]

Based on #149532 merging...

@rfcbot resolve rename-lints

[jackh726]

There is no concrete concern from me on merging this, but leaving this comment here for posterity on a potential future inconsistency:

Specifically, the difference between the behavior of associated types and associated constants is a little suspicious. Specifically, the playground link uses the associated type as a type, but the associated constant as a value. In the future, when you can use associated constants in the type system, I could very much imagine that will be also ambiguous like associated types, rather than picking the sub-trait as when used as a value. This is by itself not an issue - it's okay even if we want to live with this "inconsistency" - but if we don't, then it would be a breaking change if had to make associated constants ambiguous again.

[nikomatsakis]

@rfcbot reviewed

For the record, I do not believe the behavior of associated consts should be different than the behavior of associated types and friends. However, given that we are currently giving an "ambiguity error" in that case we can presumably make it work later on.

What I would not want is that it resolves successfully but favors the supertrait instead of the subtrait or something like that.

[Amanieu]

I opened a reference PR (rust-lang/reference#2147), though technically this only covers the method call syntax (x.y()) but not the special name resolution logic in path expressions (A::B).

Also, while looking at the name resolution code I noticed that we have 2 entirely separate code paths for resolving associated items. One is used for path expressions and method calls, while the other is used for paths in types and generic bounds. Supertrait item shadowing is currently only implemented on the former, but looking at the code it shouldn't be too difficult to support it for type-level paths as well. This would resolve the current inconsistency in behavior.

[BoxyUwU]

Do we have anyone who can actually vouch for the impl at this point? I would say errs but he doesn't work on rustc anymore. I feel hesitant to say we should stabilize a feature if there's nobody who currently understands the implementation 🤔

Especially with what amanieu has said about it being potentially incomplete(?)

[joshtriplett]

@BoxyUwU I don't think it's incomplete in the sense of not doing what it claims to do; it doesn't go as far as it could go in terms of covering all kinds of items that can be associated with a trait.

[traviscross]

Thanks to @Amanieu for adding the rule about path-based resolution to the Reference (see #148605 (comment)). With that:

@rfcbot resolve pending-reference-pr
@rfcbot reviewed

(We still need to approve the Reference PR before this PR merges; that's a separate matter and is what is tracked by S-waiting-on-documentation).

[rust-rfcbot]

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

[BoxyUwU]

I feel kind of uneasy here. Who on types has read/looked at the implementation of this feature so far? errs implemented it but isn't around any more, and I barely remember anything about this feature from when I reviewed the implementation.

The feature also doesn't seem to have any real testing in nightly, the stabilization report says no call for testing and no nightly use. I'm concerned about whether we have a good idea of what we're actually committing to/stabilizing here.

Relatedly, this feature also seems to be method lookup specific, which incidentally changed how we resolve other value paths during type checking (notably rust-lang/rfcs#3624 does not even mention associated constants). This introduces an inconsistency between associated const resolution and associated type resolution which seems undesirable to me.

The current behaviour of this feature seems driven by how the implementation of method lookup and associated type/const resolution happens to be structured in the compiler. Moreso than a principled approach to extending the language.

I would expect us to want all resolution of trait-associated items to have the shadowing behaviour to allow for better evolution of traits. Alternatively I would expect us to want this to only apply to actual method lookup, not other forms of type dependent name res which happen to go through method lookup machinery.

I don't think it makes sense to ship a "minimum" version of this feature, or do so without figuring out what it would take to make the behaviour consistent or only affect methods not other paths resolution. Explaining to users the resolution behaviour here seems awkward with the current inconsistency and I think that should be resolved before this feature is something stable users encounter.

I don't feel convinced that this feature is ready for stabilization and would like to see it be more "rounded out" and have more confidence in it.

Summarizing:

• Is there someone on types who is confident in this feature's implementation? I want this before stabilizing type system features
• Why are we not resolving the inconsistency between associated type resolution and other forms of type dependent name resolution?

[BoxyUwU]

@rfcbot concern implementation-confidence

[BoxyUwU]

@rfcbot concern inconsistent-name-res-semantics

[Amanieu]

I've got a WIP branch that adds the same resolution logic for type-level paths. This handles the following cases:

trait A {
    type T;
}
trait B: A {
    type T;
}
trait C: B {}

impl A for u32 {
    type T = i8;
}
impl B for u32 {
    type T = i16;
}
impl C for u32 {}

use std::mem::size_of;

fn main() {
    generic::<u32>();
    generic_bound::<u32>();
    // error[E0223]: ambiguous associated type
    //println!("{}", size_of::<u32::T>());
}

fn generic<T: B>() {
    println!("{}", size_of::<T::T>()); // prints 2
}

fn generic_bound<T: C<T = i16>>() {
    println!("{}", size_of::<T::T>()); // prints 2
}

It doesn't handle the non-generic u32::T case, but that's because we always require an explicit <u32 as X>::T in this case, even if trait B doesn't exist.