Stabilize the supertrait_item_shadowing feature

[Amanieu]

View all comments

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:

• Add section on method resolution regarding supertrait shadowing reference#2147

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.

[Amanieu]

#152225 fixes the inconsistency with type-level name resolution.

[jackh726]

@BoxyUwU given I got assigned to #152225, I'll just take the opportunity to do a general review of the implementation of the feature - I think that'll eventually resolve both your concerns?

[BoxyUwU]

Yeah that would resolve both of my concerns 👍 Thanks jack and amanieu :)

[jackh726]

I looked over the impl for this (and reviewed #152225). Generally happy here, though I did have a few minor things: #155172

[apiraino]

hello, checking progress, slighty unsure about the status. Is this waiting on documentation? As per comment, I still see the S-waiting-on documentation label. Is this blocking merging?

EDIT: IIUC this is waiting on #152225 (comment)

thanks

[apiraino]

is the FCP at this comment superseded by the second FCP at this comment?

[traviscross]

is the FCP at this comment superseded by the second FCP at this comment?

That's correct. I've removed the docs label; it's not waiting on that.

[BoxyUwU]

@rfcbot resolve implementation-confidence

I'd missed jacks comment here before

[BoxyUwU]

@rfcbot resolve inconsistent-name-res-semantics

[rust-rfcbot]

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

[BoxyUwU]

I would kind of like us to restart this FCP given that the design here has changed and this feature now applies to more things 🤔