Skip to content

Commit

Permalink
Prefer accessible paths in 'use' suggestions
Browse files Browse the repository at this point in the history
This fixes an issue with the following sample:

    mod foo {
	mod inaccessible {
	    pub struct X;
	}
	pub mod avail {
	    pub struct X;
	}
    }

    fn main() { X; }

Instead of suggesting both `use crate::foo::inaccessible::X;` and `use
crate::foo::avail::X;`, it should only suggest the latter.

It is done by trimming the list of suggestions from inaccessible paths
if accessible paths are present.

Visibility is checked with `is_accessible_from` now instead of being
hard-coded.

-

Some tests fixes are trivial, and others require a bit more explaining,
here are my comments:

src/test/ui/issues/issue-35675.stderr: Only needs to make the enum
public to have the suggestion make sense.

src/test/ui/issues/issue-42944.stderr: Importing the tuple struct won't
help because its constructor is not visible, so the attempted
constructor does not work. In that case, it's better not to suggest it.
The case where the constructor is public is covered in `issue-26545.rs`.
  • Loading branch information
da-x committed Jun 21, 2020
1 parent a39c778 commit fea5ab1
Show file tree
Hide file tree
Showing 13 changed files with 99 additions and 82 deletions.
65 changes: 43 additions & 22 deletions src/librustc_resolve/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ crate struct ImportSuggestion {
pub did: Option<DefId>,
pub descr: &'static str,
pub path: Path,
pub accessible: bool,
}

