Rollup merge of #103249 - petrochenkov:revaddids, r=oli-obk

resolve: Revert "Set effective visibilities for imports more precisely"

In theory the change was correct, but in practice the use of import items in HIR is limited and hacky, and it expects that (effective) visibilities for all (up to) 3 IDs of the import are set to the value reflecting (effective) visibility of the whole syntactic `use` item rather than its individual components.

Fixes #102352
r? `@oli-obk`

Author: Dylan-DPC

compiler/rustc_resolve/src/access_levels.rs

@@ -1,5 +1,4 @@
-use crate::NameBindingKind;
-use crate::Resolver;
+use crate::{ImportKind, NameBindingKind, Resolver};
 use rustc_ast::ast;
 use rustc_ast::visit;
 use rustc_ast::visit::Visitor;
@@ -45,31 +44,40 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {
         let module = self.r.get_module(module_id.to_def_id()).unwrap();
         let resolutions = self.r.resolutions(module);
 
-        for (key, name_resolution) in resolutions.borrow().iter() {
+        for (_, name_resolution) in resolutions.borrow().iter() {
             if let Some(mut binding) = name_resolution.borrow().binding() && !binding.is_ambiguity() {
                 // Set the given binding access level to `AccessLevel::Public` and
                 // sets the rest of the `use` chain to `AccessLevel::Exported` until
                 // we hit the actual exported item.
 
-                // FIXME: tag and is_public() condition must be deleted,
-                // but assertion fail occurs in import_id_for_ns
+                // FIXME: tag and is_public() condition should be removed, but assertions occur.
                 let tag = if binding.is_import() { AccessLevel::Exported } else { AccessLevel::Public };
                 if binding.vis.is_public() {
                     let mut prev_parent_id = module_id;
                     let mut level = AccessLevel::Public;
                     while let NameBindingKind::Import { binding: nested_binding, import, .. } =
                         binding.kind
                     {
-                        let id = self.r.local_def_id(self.r.import_id_for_ns(import, key.ns));
-                        self.update(
-                            id,
+                        let mut update = |node_id| self.update(
+                            self.r.local_def_id(node_id),
                             binding.vis.expect_local(),
                             prev_parent_id,
                             level,
                         );
+                        // In theory all the import IDs have individual visibilities and effective
+                        // visibilities, but in practice these IDs go straigth to HIR where all
+                        // their few uses assume that their (effective) visibility applies to the
+                        // whole syntactic `use` item. So we update them all to the maximum value
+                        // among the potential individual effective visibilities. Maybe HIR for
+                        // imports shouldn't use three IDs at all.
+                        update(import.id);
+                        if let ImportKind::Single { additional_ids, .. } = import.kind {
+                            update(additional_ids.0);
+                            update(additional_ids.1);
+                        }
 
                         level = AccessLevel::Exported;
-                        prev_parent_id = id;
+                        prev_parent_id = self.r.local_def_id(import.id);
                         binding = nested_binding;
                     }
                 }

compiler/rustc_resolve/src/imports.rs

@@ -2,7 +2,7 @@
 
 use crate::diagnostics::{import_candidates, Suggestion};
 use crate::Determinacy::{self, *};
-use crate::Namespace::{self, *};
+use crate::Namespace::*;
 use crate::{module_to_string, names_to_string, ImportSuggestion};
 use crate::{AmbiguityKind, BindingKey, ModuleKind, ResolutionError, Resolver, Segment};
 use crate::{Finalize, Module, ModuleOrUniformRoot, ParentScope, PerNS, ScopeSet};
@@ -371,31 +371,6 @@ impl<'a> Resolver<'a> {
             self.used_imports.insert(import.id);
         }
     }
-
-    /// Take primary and additional node IDs from an import and select one that corresponds to the
-    /// given namespace. The logic must match the corresponding logic from `fn lower_use_tree` that
-    /// assigns resolutons to IDs.
-    pub(crate) fn import_id_for_ns(&self, import: &Import<'_>, ns: Namespace) -> NodeId {
-        if let ImportKind::Single { additional_ids: (id1, id2), .. } = import.kind {
-            if let Some(resolutions) = self.import_res_map.get(&import.id) {
-                assert!(resolutions[ns].is_some(), "incorrectly finalized import");
-                return match ns {
-                    TypeNS => import.id,
-                    ValueNS => match resolutions.type_ns {
-                        Some(_) => id1,
-                        None => import.id,
-                    },
-                    MacroNS => match (resolutions.type_ns, resolutions.value_ns) {
-                        (Some(_), Some(_)) => id2,
-                        (Some(_), None) | (None, Some(_)) => id1,
-                        (None, None) => import.id,
-                    },
-                };
-            }
-        }
-
-        import.id
-    }
 }
 
 /// An error that may be transformed into a diagnostic later. Used to combine multiple unresolved

src/test/ui/privacy/access_levels.rs

@@ -70,5 +70,6 @@ mod half_public_import {
 
 #[rustc_effective_visibility]
 pub use half_public_import::HalfPublicImport; //~ ERROR Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+                                              //~^ ERROR Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
 
 fn main() {}

src/test/ui/privacy/access_levels.stderr

@@ -112,6 +112,12 @@ error: Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
 LL | pub use half_public_import::HalfPublicImport;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error: Public: pub, Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
+  --> $DIR/access_levels.rs:72:9
+   |
+LL | pub use half_public_import::HalfPublicImport;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error: Public: pub(crate), Exported: pub, Reachable: pub, ReachableFromImplTrait: pub
   --> $DIR/access_levels.rs:14:13
    |
@@ -124,5 +130,5 @@ error: Public: pub(crate), Exported: pub, Reachable: pub, ReachableFromImplTrait
 LL |             type B;
    |             ^^^^^^
 
-error: aborting due to 21 previous errors
+error: aborting due to 22 previous errors