Skip to content

Commit f0843b8

Browse files
petrochenkovpetrochenkov
committed
effective visibility: Remove questionable optimizations
First, they require eagerly calculating private visibility (current normal module), which is somewhat expensive. Private visibilities are also lost once calculated, instead of being cached in the table. Second, I cannot prove that the optimizations are correct. Maybe they can be partially reinstated in the future in cases when it's cheap and provably correct to do them. They will also probably be merged into `fn update` in that case. Partially fixes rust-lang#104249 Fixes rust-lang#104539
1 parent 3f20f4a commit f0843b8

File tree

6 files changed

+70
-43
lines changed

6 files changed

+70
-43
lines changed

‎compiler/rustc_middle/src/middle/privacy.rs‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
use crate::ty::{DefIdTree, TyCtxt, Visibility};
55
use rustc_data_structures::fx::FxHashMap;
66
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
7-
use rustc_hir::def::DefKind;
87
use rustc_macros::HashStable;
98
use rustc_query_system::ich::StableHashingContext;
109
use rustc_span::def_id::LocalDefId;
@@ -185,7 +184,6 @@ impl EffectiveVisibilities {
185184
);
186185
}
187186
let nominal_vis = tcx.visibility(def_id);
188-
let def_kind = tcx.opt_def_kind(def_id);
189187
// FIXME: `rustc_privacy` is not yet updated for the new logic and can set
190188
// effective visibilities that are larger than the nominal one.
191189
if !nominal_vis.is_at_least(ev.reachable_through_impl_trait, tcx) && early {
@@ -197,11 +195,6 @@ impl EffectiveVisibilities {
197195
nominal_vis
198196
);
199197
}
200-
// Fully private items are never put into the table, this is important for performance.
201-
// FIXME: Fully private `mod` items are currently put into the table.
202-
if ev.reachable_through_impl_trait == private_vis && def_kind != Some(DefKind::Mod) {
203-
span_bug!(span, "fully private item in the table {:?}: {:?}", def_id, ev.direct);
204-
}
205198
}
206199
}
207200
}

‎compiler/rustc_resolve/src/effective_visibilities.rs‎

Lines changed: 20 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,24 @@ impl Resolver<'_> {
4242
fn nearest_normal_mod(&mut self, def_id: LocalDefId) -> LocalDefId {
4343
self.get_nearest_non_block_module(def_id.to_def_id()).nearest_parent_mod().expect_local()
4444
}
45+
46+
fn private_vis_import(&mut self, binding: ImportId<'_>) -> Visibility {
47+
let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
48+
Visibility::Restricted(
49+
import
50+
.id()
51+
.map(|id| self.nearest_normal_mod(self.local_def_id(id)))
52+
.unwrap_or(CRATE_DEF_ID),
53+
)
54+
}
55+
56+
fn private_vis_def(&mut self, def_id: LocalDefId) -> Visibility {
57+
if def_id == CRATE_DEF_ID {
58+
Visibility::Public
59+
} else {
60+
Visibility::Restricted(self.nearest_normal_mod(def_id))
61+
}
62+
}
4563
}
4664

4765
impl<'a, 'b> IntoDefIdTree for &'b mut Resolver<'a> {
@@ -143,51 +161,23 @@ impl<'r, 'a> EffectiveVisibilitiesVisitor<'r, 'a> {
143161
.copied()
144162
}
145163

146-
/// The update is guaranteed to not change the table and we can skip it.
147-
fn is_noop_update(
148-
&self,
149-
parent_id: ParentId<'a>,
150-
nominal_vis: Visibility,
151-
default_vis: Visibility,
152-
) -> bool {
153-
nominal_vis == default_vis
154-
|| match parent_id {
155-
ParentId::Def(def_id) => self.r.visibilities[&def_id],
156-
ParentId::Import(binding) => binding.vis.expect_local(),
157-
} == default_vis
158-
}
159-
160164
fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) {
161-
let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
162165
let nominal_vis = binding.vis.expect_local();
163-
let default_vis = Visibility::Restricted(
164-
import
165-
.id()
166-
.map(|id| self.r.nearest_normal_mod(self.r.local_def_id(id)))
167-
.unwrap_or(CRATE_DEF_ID),
168-
);
169-
if self.is_noop_update(parent_id, nominal_vis, default_vis) {
170-
return;
171-
}
172166
self.changed |= self.import_effective_visibilities.update(
173167
binding,
174168
nominal_vis,
175-
|r| (default_vis, r),
169+
|r| (r.private_vis_import(binding), r),
176170
self.effective_vis(parent_id),
177171
parent_id.level(),
178172
&mut *self.r,
179173
);
180174
}
181175