/// Adjust the impl span so that just the `impl` keyword is taken by removing
Expand Down Expand Up @@ -640,21 +641,32 @@ impl<'a> Resolver<'a> {
let mut candidates = Vec::new();
let mut seen_modules = FxHashSet::default();
let not_local_module = crate_name.name != kw::Crate;
let mut worklist = vec![(start_module, Vec::<ast::PathSegment>::new(), not_local_module)];
let mut worklist =
vec![(start_module, Vec::<ast::PathSegment>::new(), true, not_local_module)];

while let Some((in_module, path_segments, in_module_is_extern)) = worklist.pop() {
while let Some((in_module, path_segments, accessible, in_module_is_extern)) = worklist.pop()
{
// We have to visit module children in deterministic order to avoid
// instabilities in reported imports (#43552).
in_module.for_each_child(self, |this, ident, ns, name_binding| {
// avoid imports entirely
if name_binding.is_import() && !name_binding.is_extern_crate() {
return;
}

// avoid non-importable candidates as well
if !name_binding.is_importable() {
return;
}

let child_accessible =
accessible && this.is_accessible_from(name_binding.vis, parent_scope.module);

// do not venture inside inaccessible items of other crates
if in_module_is_extern && !child_accessible {
return;
}

// collect results based on the filter function
// avoid suggesting anything from the same module in which we are resolving
if ident.name == lookup_ident.name
Expand All @@ -673,22 +685,29 @@ impl<'a> Resolver<'a> {

segms.push(ast::PathSegment::from_ident(ident));
let path = Path { span: name_binding.span, segments: segms };
// the entity is accessible in the following cases:
// 1. if it's defined in the same crate, it's always
// accessible (since private entities can be made public)
// 2. if it's defined in another crate, it's accessible
// only if both the module is public and the entity is
// declared as public (due to pruning, we don't explore
// outside crate private modules => no need to check this)
if !in_module_is_extern || name_binding.vis == ty::Visibility::Public {
let did = match res {
Res::Def(DefKind::Ctor(..), did) => this.parent(did),
_ => res.opt_def_id(),
};
if candidates.iter().all(|v: &ImportSuggestion| v.did != did) {
candidates.push(ImportSuggestion { did, descr: res.descr(), path });
let did = match res {
Res::Def(DefKind::Ctor(..), did) => this.parent(did),
_ => res.opt_def_id(),
};

if child_accessible {
// Remove invisible match if exists
if let Some(idx) = candidates
.iter()
.position(|v: &ImportSuggestion| v.did == did && !v.accessible)
{
candidates.remove(idx);
}
}

if candidates.iter().all(|v: &ImportSuggestion| v.did != did) {
candidates.push(ImportSuggestion {
did,
descr: res.descr(),
path,
accessible: child_accessible,
});
}
}
}

Expand All @@ -701,20 +720,22 @@ impl<'a> Resolver<'a> {
let is_extern_crate_that_also_appears_in_prelude =
name_binding.is_extern_crate() && lookup_ident.span.rust_2018();

let is_visible_to_user =
!in_module_is_extern || name_binding.vis == ty::Visibility::Public;

if !is_extern_crate_that_also_appears_in_prelude && is_visible_to_user {
// add the module to the lookup
if !is_extern_crate_that_also_appears_in_prelude {
let is_extern = in_module_is_extern || name_binding.is_extern_crate();
// add the module to the lookup
if seen_modules.insert(module.def_id().unwrap()) {
worklist.push((module, path_segments, is_extern));
worklist.push((module, path_segments, child_accessible, is_extern));
}
}
}
})
}

// If only some candidates are accessible, take just them
if !candidates.iter().all(|v: &ImportSuggestion| !v.accessible) {
candidates = candidates.into_iter().filter(|x| x.accessible).collect();
}

candidates
}

Expand Down
7 changes: 6 additions & 1 deletion src/librustc_resolve/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,12 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
let path = Path { span: name_binding.span, segments: path_segments };
result = Some((
module,
ImportSuggestion { did: Some(def_id), descr: "module", path },
ImportSuggestion {
did: Some(def_id),
descr: "module",
path,
accessible: true,
},
));
} else {
// add the module to the lookup
Expand Down
6 changes: 1 addition & 5 deletions src/test/ui/hygiene/globs.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,10 @@ LL | | }
| |_____- in this macro invocation
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing one of these items
help: consider importing this function
|
LL | use bar::g;
|
LL | use foo::test2::test::g;
|
LL | use foo::test::g;
|

error[E0425]: cannot find function `f` in this scope
--> $DIR/globs.rs:61:12
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/issues/issue-26545.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
mod foo {
pub struct B(pub ());
}

mod baz {
fn foo() {
B(());
//~^ ERROR cannot find function, tuple struct or tuple variant `B` in this scope [E0425]
}
}

fn main() {}
14 changes: 14 additions & 0 deletions src/test/ui/issues/issue-26545.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0425]: cannot find function, tuple struct or tuple variant `B` in this scope
--> $DIR/issue-26545.rs:7:9
|
LL | B(());
| ^ not found in this scope
|
help: consider importing this tuple struct
|
LL | use foo::B;
|

error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-35675.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn qux() -> Some {
fn main() {}

mod x {
enum Enum {
pub enum Enum {
Variant1,
Variant2(),
Variant3(usize),
Expand Down
12 changes: 6 additions & 6 deletions src/test/ui/issues/issue-42944.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
mod foo {
pub struct B(());
pub struct Bx(());
}

mod bar {
use foo::B;
use foo::Bx;

fn foo() {
B(());
//~^ ERROR expected function, tuple struct or tuple variant, found struct `B` [E0423]
Bx(());
//~^ ERROR expected function, tuple struct or tuple variant, found struct `Bx` [E0423]
}
}

mod baz {
fn foo() {
B(());
//~^ ERROR cannot find function, tuple struct or tuple variant `B` in this scope [E0425]
Bx(());
//~^ ERROR cannot find function, tuple struct or tuple variant `Bx` in this scope [E0425]
}
}

Expand Down
14 changes: 7 additions & 7 deletions src/test/ui/issues/issue-42944.stderr
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
error[E0423]: expected function, tuple struct or tuple variant, found struct `B`
error[E0423]: expected function, tuple struct or tuple variant, found struct `Bx`
--> $DIR/issue-42944.rs:9:9
|
LL | B(());
| ^ constructor is not visible here due to private fields
LL | Bx(());
| ^^ constructor is not visible here due to private fields

error[E0425]: cannot find function, tuple struct or tuple variant `B` in this scope
error[E0425]: cannot find function, tuple struct or tuple variant `Bx` in this scope
--> $DIR/issue-42944.rs:16:9
|
LL | B(());
| ^ not found in this scope
LL | Bx(());
| ^^ not found in this scope
|
help: consider importing this tuple struct
|
LL | use foo::B;
LL | use foo::Bx;
|

error: aborting due to 2 previous errors
Expand Down
4 changes: 1 addition & 3 deletions src/test/ui/issues/issue-4366-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@ error[E0423]: expected function, found module `foo`
LL | foo();
| ^^^ not a function
|
help: consider importing one of these items instead
help: consider importing this function instead
|
LL | use foo::foo;
|
LL | use m1::foo;
|

error: aborting due to 2 previous errors

Expand Down
4 changes: 1 addition & 3 deletions src/test/ui/issues/issue-4366.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ error[E0425]: cannot find function `foo` in this scope
LL | fn sub() -> isize { foo(); 1 }
| ^^^ not found in this scope
|
help: consider importing one of these items
help: consider importing this function
|
LL | use foo::foo;
|
LL | use m1::foo;
|

error: aborting due to previous error

Expand Down
18 changes: 3 additions & 15 deletions src/test/ui/privacy/privacy-ns1.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,10 @@ help: a unit struct with a similar name exists
|
LL | Baz();
| ^^^
help: consider importing one of these items instead
|
LL | use foo1::Bar;
help: consider importing this function instead
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0425]: cannot find function, tuple struct or tuple variant `Bar` in this scope
--> $DIR/privacy-ns1.rs:51:5
Expand All @@ -33,14 +29,10 @@ help: a unit struct with a similar name exists
|
LL | Baz();
| ^^^
help: consider importing one of these items
|
LL | use foo1::Bar;
help: consider importing this function
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0412]: cannot find type `Bar` in this scope
--> $DIR/privacy-ns1.rs:52:17
Expand All @@ -55,14 +47,10 @@ help: a struct with a similar name exists
|
LL | let _x: Box<Baz>;
| ^^^
help: consider importing one of these items
help: consider importing this trait
|
LL | use foo1::Bar;
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0107]: wrong number of const arguments: expected 0, found 1
--> $DIR/privacy-ns1.rs:35:17
Expand Down
18 changes: 3 additions & 15 deletions src/test/ui/privacy/privacy-ns2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,10 @@ error[E0423]: expected function, tuple struct or tuple variant, found trait `Bar
LL | Bar();
| ^^^ not a function, tuple struct or tuple variant
|
help: consider importing one of these items instead
|
LL | use foo1::Bar;
help: consider importing this function instead
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0423]: expected function, tuple struct or tuple variant, found trait `Bar`
--> $DIR/privacy-ns2.rs:26:5
Expand All @@ -26,14 +22,10 @@ help: a unit struct with a similar name exists
|
LL | Baz();
| ^^^
help: consider importing one of these items instead
|
LL | use foo1::Bar;
help: consider importing this function instead
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0573]: expected type, found function `Bar`
--> $DIR/privacy-ns2.rs:43:14
Expand All @@ -45,14 +37,10 @@ help: use `=` if you meant to assign
|
LL | let _x = Bar();
| ^
help: consider importing one of these items instead
help: consider importing this trait instead
|
LL | use foo1::Bar;
|
LL | use foo2::Bar;
|
LL | use foo3::Bar;
|

error[E0603]: trait `Bar` is private
--> $DIR/privacy-ns2.rs:63:15
Expand Down
5 changes: 1 addition & 4 deletions src/test/ui/resolve/issue-21221-1.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,8 @@ LL | use mul1::Mul;
|
LL | use mul2::Mul;
|
LL | use mul3::Mul;
|
LL | use mul4::Mul;
LL | use std::ops::Mul;
|
and 2 other candidates

error[E0405]: cannot find trait `ThisTraitReallyDoesntExistInAnyModuleReally` in this scope
--> $DIR/issue-21221-1.rs:63:6
Expand Down

0 comments on commit fea5ab1

Please sign in to comment.