Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Supertrait item shadowing #2845

Open
wants to merge 1 commit into
base: master
from
Open

RFC: Supertrait item shadowing #2845

wants to merge 1 commit into from

Conversation

@lcdr
Copy link

lcdr commented Jan 6, 2020

馃柤 Rendered

Pre-RFC thread

馃摑 Summary

Change item resolution for generics and trait objects so that a trait bound does not bring its supertraits' items into scope if the subtrait defines an item with this name itself.

This makes it possible to write the following, which is currently rejected by the compiler due to ambiguity:

trait Super {
    fn foo(&self);
}

trait Sub: Super {
    fn foo(&self);
}

fn generic_fn<S: Sub>(x: S) {
    x.foo();
}
@mcarton

This comment has been minimized.

Copy link
Member

mcarton commented Jan 6, 2020

I might be surprising that the following functions

fn generic_fn<S: Super>(x: S) {
    x.foo();
}

fn use_trait_obj(x: Box<dyn Super>) {
    x.foo();
}

would call Super::foo rather than Sub::foo, especially for people coming from OO languages expecting some kind of overriding to happen here.

I also always have hated method hiding in C#, which I've only ever seen to cause problems.

@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 6, 2020

The code you posted is actually not affected by this RFC.

The generic function already compiles today, and resolves foo to Super::foo even when something implementing Sub is passed. As an example:

impl Super for i32 {
    fn foo(&self) { println!("super"); }
}
impl Sub for i32 {
    fn foo(&self) { println!("sub"); }
}

fn generic_fn<S: Super>(x: S) {
    x.foo();
}
fn sub_generic_fn<S: Sub>(x: S) {
    generic_fn(x);
}

fn main() {
    let x: i32 = 42;
    sub_generic_fn(x);   // prints "super"
}

The trait object case is a bit different. It fails to compile, but not because of ambiguity, but because Rust doesn't have trait object upcasting.

fn use_trait_obj(x: Box<dyn Super>) {
    x.foo();
}
fn sub_use_trait_obj(x: Box<dyn Sub>) {
    use_trait_obj(x); // error: expected trait `Super`, found trait `Sub`
}

The behavior in these cases is not changed by this RFC.

I'm afraid that in this sense, the decision of shadowing vs overriding has already been made by current Rust. Changing it would be backwards-incompatible, as it would change the existing behavior of generic functions.

I've noted the potential for confusion for OOP users in the drawbacks section of the RFC. Unfortunately, this potential for confusion already exists in Rust today.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 6, 2020

In the next edition, we should make a subtrait shadowing any name from a super trait become a hard error, so this code should not compile:

trait Super {
    fn foo(&self);
}

trait Sub: Super {
    fn foo(&self);
}
@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 7, 2020

Note that a name conflict situation doesn't always arise from a subtrait adding a method with the same name as in the supertrait, but can arise when a supertrait adds a method that already happens to be in a subtrait. This fragile base class problem is the main motivation for this RFC, which aims to avoid breakage in this case.

Making name conflicts a hard error would instead make the breakage worse than it is currently. Today, a supertrait adding a conflicting method is a breaking but minor change, since users can disambiguate using UFCS. With a hard error, this would not be possible, and the subtrait would be forced to change its method's name, which is a major breaking change.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 8, 2020

I see, fair enough. It should then warn whenever you call the method without using UFCS with the subtrait in scope.

@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 8, 2020

Right now not using UFCS will raise a hard error. From context, I understand you're not proposing to replace the error with a warning, but are more interested in avoiding code that is not explicitly disambiguated. I'd be interested to hear your rationale for this.

Consider the following situations, as laid out in the RFC:

fn generic_fn<S: Sub>(x: S) {
    x.foo();
}

In this case it seems quite intuitive that x.foo() would be resolved to Sub::foo rather than Super::foo, as Sub is the trait mentioned in the bound. After all, this is the way it already works for other items of Sub that are not in conflict with Super.