182176
fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) {
183-
let default_vis = Visibility::Restricted(self.r.nearest_normal_mod(def_id));
184-
if self.is_noop_update(parent_id, nominal_vis, default_vis) {
185-
return;
186-
}
187177
self.changed |= self.def_effective_visibilities.update(
188178
def_id,
189179
nominal_vis,
190-
|r| (if def_id == CRATE_DEF_ID { Visibility::Public } else { default_vis }, r),
180+
|r| (r.private_vis_def(def_id), r),
191181
self.effective_vis(parent_id),
192182
parent_id.level(),
193183
&mut *self.r,

‎src/test/ui/privacy/effective_visibilities.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub
1717
}
1818

1919
#[rustc_effective_visibility]
20-
struct PrivStruct; //~ ERROR not in the table
21-
//~| ERROR not in the table
20+
struct PrivStruct; //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
21+
//~| ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
2222

2323
#[rustc_effective_visibility]
2424
pub union PubUnion { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
2525
#[rustc_effective_visibility]
26-
a: u8, //~ ERROR not in the table
26+
a: u8, //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
2727
#[rustc_effective_visibility]
2828
pub b: u8, //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
2929
}

‎src/test/ui/privacy/effective_visibilities.stderr‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,13 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
2222
LL | pub trait PubTrait {
2323
| ^^^^^^^^^^^^^^^^^^
2424

25-
error: not in the table
25+
error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
2626
--> $DIR/effective_visibilities.rs:20:9
2727
|
2828
LL | struct PrivStruct;
2929
| ^^^^^^^^^^^^^^^^^
3030

31-
error: not in the table
31+
error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
3232
--> $DIR/effective_visibilities.rs:20:9
3333
|
3434
LL | struct PrivStruct;
@@ -40,7 +40,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
4040
LL | pub union PubUnion {
4141
| ^^^^^^^^^^^^^^^^^^
4242

43-
error: not in the table
43+
error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
4444
--> $DIR/effective_visibilities.rs:26:13
4545
|
4646
LL | a: u8,

‎src/test/ui/privacy/effective_visibilities_invariants.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Invariant checking doesn't ICE in some cases with errors (issue #104249).
2+
3+
#![feature(staged_api)] //~ ERROR module has missing stability attribute
4+
5+
pub mod m {} //~ ERROR module has missing stability attribute
6+
7+
pub mod m { //~ ERROR the name `m` is defined multiple times
8+
// mod inner {} - ICE
9+
type Inner = u8;
10+
}
11+
12+
fn main() {}

‎src/test/ui/privacy/effective_visibilities_invariants.stderr‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
error[E0428]: the name `m` is defined multiple times
2+
--> $DIR/effective_visibilities_invariants.rs:7:1
3+
|
4+
LL | pub mod m {}
5+
| --------- previous definition of the module `m` here
6+
LL |
7+
LL | pub mod m {
8+
| ^^^^^^^^^ `m` redefined here
9+
|
10+
= note: `m` must be defined only once in the type namespace of this module
11+
12+
error: module has missing stability attribute
13+
--> $DIR/effective_visibilities_invariants.rs:3:1
14+
|
15+
LL | / #![feature(staged_api)]
16+
LL | |
17+
LL | | pub mod m {}
18+
LL | |
19+
... |
20+
LL | |
21+
LL | | fn main() {}
22+
| |____________^
23+
24+
error: module has missing stability attribute
25+
--> $DIR/effective_visibilities_invariants.rs:5:1
26+
|
27+
LL | pub mod m {}
28+
| ^^^^^^^^^^^^
29+
30+
error: aborting due to 3 previous errors
31+
32+
For more information about this error, try `rustc --explain E0428`.

0 commit comments

Comments
 (0)