Method resolution should obey usual shadowing rules for glob imports & prelude

[matklad]

Today item import take precedence over glob imports which take precedence over prelude imports. This works for traits:

mod a {
    pub trait X {
        fn x(&self) { println!("a"); }
    }

    impl X for () {}
}

mod b {
    pub trait X {
        fn x(&self) { println!("b"); }
    }

    impl X for () {}
}

use a::X;
use b::*;

fn main() {
    ().x(); // Works and prints "a", b/c b is shadowed
}

However, this logic does not work for trait methods. If we rename b::X to b::Y, the code fails to compile:

mod a {
    pub trait X {
        fn x(&self) { println!("a"); }
    }

    impl X for () {}
}

mod b {
    pub trait Y {
        fn x(&self) { println!("b"); }
    }

    impl Y for () {}
}

use a::X;
use b::*;

fn main() {
    ().x(); // error: multiple `x` found
}

I think this problematic for two reasons:

• it seems inconsistent with how other names work
• it makes adding a trait to prelude in std and other crates a breaking change (see Tracking issue for the 2018 edition’s prelude #51418)

Could we perhaps fix it?

r? @rust-lang/lang

[petrochenkov]

There was an attempt to do it in #31908 and apparently there was breakage.

[petrochenkov]

We can probably redo that experiment again, look how many crates break, maybe make the change edition-specific.

[matklad]

Hm, not sure that #31908 is exactly what I am thinking about. That PR tweaks the logic of shadowing the trait itself, but not the methods.

That is, I imagine the rule like "if both Foo::x and Bar::x are applicable methods, but Foo trait is brought in scope via a glob/prelude import and Bar is imported/defined explicitly, do not report error and resolve ambiguity in favor of Bar".

[sunjay]

I think I ran into a problem that is related to this during a workshop yesterday.

Here's a minimal reproduction of the problem:

mod submodule {
    pub trait MyThing {
        fn foo() {}
    }
    
    pub struct A {}
    
    impl MyThing for A {}
}

use submodule::*;

type MyThing = A;

fn main() {
    MyThing::foo();
}

I'm using a submodule here, but in the workshop this was coming from an external crate and the problem was much less obvious because there were many other items being imported.

The problem was that both the participant and I expected that since we were using a glob import, the trait MyThing would already be imported, but the error message was recommending that we import it:

error[E0599]: no function or associated item named `foo` found for type `submodule::A` in the current scope
  --> src/main.rs:16:14
   |
6  |     pub struct A {}
   |     ------------ function or associated item `foo` not found for this
...
16 |     MyThing::foo();
   |     ---------^^^
   |     |
   |     function or associated item not found in `submodule::A`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope, perhaps add a `use` for it:
   |
11 | use crate::submodule::MyThing;
   |

Usually, when you redefine a type in Rust, it's an error, but for glob imports the shadowing can lead to these kinds of errors that aren't really obvious given the semantics that many of us expect.

I think for my particular case it might be better to adjust the error message to deal with this edge case rather than adjust the trait resolution behaviour at all.

[eddyb]

@petrochenkov Can we try doing this again, or at least having the information propagated to the type-checker?

I don't know why I never thought of this (nor recall seeing this issue), but it seems like a great solution for putting more traits in the prelude (with as _) and not worry about introducing ambiguity to users.

cc @rust-lang/libs

[petrochenkov]

@eddyb

or at least having the information propagated to the type-checker?

Resolver builds sets of trait in scope for every method location and when it adds a trait into the set it knows how it was introduced - with a glob, or with a single import or definition, or with prelude.
So it certainly can pass it further to type checker for use.

[petrochenkov]

Note that traits don't shadow each other in the "trait in scope" set, but rather form a union

mod m {
    pub trait Trait1 { fn foo() {} }
    pub trait Trait2 { fn bar() {} }
    
    impl Trait1 for u8 {}
    impl Trait2 for u8 {}
}

use m::Trait1 as Trait;

fn main() {
    use m::Trait2 as Trait;
    
    u8::bar(); // OK
    u8::foo(); // OK too, despite Trait2 "shadowing" Trait1
}

so if the priority for method resolution is based on "glob"-ness, then the results could be slightly inconsistent with usual resolution

mod m {
    pub trait Trait1 { fn foo() {} }
    
    impl Trait1 for u8 {}
}
mod n {
    pub trait Trait2 { fn foo() {} }
    
    impl Trait2 for u8 {}
}


use m::Trait1;

fn main() {
    use n::*;
    
    u8::foo(); // Now: ERROR multiple applicable items in scope
               // Proposed: OK, Trait1::foo is selected despite Trait2 being
               //           "closer in scopes" and "shadowing" Trait1.
}

[eddyb]

I would expect them to shadow eachother when the method name clashes, i.e. completely disregarding the trait name.

[Centril]

T-Lang did not have sufficient time to discuss this; leaving for next week.

[pnkfelix]

The T-lang team discussed this in their weekly meeting today.

Our basic reaction was that the change proposed here would at least require some investigation to determine what all the implications are.

• From our understanding of the discussion here, it appears to be an open question whether or not this change would solely cause strictly more code to be accepted (and not change the behavior of programs that are accepted today).

We think that a PR that implements the change being described (since that sounds relatively simple?) would be a great first step in such an investigation, since that would be something we could get crater results on.

• Note as well that a successful crater run is a necessary condition, but it is not a sufficient condition on its own to justify the change.
• Some T-lang members want to ensure we also consider the overall effect on the semantics of method resolution as well.

[fschutt]

Will this also fix the bug with glob imports shadowing public imports? - i.e.:

mod a {

    mod b {
        pub fn something() { } 
    }

    pub use b::*;
    use b::something; // overrides "pub use b::something"
    
    fn other() { something() }
}

fn main() {
    // error: "function something is private"!
    a::something();
}

playground

I ran into this problem a few days ago and could explain why the function was private, even though I re-exported it via pub use. This is a problem because now I need to check every time if I have a use and a pub use statement referencing the same module, which is tedious. Not sure if this is the right place to ask this, but it seemed related.

[matklad]

@fschutt seems unrelated to this issue, which is specifically for method resolution. Your example seems to be expected behavior: non-glob imports shadow (are stronger than) glob ones.