Disallow methods from traits that are not in scope

[jseyfried]

This PR only allows a trait method to be used if the trait is in scope (fixes #31379).
This is a [breaking-change]. For example, the following would break:

mod foo {
    pub trait T { fn f(&self) {} }
    impl T for () {}
}

mod bar { pub use foo::T; }

fn main() {
    pub use bar::*;
    struct T; // This shadows the trait `T`,
    ().f() // making this an error.
}

r? @nikomatsakis

[oli-obk]

to be fully consistent, we need to also update E0256 ("import T conflicts with type in this module"), because "use self::T;" will still work in the generic function g, but not in h.

[nikomatsakis]

Now that I see the impact, I have some concerns about both backwards compat, and that this might just not be the right rule. I hadn't fully considered that shadowing with any arbitrary type would rule out using the methods from a trait as well.

For example, this change does not impact the iterator naming pattern, because we name the types IntoIter and the trait IntoIterator, but if you adopted a variant that did not abbreviate, this change might well cause a problem for you (that said, this variant would be unergonomic to begin with, since you cannot easily write impl IntoIterator for IntoIterator).

Another example might be fn foo<Iterator>(i: Iterator) where Iterator: Iterator, though again the shadowing there probably means that pattern wouldn't work either.

I've yet to come up with an actual problematic example, but at minimum a crater run seems worthwhile.

cc @rust-lang/lang

[nikomatsakis]

Crater run executing.

[nikomatsakis]

Crater run result: https://gist.github.com/nikomatsakis/23a0ed555ecf83287c25

5 regressions.

[jseyfried]

Ok, I believe these are our options:

1. Write PRs against the offending crates and then land this as is.
2. Implement warnings and have a warning cycle. This would be a little invasive but is definitely doable.
3. Compromise and forbid prelude- or glob-imported traits shadowed by a type in the same module but continue allowing all traits from ancestor scopes. I believe this would fix 4 of the 5 regressions but I'd have to look in more detail to be sure.
4. Do nothing.

[bors]

☔ The latest upstream changes (presumably #31726) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

@jseyfried

Compromise and forbid prelude- or glob-imported traits shadowed by a type in the same module but continue allowing all traits from ancestor scopes. I believe this would fix 4 of the 5 regressions but I'd have to look in more detail to be sure.

Having thought about this some more, I feel like this rule may in fact be the one that I find most intuitive. Admittedly, I argued the opposite in the past, but thinking about examples like this sort of changed my mind:

fn foo<Default>(...) -> Default {
    ...
    let x = SomeType::default();
    ...
}

[jseyfried]

That example would still work since it uses UFCS, but your point stands.

I updated this PR to use the compromise rule (i.e. allow traits from ancestor scopes). The only remaining regression is matrix-0.21.0, where this use of the method fold of core::iter::Iterator breaks since it is shadowed. We could fix this last regression by always allowing traits from the prelude so that the only newly forbidden traits are shadowed glob-imported re-exports.

Thinking about this some more, I actually prefer this approach since I think we should consider the prelude not as something that gets imported into all* modules' scopes but instead as its own scope that is the parent of all modules' scopes. @nrc suggests this interpretation in his sets of scopes blog post.

This interpretation would mean that we permanently treat the prelude like we currently treat all private imports; for example, we would never allow prelude-"imported" names to be used in a crate relative path or imported into another module.

*except for no_implicit_prelude modules, of course

[nikomatsakis]

@jseyfried

That example would still work since it uses UFCS, but your point stands.

I would not expect my example to work, even though it uses UFCS. The set of traits in scope for a UFCS call like Type::method() (as opposed to Trait::method()) is the same set we would use for method dispatch.

We could fix this last regression by always allowing traits from the prelude so that the only newly forbidden traits are shadowed glob-imported re-exports.

Hmm. I would find this a little surprising...

Thinking about this some more, I actually prefer this approach since I think we should consider the prelude not as something that gets imported into all* modules' scopes but instead as its own scope that is the parent of all modules' scopes.

...but when you put it like that, it kind of makes sense.

cc @rust-lang/lang, thoughts?

This interpretation would mean that we permanently treat the prelude like we currently treat all private imports; for example, we would never allow prelude-"imported" names to be used in a crate relative path or imported into another module.

I think both @nrc and I would like to change "private imports" so that they behave precisely like all other items (e.g., use super::* should also get the use statements from the parent). Not sure how that affects this paragraph. Not much I guess except that prelude things might just be different from explicit use foo::bar imports.

[nikomatsakis]

ok, chatted briefly with @aturon on IRC, conclusion was that we should discuss again in the next @rust-lang/lang meeting :)

[jseyfried]

The set of traits in scope for a UFCS call ... is the same set we would use for method dispatch.

Makes sense -- for some reason I thought the trait search only applied to non-UFCS methods.

I think both @nrc and I would like to change "private imports" so that they behave precisely like all other items

As would @petrochenkov and I. I think the prelude should be treated differently from private imports (well, the prelude is already treated differently since it is shadowable by globs) in part because otherwise,

mod foo {
    pub type Result = ();
}

mod bar {
    // use super::*; // uncommenting this line would import `Result` from `super`'s prelude ...
    use foo::*;

    fn f() -> Result { () } // ... making this an ambiguity error.
}

It seems wrong for the prelude to be shadowable but for the names imported from another module's prelude to not be shadowable (and to shadow the original prelude).

@petrochenkov also argues for treating the prelude differently in #31726.

[nikomatsakis]

@jseyfried ok, I'm basically convinced that treating the prelude as a 'level above' in the scope makes sense.

[bors]

☔ The latest upstream changes (presumably #32141) made this pull request unmergeable. Please resolve the merge conflicts.

[jseyfried]

@nikomatsakis I amended this PR to treat the prelude as a level above in the scope hierarchy, i.e. to always allow prelude traits and only disallow shadowed glob-imported re-exports.

[nikomatsakis]

After discussing in @rust-lang/lang, decided to adopt this approach where prelude is a scope above the other scopes. Thanks @jseyfried for your patience as always!

[jseyfried]

@nikomatsakis no problem! This is ready to land pending your review (there's not much to review). After this lands, the rebase of #32167 will remove ModuleS's field shadowed_traits.

[nikomatsakis]

@bors r+

@jseyfried heh, when I looked at the code this morning, I was a bit surprised -- I expected a bit more. Anyway, going to work hard on getting to your other PRs now!

[bors]

📌 Commit ed8a2e2 has been approved by nikomatsakis

[bors]

⌛ Testing commit ed8a2e2 with merge 64a13a4...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild