Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve: Cleanup visibility resolution for enum variants and trait items #80762

Merged
merged 2 commits into from
Feb 11, 2021
Merged

resolve: Cleanup visibility resolution for enum variants and trait items #80762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4b6b62
resolve: Cleanup visibility resolution in enums and traits
petrochenkov Jan 6, 2021
8e74842
resolve: Remove visibility hacks for enum variants and trait items
petrochenkov Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 39 additions & 63 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
81 changes: 7 additions & 74 deletions compiler/rustc_resolve/src/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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();
Expand All @@ -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() {
Expand Down
21 changes: 3 additions & 18 deletions compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 6 additions & 5 deletions src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a change in behaviour, or is the lint taking precedence over the "enum is private" error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behavior, now glob import from enum with private variants behaves identically to a glob import from a module with private items.
Previously, in the world without pub(restricted), this required special logic and separate error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely seems like better behaviour, but is this something that would need to undergo a lang team FCP, or is this a change that has already been discussed?

Copy link
Contributor Author

@petrochenkov petrochenkov Jan 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe.
When the error was added in 8f359d5 (as a part of hardening private-in-public errors), the glob case wasn't really discussed.

The glob error was added pretty conservatively to seal any possible sources of leakage, but even then it was inconsistent with behavior of other glob imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's cc them just to make sure, though it's likely this isn't large enough to warrant an FCP.

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,
Expand Down
51 changes: 29 additions & 22 deletions src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`.
Loading