Fix invalid suggestions and misleading notes for imports

compiler/rustc_resolve/src/diagnostics/impls.rs

@@ -2496,6 +2496,39 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         Some(path)
     }
 
+    /// Returns the original import's source path if it resolves to `source_res` from the use site.
+    fn import_source_suggestion_path(
+        &self,
+        import: Import<'ra>,
+        source: Ident,
+        source_res: Res,
+        parent_scope: &ParentScope<'ra>,
+    ) -> Option<Vec<Ident>> {
+        let path = Path {
+            span: source.span,
+            segments: import
+                .module_path
+                .iter()
+                .filter(|segment| segment.ident.name != kw::PathRoot)
+                .map(|segment| segment.ident.clone())
+                .chain(std::iter::once(source))
+                .map(ast::PathSegment::from_ident)
+                .collect(),
+        };
+        let segments = Segment::from_path(&path);
+        let resolves_to_source =
+            match self.cm().maybe_resolve_path(&segments, source_res.ns(), parent_scope, None) {
+                PathResult::NonModule(partial_res) => partial_res.full_res() == Some(source_res),
+                PathResult::Module(ModuleOrUniformRoot::Module(module)) => {
+                    module.res() == Some(source_res)
+                }
+                PathResult::Module(_) | PathResult::Indeterminate | PathResult::Failed { .. } => {
+                    false
+                }
+            };
+        resolves_to_source.then(|| path.segments.iter().map(|segment| segment.ident).collect())
+    }
+
     fn shorten_candidate_path(
         &self,
         suggestion: &mut ImportSuggestion,
@@ -2712,111 +2745,101 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             }
         }
 
+        let has_direct_suggestion = !sugg_paths.is_empty();
         // Print the whole import chain to make it easier to see what happens.
         let first_binding = decl;
-        let mut next_binding = Some(decl);
+        let mut next_binding = Some((decl, false));
         let mut next_ident = ident;
