From b4b6b62e70f94c949b82f669498a925e3d4c3c2b Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 6 Jan 2021 17:19:47 +0300 Subject: [PATCH 1/2] resolve: Cleanup visibility resolution in enums and traits --- .../rustc_resolve/src/build_reduced_graph.rs | 105 +++++++----------- 1 file changed, 42 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index c4ee4df212863..c4f3b3ff85743 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/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,43 @@ 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); + // 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)); } visit::walk_assoc_item(self, item, ctxt); @@ -1490,19 +1475,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); From 8e748420896cfbf55e1a3cd8c51e4aba499cfb2d Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 6 Jan 2021 18:07:47 +0300 Subject: [PATCH 2/2] resolve: Remove visibility hacks for enum variants and trait items Special treatment like this was necessary before `pub(restricted)` had been implemented and only two visibilities existed - `pub` and non-`pub`. Now it's no longer necessary and the desired behavior follows from `pub(restricted)`-style visibilities naturally assigned to enum variants and trait items. --- .../rustc_resolve/src/build_reduced_graph.rs | 5 +- compiler/rustc_resolve/src/imports.rs | 81 ++----------------- compiler/rustc_resolve/src/lib.rs | 21 +---- ...sue-46209-private-enum-variant-reexport.rs | 11 +-- ...46209-private-enum-variant-reexport.stderr | 51 +++++++----- .../ui/privacy/private-variant-reexport.rs | 9 ++- .../privacy/private-variant-reexport.stderr | 37 ++++++--- src/test/ui/variants/variant-namespacing.rs | 2 +- 8 files changed, 80 insertions(+), 137 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index c4f3b3ff85743..79ed0b5308dab 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -1398,10 +1398,7 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> { let parent = self.parent_scope.module; let expansion = self.parent_scope.expansion; let res = Res::Def(def_kind, def_id); - // 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)); + self.r.define(parent, item.ident, ns, (res, vis, item.span, expansion)); } visit::walk_assoc_item(self, item, ctxt); diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index cb1f0834ce7ac..bd0296751a535 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/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() { diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 819fabdd1f177..bca3c7b1b03d3 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/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 { diff --git a/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs b/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs index d54c993147938..8f9dc4bc945df 100644 --- a/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs +++ b/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, diff --git a/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr b/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr index b876bab6c542f..d4d9b31ed831f 100644 --- a/src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr +++ b/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`. diff --git a/src/test/ui/privacy/private-variant-reexport.rs b/src/test/ui/privacy/private-variant-reexport.rs index 0722b815c147a..ce1b0d321ca50 100644 --- a/src/test/ui/privacy/private-variant-reexport.rs +++ b/src/test/ui/privacy/private-variant-reexport.rs @@ -1,17 +1,18 @@ mod m1 { - pub use ::E::V; //~ ERROR variant `V` is private and cannot be re-exported + pub use ::E::V; //~ ERROR `V` is private, and cannot be re-exported } mod m2 { - pub use ::E::{V}; //~ ERROR variant `V` is private and cannot be re-exported + pub use ::E::{V}; //~ ERROR `V` is private, and cannot be re-exported } mod m3 { - pub use ::E::V::{self}; //~ ERROR variant `V` is private and cannot be re-exported + pub use ::E::V::{self}; //~ ERROR `V` is private, and cannot be re-exported } +#[deny(unused_imports)] mod m4 { - pub use ::E::*; //~ ERROR enum is private and its variants cannot be re-exported + pub use ::E::*; //~ ERROR glob import doesn't reexport anything } enum E { V } diff --git a/src/test/ui/privacy/private-variant-reexport.stderr b/src/test/ui/privacy/private-variant-reexport.stderr index 8e4c345286247..7a4c3234dbe6f 100644 --- a/src/test/ui/privacy/private-variant-reexport.stderr +++ b/src/test/ui/privacy/private-variant-reexport.stderr @@ -1,29 +1,48 @@ -error: variant `V` is private and cannot be re-exported +error[E0364]: `V` is private, and cannot be re-exported + --> $DIR/private-variant-reexport.rs:2:13 + | +LL | pub use ::E::V; + | ^^^^^^ + | +note: consider marking `V` as `pub` in the imported module --> $DIR/private-variant-reexport.rs:2:13 | LL | pub use ::E::V; | ^^^^^^ -... -LL | enum E { V } - | ------ help: consider making the enum public: `pub enum E` -error: variant `V` is private and cannot be re-exported +error[E0364]: `V` is private, and cannot be re-exported + --> $DIR/private-variant-reexport.rs:6:19 + | +LL | pub use ::E::{V}; + | ^ + | +note: consider marking `V` as `pub` in the imported module --> $DIR/private-variant-reexport.rs:6:19 | LL | pub use ::E::{V}; | ^ -error: variant `V` is private and cannot be re-exported +error[E0365]: `V` is private, and cannot be re-exported --> $DIR/private-variant-reexport.rs:10:22 | LL | pub use ::E::V::{self}; - | ^^^^ + | ^^^^ re-export of private `V` + | + = note: consider declaring type or module `V` with `pub` -error: enum is private and its variants cannot be re-exported - --> $DIR/private-variant-reexport.rs:14:13 +error: glob import doesn't reexport anything because no candidate is public enough + --> $DIR/private-variant-reexport.rs:15:13 | LL | pub use ::E::*; | ^^^^^^ + | +note: the lint level is defined here + --> $DIR/private-variant-reexport.rs:13:8 + | +LL | #[deny(unused_imports)] + | ^^^^^^^^^^^^^^ error: aborting due to 4 previous errors +Some errors have detailed explanations: E0364, E0365. +For more information about an error, try `rustc --explain E0364`. diff --git a/src/test/ui/variants/variant-namespacing.rs b/src/test/ui/variants/variant-namespacing.rs index ceb7d27451452..975e471fe34fa 100644 --- a/src/test/ui/variants/variant-namespacing.rs +++ b/src/test/ui/variants/variant-namespacing.rs @@ -1,6 +1,6 @@ // aux-build:variant-namespacing.rs -enum E { +pub enum E { Struct { a: u8 }, Tuple(u8), Unit,