Rollup merge of #91412 - compiler-errors:issue-86035, r=oli-obk

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

1. Fix up the `parent_map` query to prefer visible parents that _don't_ export items with the name `_`.
    * I'm not sure if I modified this query properly. Not sure if we want to check for other idents than `kw::Underscore`.
    * This also has the side-effect of not doing BFS on any modules re-exported as `_`, but I think that's desirable here too (or else we get suggestions for paths like `a::_::b` like in [this doctest example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d9505ea45bb80adf40bb991298f952be)).
2. Bail in `try_print_visible_def_path` if the `def_id` is re-exported as `_`, which will fall back to printing the def-id's real (definition) path.
    * Side-effect of this is that we print paths that are not actually public, but it seems we already sometimes suggest `use`ing paths that are private anyways. See [this doctest example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bad513ed3241f8ff87579eed8046ad10) for demonstration of current behavior.
3. Suggest a glob-import (for example `my_library::prelude::*`) if the trait in question is only pub-exported as `_`, as a fallback.
    ```
    use foo::bar::prelude::*; // trait MyTrait
    ```
    * I think this is good fallback behavior to suggest instead of doing nothing. Thanks to the original issue filer for suggesting this.

I was somewhat opinionated about behaviors in this PR, and I'm totally open to limiting the impact of my changes or only landing parts of this. Happy to receive feedback if there are better ways to the same end.

Fixes #86035

Author: matthiaskrgr

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