-        while let Some(binding) = next_binding {
+        while let Some((binding, path_to_binding_accessible)) = next_binding {
             let name = next_ident;
             next_binding = match binding.kind {
                 _ if res == Res::Err => None,
-                DeclKind::Import { source_decl, import, .. } => match import.kind {
-                    _ if source_decl.span.is_dummy() => None,
-                    ImportKind::Single { source, .. } => {
-                        next_ident = source;
-                        Some(source_decl)
-                    }
-                    ImportKind::Glob { .. }
-                    | ImportKind::MacroUse { .. }
-                    | ImportKind::MacroExport => Some(source_decl),
-                    ImportKind::ExternCrate { .. } => None,
-                },
-                _ => None,
-            };
-
-            match binding.kind {
+                DeclKind::Import { source_decl, .. } if source_decl.span.is_dummy() => None,
                 DeclKind::Import { source_decl, import, .. } => {
-                    let through_reexport = !matches!(source_decl.kind, DeclKind::Def(..));
-                    let uses_relative_path = import
-                        .module_path
-                        .first()
-                        .is_some_and(|seg| matches!(seg.ident.name, kw::SelfLower | kw::Super));
-                    let res_def_id = res.opt_def_id();
-                    let path = if uses_relative_path {
-                        // A path recovered from `self`/`super` is only useful if both the
-                        // target and every module segment can be named from the failing use site.
-                        let module_path = if let Some(ModuleOrUniformRoot::Module(module)) =
-                            import.imported_module.get()
-                            && module.is_local()
-                            && let Some(module_path) = self.module_path_names(module)
-                            && let Some(mut def_id) = module.opt_def_id()
-                            && res_def_id.is_none_or(|def_id| {
-                                self.is_accessible_from(
-                                    self.tcx.visibility(def_id),
-                                    parent_scope.module,
-                                )
-                            }) {
-                            // `module_path_names` tells us the resolved module's canonical path.
-                            // Before suggesting that path from the failing use site, make sure
-                            // every segment in it can actually be named from there.
-                            let mut visible_from_use_site = true;
-                            while let Some(parent) = self.tcx.opt_parent(def_id) {
-                                if !self.is_accessible_from(
-                                    self.tcx.visibility(def_id),
-                                    parent_scope.module,
-                                ) {
-                                    visible_from_use_site = false;
-                                    break;
-                                }
-                                if parent.is_top_level_module() {
-                                    break;
-                                }
-                                def_id = parent;
+                    let source = match import.kind {
+                        ImportKind::Single { source, .. } => Some(source),
+                        ImportKind::Glob { .. }
+                        | ImportKind::MacroUse { .. }
+                        | ImportKind::MacroExport => Some(next_ident),
+                        ImportKind::ExternCrate { .. } => None,
+                    };
+                    if let Some(source) = source {
+                        next_ident = source;
+                        let source_res = source_decl.res();
+                        let through_reexport = source_decl.is_import();
+                        let mut path_is_accessible = has_direct_suggestion && !through_reexport;
+                        if let Some(path) = self.import_source_suggestion_path(
+                            import,
+                            source,
+                            source_res,
+                            &parent_scope,
+                        ) {
+                            path_is_accessible = true;
+                            sugg_paths.push((path, through_reexport));
+                        } else if !has_direct_suggestion && let Some(ns) = source_res.ns() {
+                            let candidate = self
+                                .lookup_import_candidates(source, ns, &parent_scope, |res| {
+                                    res == source_res
+                                })
+                                .into_iter()
+                                .filter(|candidate| candidate.accessible)
+                                .map(|mut candidate| {
+                                    self.shorten_candidate_path(
+                                        &mut candidate,
+                                        parent_scope.module,
+                                    );
+                                    candidate
+                                })
+                                .min_by_key(|candidate| {
+                                    (
+                                        candidate.path.segments.len(),
+                                        candidate.path.segments[0].ident.name == sym::core,
+                                    )
+                                });
+                            if let Some(candidate) = candidate {
+                                // The same item may have several re-exports. Only label this one
+                                // if the suggested path resolves to its binding.
+                                let segments = Segment::from_path(&candidate.path);
+                                path_is_accessible = if let Some((last, prefix)) =
+                                    segments.split_last()
+                                    && let PathResult::Module(module) = self
+                                        .cm()
+                                        .maybe_resolve_path(prefix, None, &parent_scope, None)
+                                {
+                                    self.cm()
+                                        .maybe_resolve_ident_in_module(
+                                            module,
+                                            last.ident,
+                                            ns,
+                                            &parent_scope,
+                                            None,
+                                        )
+                                        .is_ok_and(|decl| decl == source_decl)
+                                } else {
+                                    false
+                                };
+                                let path =
+                                    candidate.path.segments.iter().map(|seg| seg.ident).collect();
+                                sugg_paths.push((path, candidate.via_import));
                             }
-                            if visible_from_use_site { Some(module_path) } else { None }
-                        } else {
-                            None
-                        };
-
-                        module_path.map(|module_path| {
-                            // `import.module_path` is relative to the import's module, not to the
-                            // failing use site.
-                            let mut path = Path {
-                                span: ident.span,
-                                segments: module_path
-                                    .into_iter()
-                                    .chain(std::iter::once(ident.name))
-                                    .map(|name| {
-                                        ast::PathSegment::from_ident(Ident::with_dummy_span(name))
-                                    })
-                                    .collect(),
-                            };
-                            self.shorten_import_path(res_def_id, &mut path, parent_scope.module);
-                            path.segments.iter().map(|seg| seg.ident).collect()
-                        })
+                        }
+                        Some((source_decl, path_is_accessible))
                     } else {
-                        // Don't include `{{root}}` in suggestions - it's an internal symbol
-                        // that should never be shown to users.
-                        Some(
-                            import
-                                .module_path
-                                .iter()
-                                .filter(|seg| seg.ident.name != kw::PathRoot)
-                                .map(|seg| seg.ident.clone())
-                                .chain(std::iter::once(ident))
-                                .collect::<Vec<_>>(),
-                        )
-                    };
-                    if let Some(path) = path {
-                        sugg_paths.push((path, through_reexport));
+                        None
                     }
                 }
-                DeclKind::Def(..) => {}
-            }
+                _ => None,
+            };
             let first = binding == first_binding;
             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() {
+            if !first
+                // The same `Res` may be reachable through a different import binding.
+                && self.is_accessible_from(binding.vis(), parent_scope.module)
+                && path_to_binding_accessible
+                // A circular import chain can lead back to the failing import itself.
+                && !binding.span.contains(ident.span)
+            {
                 let desc = match binding.kind {
                     DeclKind::Import { .. } => "re-export",
                     _ => "directly",
@@ -2862,6 +2885,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     // imports. `tests/ui/imports/issue-55884-2.rs`
                     continue;
                 }
+                // The suggested path may use a different name than the private alias.
+                let ident = *sugg.last().expect("at least one segment");
                 let path = join_path_idents(sugg);
                 let sugg = if reexport {
                     diagnostics::ImportIdent::ThroughReExport { span: dedup_span, ident, path }

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

@@ -0,0 +1,22 @@
+//@ edition:2015
+//@ run-rustfix
+
+// Suggest a usable import path instead of a shadowed or circular re-export.
+
+#![allow(unused_imports)]
+
+mod options {
+    pub struct ParseOptions {}
+}
+
+mod parser {
+    pub use options::*;
+    // Private single import shadows public glob import, but arrives too late for initial
+    // resolution of `use parser::ParseOptions` because it depends on that resolution itself.
+    #[allow(hidden_glob_reexports)]
+    use ParseOptions;
+}
+
+pub use options::ParseOptions; //~ ERROR struct import `ParseOptions` is private
+
+fn main() {}

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

@@ -1,4 +1,10 @@
 //@ edition:2015
+//@ run-rustfix
+
+// Suggest a usable import path instead of a shadowed or circular re-export.
+
+#![allow(unused_imports)]
+
 mod options {
     pub struct ParseOptions {}
 }

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

@@ -1,26 +1,26 @@
 error[E0603]: struct import `ParseOptions` is private
-  --> $DIR/issue-55884-2.rs:14:17
+  --> $DIR/issue-55884-2.rs:20:17
    |
 LL | pub use parser::ParseOptions;
    |                 ^^^^^^^^^^^^ private struct import
    |
 note: the struct import `ParseOptions` is defined here...
-  --> $DIR/issue-55884-2.rs:11:9
+  --> $DIR/issue-55884-2.rs:17:9
    |
 LL |     use ParseOptions;
    |         ^^^^^^^^^^^^
 note: ...and refers to the struct import `ParseOptions` which is defined here...
-  --> $DIR/issue-55884-2.rs:14:9
+  --> $DIR/issue-55884-2.rs:20:9
    |
 LL | pub use parser::ParseOptions;
-   |         ^^^^^^^^^^^^^^^^^^^^ you could import this re-export
+   |         ^^^^^^^^^^^^^^^^^^^^
 note: ...and refers to the struct import `ParseOptions` which is defined here...
-  --> $DIR/issue-55884-2.rs:7:13
+  --> $DIR/issue-55884-2.rs:13:13
    |
 LL |     pub use options::*;
-   |             ^^^^^^^^^^ you could import this re-export
+   |             ^^^^^^^^^^
 note: ...and refers to the struct `ParseOptions` which is defined here
-  --> $DIR/issue-55884-2.rs:3:5
+  --> $DIR/issue-55884-2.rs:9:5
    |
 LL |     pub struct ParseOptions {}
    |     ^^^^^^^^^^^^^^^^^^^^^^^ you could import this directly

tests/ui/imports/private-import-alias-private-parent-issue-149418.rs

@@ -0,0 +1,28 @@
+//@ check-fail
+//@ edition: 2024
+
+// Private import aliases should not suggest paths through private parent modules.
+
+mod delicious_snacks {
+    use self::fruits::PEAR as fruit;
+
+    mod fruits {
+        pub const PEAR: &str = "Pear";
+        pub const APPLE: &str = "Apple";
+    }
+}
+
+mod delicious_snacks_without_self {
+    use fruits::PEAR as fruit;
+
+    mod fruits {
+        pub const PEAR: &str = "Pear";
+    }
+}
+
+fn main() {
+    let _ = delicious_snacks::fruit;
+    //~^ ERROR constant import `fruit` is private
+    let _ = delicious_snacks_without_self::fruit;
+    //~^ ERROR constant import `fruit` is private
+}

tests/ui/imports/private-import-alias-private-parent-issue-149418.stderr

@@ -0,0 +1,37 @@
+error[E0603]: constant import `fruit` is private
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:24:31
+   |
+LL |     let _ = delicious_snacks::fruit;
+   |                               ^^^^^ private constant import
+   |
+note: the constant import `fruit` is defined here...
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:7:9
+   |
+LL |     use self::fruits::PEAR as fruit;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+note: ...and refers to the constant `PEAR` which is defined here
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:10:9
+   |
+LL |         pub const PEAR: &str = "Pear";
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error[E0603]: constant import `fruit` is private
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:26:44
+   |
+LL |     let _ = delicious_snacks_without_self::fruit;
+   |                                            ^^^^^ private constant import
+   |
+note: the constant import `fruit` is defined here...
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:16:9
+   |
+LL |     use fruits::PEAR as fruit;
+   |         ^^^^^^^^^^^^^^^^^^^^^
+note: ...and refers to the constant `PEAR` which is defined here
+  --> $DIR/private-import-alias-private-parent-issue-149418.rs:19:9
+   |
+LL |         pub const PEAR: &str = "Pear";
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0603`.