Skip to content

Commit

Permalink
Auto merge of #109437 - petrochenkov:effvisopt, r=davidtwco
Browse files Browse the repository at this point in the history
resolve: Restore some effective visibility optimizations

Something similar was previously removed as a part of #104602.
So we can see [bitmaps-3.1.0](https://github.com/rust-lang/rustc-perf/tree/master/collector/compile-benchmarks/bitmaps-3.1.0), [match-stress](https://github.com/rust-lang/rustc-perf/tree/master/collector/compile-benchmarks/match-stress) and [unused-warnings](https://github.com/rust-lang/rustc-perf/tree/master/collector/compile-benchmarks/unused-warnings) in regressions there, and in improvements in this PR.
After this PR all table changes should also be "locally correct" after every update.
  • Loading branch information
bors committed Apr 5, 2023
2 parents 2e486be + 60c6dc0 commit 2eaeb1e
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 14 deletions.
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

0 comments on commit 2eaeb1e

Please sign in to comment.