Perhaps you're more concerned about a more ambiguous situation, such as both Sub and Super explicitly being mentioned in the bound:

fn generic_fn<S: Sub+Super>(x: S) {
    x.foo();
}

Under this RFC, shadowing would not apply in this case, as Super is explicitly brought into scope. x.foo() does not resolve and results in a compile error asking you to use UFCS to disambiguate.

This is just a brief summary, the RFC text discusses a few more cases, if I didn't list the one you're concerned about.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 8, 2020

If I understand the current situation, if you bring the subtrait into scope then calling foo without UFCS is a hard error, but this gets relaxed when using generic functions, which act like only the named trait is in scope.

At first blush, I'd suggest making the generic function situation stricter so that it matches the non-generic function situation. It's tricky however what if I write:

fn generic_fn<S: ::bar::Super>(x: S) {
    x.foo();
}
fn sub_generic_fn<S: ::baz::Sub>(x: S) {
    generic_fn(x);
}

I'd say this should hard error without UFCS, even without the call to generic_fn from sub_generic_fn. Yet, returning this error requires considering all traits brought into scope anywhere within the module.

As an aside, we could error if sub_generic_fn called x.foo() if Super adding foo transforms

trait Sub: Super { fn foo(&self); }

into

trait Sub: Super { fn foo(this: &Self); }

but that's insufficient for your example. I'm unsure how aggressive rustc should fight this, but maybe clippy could go further?

I'd be interested to hear your rationale for this.

Ambiguity makes reading code almost impossible, which makes correct code almost impossible.

I'd maybe phrase the compromise goal like: If a human reads the code without compiler assistance, and finds one valid resolution, then either (a) they found the only valid resolution, or (b) the resolution they found explicitly indicates where one searches for other valid resolutions.

We push (b) fairly heavily already:

  • Add<LHS>-like traits get confusing, but the LHS acts like a warning satisfying (b).
  • Inherent methods overriding trait methods gets confusing, but falls under (b) if we accept that readers should check the inherent blocks first.
  • default methods in traits fall under (b) in that you must check the actual impl
  • specialization expands (b) but only with the default keyword.

All these (b) cases become quite painful when identifying the applicable impl requires careful type checking.

I think you're proposal fails (b), and so does the current behavior maybe, because the incoherence lacks any warnings. I do think your note about favoring the supertrait improves this considerably, except many traits come with numerous supertraits, and devs from OO languages expect the dangerous opposite, like @mcarton notes.

@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 9, 2020

If I understand the current situation, if you bring the subtrait into scope then calling foo without UFCS is a hard error, but this gets relaxed when using generic functions, which act like only the named trait is in scope.

My reply to mcarton may have confused you here. mcarton's example is not actually affected by this RFC. To summarize current behavior:

  • For both generic functions and trait objects, calling x.foo() with a bound on Sub is an error and requires UFCS.
  • For both generic functions and trait objects, calling x.foo() with a bound on Super will resolve to Super::foo.
  • The only difference between them is that trait objects don't support upcasting, which is not the subject of this RFC.

I'd say this should hard error without UFCS, even without the call to generic_fn from sub_generic_fn. Yet, returning this error requires considering all traits brought into scope anywhere within the module.

Again, this is unaffected by and unrelated to the RFC. This RFC only discusses what should happen for item resolution when the subtrait is in scope.

I'd maybe phrase the compromise goal like: If a human reads the code without compiler assistance, and finds one valid resolution, then either (a) they found the only valid resolution, or (b) the resolution they found explicitly indicates where one searches for other valid resolutions.

I agree with you, a human should be able to find the resolution that will be chosen by the compiler without compiler assistance. To do this, a human will need to look at a list of places that might contain an item with the name they are looking for. The important part is that this RFC should not make this lookup any more complex than it already is.

Let's consider the lookup order for code that compiles and resolves to a clear implementation:

