Skip to content

privacy: Assert that compared visibilities are (usually) ordered #155257

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

Open
petrochenkov wants to merge 1 commit into
base: main
Choose a base branch
from
Open

privacy: Assert that compared visibilities are (usually) ordered #155257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/coherence/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ fn visit_implementation_of_const_param_ty(checker: &Checker<'_>) -> Result<(), E
let struct_vis = tcx.visibility(adt.did());
for variant in adt.variants() {
for field in &variant.fields {
if !field.vis.is_at_least(struct_vis, tcx) {
if struct_vis.greater_than(field.vis, tcx) {
let span = tcx.hir_expect_item(impl_did).expect_impl().self_ty.span;
return Err(tcx
.dcx()
Expand Down
30 changes: 18 additions & 12 deletions compiler/rustc_middle/src/middle/privacy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//! outside their scopes. This pass will also generate a set of exported items
//! which are available for use externally when compiled as a library.

use std::cmp::Ordering;
use std::hash::Hash;

use rustc_data_structures::fx::{FxIndexMap, IndexEntry};
Expand Down Expand Up @@ -82,7 +83,9 @@ impl EffectiveVisibility {
for l in Level::all_levels() {
let rhs_vis = self.at_level_mut(l);
let lhs_vis = *lhs.at_level(l);
if rhs_vis.is_at_least(lhs_vis, tcx) {
// FIXME: figure out why unordered visibilities occur here,
// and what the behavior for them should be.
if rhs_vis.partial_cmp(lhs_vis, tcx) == Ok(Ordering::Greater) {
*rhs_vis = lhs_vis;
};
}
Expand Down Expand Up @@ -139,9 +142,7 @@ impl EffectiveVisibilities {
for l in Level::all_levels() {
let vis_at_level = eff_vis.at_level(l);
let old_vis_at_level = old_eff_vis.at_level_mut(l);
if vis_at_level != old_vis_at_level
&& vis_at_level.is_at_least(*old_vis_at_level, tcx)
{
if vis_at_level.greater_than(*old_vis_at_level, tcx) {
*old_vis_at_level = *vis_at_level
}
}
Expand All @@ -160,16 +161,16 @@ impl EffectiveVisibilities {
// and all effective visibilities are larger or equal than private visibility.
let private_vis = Visibility::Restricted(tcx.parent_module_from_def_id(def_id));
let span = tcx.def_span(def_id.to_def_id());
if !ev.direct.is_at_least(private_vis, tcx) {
if private_vis.greater_than(ev.direct, tcx) {
span_bug!(span, "private {:?} > direct {:?}", private_vis, ev.direct);
}
if !ev.reexported.is_at_least(ev.direct, tcx) {
if ev.direct.greater_than(ev.reexported, tcx) {
span_bug!(span, "direct {:?} > reexported {:?}", ev.direct, ev.reexported);
}
if !ev.reachable.is_at_least(ev.reexported, tcx) {
if ev.reexported.greater_than(ev.reachable, tcx) {
span_bug!(span, "reexported {:?} > reachable {:?}", ev.reexported, ev.reachable);
}
if !ev.reachable_through_impl_trait.is_at_least(ev.reachable, tcx) {
if ev.reachable.greater_than(ev.reachable_through_impl_trait, tcx) {
span_bug!(
span,
"reachable {:?} > reachable_through_impl_trait {:?}",
Expand All @@ -183,7 +184,7 @@ impl EffectiveVisibilities {
let is_impl = matches!(tcx.def_kind(def_id), DefKind::Impl { .. });
if !is_impl && tcx.trait_impl_of_assoc(def_id.to_def_id()).is_none() {
let nominal_vis = tcx.visibility(def_id);
if !nominal_vis.is_at_least(ev.reachable, tcx) {
if ev.reachable.greater_than(nominal_vis, tcx) {
span_bug!(
span,
"{:?}: reachable {:?} > nominal {:?}",
Expand Down Expand Up @@ -242,8 +243,11 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
if !(inherited_effective_vis_at_prev_level == inherited_effective_vis_at_level
&& level != l)
{
// FIXME: figure out why unordered visibilities occur here,
// and what the behavior for them should be.
calculated_effective_vis = if let Some(max_vis) = max_vis
&& !max_vis.is_at_least(inherited_effective_vis_at_level, tcx)
&& inherited_effective_vis_at_level.partial_cmp(max_vis, tcx)
== Ok(Ordering::Greater)
{
max_vis
} else {
Expand All @@ -252,8 +256,10 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
}
// effective visibility can't be decreased at next update call for the
// same id
if *current_effective_vis_at_level != calculated_effective_vis
&& calculated_effective_vis.is_at_least(*current_effective_vis_at_level, tcx)
// FIXME: figure out why unordered visibilities occur here,
// and what the behavior for them should be.
if calculated_effective_vis.partial_cmp(*current_effective_vis_at_level, tcx)
== Ok(Ordering::Greater)
{
changed = true;
*current_effective_vis_at_level = calculated_effective_vis;
Expand Down
39 changes: 30 additions & 9 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#![allow(rustc::usage_of_ty_tykind)]

use std::cmp::Ordering;
use std::fmt::Debug;
use std::hash::{Hash, Hasher};
use std::marker::PhantomData;
Expand Down Expand Up @@ -379,18 +380,38 @@ impl<Id: Into<DefId>> Visibility<Id> {
}
}

/// Returns `true` if this visibility is at least as accessible as the given visibility
pub fn is_at_least(self, vis: Visibility<impl Into<DefId>>, tcx: TyCtxt<'_>) -> bool {
match vis {
Visibility::Public => self.is_public(),
Visibility::Restricted(id) => self.is_accessible_from(id, tcx),
pub fn partial_cmp(
self,
vis: Visibility<impl Into<DefId>>,
tcx: TyCtxt<'_>,
) -> Result<Ordering, (DefId, DefId)> {
Copy link
Copy Markdown
Contributor Author

@petrochenkov petrochenkov Apr 21, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This can be Option<Ordering>, but then we'll need to add some Debug and Copy bounds on fn greater_than.
Not sure what is better, maybe the bounds, because that would be a purely compile-time trouble.

View changes since the review

match (self, vis) {
(Visibility::Public, Visibility::Public) => Ok(Ordering::Equal),
(Visibility::Public, Visibility::Restricted(_)) => Ok(Ordering::Greater),
(Visibility::Restricted(_), Visibility::Public) => Ok(Ordering::Less),
(Visibility::Restricted(lhs_id), Visibility::Restricted(rhs_id)) => {
let lhs_id = lhs_id.into();
let rhs_id = rhs_id.into();
if lhs_id == rhs_id {
Ok(Ordering::Equal)
} else if tcx.is_descendant_of(rhs_id, lhs_id) {
Ok(Ordering::Greater)
} else if tcx.is_descendant_of(lhs_id, rhs_id) {
Ok(Ordering::Less)
} else {
Err((lhs_id, rhs_id))
}
}
}
}
}

impl<Id: Into<DefId> + Copy> Visibility<Id> {
pub fn min(self, vis: Visibility<Id>, tcx: TyCtxt<'_>) -> Visibility<Id> {
if self.is_at_least(vis, tcx) { vis } else { self }
/// Returns `true` if this visibility is strictly larger than the given visibility.
#[track_caller]
pub fn greater_than(self, vis: Visibility<impl Into<DefId>>, tcx: TyCtxt<'_>) -> bool {
match self.partial_cmp(vis, tcx) {
Ok(ord) => ord.is_gt(),
Err((lhs_id, rhs_id)) => bug!("unordered visibilities: {lhs_id:?} and {rhs_id:?}"),
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_privacy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ fn assoc_has_type_of(tcx: TyCtxt<'_>, item: &ty::AssocItem) -> bool {
}

fn min(vis1: ty::Visibility, vis2: ty::Visibility, tcx: TyCtxt<'_>) -> ty::Visibility {
if vis1.is_at_least(vis2, tcx) { vis2 } else { vis1 }
if vis1.greater_than(vis2, tcx) { vis2 } else { vis1 }
}

/// Visitor used to determine impl visibility and reachability.
Expand Down Expand Up @@ -1465,7 +1465,7 @@ impl SearchInterfaceForPrivateItemsVisitor<'_> {
};

let vis = self.tcx.local_visibility(local_def_id);
if self.hard_error && !vis.is_at_least(self.required_visibility, self.tcx) {
if self.hard_error && self.required_visibility.greater_than(vis, self.tcx) {
let vis_descr = match vis {
ty::Visibility::Public => "public",
ty::Visibility::Restricted(vis_def_id) => {
Expand Down Expand Up @@ -1499,7 +1499,7 @@ impl SearchInterfaceForPrivateItemsVisitor<'_> {

let reachable_at_vis = *effective_vis.at_level(Level::Reachable);

if !vis.is_at_least(reachable_at_vis, self.tcx) {
if reachable_at_vis.greater_than(vis, self.tcx) {
let lint = if self.in_primary_interface {
lint::builtin::PRIVATE_INTERFACES
} else {
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -922,7 +922,7 @@ impl<'a, 'ra, 'tcx> DefCollector<'a, 'ra, 'tcx> {
let field_vis = self
.try_resolve_visibility(&field.vis, false)
.unwrap_or(Visibility::Public);
if ctor_vis.is_at_least(field_vis, self.r.tcx) {
if ctor_vis.greater_than(field_vis, self.r.tcx) {
ctor_vis = field_vis;
}
field_visibilities.push(field_vis.to_def_id());
Expand Down
36 changes: 17 additions & 19 deletions compiler/rustc_resolve/src/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! A bunch of methods and structures more or less related to resolving imports.

use std::cmp::Ordering;
use std::mem;

use rustc_ast::{Item, NodeId};
Expand Down Expand Up @@ -373,24 +374,21 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
pub(crate) fn import_decl_vis(&self, decl: Decl<'ra>, import: ImportSummary) -> Visibility {
assert!(import.vis.is_accessible_from(import.nearest_parent_mod, self.tcx));
let decl_vis = decl.vis();
if decl_vis.is_at_least(import.vis, self.tcx) {
// Ordered, import is less visible than the imported declaration, or the same,
// use the import's visibility.
import.vis
} else if decl_vis.is_accessible_from(import.nearest_parent_mod, self.tcx) {
// Ordered, imported declaration is less visible than the import, but is still visible
if decl_vis.partial_cmp(import.vis, self.tcx) == Ok(Ordering::Less)
&& decl_vis.is_accessible_from(import.nearest_parent_mod, self.tcx)
&& pub_use_of_private_extern_crate_hack(import, decl).is_none()
{
// Imported declaration is less visible than the import, but is still visible
// from the current module, use the declaration's visibility.
assert!(import.vis.is_at_least(decl_vis, self.tcx));
if pub_use_of_private_extern_crate_hack(import, decl).is_some() {
import.vis
} else {
decl_vis.expect_local()
}
decl_vis.expect_local()
} else {
// Ordered or not, the imported declaration is too private for the current module.
// Good case - imported declaration is more visible than the import, or the same,
// use the import's visibility.
// Bad case - imported declaration is too private for the current module.
// It doesn't matter what visibility we choose here (except in the `PRIVATE_MACRO_USE`
// case), because either some error will be reported, or the import declaration
// will be thrown away (unfortunately cannot use delayed bug here for this reason).
// and `PUB_USE_OF_PRIVATE_EXTERN_CRATE` cases), because either some error will be
// reported, or the import declaration will be thrown away (unfortunately cannot use
// delayed bug here for this reason).
// Use import visibility to keep the all declaration visibilities in a module ordered.
import.vis
}
Expand All @@ -403,7 +401,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {

if let ImportKind::Glob { ref max_vis, .. } = import.kind
&& (vis == import.vis
|| max_vis.get().is_none_or(|max_vis| vis.is_at_least(max_vis, self.tcx)))
|| max_vis.get().is_none_or(|max_vis| vis.greater_than(max_vis, self.tcx)))
{
max_vis.set_unchecked(Some(vis))
}
Expand Down Expand Up @@ -474,7 +472,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
// FIXME: remove this when `warn_ambiguity` is removed (#149195).
self.arenas.alloc_decl((*old_glob_decl).clone())
}
} else if !old_glob_decl.vis().is_at_least(glob_decl.vis(), self.tcx) {
} else if glob_decl.vis().greater_than(old_glob_decl.vis(), self.tcx) {
// We are glob-importing the same item but with greater visibility.
// All visibilities here are ordered because all of them are ancestors of `module`.
// FIXME: Update visibility in place, but without regressions
Expand Down Expand Up @@ -1242,7 +1240,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
});
}
if let Some(max_vis) = max_vis.get()
&& !max_vis.is_at_least(import.vis, self.tcx)
&& import.vis.greater_than(max_vis, self.tcx)
{
let def_id = self.local_def_id(id);
self.lint_buffer.buffer_lint(
Expand Down Expand Up @@ -1477,7 +1475,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
return;
};

if !binding.vis().is_at_least(import.vis, this.tcx) {
if import.vis.greater_than(binding.vis(), this.tcx) {
reexport_error = Some((ns, binding));
if let Visibility::Restricted(binding_def_id) = binding.vis()
&& binding_def_id.is_top_level_module()
Expand Down
Loading