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.

[lcnr]

Adding T-types given that it's in method selection which often has some subtle type system/type inference stuff going on. cc @rust-lang/types

The implementation there does look good enough to me, so 👍

[traviscross]

On the lang call today, we talked about this. We're all eager to propose FCP merge here. One last item before we do so is naming the lints. Among the three of us on the call at that point, we were OK with leaning into the gerunds here and favored:

• shadowing_supertrait_items: Def-site lint.
• resolving_to_items_shadowing_supertrait_items: Use-site lint.

The latter one, we all acknowledged, is rather long, but it's an allow-by-default lint and is expected to be a bit niche; we preferred the precision to trying to shorten it.

If these could be updated (separately, probably) and made part of this stabilization and stabilization report, we'll get this going.

On the question of whether the lint should fire when using paths (e.g. T::f or T::f(..)), we felt it should, but probably none of us would hold up the stabilization for it.

---

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

@ehuss, what do you think? Is there anything we should touch in the Reference as part of this stabilization, or would you expect the Reference to not speak to this until we land @yaahc's work on name resolution?

[traviscross]

Heads-up about this likely upcoming stabilization, @rust-lang/fls.

[ehuss]

Is this more related to the method call work? Is this essentially part of https://doc.rust-lang.org/nightly/reference/expressions/method-call-expr.html#r-expr.method.ambiguous-target or https://doc.rust-lang.org/nightly/reference/expressions/method-call-expr.html#r-expr.method.ambiguous-search?

[traviscross]

That's a good point. This touches method resolution, which is documented, in addition to name resolution, which is what @yaahc is documenting. Probably we'd want to at least say something in method resolution. @Amanieu, sound right?

[joshtriplett]

Based on the stabilization report, I think this is ready to start a stabilization FCP:

@rfcbot merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• pending-reference-pr (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.

[scottmcm]

The one thing I'm thinking about here is cases that are in the other order, perhaps because of upgrading minor versions of different packages on different schedules -- like the super trait had the method first then the sub-trait got it later. So I do wonder if there would be a way to have a warning to attempt to detect that problem at the callsite as more than allow-by-default, but I can't really come up with one.

Overall this will certainly be helpful, and any potential footgun from picking the sub-trait feels better (or at least no worse) than existing footguns from a type adding an inherent method that hides a trait method.
@rfcbot reviewed

I suspect there's more work that would be nice as a follow-up (that allow-by-default lint would be a good thing to start the "I'm a popular library that wants to be extra careful about what I'm doing" group we've talked about before, and we'll plausibly find some interesting cases around different signatures that might change linting) but none of that's blocking.

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