[WIP] Traits in scope and def-site hygiene #65351

petrochenkov · 2019-10-13T00:45:40Z

This PR makes all traits defined in or imported into the module M to be in scope in that module for the purposes of method resolution, even if those traits or imports are produced by opaque macros.

We should probably redefine and reimplement the collection of traits in scope without hitting the issues described in the inline comments.
However, maybe it's not so urgent and we can make an issue and leave this as is for now? That will immediately unblock removal of gensyms and implementation of underscores in terms of def-site hygiene and unique macros.

cc #64623
r? @matthewjasper

…tion Ignoring macro 2.0 hygiene.

petrochenkov · 2019-10-13T00:59:27Z

src/librustc_resolve/late.rs

-                ).is_none() {
-                    continue
-                }
-                if self.r.resolve_ident_in_module_unadjusted(


So, what I don't like here is that we are conflating the strict part (determining the set of traits in scope) with the optimization part (pruning the traits that certainly don't fit).
For trait aliases we must perform the first part as well (we don't do that currently, as you can see below), but for them we cannot perform the second part (or at least let's assume that we cannot).

So, traits "from macros" should be rejected without using the ident.

petrochenkov · 2019-10-13T01:07:35Z

src/librustc_resolve/late.rs

-                let mut ident = ident;
-                if ident.span.glob_adjust(
-                    module.expansion,
-                    binding.span,


This is not the right span to determine whether the trait is in scope, it's a span of the whole import or the whole item, while we need to use a span of a single identifier, like span(Foo) in use Trait as Foo;.
Otherwise we can't correctly process imports constructed from parts, e.g.

// Currently on nightly #![feature(decl_macro)] mod m { pub trait Tr { fn method(&self) {} } impl Tr for u8 {} } macro gen_import($Br: ident) { use m::Tr as $Br; } gen_import!(Br); fn main() { 0u8.method(); // ERROR: no method named `method`, despite passing `Br` from the outside }

petrochenkov · 2019-10-13T01:10:21Z

src/librustc_resolve/late.rs

+            let is_candidate = binding.module().map_or(true, |trait_module| {
+                self.r.resolutions(trait_module).borrow().iter().any(|resolution| {
+                    let (&(assoc_ident, assoc_ns), _) = resolution;
+                    assoc_ns == ns && assoc_ident.name == ident.name


This is an almost perfect pruning heuristic in practice, traits rarely have different associated items with the same name in the same namespace, but different hygiene.
So, for optimization we can avoid dealing with hygiene.

petrochenkov · 2019-10-13T01:12:31Z

src/test/ui/hygiene/traits-in-scope-fixme.rs

+
+fn main() {
+    0u8.single(); // FIXME, should be an error, `Single` shouldn't be in scope here
+    0u8.glob(); // FIXME, should be an error, `Glob` shouldn't be in scope here


Curiously, we didn't have test cases covering imports of traits, where the traits are not produced by opaque macros (so their methods can be resolved), but the imports are produced by opaque macros (so the traits are not in scope).
So the PR didn't cause any regressions in the existing test suite until I added this test.

petrochenkov · 2019-10-16T08:11:16Z

I won't have time to work on this.

…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? `@matthewjasper`

…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? ``@matthewjasper``

…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? ```@matthewjasper```

petrochenkov added 2 commits October 13, 2019 01:46

resolve: Make traits to always be in scope for associated item resolu…

48fbbee

…tion Ignoring macro 2.0 hygiene.

Add a test demonstrating unhygienic behavior of traits in scope

745b239

rust-highfive assigned matthewjasper Oct 13, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2019

petrochenkov mentioned this pull request Oct 13, 2019

Remove last uses of gensyms #64623

Merged

petrochenkov commented Oct 13, 2019

View reviewed changes

petrochenkov closed this Oct 16, 2019

petrochenkov mentioned this pull request Jan 6, 2021

resolve: Simplify collection of traits in scope #80765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Traits in scope and def-site hygiene #65351

[WIP] Traits in scope and def-site hygiene #65351

petrochenkov commented Oct 13, 2019 •

edited

Loading

petrochenkov Oct 13, 2019

petrochenkov Oct 13, 2019

petrochenkov Oct 13, 2019 •

edited

Loading

petrochenkov Oct 13, 2019

petrochenkov Oct 13, 2019 •

edited

Loading

petrochenkov commented Oct 16, 2019

[WIP] Traits in scope and def-site hygiene #65351

[WIP] Traits in scope and def-site hygiene #65351

Conversation

petrochenkov commented Oct 13, 2019 • edited Loading

petrochenkov Oct 13, 2019

Choose a reason for hiding this comment

petrochenkov Oct 13, 2019

Choose a reason for hiding this comment

petrochenkov Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

petrochenkov Oct 13, 2019

Choose a reason for hiding this comment

petrochenkov Oct 13, 2019 • edited Loading

Choose a reason for hiding this comment

petrochenkov commented Oct 16, 2019

petrochenkov commented Oct 13, 2019 •

edited

Loading

petrochenkov Oct 13, 2019 •

edited

Loading

petrochenkov Oct 13, 2019 •

edited

Loading