To find the resolution for an item, a user needs to:

  • Go through each trait mentioned in the trait bound. For each trait:
    • If the trait contains a matching item, this will be the one chosen by the compiler. Done.
    • If not: recursively consider each of the trait's supertraits.

That's all the user needs to do today. Note that inherent impls aren't even necessary to consider, because generics/trait objects already abstract over the specific type.

This RFC actually doesn't change this 'algorithm' at all, it just makes more situations compile than before. Specifically, it will allow situations where sub- and supertraits have conflicting item names. Since subtraits shadow supertraits, you still only need to go up the hierarchy until you find a matching name. This name will always be the one that will be chosen.

Therefore the user doesn't need to change their way of reasoning, and the newly allowed situations will intuitively make sense to a user who is used to this lookup order.
Note that this only works because subtraits shadow supertraits, if it were the other way around, it would add complexity.

@burdges

This comment has been minimized.

Copy link

burdges commented Jan 9, 2020

I understand now. I like the prejudice towards the explicit bounds, meaning T: Sub+Super hard errors. I also like that T: Sub always resolves to a method in Sub unless no such method exists.

In this vein, I'm now curious about this case where trait Sub : Super1 + Super2 { } but both Super2 and Super3 introduce a name collision, but not with Sub. We cannot choose one as both are named, not even if Super1 : Super2, so we should make that name require UFCS, right?

After this change, Sub could fix that name collision by adding the name itself, but doing so makes future name collisions more likely if anyone does T: Sub + Super1, but maybe not such a concern.

@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 9, 2020

Glad I was able to clear up the confusion.

You're completely right, if multiple supertraits are in conflict with each other, it will still be ambiguous. In this case this RFC will require UFCS. See here for more.

You're also right that this RFC makes it possible for the subtrait to manually fix the collision, as detailed here.

It's true that this would in turn make collisions more likely for Sub + Super1, but this isn't much of an issue. Since Sub implies Super1 anyways, there isn't much reason to include both in the bound, and if both happen to be included, Super1's mention can be removed without problems.

@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Jan 9, 2020

One situation I haven鈥檛 seen mentioned is when a method already exists on the super trait, then a method shadowing it is added to the sub trait.

Given code exists like this (probably with each item spread across a set of three crates):

trait Super {
    fn foo(&self);
}

trait Sub: Super {
}

fn generic_fn<S: Sub>(x: S) {
    x.foo();
}

It would currently compile. Currently if Sub were to add a foo method this would cause an ambiguity error in generic_fn, causing the maintainer to fix that via UFCS. With the change proposed in this RFC it would instead change to calling the shadowed method, which may result in a confusing compile error if the signatures don鈥檛 match, or a silent change in behaviour if they are close enough.

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Jan 10, 2020

@Nemo157 That is true, but adding a new method onto a trait is already a semver breaking change, so it would require a major version bump to the crate which defines Sub. So there's no problem.

But the situation is more complicated once you add in default methods and sealed traits. So if this RFC is accepted then the semver requirements would probably have to be changed: adding a method which overrides a super method is now always a major breaking change.

@lcdr

This comment has been minimized.

Copy link
Author

lcdr commented Jan 10, 2020

Thanks for pointing out this situation, I hadn't considered the implications for semver in this case.

I agree with Pauan, this should be fine for non-defaulted items due to semver, and would require an amendment to the semver rules for defaulted items.

If I understand the semver rules correctly, it would currently be classified as minor, since you could have disambiguated using UFCS, but this doesn't seem like a good practice when it can result in silent changes in behavior. So this should probably be amended to be a major change.

However, the amendment should likely be more specific than "added shadowing of existing methods is a major change": As mentioned above, there's a situation in which a subtrait can fix an introduced collision between supertraits by shadowing and manually redirecting to the desired supertrait. This kind of fixing should be allowed as a minor change, since it is intended to keep existing code compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.