Skip to content

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

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 10 additions & 26 deletions src/librustc_resolve/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2038,35 +2038,19 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}

for &(trait_name, binding) in traits.as_ref().unwrap().iter() {
// Traits have pseudo-modules that can be used to search for the given ident.
if let Some(module) = binding.module() {
let mut ident = ident;
if ident.span.glob_adjust(
module.expansion,
binding.span,
Copy link
Contributor Author

@petrochenkov petrochenkov Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

).is_none() {
continue
}
if self.r.resolve_ident_in_module_unadjusted(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ModuleOrUniformRoot::Module(module),
ident,
ns,
&self.parent_scope,
false,
module.span,
).is_ok() {
let import_ids = self.find_transitive_imports(&binding.kind, trait_name);
let trait_def_id = module.def_id().unwrap();
found_traits.push(TraitCandidate { def_id: trait_def_id, import_ids });
}
} else if let Res::Def(DefKind::TraitAlias, _) = binding.res() {
// For now, just treat all trait aliases as possible candidates, since we don't
// know if the ident is somewhere in the transitive bounds.
// Prune the trait list on best effort basis. We reject traits not having associated
// items with the necessary name and namespace. We don't reject trait aliases because
// we don't have access to their associted items.
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
});
if is_candidate {
let import_ids = self.find_transitive_imports(&binding.kind, trait_name);
let trait_def_id = binding.res().def_id();
found_traits.push(TraitCandidate { def_id: trait_def_id, import_ids });
} else {
bug!("candidate is not trait or trait alias?")
}
}
}
Expand Down
29 changes: 29 additions & 0 deletions src/test/ui/hygiene/traits-in-scope-fixme.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// check-pass

#![feature(decl_macro)]

mod single {
pub trait Single {
fn single(&self) {}
}

impl Single for u8 {}
}
mod glob {
pub trait Glob {
fn glob(&self) {}
}

impl Glob for u8 {}
}

macro gen_imports() {
use single::Single;
use glob::*;
}
gen_imports!();

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
Copy link
Contributor Author

@petrochenkov petrochenkov Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}