Auto merge of #80762 - petrochenkov:visclean, r=varkor

resolve: Cleanup visibility resolution for enum variants and trait items

by always delegating it to `fn resolve_visibility`.

Also remove some special treatment of visibility on enum variants and trait items remaining from pre-`pub(restricted)` times.

Author: bors

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -258,16 +258,16 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                 Ok(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)))
             }
             ast::VisibilityKind::Inherited => {
-                if matches!(self.parent_scope.module.kind, ModuleKind::Def(DefKind::Enum, _, _)) {
-                    // Any inherited visibility resolved directly inside an enum
-                    // (e.g. variants or fields) inherits from the visibility of the enum.
-                    let parent_enum = self.parent_scope.module.def_id().unwrap().expect_local();
-                    Ok(self.r.visibilities[&parent_enum])
-                } else {
-                    // If it's not in an enum, its visibility is restricted to the `mod` item
-                    // that it's defined in.
-                    Ok(ty::Visibility::Restricted(self.parent_scope.module.nearest_parent_mod))
-                }
+                Ok(match self.parent_scope.module.kind {
+                    // Any inherited visibility resolved directly inside an enum or trait
+                    // (i.e. variants, fields, and trait items) inherits from the visibility
+                    // of the enum or trait.
+                    ModuleKind::Def(DefKind::Enum | DefKind::Trait, def_id, _) => {
+                        self.r.visibilities[&def_id.expect_local()]
+                    }
+                    // Otherwise, the visibility is restricted to the nearest parent `mod` item.
+                    _ => ty::Visibility::Restricted(self.parent_scope.module.nearest_parent_mod),
+                })
             }
             ast::VisibilityKind::Restricted { ref path, id, .. } => {
                 // For visibilities we are not ready to provide correct implementation of "uniform
@@ -1365,58 +1365,40 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {
             return;
         }
 
+        let vis = self.resolve_visibility(&item.vis);
         let local_def_id = self.r.local_def_id(item.id);
         let def_id = local_def_id.to_def_id();
-        let vis = match ctxt {
-            AssocCtxt::Trait => {
-                let (def_kind, ns) = match item.kind {
-                    AssocItemKind::Const(..) => (DefKind::AssocConst, ValueNS),
-                    AssocItemKind::Fn(box FnKind(_, ref sig, _, _)) => {
-                        if sig.decl.has_self() {
-                            self.r.has_self.insert(def_id);
-                        }
-                        (DefKind::AssocFn, ValueNS)
-                    }
-                    AssocItemKind::TyAlias(..) => (DefKind::AssocTy, TypeNS),
-                    AssocItemKind::MacCall(_) => bug!(), // handled above
-                };
 
-                let parent = self.parent_scope.module;
-                let expansion = self.parent_scope.expansion;
-                let res = Res::Def(def_kind, def_id);
-                // Trait item visibility is inherited from its trait when not specified explicitly.
-                let vis = match &item.vis.kind {
-                    ast::VisibilityKind::Inherited => {
-                        self.r.visibilities[&parent.def_id().unwrap().expect_local()]
+        if !(ctxt == AssocCtxt::Impl
+            && matches!(item.vis.kind, ast::VisibilityKind::Inherited)
+            && self
+                .r
+                .trait_impl_items
+                .contains(&ty::DefIdTree::parent(&*self.r, def_id).unwrap().expect_local()))
+        {
+            // Trait impl item visibility is inherited from its trait when not specified
+            // explicitly. In that case we cannot determine it here in early resolve,
+            // so we leave a hole in the visibility table to be filled later.
+            self.r.visibilities.insert(local_def_id, vis);
+        }
+
+        if ctxt == AssocCtxt::Trait {
+            let (def_kind, ns) = match item.kind {
+                AssocItemKind::Const(..) => (DefKind::AssocConst, ValueNS),
+                AssocItemKind::Fn(box FnKind(_, ref sig, _, _)) => {
+                    if sig.decl.has_self() {
+                        self.r.has_self.insert(def_id);
                     }
-                    _ => self.resolve_visibility(&item.vis),
-                };
-                // FIXME: For historical reasons the binding visibility is set to public,
-                // use actual visibility here instead, using enum variants as an example.
-                let vis_hack = ty::Visibility::Public;
-                self.r.define(parent, item.ident, ns, (res, vis_hack, item.span, expansion));
-                Some(vis)
-            }
-            AssocCtxt::Impl => {
-                // Trait impl item visibility is inherited from its trait when not specified
-                // explicitly. In that case we cannot determine it here in early resolve,
-                // so we leave a hole in the visibility table to be filled later.
-                // Inherent impl item visibility is never inherited from other items.
-                if matches!(item.vis.kind, ast::VisibilityKind::Inherited)
-                    && self
-                        .r
-                        .trait_impl_items
-                        .contains(&ty::DefIdTree::parent(&*self.r, def_id).unwrap().expect_local())
-                {
-                    None
-                } else {
-                    Some(self.resolve_visibility(&item.vis))
+                    (DefKind::AssocFn, ValueNS)
                 }
-            }
-        };
+                AssocItemKind::TyAlias(..) => (DefKind::AssocTy, TypeNS),
+                AssocItemKind::MacCall(_) => bug!(), // handled above
+            };
 
-        if let Some(vis) = vis {
-            self.r.visibilities.insert(local_def_id, vis);
+            let parent = self.parent_scope.module;
+            let expansion = self.parent_scope.expansion;
+            let res = Res::Def(def_kind, def_id);
+            self.r.define(parent, item.ident, ns, (res, vis, item.span, expansion));
         }
 
         visit::walk_assoc_item(self, item, ctxt);
@@ -1490,19 +1472,13 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {
         }
 
         let parent = self.parent_scope.module;
-        let vis = match variant.vis.kind {
-            // Variant visibility is inherited from its enum when not specified explicitly.
-            ast::VisibilityKind::Inherited => {
-                self.r.visibilities[&parent.def_id().unwrap().expect_local()]
-            }
-            _ => self.resolve_visibility(&variant.vis),
-        };
         let expn_id = self.parent_scope.expansion;
         let ident = variant.ident;
 
         // Define a name in the type namespace.
         let def_id = self.r.local_def_id(variant.id);
         let res = Res::Def(DefKind::Variant, def_id.to_def_id());
+        let vis = self.resolve_visibility(&variant.vis);
         self.r.define(parent, ident, TypeNS, (res, vis, variant.span, expn_id));
         self.r.visibilities.insert(def_id, vis);
 

compiler/rustc_resolve/src/imports.rs

@@ -18,11 +18,10 @@ use rustc_errors::{pluralize, struct_span_err, Applicability};
 use rustc_hir::def::{self, PartialRes};
 use rustc_hir::def_id::DefId;
 use rustc_middle::hir::exports::Export;
+use rustc_middle::span_bug;
 use rustc_middle::ty;
-use rustc_middle::{bug, span_bug};
 use rustc_session::lint::builtin::{PUB_USE_OF_PRIVATE_EXTERN_CRATE, UNUSED_IMPORTS};
 use rustc_session::lint::BuiltinLintDiagnostics;
-use rustc_session::DiagnosticMessageId;
 use rustc_span::hygiene::ExpnId;
 use rustc_span::lev_distance::find_best_match_for_name;
 use rustc_span::symbol::{kw, Ident, Symbol};
@@ -456,13 +455,13 @@ impl<'a> Resolver<'a> {
         binding: &'a NameBinding<'a>,
         import: &'a Import<'a>,
     ) -> &'a NameBinding<'a> {
-        let vis = if binding.pseudo_vis().is_at_least(import.vis.get(), self) ||
+        let vis = if binding.vis.is_at_least(import.vis.get(), self) ||
                      // cf. `PUB_USE_OF_PRIVATE_EXTERN_CRATE`
                      !import.is_glob() && binding.is_extern_crate()
         {
             import.vis.get()
         } else {
-            binding.pseudo_vis()
+            binding.vis
         };
 
         if let ImportKind::Glob { ref max_vis, .. } = import.kind {
@@ -1178,7 +1177,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
         self.r.per_ns(|this, ns| {
             if let Ok(binding) = source_bindings[ns].get() {
                 let vis = import.vis.get();
-                if !binding.pseudo_vis().is_at_least(vis, &*this) {
+                if !binding.vis.is_at_least(vis, &*this) {
                     reexport_error = Some((ns, binding));
                 } else {
                     any_successful_reexport = true;
@@ -1362,7 +1361,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
                 Some(None) => import.parent_scope.module,
                 None => continue,
             };
-            if self.r.is_accessible_from(binding.pseudo_vis(), scope) {
+            if self.r.is_accessible_from(binding.vis, scope) {
                 let imported_binding = self.r.import(binding, import);
                 let _ = self.r.try_define(import.parent_scope.module, key, imported_binding);
             }
@@ -1380,9 +1379,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
 
         let mut reexports = Vec::new();
 
-        module.for_each_child(self.r, |this, ident, ns, binding| {
-            // Filter away ambiguous imports and anything that has def-site
-            // hygiene.
+        module.for_each_child(self.r, |this, ident, _, binding| {
+            // Filter away ambiguous imports and anything that has def-site hygiene.
             // FIXME: Implement actual cross-crate hygiene.
             let is_good_import =
                 binding.is_import() && !binding.is_ambiguity() && !ident.span.from_expansion();
@@ -1392,71 +1390,6 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
                     reexports.push(Export { ident, res, span: binding.span, vis: binding.vis });
                 }
             }
-
-            if let NameBindingKind::Import { binding: orig_binding, import, .. } = binding.kind {
-                if ns == TypeNS
-                    && orig_binding.is_variant()
-                    && !orig_binding.vis.is_at_least(binding.vis, &*this)
-                {
-                    let msg = match import.kind {
-                        ImportKind::Single { .. } => {
-                            format!("variant `{}` is private and cannot be re-exported", ident)
-                        }
-                        ImportKind::Glob { .. } => {
-                            let msg = "enum is private and its variants \
-                                           cannot be re-exported"
-                                .to_owned();
-                            let error_id = (
-                                DiagnosticMessageId::ErrorId(0), // no code?!
-                                Some(binding.span),
-                                msg.clone(),
-                            );
-                            let fresh =
-                                this.session.one_time_diagnostics.borrow_mut().insert(error_id);
-                            if !fresh {
-                                return;
-                            }
-                            msg
-                        }
-                        ref s => bug!("unexpected import kind {:?}", s),
-                    };
-                    let mut err = this.session.struct_span_err(binding.span, &msg);
-
-                    let imported_module = match import.imported_module.get() {
-                        Some(ModuleOrUniformRoot::Module(module)) => module,
-                        _ => bug!("module should exist"),
-                    };
-                    let parent_module = imported_module.parent.expect("parent should exist");
-                    let resolutions = this.resolutions(parent_module).borrow();
-                    let enum_path_segment_index = import.module_path.len() - 1;
-                    let enum_ident = import.module_path[enum_path_segment_index].ident;
-
-                    let key = this.new_key(enum_ident, TypeNS);
-                    let enum_resolution = resolutions.get(&key).expect("resolution should exist");
-                    let enum_span =
-                        enum_resolution.borrow().binding.expect("binding should exist").span;
-                    let enum_def_span = this.session.source_map().guess_head_span(enum_span);
-                    let enum_def_snippet = this
-                        .session
-                        .source_map()
-                        .span_to_snippet(enum_def_span)
-                        .expect("snippet should exist");
-                    // potentially need to strip extant `crate`/`pub(path)` for suggestion
-                    let after_vis_index = enum_def_snippet
-                        .find("enum")
-                        .expect("`enum` keyword should exist in snippet");
-                    let suggestion = format!("pub {}", &enum_def_snippet[after_vis_index..]);
-
-                    this.session.diag_span_suggestion_once(
-                        &mut err,
-                        DiagnosticMessageId::ErrorId(0),
-                        enum_def_span,
-                        "consider making the enum public",
-                        suggestion,
-                    );
-                    err.emit();
-                }
-            }
         });
 
         if !reexports.is_empty() {

compiler/rustc_resolve/src/lib.rs

@@ -750,27 +750,12 @@ impl<'a> NameBinding<'a> {
     fn is_possibly_imported_variant(&self) -> bool {
         match self.kind {
             NameBindingKind::Import { binding, .. } => binding.is_possibly_imported_variant(),
-            _ => self.is_variant(),
-        }
-    }
-
-    // We sometimes need to treat variants as `pub` for backwards compatibility.
-    fn pseudo_vis(&self) -> ty::Visibility {
-        if self.is_variant() && self.res().def_id().is_local() {
-            ty::Visibility::Public
-        } else {
-            self.vis
-        }
-    }
-
-    fn is_variant(&self) -> bool {
-        matches!(
-            self.kind,
             NameBindingKind::Res(
                 Res::Def(DefKind::Variant | DefKind::Ctor(CtorOf::Variant, ..), _),
                 _,
-            )
-        )
+            ) => true,
+            NameBindingKind::Res(..) | NameBindingKind::Module(..) => false,
+        }
     }
 
     fn is_extern_crate(&self) -> bool {

src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs

@@ -1,15 +1,16 @@
 #![feature(crate_visibility_modifier)]
 
+#[deny(unused_imports)]
 mod rank {
     pub use self::Professor::*;
-    //~^ ERROR enum is private and its variants cannot be re-exported
+    //~^ ERROR glob import doesn't reexport anything
     pub use self::Lieutenant::{JuniorGrade, Full};
-    //~^ ERROR variant `JuniorGrade` is private and cannot be re-exported
-    //~| ERROR variant `Full` is private and cannot be re-exported
+    //~^ ERROR `JuniorGrade` is private, and cannot be re-exported
+    //~| ERROR `Full` is private, and cannot be re-exported
     pub use self::PettyOfficer::*;
-    //~^ ERROR enum is private and its variants cannot be re-exported
+    //~^ ERROR glob import doesn't reexport anything
     pub use self::Crewman::*;
-    //~^ ERROR enum is private and its variants cannot be re-exported
+    //~^ ERROR glob import doesn't reexport anything
 
     enum Professor {
         Adjunct,

src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr

@@ -1,44 +1,51 @@
-error: variant `JuniorGrade` is private and cannot be re-exported
-  --> $DIR/issue-46209-private-enum-variant-reexport.rs:6:32
+error[E0364]: `JuniorGrade` is private, and cannot be re-exported
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:7:32
+   |
+LL |     pub use self::Lieutenant::{JuniorGrade, Full};
+   |                                ^^^^^^^^^^^
+   |
+note: consider marking `JuniorGrade` as `pub` in the imported module
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:7:32
    |
 LL |     pub use self::Lieutenant::{JuniorGrade, Full};
    |                                ^^^^^^^^^^^
-...
-LL |     enum Lieutenant {
-   |     --------------- help: consider making the enum public: `pub enum Lieutenant`
 
-error: variant `Full` is private and cannot be re-exported
-  --> $DIR/issue-46209-private-enum-variant-reexport.rs:6:45
+error[E0364]: `Full` is private, and cannot be re-exported
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:7:45
+   |
+LL |     pub use self::Lieutenant::{JuniorGrade, Full};
+   |                                             ^^^^
+   |
+note: consider marking `Full` as `pub` in the imported module
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:7:45
    |
 LL |     pub use self::Lieutenant::{JuniorGrade, Full};
    |                                             ^^^^
 
-error: enum is private and its variants cannot be re-exported
-  --> $DIR/issue-46209-private-enum-variant-reexport.rs:4:13
+error: glob import doesn't reexport anything because no candidate is public enough
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:5:13
    |
 LL |     pub use self::Professor::*;
    |             ^^^^^^^^^^^^^^^^^^
-...
-LL |     enum Professor {
-   |     -------------- help: consider making the enum public: `pub enum Professor`
+   |
+note: the lint level is defined here
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:3:8
+   |
+LL | #[deny(unused_imports)]
+   |        ^^^^^^^^^^^^^^
 
-error: enum is private and its variants cannot be re-exported
-  --> $DIR/issue-46209-private-enum-variant-reexport.rs:9:13
+error: glob import doesn't reexport anything because no candidate is public enough
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:10:13
    |
 LL |     pub use self::PettyOfficer::*;
    |             ^^^^^^^^^^^^^^^^^^^^^
-...
-LL |     pub(in rank) enum PettyOfficer {
-   |     ------------------------------ help: consider making the enum public: `pub enum PettyOfficer`
 
-error: enum is private and its variants cannot be re-exported
-  --> $DIR/issue-46209-private-enum-variant-reexport.rs:11:13
+error: glob import doesn't reexport anything because no candidate is public enough
+  --> $DIR/issue-46209-private-enum-variant-reexport.rs:12:13
    |
 LL |     pub use self::Crewman::*;
    |             ^^^^^^^^^^^^^^^^
-...
-LL |     crate enum Crewman {
-   |     ------------------ help: consider making the enum public: `pub enum Crewman`
 
 error: aborting due to 5 previous errors
 
+For more information about this error, try `rustc --explain E0364`.