Skip to content

Commit 178108b

Browse files
borsbors
committed
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.
2 parents 07194ff + 8e74842 commit 178108b

8 files changed

+118
-196
lines changed

Diff for: ‎compiler/rustc_resolve/src/build_reduced_graph.rs

+39-63
Original file line numberDiff line numberDiff line change
@@ -258,16 +258,16 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
258258
Ok(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)))
259259
}
260260
ast::VisibilityKind::Inherited => {
261-
if matches!(self.parent_scope.module.kind, ModuleKind::Def(DefKind::Enum, _, _)) {
262-
// Any inherited visibility resolved directly inside an enum
263-
// (e.g. variants or fields) inherits from the visibility of the enum.
264-
let parent_enum = self.parent_scope.module.def_id().unwrap().expect_local();
265-
Ok(self.r.visibilities[&parent_enum])
266-
} else {
267-
// If it's not in an enum, its visibility is restricted to the `mod` item
268-
// that it's defined in.
269-
Ok(ty::Visibility::Restricted(self.parent_scope.module.nearest_parent_mod))
270-
}
261+
Ok(match self.parent_scope.module.kind {
262+
// Any inherited visibility resolved directly inside an enum or trait
263+
// (i.e. variants, fields, and trait items) inherits from the visibility
264+
// of the enum or trait.
265+
ModuleKind::Def(DefKind::Enum | DefKind::Trait, def_id, _) => {
266+
self.r.visibilities[&def_id.expect_local()]
267+
}
268+
// Otherwise, the visibility is restricted to the nearest parent `mod` item.
269+
_ => ty::Visibility::Restricted(self.parent_scope.module.nearest_parent_mod),
270+
})
271271
}
272272
ast::VisibilityKind::Restricted { ref path, id, .. } => {
273273
// 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> {
13651365
return;
13661366
}
13671367

