Improve suggestions for importing out-of-scope traits reexported as _

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

@@ -17,7 +17,7 @@ use rustc_session::utils::NativeLibKind;
 use rustc_session::{Session, StableCrateId};
 use rustc_span::hygiene::{ExpnHash, ExpnId};
 use rustc_span::source_map::{Span, Spanned};
-use rustc_span::symbol::Symbol;
+use rustc_span::symbol::{kw, Symbol};
 
 use rustc_data_structures::sync::Lrc;
 use smallvec::SmallVec;
@@ -295,6 +295,10 @@ pub fn provide(providers: &mut Providers) {
             use std::collections::vec_deque::VecDeque;
 
             let mut visible_parent_map: DefIdMap<DefId> = Default::default();
+            // This is a secondary visible_parent_map, storing the DefId of parents that re-export
+            // the child as `_`. Since we prefer parents that don't do this, merge this map at the
+            // end, only if we're missing any keys from the former.
+            let mut fallback_map: DefIdMap<DefId> = Default::default();
 
             // Issue 46112: We want the map to prefer the shortest
             // paths when reporting the path to an item. Therefore we
@@ -317,12 +321,17 @@ pub fn provide(providers: &mut Providers) {
                 bfs_queue.push_back(DefId { krate: cnum, index: CRATE_DEF_INDEX });
             }
 
-            let mut add_child = |bfs_queue: &mut VecDeque<_>, child: &Export, parent: DefId| {
-                if !child.vis.is_public() {
+            let mut add_child = |bfs_queue: &mut VecDeque<_>, export: &Export, parent: DefId| {
+                if !export.vis.is_public() {
                     return;
                 }
 
-                if let Some(child) = child.res.opt_def_id() {
+                if let Some(child) = export.res.opt_def_id() {
+                    if export.ident.name == kw::Underscore {
+                        fallback_map.insert(child, parent);
+                        return;
+                    }
+
                     match visible_parent_map.entry(child) {
                         Entry::Occupied(mut entry) => {
                             // If `child` is defined in crate `cnum`, ensure
@@ -345,6 +354,12 @@ pub fn provide(providers: &mut Providers) {
                 }
             }
 
+            // Fill in any missing entries with the (less preferable) path ending in `::_`.
+            // We still use this path in a diagnostic that suggests importing `::*`.
+            for (child, parent) in fallback_map {
+                visible_parent_map.entry(child).or_insert(parent);
+            }
+
             visible_parent_map
         },
 

compiler/rustc_middle/src/ty/print/pretty.rs

@@ -319,6 +319,9 @@ pub trait PrettyPrinter<'tcx>:
     ///
     /// `callers` is a chain of visible_parent's leading to `def_id`,
     /// to support cycle detection during recursion.
+    ///
+    /// This method returns false if we can't print the visible path, so
+    /// `print_def_path` can fall back on the item's real definition path.
     fn try_print_visible_def_path_recur(
         mut self,
         def_id: DefId,
@@ -405,19 +408,7 @@ pub trait PrettyPrinter<'tcx>:
             Some(parent) => parent,
             None => return Ok((self, false)),
         };
-        if callers.contains(&visible_parent) {
-            return Ok((self, false));
-        }
-        callers.push(visible_parent);
-        // HACK(eddyb) this bypasses `path_append`'s prefix printing to avoid
-        // knowing ahead of time whether the entire path will succeed or not.
-        // To support printers that do not implement `PrettyPrinter`, a `Vec` or
-        // linked list on the stack would need to be built, before any printing.
-        match self.try_print_visible_def_path_recur(visible_parent, callers)? {
-            (cx, false) => return Ok((cx, false)),
-            (cx, true) => self = cx,
-        }
-        callers.pop();
+
         let actual_parent = self.tcx().parent(def_id);
         debug!(
             "try_print_visible_def_path: visible_parent={:?} actual_parent={:?}",
@@ -463,14 +454,21 @@ pub trait PrettyPrinter<'tcx>:
             // `visible_parent_map`), looking for the specific child we currently have and then
             // have access to the re-exported name.
             DefPathData::TypeNs(ref mut name) if Some(visible_parent) != actual_parent => {
+                // Item might be re-exported several times, but filter for the one
+                // that's public and whose identifier isn't `_`.
                 let reexport = self
                     .tcx()
                     .item_children(visible_parent)
                     .iter()
-                    .find(|child| child.res.opt_def_id() == Some(def_id))
+                    .filter(|child| child.res.opt_def_id() == Some(def_id))
+                    .find(|child| child.vis.is_public() && child.ident.name != kw::Underscore)
                     .map(|child| child.ident.name);
-                if let Some(reexport) = reexport {
-                    *name = reexport;
+
+                if let Some(new_name) = reexport {
+                    *name = new_name;
+                } else {
+                    // There is no name that is public and isn't `_`, so bail.
+                    return Ok((self, false));
                 }
             }
             // Re-exported `extern crate` (#43189).
@@ -481,6 +479,20 @@ pub trait PrettyPrinter<'tcx>:
         }
         debug!("try_print_visible_def_path: data={:?}", data);
 
+        if callers.contains(&visible_parent) {
+            return Ok((self, false));
+        }
+        callers.push(visible_parent);
+        // HACK(eddyb) this bypasses `path_append`'s prefix printing to avoid
+        // knowing ahead of time whether the entire path will succeed or not.
+        // To support printers that do not implement `PrettyPrinter`, a `Vec` or
+        // linked list on the stack would need to be built, before any printing.
+        match self.try_print_visible_def_path_recur(visible_parent, callers)? {
+            (cx, false) => return Ok((cx, false)),
+            (cx, true) => self = cx,
+        }
+        callers.pop();
+
         Ok((self.path_append(Ok, &DisambiguatedDefPathData { data, disambiguator: 0 })?, true))
     }
 

compiler/rustc_typeck/src/check/method/suggest.rs

@@ -12,7 +12,7 @@ use rustc_hir::{ExprKind, Node, QPath};
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_middle::ty::fast_reject::{simplify_type, SimplifyParams, StripReferences};
 use rustc_middle::ty::print::with_crate_prefix;
-use rustc_middle::ty::{self, ToPredicate, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::{self, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable};
 use rustc_span::lev_distance;
 use rustc_span::symbol::{kw, sym, Ident};
 use rustc_span::{source_map, FileName, MultiSpan, Span, Symbol};
@@ -1310,25 +1310,66 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         mut msg: String,
         candidates: Vec<DefId>,
     ) {
+        let parent_map = self.tcx.visible_parent_map(());
+
+        // Separate out candidates that must be imported with a glob, because they are named `_`
+        // and cannot be referred with their identifier.
+        let (candidates, globs): (Vec<_>, Vec<_>) = candidates.into_iter().partition(|trait_did| {
+            if let Some(parent_did) = parent_map.get(trait_did) {
+                // If the item is re-exported as `_`, we should suggest a glob-import instead.
+                if Some(*parent_did) != self.tcx.parent(*trait_did)
+                    && self
+                        .tcx
+                        .item_children(*parent_did)
+                        .iter()
+                        .filter(|child| child.res.opt_def_id() == Some(*trait_did))
+                        .all(|child| child.ident.name == kw::Underscore)
+                {
+                    return false;
+                }
+            }
+
+            true
+        });
+
         let module_did = self.tcx.parent_module(self.body_id);
         let (span, found_use) = find_use_placement(self.tcx, module_did);
         if let Some(span) = span {
-            let path_strings = candidates.iter().map(|did| {
+            let path_strings = candidates.iter().map(|trait_did| {
                 // Produce an additional newline to separate the new use statement
                 // from the directly following item.
                 let additional_newline = if found_use { "" } else { "\n" };
                 format!(
                     "use {};\n{}",
-                    with_crate_prefix(|| self.tcx.def_path_str(*did)),
+                    with_crate_prefix(|| self.tcx.def_path_str(*trait_did)),
                     additional_newline
                 )
             });
 
-            err.span_suggestions(span, &msg, path_strings, Applicability::MaybeIncorrect);
+            let glob_path_strings = globs.iter().map(|trait_did| {
+                let parent_did = parent_map.get(trait_did).unwrap();
+
+                // Produce an additional newline to separate the new use statement
+                // from the directly following item.
+                let additional_newline = if found_use { "" } else { "\n" };
+                format!(
+                    "use {}::*; // trait {}\n{}",
+                    with_crate_prefix(|| self.tcx.def_path_str(*parent_did)),
+                    self.tcx.item_name(*trait_did),
+                    additional_newline
+                )
+            });
+
+            err.span_suggestions(
+                span,
+                &msg,
+                path_strings.chain(glob_path_strings),
+                Applicability::MaybeIncorrect,
+            );
         } else {
-            let limit = if candidates.len() == 5 { 5 } else { 4 };
+            let limit = if candidates.len() + globs.len() == 5 { 5 } else { 4 };
             for (i, trait_did) in candidates.iter().take(limit).enumerate() {
-                if candidates.len() > 1 {
+                if candidates.len() + globs.len() > 1 {
                     msg.push_str(&format!(
                         "\ncandidate #{}: `use {};`",
                         i + 1,
@@ -1341,8 +1382,28 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     ));
                 }
             }
+            for (i, trait_did) in
+                globs.iter().take(limit.saturating_sub(candidates.len())).enumerate()
+            {
+                let parent_did = parent_map.get(trait_did).unwrap();
+
+                if candidates.len() + globs.len() > 1 {
+                    msg.push_str(&format!(
+                        "\ncandidate #{}: `use {}::*; // trait {}`",
+                        candidates.len() + i + 1,
+                        with_crate_prefix(|| self.tcx.def_path_str(*parent_did)),
+                        self.tcx.item_name(*trait_did),
+                    ));
+                } else {
+                    msg.push_str(&format!(
+                        "\n`use {}::*; // trait {}`",
+                        with_crate_prefix(|| self.tcx.def_path_str(*parent_did)),
+                        self.tcx.item_name(*trait_did),
+                    ));
+                }
+            }
             if candidates.len() > limit {
-                msg.push_str(&format!("\nand {} others", candidates.len() - limit));
+                msg.push_str(&format!("\nand {} others", candidates.len() + globs.len() - limit));
             }
             err.note(&msg);
         }

src/test/ui/imports/auxiliary/overlapping_pub_trait_source.rs

@@ -0,0 +1,13 @@
+/* This crate declares an item as both `prelude::*` and `m::Tr`.
+ * The compiler should always suggest `m::Tr`. */
+
+pub struct S;
+
+pub mod prelude {
+    pub use crate::m::Tr as _;
+}
+
+pub mod m {
+    pub trait Tr { fn method(&self); }
+    impl Tr for crate::S { fn method(&self) {} }
+}

src/test/ui/imports/auxiliary/unnamed_pub_trait_source.rs

@@ -0,0 +1,13 @@
+/* This crate declares an item that is unnamed.
+ * Its only public path is through `prelude::*`. */
+
+pub struct S;
+
+mod m {
+    pub trait Tr { fn method(&self); }
+    impl Tr for crate::S { fn method(&self) {} }
+}
+
+pub mod prelude {
+    pub use crate::m::Tr as _;
+}

src/test/ui/imports/overlapping_pub_trait.rs

@@ -0,0 +1,15 @@
+// aux-build:overlapping_pub_trait_source.rs
+
+/*
+ * This crate declares two public paths, `m::Tr` and `prelude::_`. Make sure we prefer the former.
+ */
+extern crate overlapping_pub_trait_source;
+
+fn main() {
+    //~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
+    //~| SUGGESTION overlapping_pub_trait_source::m::Tr
+    use overlapping_pub_trait_source::S;
+    S.method();
+    //~^ ERROR no method named `method` found for struct `S` in the current scope [E0599]
+    //~| HELP items from traits can only be used if the trait is in scope
+}

src/test/ui/imports/overlapping_pub_trait.stderr

@@ -0,0 +1,20 @@
+error[E0599]: no method named `method` found for struct `S` in the current scope
+  --> $DIR/overlapping_pub_trait.rs:12:7
+   |
+LL |     S.method();
+   |       ^^^^^^ method not found in `S`
+   |
+  ::: $DIR/auxiliary/overlapping_pub_trait_source.rs:11:23
+   |
+LL |     pub trait Tr { fn method(&self); }
+   |                       ------ the method is available for `S` here
+   |
+   = help: items from traits can only be used if the trait is in scope
+help: the following trait is implemented but not in scope; perhaps add a `use` for it:
+   |
+LL | use overlapping_pub_trait_source::m::Tr;
+   |
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0599`.

src/test/ui/imports/unnamed_pub_trait.rs

@@ -0,0 +1,16 @@
+// aux-build:unnamed_pub_trait_source.rs
+
+/*
+ * This crate declares an unnameable public path for our item. Make sure we don't suggest
+ * importing it by name, and instead we suggest importing it by glob.
+ */
+extern crate unnamed_pub_trait_source;
+
+fn main() {
+    //~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
+    //~| SUGGESTION unnamed_pub_trait_source::prelude::*; // trait Tr
+    use unnamed_pub_trait_source::S;
+    S.method();
+    //~^ ERROR no method named `method` found for struct `S` in the current scope [E0599]
+    //~| HELP items from traits can only be used if the trait is in scope
+}

src/test/ui/imports/unnamed_pub_trait.stderr

@@ -0,0 +1,20 @@
+error[E0599]: no method named `method` found for struct `S` in the current scope
+  --> $DIR/unnamed_pub_trait.rs:13:7
+   |
+LL |     S.method();
+   |       ^^^^^^ method not found in `S`
+   |
+  ::: $DIR/auxiliary/unnamed_pub_trait_source.rs:7:23
+   |
+LL |     pub trait Tr { fn method(&self); }
+   |                       ------ the method is available for `S` here
+   |
+   = help: items from traits can only be used if the trait is in scope
+help: the following trait is implemented but not in scope; perhaps add a `use` for it:
+   |
+LL | use unnamed_pub_trait_source::prelude::*; // trait Tr
+   |
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0599`.