Auto merge of rust-lang#118066 - estebank:structured-use-suggestion, r=b-naber

Structured `use` suggestion on privacy error

When encoutering a privacy error on an item through a re-export that is accessible in an alternative path, provide a structured suggestion with that path.

```
error[E0603]: module import `mem` is private
  --> $DIR/private-std-reexport-suggest-public.rs:4:14
   |
LL |     use foo::mem;
   |              ^^^ private module import
   |
note: the module import `mem` is defined here...
  --> $DIR/private-std-reexport-suggest-public.rs:8:9
   |
LL |     use std::mem;
   |         ^^^^^^^^
note: ...and refers to the module `mem` which is defined here
  --> $SRC_DIR/std/src/lib.rs:LL:COL
   |
   = note: you could import this
help: import `mem` through the re-export
   |
LL |     use std::mem;
   |         ~~~~~~~~
```

Fix rust-lang#42909.

Author: bors

compiler/rustc_resolve/src/diagnostics.rs

@@ -1697,6 +1697,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             struct_span_err!(self.tcx.sess, ident.span, E0603, "{} `{}` is private", descr, ident);
         err.span_label(ident.span, format!("private {descr}"));
 
+        let mut not_publicly_reexported = false;
         if let Some((this_res, outer_ident)) = outermost_res {
             let import_suggestions = self.lookup_import_candidates(
                 outer_ident,
@@ -1717,6 +1718,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             );
             // If we suggest importing a public re-export, don't point at the definition.
             if point_to_def && ident.span != outer_ident.span {
+                not_publicly_reexported = true;
                 err.span_label(
                     outer_ident.span,
                     format!("{} `{outer_ident}` is not publicly re-exported", this_res.descr()),
@@ -1749,10 +1751,51 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             }
         }
 
+        let mut sugg_paths = vec![];
+        if let Some(mut def_id) = res.opt_def_id() {
+            // We can't use `def_path_str` in resolve.
+            let mut path = vec![def_id];
+            while let Some(parent) = self.tcx.opt_parent(def_id) {
+                def_id = parent;
+                if !def_id.is_top_level_module() {
+                    path.push(def_id);
+                } else {
+                    break;
+                }
+            }
+            // We will only suggest importing directly if it is accessible through that path.
+            let path_names: Option<Vec<String>> = path
+                .iter()
+                .rev()
+                .map(|def_id| {
+                    self.tcx.opt_item_name(*def_id).map(|n| {
+                        if def_id.is_top_level_module() {
+                            "crate".to_string()
+                        } else {
+                            n.to_string()
+                        }
+                    })
+                })
+                .collect();
+            if let Some(def_id) = path.get(0)
+                && let Some(path) = path_names
+            {
+                if let Some(def_id) = def_id.as_local() {
+                    if self.effective_visibilities.is_directly_public(def_id) {
+                        sugg_paths.push((path, false));
+                    }
+                } else if self.is_accessible_from(self.tcx.visibility(def_id), parent_scope.module)
+                {
+                    sugg_paths.push((path, false));
+                }
+            }
+        }
+
         // Print the whole import chain to make it easier to see what happens.
         let first_binding = binding;
         let mut next_binding = Some(binding);
         let mut next_ident = ident;
+        let mut path = vec![];
         while let Some(binding) = next_binding {
             let name = next_ident;
             next_binding = match binding.kind {
@@ -1771,6 +1814,21 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                 _ => None,
             };
 
+            match binding.kind {
+                NameBindingKind::Import { import, .. } => {
+                    for segment in import.module_path.iter().skip(1) {
+                        path.push(segment.ident.to_string());
+                    }
+                    sugg_paths.push((
+                        path.iter()
+                            .cloned()
+                            .chain(vec![ident.to_string()].into_iter())
+                            .collect::<Vec<_>>(),
+                        true, // re-export
+                    ));
+                }
+                NameBindingKind::Res(_) | NameBindingKind::Module(_) => {}
+            }
             let first = binding == first_binding;
             let msg = format!(
                 "{and_refers_to}the {item} `{name}`{which} is defined here{dots}",
@@ -1782,7 +1840,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             let def_span = self.tcx.sess.source_map().guess_head_span(binding.span);
             let mut note_span = MultiSpan::from_span(def_span);
             if !first && binding.vis.is_public() {
-                note_span.push_span_label(def_span, "consider importing it directly");
+                let desc = match binding.kind {
+                    NameBindingKind::Import { .. } => "re-export",
+                    _ => "directly",
+                };
+                note_span.push_span_label(def_span, format!("you could import this {desc}"));
             }
             // Final step in the import chain, point out if the ADT is `non_exhaustive`
             // which is probably why this privacy violation occurred.
@@ -1796,6 +1858,29 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             }
             err.span_note(note_span, msg);
         }
+        // We prioritize shorter paths, non-core imports and direct imports over the alternatives.
+        sugg_paths.sort_by_key(|(p, reexport)| (p.len(), p[0] == "core", *reexport));
+        for (sugg, reexport) in sugg_paths {
+            if not_publicly_reexported {
+                break;
+            }
+            if sugg.len() <= 1 {
+                // A single path segment suggestion is wrong. This happens on circular imports.
+                // `tests/ui/imports/issue-55884-2.rs`
+                continue;
+            }
+            let path = sugg.join("::");
+            err.span_suggestion_verbose(
+                dedup_span,
+                format!(
+                    "import `{ident}` {}",
+                    if reexport { "through the re-export" } else { "directly" }
+                ),
+                path,
+                Applicability::MachineApplicable,
+            );
+            break;
+        }
 
         err.emit();
     }

