Skip to content

resolve: Restore some effective visibility optimizations #109437

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.

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

Merged
merged 1 commit into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 28 additions & 8 deletions compiler/rustc_resolve/src/effective_visibilities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,6 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
}
}

fn cheap_private_vis(&self, parent_id: ParentId<'_>) -> Option<Visibility> {
matches!(parent_id, ParentId::Def(_)).then_some(self.current_private_vis)
}

fn effective_vis_or_private(&mut self, parent_id: ParentId<'a>) -> EffectiveVisibility {
// Private nodes are only added to the table for caching, they could be added or removed at
// any moment without consequences, so we don't set `changed` to true when adding them.
Expand All @@ -172,29 +168,53 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
}
}

/// All effective visibilities for a node are larger or equal than private visibility
/// for that node (see `check_invariants` in middle/privacy.rs).
/// So if either parent or nominal visibility is the same as private visibility, then
/// `min(parent_vis, nominal_vis) <= private_vis`, and the update logic is guaranteed
/// to not update anything and we can skip it.
///
/// We are checking this condition only if the correct value of private visibility is
/// cheaply available, otherwise it does't make sense performance-wise.
///
/// `None` is returned if the update can be skipped,
/// and cheap private visibility is returned otherwise.
fn may_update(
&self,
nominal_vis: Visibility,
parent_id: ParentId<'_>,
) -> Option<Option<Visibility>> {
match parent_id {
ParentId::Def(def_id) => (nominal_vis != self.current_private_vis
&& self.r.visibilities[&def_id] != self.current_private_vis)
.then_some(Some(self.current_private_vis)),
ParentId::Import(_) => Some(None),
}
}

fn update_import(&mut self, binding: ImportId<'a>, parent_id: ParentId<'a>) {
let nominal_vis = binding.vis.expect_local();
let private_vis = self.cheap_private_vis(parent_id);
let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return };
let inherited_eff_vis = self.effective_vis_or_private(parent_id);
let tcx = self.r.tcx;
self.changed |= self.import_effective_visibilities.update(
binding,
nominal_vis,
|| private_vis.unwrap_or_else(|| self.r.private_vis_import(binding)),
|| cheap_private_vis.unwrap_or_else(|| self.r.private_vis_import(binding)),
inherited_eff_vis,
parent_id.level(),
tcx,
);
}

fn update_def(&mut self, def_id: LocalDefId, nominal_vis: Visibility, parent_id: ParentId<'a>) {
let private_vis = self.cheap_private_vis(parent_id);
let Some(cheap_private_vis) = self.may_update(nominal_vis, parent_id) else { return };
let inherited_eff_vis = self.effective_vis_or_private(parent_id);
let tcx = self.r.tcx;
self.changed |= self.def_effective_visibilities.update(
def_id,
nominal_vis,
|| private_vis.unwrap_or_else(|| self.r.private_vis_def(def_id)),
|| cheap_private_vis.unwrap_or_else(|| self.r.private_vis_def(def_id)),
inherited_eff_vis,
parent_id.level(),
tcx,
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/privacy/effective_visibilities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ mod outer { //~ ERROR Direct: pub(crate), Reexported: pub(crate), Reachable: pub
}

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

#[rustc_effective_visibility]
pub union PubUnion { //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
#[rustc_effective_visibility]
a: u8, //~ ERROR Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
a: u8, //~ ERROR not in the table
#[rustc_effective_visibility]
pub b: u8, //~ ERROR Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImplTrait: pub
}
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/privacy/effective_visibilities.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
LL | pub trait PubTrait {
| ^^^^^^^^^^^^^^^^^^

error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
error: not in the table
--> $DIR/effective_visibilities.rs:21:9
|
LL | struct PrivStruct;
| ^^^^^^^^^^^^^^^^^

error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
error: not in the table
--> $DIR/effective_visibilities.rs:21:9
|
LL | struct PrivStruct;
Expand All @@ -52,7 +52,7 @@ error: Direct: pub(crate), Reexported: pub, Reachable: pub, ReachableThroughImpl
LL | pub union PubUnion {
| ^^^^^^^^^^^^^^^^^^

error: Direct: pub(self), Reexported: pub(self), Reachable: pub(self), ReachableThroughImplTrait: pub(self)
error: not in the table
--> $DIR/effective_visibilities.rs:27:13
|
LL | a: u8,
Expand Down