Rollup merge of #155213 - petrochenkov:importvis, r=mu001999

resolve: Make sure visibilities of import declarations make sense

That they are all ordered inside the module and not more private than the module itself.

The `import_decl_vis` logic is also reused when reporting `ambiguous_import_visibilities` lint.
Some asserts are hardened.
Some relevant tests are added.

Extracted from #154149.

Author: JonathanBrouwer

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -251,16 +251,15 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     pub(crate) fn build_reduced_graph_external(&self, module: ExternModule<'ra>) {
         let def_id = module.def_id();
         let children = self.tcx.module_children(def_id);
-        let parent_scope = ParentScope::module(module.to_module(), self.arenas);
         for (i, child) in children.iter().enumerate() {
-            self.build_reduced_graph_for_external_crate_res(child, parent_scope, i, None)
+            self.build_reduced_graph_for_external_crate_res(child, module, i, None)
         }
         for (i, child) in
             self.cstore().ambig_module_children_untracked(self.tcx, def_id).enumerate()
         {
             self.build_reduced_graph_for_external_crate_res(
                 &child.main,
-                parent_scope,
+                module,
                 children.len() + i,
                 Some(&child.second),
             )
@@ -271,11 +270,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     fn build_reduced_graph_for_external_crate_res(
         &self,
         child: &ModChild,
-        parent_scope: ParentScope<'ra>,
+        parent: ExternModule<'ra>,
         child_index: usize,
         ambig_child: Option<&ModChild>,
     ) {
-        let parent = parent_scope.module.expect_extern();
         let child_span = |this: &Self, reexport_chain: &[Reexport], res: def::Res<_>| {
             this.def_span(
                 reexport_chain
@@ -288,7 +286,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         let ident = IdentKey::new(orig_ident);
         let span = child_span(self, reexport_chain, res);
         let res = res.expect_non_local();
-        let expansion = parent_scope.expansion;
+        let expansion = LocalExpnId::ROOT;
         let ambig = ambig_child.map(|ambig_child| {
             let ModChild { ident: _, res, vis, ref reexport_chain } = *ambig_child;
             let span = child_span(self, reexport_chain, res);

compiler/rustc_resolve/src/ident.rs

@@ -5,7 +5,6 @@ use Namespace::*;
 use rustc_ast::{self as ast, NodeId};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
-use rustc_middle::ty::Visibility;
 use rustc_middle::{bug, span_bug};
 use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
 use rustc_session::parse::feature_err;
@@ -24,9 +23,9 @@ use crate::late::{
 use crate::macros::{MacroRulesScope, sub_namespace_match};
 use crate::{
     AmbiguityError, AmbiguityKind, AmbiguityWarning, BindingKey, CmResolver, Decl, DeclKind,
-    Determinacy, Finalize, IdentKey, ImportKind, LateDecl, LocalModule, Module, ModuleKind,
-    ModuleOrUniformRoot, ParentScope, PathResult, PrivacyError, Res, ResolutionError, Resolver,
-    Scope, ScopeSet, Segment, Stage, Symbol, Used, errors,
+    Determinacy, Finalize, IdentKey, ImportKind, ImportSummary, LateDecl, LocalModule, Module,
+    ModuleKind, ModuleOrUniformRoot, ParentScope, PathResult, PrivacyError, Res, ResolutionError,
+    Resolver, Scope, ScopeSet, Segment, Stage, Symbol, Used, errors,
 };
 
 #[derive(Copy, Clone)]
@@ -485,11 +484,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                         // We do not need to report them if we are either in speculative resolution,
                         // or in late resolution when everything is already imported and expanded
                         // and no ambiguities exist.
-                        let import_vis = match finalize {
+                        let import = match finalize {
                             None | Some(Finalize { stage: Stage::Late, .. }) => {
                                 return ControlFlow::Break(Ok(decl));
                             }
-                            Some(Finalize { import_vis, .. }) => import_vis,
+                            Some(Finalize { import, .. }) => import,
                         };
 
                         if let Some(&(innermost_decl, _)) = innermost_results.first() {
@@ -503,7 +502,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                 decl,
                                 scope,
                                 &innermost_results,
-                                import_vis,
+                                import,
                             ) {
                                 // No need to search for more potential ambiguities, one is enough.
                                 return ControlFlow::Break(Ok(innermost_decl));
@@ -790,19 +789,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         decl: Decl<'ra>,
         scope: Scope<'ra>,
         innermost_results: &[(Decl<'ra>, Scope<'ra>)],
-        import_vis: Option<Visibility>,
+        import: Option<ImportSummary>,
     ) -> bool {
         let (innermost_decl, innermost_scope) = innermost_results[0];
         let (res, innermost_res) = (decl.res(), innermost_decl.res());
         let ambig_vis = if res != innermost_res {
             None
-        } else if let Some(import_vis) = import_vis
-            && let min =
-                (|d: Decl<'_>| d.vis().min(import_vis.to_def_id(), self.tcx).expect_local())
-            && let (min1, min2) = (min(decl), min(innermost_decl))
-            && min1 != min2
+        } else if let Some(import) = import
+            && let vis1 = self.import_decl_vis(decl, import)
+            && let vis2 = self.import_decl_vis(innermost_decl, import)
+            && vis1 != vis2
         {
-            Some((min1, min2))
+            Some((vis1, vis2))
         } else {
             return false;
         };

compiler/rustc_resolve/src/imports.rs

@@ -37,8 +37,9 @@ use crate::errors::{
 use crate::ref_mut::CmCell;
 use crate::{
     AmbiguityError, BindingKey, CmResolver, Decl, DeclData, DeclKind, Determinacy, Finalize,
-    IdentKey, ImportSuggestion, LocalModule, ModuleOrUniformRoot, ParentScope, PathResult, PerNS,
-    Res, ResolutionError, Resolver, ScopeSet, Segment, Used, module_to_string, names_to_string,
+    IdentKey, ImportSuggestion, ImportSummary, LocalModule, ModuleOrUniformRoot, ParentScope,
+    PathResult, PerNS, Res, ResolutionError, Resolver, ScopeSet, Segment, Used, module_to_string,
+    names_to_string,
 };
 
 /// A potential import declaration in the process of being planted into a module.
@@ -267,6 +268,14 @@ impl<'ra> ImportData<'ra> {
             ImportKind::MacroExport => Reexport::MacroExport,
         }
     }
+
+    fn summary(&self) -> ImportSummary {
+        ImportSummary {
+            vis: self.vis,
+            nearest_parent_mod: self.parent_scope.module.nearest_parent_mod().expect_local(),
+            is_single: matches!(self.kind, ImportKind::Single { .. }),
+        }
+    }
 }
 
 /// Records information about the resolution of a name in a namespace of a module.
@@ -327,9 +336,9 @@ struct UnresolvedImportError {
 
 // Reexports of the form `pub use foo as bar;` where `foo` is `extern crate foo;`
 // are permitted for backward-compatibility under a deprecation lint.
-fn pub_use_of_private_extern_crate_hack(import: Import<'_>, decl: Decl<'_>) -> Option<NodeId> {
-    match (&import.kind, &decl.kind) {
-        (ImportKind::Single { .. }, DeclKind::Import { import: decl_import, .. })
+fn pub_use_of_private_extern_crate_hack(import: ImportSummary, decl: Decl<'_>) -> Option<NodeId> {
+    match (import.is_single, decl.kind) {
+        (true, DeclKind::Import { import: decl_import, .. })
             if let ImportKind::ExternCrate { id, .. } = decl_import.kind
                 && import.vis.is_public() =>
         {
@@ -361,31 +370,50 @@ fn remove_same_import<'ra>(d1: Decl<'ra>, d2: Decl<'ra>) -> (Decl<'ra>, Decl<'ra
 }
 
 impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
+    pub(crate) fn import_decl_vis(&self, decl: Decl<'ra>, import: ImportSummary) -> Visibility {
+        assert!(import.vis.is_accessible_from(import.nearest_parent_mod, self.tcx));
+        let decl_vis = decl.vis();
+        if decl_vis.is_at_least(import.vis, self.tcx) {
+            // Ordered, import is less visible than the imported declaration, or the same,
+            // use the import's visibility.
+            import.vis
+        } else if decl_vis.is_accessible_from(import.nearest_parent_mod, self.tcx) {
+            // Ordered, imported declaration is less visible than the import, but is still visible
+            // from the current module, use the declaration's visibility.
+            assert!(import.vis.is_at_least(decl_vis, self.tcx));
+            if pub_use_of_private_extern_crate_hack(import, decl).is_some() {
+                import.vis
+            } else {
+                decl_vis.expect_local()
+            }
+        } else {
+            // Ordered or not, the imported declaration is too private for the current module.
+            // It doesn't matter what visibility we choose here (except in the `PRIVATE_MACRO_USE`
+            // case), because either some error will be reported, or the import declaration
+            // will be thrown away (unfortunately cannot use delayed bug here for this reason).
+            // Use import visibility to keep the all declaration visibilities in a module ordered.
+            import.vis
+        }
+    }
+
     /// Given an import and the declaration that it points to,
     /// create the corresponding import declaration.
     pub(crate) fn new_import_decl(&self, decl: Decl<'ra>, import: Import<'ra>) -> Decl<'ra> {
-        let import_vis = import.vis.to_def_id();
-        let vis = if decl.vis().is_at_least(import_vis, self.tcx)
-            || pub_use_of_private_extern_crate_hack(import, decl).is_some()
-        {
-            import_vis
-        } else {
-            decl.vis()
-        };
+        let vis = self.import_decl_vis(decl, import.summary());
 
         if let ImportKind::Glob { ref max_vis, .. } = import.kind
-            && (vis == import_vis
+            && (vis == import.vis
                 || max_vis.get().is_none_or(|max_vis| vis.is_at_least(max_vis, self.tcx)))
         {
-            max_vis.set_unchecked(Some(vis.expect_local()))
+            max_vis.set_unchecked(Some(vis))
         }
 
         self.arenas.alloc_decl(DeclData {
             kind: DeclKind::Import { source_decl: decl, import },
             ambiguity: CmCell::new(None),
             warn_ambiguity: CmCell::new(false),
             span: import.span,
-            vis: CmCell::new(vis),
+            vis: CmCell::new(vis.to_def_id()),
             expansion: import.parent_scope.expansion,
             parent_module: Some(import.parent_scope.module),
         })
@@ -448,6 +476,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             }
         } else if !old_glob_decl.vis().is_at_least(glob_decl.vis(), self.tcx) {
             // We are glob-importing the same item but with greater visibility.
+            // All visibilities here are ordered because all of them are ancestors of `module`.
             // FIXME: Update visibility in place, but without regressions
             // (#152004, #151124, #152347).
             glob_decl
@@ -471,7 +500,10 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         decl: Decl<'ra>,
         warn_ambiguity: bool,
     ) -> Result<(), Decl<'ra>> {
+        assert!(!decl.warn_ambiguity.get());
+        assert!(decl.ambiguity.get().is_none());
         let module = decl.parent_module.unwrap().expect_local();
+        assert!(self.is_accessible_from(decl.vis(), module.to_module()));
         let res = decl.res();
         self.check_reserved_macro_name(ident.name, orig_ident_span, res);
         // Even if underscore names cannot be looked up, we still need to add them to modules,
@@ -487,7 +519,6 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             orig_ident_span,
             warn_ambiguity,
             |this, resolution| {
-                assert!(!decl.warn_ambiguity.get());
                 if decl.is_glob_import() {
                     resolution.glob_decl = Some(match resolution.glob_decl {
                         Some(old_decl) => this.select_glob_decl(
@@ -1261,7 +1292,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     &import.parent_scope,
                     Some(Finalize {
                         report_private: false,
-                        import_vis: Some(import.vis),
+                        import: Some(import.summary()),
                         ..finalize
                     }),
                     bindings[ns].get().decl(),
@@ -1461,7 +1492,9 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         // All namespaces must be re-exported with extra visibility for an error to occur.
         if !any_successful_reexport {
             let (ns, binding) = reexport_error.unwrap();
-            if let Some(extern_crate_id) = pub_use_of_private_extern_crate_hack(import, binding) {
+            if let Some(extern_crate_id) =
+                pub_use_of_private_extern_crate_hack(import.summary(), binding)
+            {
                 let extern_crate_sp = self.tcx.source_span(self.local_def_id(extern_crate_id));
                 self.lint_buffer.buffer_lint(
                     PUB_USE_OF_PRIVATE_EXTERN_CRATE,

compiler/rustc_resolve/src/lib.rs

@@ -2722,6 +2722,15 @@ enum Stage {
     Late,
 }
 
+/// Parts of import data required for finalizing import resolution.
+/// Does not carry a lifetime, so it can be stored in `Finalize`.
+#[derive(Copy, Clone, Debug)]
+struct ImportSummary {
+    vis: Visibility,
+    nearest_parent_mod: LocalDefId,
+    is_single: bool,
+}
+
 /// Invariant: if `Finalize` is used, expansion and import resolution must be complete.
 #[derive(Copy, Clone, Debug)]
 struct Finalize {
@@ -2740,8 +2749,8 @@ struct Finalize {
     used: Used = Used::Other,
     /// Finalizing early or late resolution.
     stage: Stage = Stage::Early,
-    /// Nominal visibility of the import item, in case we are resolving an import's final segment.
-    import_vis: Option<Visibility> = None,
+    /// Some import data, in case we are resolving an import's final segment.
+    import: Option<ImportSummary> = None,
 }
 
 impl Finalize {

tests/ui/imports/ambiguous-import-visibility-globglob-priv-pass.rs

@@ -0,0 +1,23 @@
+//@ check-pass
+
+mod m {
+    pub struct S {}
+}
+
+mod one_private {
+    use crate::m::*;
+    pub use crate::m::*;
+}
+
+// One of the ambiguous imports is not visible from here,
+// and does not contribute to the ambiguity.
+use crate::one_private::S;
+
+// Separate module to make visibilities `in crate::inner` and `in crate::one_private` unordered.
+mod inner {
+    // One of the ambiguous imports is not visible from here,
+    // and does not contribute to the ambiguity.
+    use crate::one_private::S;
+}
+
+fn main() {}

tests/ui/imports/ambiguous-import-visibility-globglob-priv.rs

@@ -0,0 +1,21 @@
+mod m {
+    pub struct S {}
+}
+
+mod both {
+    pub mod private {
+        use crate::m::*;
+        pub(super) use crate::m::*;
+    }
+}
+
+use crate::both::private::S;
+//~^ ERROR struct import `S` is private
+
+// Separate module to make visibilities `in crate::inner` and `in crate::both(::private)` unordered.
+mod inner {
+    use crate::both::private::S;
+    //~^ ERROR struct import `S` is private
+}
+
+fn main() {}

tests/ui/imports/ambiguous-import-visibility-globglob-priv.stderr

@@ -0,0 +1,47 @@
+error[E0603]: struct import `S` is private
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:12:27
+   |
+LL | use crate::both::private::S;
+   |                           ^ private struct import
+   |
+note: the struct import `S` is defined here...
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
+   |
+LL |         pub(super) use crate::m::*;
+   |                        ^^^^^^^^^^^
+note: ...and refers to the struct `S` which is defined here
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
+   |
+LL |     pub struct S {}
+   |     ^^^^^^^^^^^^ you could import this directly
+help: import `S` through the re-export
+   |
+LL - use crate::both::private::S;
+LL + use m::S;
+   |
+
+error[E0603]: struct import `S` is private
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:17:31
+   |
+LL |     use crate::both::private::S;
+   |                               ^ private struct import
+   |
+note: the struct import `S` is defined here...
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:8:24
+   |
+LL |         pub(super) use crate::m::*;
+   |                        ^^^^^^^^^^^
+note: ...and refers to the struct `S` which is defined here
+  --> $DIR/ambiguous-import-visibility-globglob-priv.rs:2:5
+   |
+LL |     pub struct S {}
+   |     ^^^^^^^^^^^^ you could import this directly
+help: import `S` through the re-export
+   |
+LL -     use crate::both::private::S;
+LL +     use m::S;
+   |
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0603`.