1368+
let vis = self.resolve_visibility(&item.vis);
13681369
let local_def_id = self.r.local_def_id(item.id);
13691370
let def_id = local_def_id.to_def_id();
1370-
let vis = match ctxt {
1371-
AssocCtxt::Trait => {
1372-
let (def_kind, ns) = match item.kind {
1373-
AssocItemKind::Const(..) => (DefKind::AssocConst, ValueNS),
1374-
AssocItemKind::Fn(box FnKind(_, ref sig, _, _)) => {
1375-
if sig.decl.has_self() {
1376-
self.r.has_self.insert(def_id);
1377-
}
1378-
(DefKind::AssocFn, ValueNS)
1379-
}
1380-
AssocItemKind::TyAlias(..) => (DefKind::AssocTy, TypeNS),
1381-
AssocItemKind::MacCall(_) => bug!(), // handled above
1382-
};
13831371

1384-
let parent = self.parent_scope.module;
1385-
let expansion = self.parent_scope.expansion;
1386-
let res = Res::Def(def_kind, def_id);
1387-
// Trait item visibility is inherited from its trait when not specified explicitly.
1388-
let vis = match &item.vis.kind {
1389-
ast::VisibilityKind::Inherited => {
1390-
self.r.visibilities[&parent.def_id().unwrap().expect_local()]
1372+
if !(ctxt == AssocCtxt::Impl
1373+
&& matches!(item.vis.kind, ast::VisibilityKind::Inherited)
1374+
&& self
1375+
.r
1376+
.trait_impl_items
1377+
.contains(&ty::DefIdTree::parent(&*self.r, def_id).unwrap().expect_local()))
1378+
{
1379+
// Trait impl item visibility is inherited from its trait when not specified
1380+
// explicitly. In that case we cannot determine it here in early resolve,
1381+
// so we leave a hole in the visibility table to be filled later.
1382+
self.r.visibilities.insert(local_def_id, vis);
1383+
}
1384+
1385+
if ctxt == AssocCtxt::Trait {
1386+
let (def_kind, ns) = match item.kind {
1387+
AssocItemKind::Const(..) => (DefKind::AssocConst, ValueNS),
1388+
AssocItemKind::Fn(box FnKind(_, ref sig, _, _)) => {
1389+
if sig.decl.has_self() {
1390+
self.r.has_self.insert(def_id);
13911391
}
1392-
_ => self.resolve_visibility(&item.vis),
1393-
};
1394-
// FIXME: For historical reasons the binding visibility is set to public,
1395-
// use actual visibility here instead, using enum variants as an example.
1396-
let vis_hack = ty::Visibility::Public;
1397-
self.r.define(parent, item.ident, ns, (res, vis_hack, item.span, expansion));
1398-
Some(vis)
1399-
}
1400-
AssocCtxt::Impl => {
1401-
// Trait impl item visibility is inherited from its trait when not specified
1402-
// explicitly. In that case we cannot determine it here in early resolve,
1403-
// so we leave a hole in the visibility table to be filled later.
1404-
// Inherent impl item visibility is never inherited from other items.
1405-
if matches!(item.vis.kind, ast::VisibilityKind::Inherited)
1406-
&& self
1407-
.r
1408-
.trait_impl_items
1409-
.contains(&ty::DefIdTree::parent(&*self.r, def_id).unwrap().expect_local())
1410-
{
1411-
None
1412-
} else {
1413-
Some(self.resolve_visibility(&item.vis))
1392+
(DefKind::AssocFn, ValueNS)
14141393
}
1415-
}
1416-
};
1394+
AssocItemKind::TyAlias(..) => (DefKind::AssocTy, TypeNS),
1395+
AssocItemKind::MacCall(_) => bug!(), // handled above
1396+
};
14171397

1418-
if let Some(vis) = vis {
1419-
self.r.visibilities.insert(local_def_id, vis);
1398+
let parent = self.parent_scope.module;
1399+
let expansion = self.parent_scope.expansion;
1400+
let res = Res::Def(def_kind, def_id);
1401+
self.r.define(parent, item.ident, ns, (res, vis, item.span, expansion));
14201402
}
14211403

14221404
visit::walk_assoc_item(self, item, ctxt);
@@ -1490,19 +1472,13 @@ impl<'a, 'b> Visitor<'b> for BuildReducedGraphVisitor<'a, 'b> {
14901472
}
14911473

14921474
let parent = self.parent_scope.module;
1493-
let vis = match variant.vis.kind {
1494-
// Variant visibility is inherited from its enum when not specified explicitly.
1495-
ast::VisibilityKind::Inherited => {
1496-
self.r.visibilities[&parent.def_id().unwrap().expect_local()]
1497-
}
1498-
_ => self.resolve_visibility(&variant.vis),
1499-
};
15001475
let expn_id = self.parent_scope.expansion;
15011476
let ident = variant.ident;
15021477

15031478
// Define a name in the type namespace.
15041479
let def_id = self.r.local_def_id(variant.id);
15051480
let res = Res::Def(DefKind::Variant, def_id.to_def_id());
1481+
let vis = self.resolve_visibility(&variant.vis);
15061482
self.r.define(parent, ident, TypeNS, (res, vis, variant.span, expn_id));
15071483
self.r.visibilities.insert(def_id, vis);
15081484

Diff for: ‎compiler/rustc_resolve/src/imports.rs

+7-74
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,10 @@ use rustc_errors::{pluralize, struct_span_err, Applicability};
1818
use rustc_hir::def::{self, PartialRes};
1919
use rustc_hir::def_id::DefId;
2020
use rustc_middle::hir::exports::Export;
21+
use rustc_middle::span_bug;
2122
use rustc_middle::ty;
22-
use rustc_middle::{bug, span_bug};
2323
use rustc_session::lint::builtin::{PUB_USE_OF_PRIVATE_EXTERN_CRATE, UNUSED_IMPORTS};
2424
use rustc_session::lint::BuiltinLintDiagnostics;
25-
use rustc_session::DiagnosticMessageId;
2625
use rustc_span::hygiene::ExpnId;
2726
use rustc_span::lev_distance::find_best_match_for_name;
2827
use rustc_span::symbol::{kw, Ident, Symbol};
@@ -456,13 +455,13 @@ impl<'a> Resolver<'a> {
456455
binding: &'a NameBinding<'a>,
457456
import: &'a Import<'a>,
458457
) -> &'a NameBinding<'a> {
459-
let vis = if binding.pseudo_vis().is_at_least(import.vis.get(), self) ||
458+
let vis = if binding.vis.is_at_least(import.vis.get(), self) ||
460459
// cf. `PUB_USE_OF_PRIVATE_EXTERN_CRATE`
461460
!import.is_glob() && binding.is_extern_crate()
462461
{
463462
import.vis.get()
464463
} else {
465-
binding.pseudo_vis()
464+
binding.vis
466465
};
467466

468467
if let ImportKind::Glob { ref max_vis, .. } = import.kind {
@@ -1178,7 +1177,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
11781177
self.r.per_ns(|this, ns| {
11791178
if let Ok(binding) = source_bindings[ns].get() {
11801179
let vis = import.vis.get();
1181-
if !binding.pseudo_vis().is_at_least(vis, &*this) {
1180+
if !binding.vis.is_at_least(vis, &*this) {
11821181
reexport_error = Some((ns, binding));
11831182
} else {
11841183
any_successful_reexport = true;
@@ -1362,7 +1361,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
13621361
Some(None) => import.parent_scope.module,
13631362
None => continue,
13641363
};
1365-
if self.r.is_accessible_from(binding.pseudo_vis(), scope) {
1364+
if self.r.is_accessible_from(binding.vis, scope) {
13661365
let imported_binding = self.r.import(binding, import);
13671366
let _ = self.r.try_define(import.parent_scope.module, key, imported_binding);
13681367
}
@@ -1380,9 +1379,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
13801379

13811380
let mut reexports = Vec::new();
13821381

1383-
module.for_each_child(self.r, |this, ident, ns, binding| {
1384-
// Filter away ambiguous imports and anything that has def-site
1385-
// hygiene.
1382+
module.for_each_child(self.r, |this, ident, _, binding| {
1383+
// Filter away ambiguous imports and anything that has def-site hygiene.
13861384
// FIXME: Implement actual cross-crate hygiene.
13871385
let is_good_import =
13881386
binding.is_import() && !binding.is_ambiguity() && !ident.span.from_expansion();
@@ -1392,71 +1390,6 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
13921390
reexports.push(Export { ident, res, span: binding.span, vis: binding.vis });
13931391
}
13941392
}
1395-
1396-
if let NameBindingKind::Import { binding: orig_binding, import, .. } = binding.kind {
1397-
if ns == TypeNS
1398-
&& orig_binding.is_variant()
1399-
&& !orig_binding.vis.is_at_least(binding.vis, &*this)
1400-
{
1401-
let msg = match import.kind {
1402-
ImportKind::Single { .. } => {
1403-
format!("variant `{}` is private and cannot be re-exported", ident)
1404-
}
1405-
ImportKind::Glob { .. } => {
1406-
let msg = "enum is private and its variants \
1407-
cannot be re-exported"
1408-
.to_owned();
1409-
let error_id = (
1410-
DiagnosticMessageId::ErrorId(0), // no code?!
1411-
Some(binding.span),
1412-
msg.clone(),
1413-
);
1414-
let fresh =
1415-
this.session.one_time_diagnostics.borrow_mut().insert(error_id);
1416-
if !fresh {
1417-
return;
1418-
}
1419-
msg
1420-
}
1421-
ref s => bug!("unexpected import kind {:?}", s),
1422-
};
1423-
let mut err = this.session.struct_span_err(binding.span, &msg);
1424-
1425-
let imported_module = match import.imported_module.get() {
1426-
Some(ModuleOrUniformRoot::Module(module)) => module,
1427-
_ => bug!("module should exist"),
1428-
};
1429-
let parent_module = imported_module.parent.expect("parent should exist");
1430-
let resolutions = this.resolutions(parent_module).borrow();
1431-
let enum_path_segment_index = import.module_path.len() - 1;
1432-
let enum_ident = import.module_path[enum_path_segment_index].ident;
1433-
1434-
let key = this.new_key(enum_ident, TypeNS);
1435-
let enum_resolution = resolutions.get(&key).expect("resolution should exist");
1436-
let enum_span =
1437-
enum_resolution.borrow().binding.expect("binding should exist").span;
1438-
let enum_def_span = this.session.source_map().guess_head_span(enum_span);
1439-
let enum_def_snippet = this
1440-
.session
1441-
.source_map()
1442-
.span_to_snippet(enum_def_span)
1443-
.expect("snippet should exist");
1444-
// potentially need to strip extant `crate`/`pub(path)` for suggestion
1445-
let after_vis_index = enum_def_snippet
1446-
.find("enum")
1447-
.expect("`enum` keyword should exist in snippet");
1448-
let suggestion = format!("pub {}", &enum_def_snippet[after_vis_index..]);
1449-
1450-
this.session.diag_span_suggestion_once(
1451-
&mut err,
1452-
DiagnosticMessageId::ErrorId(0),
1453-
enum_def_span,
1454-
"consider making the enum public",
1455-
suggestion,
1456-
);
1457-
err.emit();
1458-
}
1459-
}
14601393
});
14611394

14621395
if !reexports.is_empty() {

Diff for: ‎compiler/rustc_resolve/src/lib.rs

+3-18
Original file line numberDiff line numberDiff line change
@@ -750,27 +750,12 @@ impl<'a> NameBinding<'a> {
750750
fn is_possibly_imported_variant(&self) -> bool {
751751
match self.kind {
752752
NameBindingKind::Import { binding, .. } => binding.is_possibly_imported_variant(),
753-
_ => self.is_variant(),
754-
}
755-
}
756-
757-
// We sometimes need to treat variants as `pub` for backwards compatibility.
758-
fn pseudo_vis(&self) -> ty::Visibility {
759-
if self.is_variant() && self.res().def_id().is_local() {
760-
ty::Visibility::Public
761-
} else {
762-
self.vis
763-
}
764-
}
765-
766-
fn is_variant(&self) -> bool {
767-
matches!(
768-
self.kind,
769753
NameBindingKind::Res(
770754
Res::Def(DefKind::Variant | DefKind::Ctor(CtorOf::Variant, ..), _),
771755
_,
772-
)
773-
)
756+
) => true,
757+
NameBindingKind::Res(..) | NameBindingKind::Module(..) => false,
758+
}
774759
}
775760

776761
fn is_extern_crate(&self) -> bool {

Diff for: ‎src/test/ui/privacy/issue-46209-private-enum-variant-reexport.rs

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
11
#![feature(crate_visibility_modifier)]
22

3+
#[deny(unused_imports)]
34
mod rank {
45
pub use self::Professor::*;
5-
//~^ ERROR enum is private and its variants cannot be re-exported
6+
//~^ ERROR glob import doesn't reexport anything
67
pub use self::Lieutenant::{JuniorGrade, Full};
7-
//~^ ERROR variant `JuniorGrade` is private and cannot be re-exported
8-
//~| ERROR variant `Full` is private and cannot be re-exported
8+
//~^ ERROR `JuniorGrade` is private, and cannot be re-exported
9+
//~| ERROR `Full` is private, and cannot be re-exported
910
pub use self::PettyOfficer::*;
10-
//~^ ERROR enum is private and its variants cannot be re-exported
11+
//~^ ERROR glob import doesn't reexport anything
1112
pub use self::Crewman::*;
12-
//~^ ERROR enum is private and its variants cannot be re-exported
13+
//~^ ERROR glob import doesn't reexport anything
1314

1415
enum Professor {
1516
Adjunct,

Diff for: ‎src/test/ui/privacy/issue-46209-private-enum-variant-reexport.stderr

+29-22
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,51 @@
1-
error: variant `JuniorGrade` is private and cannot be re-exported
2-
--> $DIR/issue-46209-private-enum-variant-reexport.rs:6:32
1+
error[E0364]: `JuniorGrade` is private, and cannot be re-exported
2+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:7:32
3+
|
4+
LL | pub use self::Lieutenant::{JuniorGrade, Full};
5+
| ^^^^^^^^^^^
6+
|
7+
note: consider marking `JuniorGrade` as `pub` in the imported module
8+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:7:32
39
|
410
LL | pub use self::Lieutenant::{JuniorGrade, Full};
511
| ^^^^^^^^^^^
6-
...
7-
LL | enum Lieutenant {
8-
| --------------- help: consider making the enum public: `pub enum Lieutenant`
912

10-
error: variant `Full` is private and cannot be re-exported
11-
--> $DIR/issue-46209-private-enum-variant-reexport.rs:6:45
13+
error[E0364]: `Full` is private, and cannot be re-exported
14+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:7:45
15+
|
16+
LL | pub use self::Lieutenant::{JuniorGrade, Full};
17+
| ^^^^
18+
|
19+
note: consider marking `Full` as `pub` in the imported module
20+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:7:45
1221
|
1322
LL | pub use self::Lieutenant::{JuniorGrade, Full};
1423
| ^^^^
1524

16-
error: enum is private and its variants cannot be re-exported
17-
--> $DIR/issue-46209-private-enum-variant-reexport.rs:4:13
25+
error: glob import doesn't reexport anything because no candidate is public enough
26+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:5:13
1827
|
1928
LL | pub use self::Professor::*;
2029
| ^^^^^^^^^^^^^^^^^^
21-
...
22-
LL | enum Professor {
23-
| -------------- help: consider making the enum public: `pub enum Professor`
30+
|
31+
note: the lint level is defined here
32+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:3:8
33+
|
34+
LL | #[deny(unused_imports)]
35+
| ^^^^^^^^^^^^^^
2436

25-
error: enum is private and its variants cannot be re-exported
26-
--> $DIR/issue-46209-private-enum-variant-reexport.rs:9:13
37+
error: glob import doesn't reexport anything because no candidate is public enough
38+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:10:13
2739
|
2840
LL | pub use self::PettyOfficer::*;
2941
| ^^^^^^^^^^^^^^^^^^^^^
30-
...
31-
LL | pub(in rank) enum PettyOfficer {
32-
| ------------------------------ help: consider making the enum public: `pub enum PettyOfficer`
3342

34-
error: enum is private and its variants cannot be re-exported
35-
--> $DIR/issue-46209-private-enum-variant-reexport.rs:11:13
43+
error: glob import doesn't reexport anything because no candidate is public enough
44+
--> $DIR/issue-46209-private-enum-variant-reexport.rs:12:13
3645
|
3746
LL | pub use self::Crewman::*;
3847
| ^^^^^^^^^^^^^^^^
39-
...
40-
LL | crate enum Crewman {
41-
| ------------------ help: consider making the enum public: `pub enum Crewman`
4248

4349
error: aborting due to 5 previous errors
4450

51+
For more information about this error, try `rustc --explain E0364`.

0 commit comments

Comments
 (0)