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.

[jackh726]

The FCP should have included T-types because, as @lcnr said in #148605 (comment), this affects method resolution which is under the team's purview.

[traviscross]

@rfcbot cancel

[rust-rfcbot]

@traviscross proposal cancelled.

[traviscross]

@rfcbot fcp merge lang,types

(We still need a Reference PR. I'll refile my concern for this. I'm reproposing FCP anyway so as to be able to recheck the boxes that have already been checked.)

This proposed FCP includes the rename of the lints in #148605 (comment).

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @joshtriplett
• @lcnr
• @nikomatsakis
• @oli-obk
• @scottmcm
• @spastorino
• @tmandry
• @traviscross

Concerns:

• pending-reference-pr (Stabilize the supertrait_item_shadowing feature #148605 (comment))
• rename-lints (Stabilize the supertrait_item_shadowing feature #148605 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

@rfcbot concern pending-reference-pr

The stabilization affects the rules of method resolution and of name resolution. Let's write these rules out in the Reference. Method resolution is documented and the rules for this can be integrated there. Name resolution is currently a stub (but not for much longer, due to @yaahc's work in collaboration with @petrochenkov); in that case, the specific rules for this can be added to the stub and will be later integrated when the chapter is filled in.

For my part, generally, as a lang matter, I want to see the proposed language for the Reference before checking a box or proposing FCP myself (it's OK if there's further editorial work to do), as reviewing the proposed Reference text is part of understanding the change we're making to Rust. Consequently, I don't think something should go into FCP before that language is available. For that reason, I'll file a concern here.

(Originally #148605 (comment).)

[tmandry]

Having reviewed the RFC text, I feel like I understand this change well enough to check a box. The reference PR will still be useful to check for any potential surprises, and I'll lean on the concern filed by @traviscross for that.

@rfcbot reviewed

[traviscross]

@rfcbot concern rename-lints

So we don't lose track of it, I'll file a concern too about the fact that the lints need to be renamed in accordance with #148605 (comment). The proposed FCP includes that decision (which I'll now edit into the comment proposing FCP).

[Amanieu]

I did a bit of testing and I think the current behavior of the feature and lints is inconsistent depending on the type of associated item. Playground link

	Behavior	Def-site lint	Use-site lint
Associated type X::T	⚠️ [E0223]: ambiguous associated type	Triggers	⚠️ Doesn't trigger
Associated constant X::C	Picks sub-trait constant	Triggers	⚠️ Doesn't trigger
Associated function X::foo	Picks sub-trait function	Triggers	⚠️ Doesn't trigger
Method call x.bar()	Picks sub-trait method	Triggers	Triggers

Before we stabilize this, we should consider if this is the behavior we actually want.

[Amanieu]

PR to rename the lints: #149532

[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.