Prefer accessible paths in 'use' suggestions

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

src/librustc_resolve/diagnostics.rs

@@ -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
@@ -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
@@ -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,
+                            });
+                        }
                     }
                 }
 
@@ -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
     }
 

src/librustc_resolve/late/diagnostics.rs

@@ -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

src/test/ui/hygiene/globs.stderr

@@ -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

src/test/ui/issues/issue-26545.rs

@@ -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() {}

src/test/ui/issues/issue-26545.stderr

@@ -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`.

src/test/ui/issues/issue-35675.rs

@@ -33,7 +33,7 @@ fn qux() -> Some {
 fn main() {}
 
 mod x {
-    enum Enum {
+    pub enum Enum {
         Variant1,
         Variant2(),
         Variant3(usize),

src/test/ui/issues/issue-42944.rs

@@ -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]
     }
 }
 

src/test/ui/issues/issue-42944.stderr

@@ -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

src/test/ui/issues/issue-4366-2.stderr

@@ -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
 

src/test/ui/issues/issue-4366.stderr

@@ -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
 

src/test/ui/privacy/privacy-ns1.stderr

@@ -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
@@ -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
@@ -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

src/test/ui/privacy/privacy-ns2.stderr

@@ -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
@@ -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
@@ -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

src/test/ui/resolve/issue-21221-1.stderr

@@ -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