tests/ui/imports/issue-55884-2.stderr

@@ -13,17 +13,21 @@ note: ...and refers to the struct import `ParseOptions` which is defined here...
   --> $DIR/issue-55884-2.rs:13:9
    |
 LL | pub use parser::ParseOptions;
-   |         ^^^^^^^^^^^^^^^^^^^^ consider importing it directly
+   |         ^^^^^^^^^^^^^^^^^^^^ you could import this re-export
 note: ...and refers to the struct import `ParseOptions` which is defined here...
   --> $DIR/issue-55884-2.rs:6:13
    |
 LL |     pub use options::*;
-   |             ^^^^^^^^^^ consider importing it directly
+   |             ^^^^^^^^^^ you could import this re-export
 note: ...and refers to the struct `ParseOptions` which is defined here
   --> $DIR/issue-55884-2.rs:2:5
    |
 LL |     pub struct ParseOptions {}
-   |     ^^^^^^^^^^^^^^^^^^^^^^^ consider importing it directly
+   |     ^^^^^^^^^^^^^^^^^^^^^^^ you could import this directly
+help: import `ParseOptions` through the re-export
+   |
+LL | pub use parser::ParseOptions;
+   |         ~~~~~~~~~~~~~~~~~~~~
 
 error: aborting due to 1 previous error
 

tests/ui/imports/private-std-reexport-suggest-public.fixed

@@ -0,0 +1,9 @@
+// run-rustfix
+#![allow(unused_imports)]
+fn main() {
+    use std::mem; //~ ERROR module import `mem` is private
+}
+
+pub mod foo {
+    use std::mem;
+}

tests/ui/imports/private-std-reexport-suggest-public.rs

@@ -0,0 +1,9 @@
+// run-rustfix
+#![allow(unused_imports)]
+fn main() {
+    use foo::mem; //~ ERROR module import `mem` is private
+}
+
+pub mod foo {
+    use std::mem;
+}

tests/ui/imports/private-std-reexport-suggest-public.stderr

@@ -0,0 +1,23 @@
+error[E0603]: module import `mem` is private
+  --> $DIR/private-std-reexport-suggest-public.rs:4:14
+   |
+LL |     use foo::mem;
+   |              ^^^ private module import
+   |
+note: the module import `mem` is defined here...
+  --> $DIR/private-std-reexport-suggest-public.rs:8:9
+   |
+LL |     use std::mem;
+   |         ^^^^^^^^
+note: ...and refers to the module `mem` which is defined here
+  --> $SRC_DIR/std/src/lib.rs:LL:COL
+   |
+   = note: you could import this directly
+help: import `mem` through the re-export
+   |
+LL |     use std::mem;
+   |         ~~~~~~~~
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0603`.

tests/ui/privacy/privacy2.stderr

@@ -19,7 +19,7 @@ note: ...and refers to the function `foo` which is defined here
   --> $DIR/privacy2.rs:16:1
    |
 LL | pub fn foo() {}
-   | ^^^^^^^^^^^^ consider importing it directly
+   | ^^^^^^^^^^^^ you could import this directly
 
 error: requires `sized` lang_item
 

tests/ui/proc-macro/disappearing-resolution.stderr

@@ -19,7 +19,11 @@ note: ...and refers to the derive macro `Empty` which is defined here
   --> $DIR/auxiliary/test-macros.rs:25:1
    |
 LL | pub fn empty_derive(_: TokenStream) -> TokenStream {
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ consider importing it directly
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ you could import this directly
+help: import `Empty` directly
+   |
+LL | use test_macros::Empty;
+   |     ~~~~~~~~~~~~~~~~~~
 
 error: aborting due to 2 previous errors