Auto merge of #155257 - petrochenkov:visatleast, r=adwinwhite

privacy: Assert that compared visibilities are (usually) ordered



And make "greater than" (`>`) the new primary operation for comparing visibilities instead of "is at least" (`>=`).

Author: bors

compiler/rustc_hir_analysis/src/coherence/builtin.rs

@@ -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()

compiler/rustc_middle/src/middle/privacy.rs

@@ -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};
@@ -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) == Some(Ordering::Greater) {
                 *rhs_vis = lhs_vis;
             };
         }
@@ -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
                     }
                 }
@@ -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 {:?}",
@@ -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 {:?}",
@@ -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)
+                            == Some(Ordering::Greater)
                     {
                         max_vis
                     } else {
@@ -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)
+                    == Some(Ordering::Greater)
                 {
                     changed = true;
                     *current_effective_vis_at_level = calculated_effective_vis;

compiler/rustc_middle/src/ty/mod.rs

@@ -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;
@@ -379,18 +380,46 @@ 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<'_>,
+    ) -> Option<Ordering> {
+        match (self, vis) {
+            (Visibility::Public, Visibility::Public) => Some(Ordering::Equal),
+            (Visibility::Public, Visibility::Restricted(_)) => Some(Ordering::Greater),
+            (Visibility::Restricted(_), Visibility::Public) => Some(Ordering::Less),
+            (Visibility::Restricted(lhs_id), Visibility::Restricted(rhs_id)) => {
+                let (lhs_id, rhs_id) = (lhs_id.into(), rhs_id.into());
+                if lhs_id == rhs_id {
+                    Some(Ordering::Equal)
+                } else if tcx.is_descendant_of(rhs_id, lhs_id) {
+                    Some(Ordering::Greater)
+                } else if tcx.is_descendant_of(lhs_id, rhs_id) {
+                    Some(Ordering::Less)
+                } else {
+                    None
+                }
+            }
         }
     }
 }
 
-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 }
+impl<Id: Into<DefId> + Debug + Copy> Visibility<Id> {
+    /// Returns `true` if this visibility is strictly larger than the given visibility.
+    #[track_caller]
+    pub fn greater_than(
+        self,
+        vis: Visibility<impl Into<DefId> + Debug + Copy>,
+        tcx: TyCtxt<'_>,
+    ) -> bool {
+        match self.partial_cmp(vis, tcx) {
+            Some(ord) => ord.is_gt(),
+            None => {
+                tcx.dcx().delayed_bug(format!("unordered visibilities: {self:?} and {vis:?}"));
+                false
+            }
+        }
     }
 }
 

compiler/rustc_privacy/src/lib.rs

@@ -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.
@@ -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) => {
@@ -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 {

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -924,7 +924,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());

compiler/rustc_resolve/src/imports.rs

@@ -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 itertools::Itertools;
@@ -374,24 +375,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) == Some(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
         }
@@ -404,7 +402,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))
         }
@@ -475,7 +473,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
@@ -1251,7 +1249,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(
@@ -1500,7 +1498,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()