Rollup merge of #120284 - petrochenkov:typrivisit2, r=oli-obk

privacy: Refactor top-level visiting in `TypePrivacyVisitor`

Full hierarchical visiting (`nested_filter::All`) is not necessary, visiting all item-likes in isolation is enough.
Tracking current item is not necessary, just keeping the current `mod` item is enough.
`visit_generic_arg` should behave like its default version, including checking types of const arguments.
Some comments, including FIXMEs, are also added.

Noticed while reading code to review #113671.
r? ``@oli-obk``

Author: fmease

compiler/rustc_privacy/src/lib.rs

@@ -24,13 +24,13 @@ use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId, CRATE_DEF_ID};
 use rustc_hir::intravisit::{self, Visitor};
 use rustc_hir::{AssocItemKind, ForeignItemKind, ItemId, PatKind};
-use rustc_middle::bug;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::privacy::{EffectiveVisibilities, EffectiveVisibility, Level};
 use rustc_middle::query::Providers;
 use rustc_middle::ty::GenericArgs;
 use rustc_middle::ty::{self, Const, GenericParamDefKind};
 use rustc_middle::ty::{TraitRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor};
+use rustc_middle::{bug, span_bug};
 use rustc_session::lint;
 use rustc_span::hygiene::Transparency;
 use rustc_span::symbol::{kw, sym, Ident};
@@ -1064,29 +1064,22 @@ impl<'tcx> Visitor<'tcx> for NamePrivacyVisitor<'tcx> {
 
 struct TypePrivacyVisitor<'tcx> {
     tcx: TyCtxt<'tcx>,
+    module_def_id: LocalModDefId,
     maybe_typeck_results: Option<&'tcx ty::TypeckResults<'tcx>>,
-    current_item: LocalDefId,
     span: Span,
 }
 
 impl<'tcx> TypePrivacyVisitor<'tcx> {
-    /// Gets the type-checking results for the current body.
-    /// As this will ICE if called outside bodies, only call when working with
-    /// `Expr` or `Pat` nodes (they are guaranteed to be found only in bodies).
-    #[track_caller]
-    fn typeck_results(&self) -> &'tcx ty::TypeckResults<'tcx> {
-        self.maybe_typeck_results
-            .expect("`TypePrivacyVisitor::typeck_results` called outside of body")
-    }
-
     fn item_is_accessible(&self, did: DefId) -> bool {
-        self.tcx.visibility(did).is_accessible_from(self.current_item, self.tcx)
+        self.tcx.visibility(did).is_accessible_from(self.module_def_id, self.tcx)
     }
 
     // Take node-id of an expression or pattern and check its type for privacy.
     fn check_expr_pat_type(&mut self, id: hir::HirId, span: Span) -> bool {
         self.span = span;
-        let typeck_results = self.typeck_results();
+        let typeck_results = self
+            .maybe_typeck_results
+            .unwrap_or_else(|| span_bug!(span, "`hir::Expr` or `hir::Pat` outside of a body"));
         let result: ControlFlow<()> = try {
             self.visit(typeck_results.node_type(id))?;
             self.visit(typeck_results.node_args(id))?;
@@ -1107,35 +1100,13 @@ impl<'tcx> TypePrivacyVisitor<'tcx> {
 }
 
 impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
-    type NestedFilter = nested_filter::All;
-
-    /// We want to visit items in the context of their containing
-    /// module and so forth, so supply a crate for doing a deep walk.
-    fn nested_visit_map(&mut self) -> Self::Map {
-        self.tcx.hir()
-    }
-
-    fn visit_mod(&mut self, _m: &'tcx hir::Mod<'tcx>, _s: Span, _n: hir::HirId) {
-        // Don't visit nested modules, since we run a separate visitor walk
-        // for each module in `effective_visibilities`
-    }
-
-    fn visit_nested_body(&mut self, body: hir::BodyId) {
+    fn visit_nested_body(&mut self, body_id: hir::BodyId) {
         let old_maybe_typeck_results =
-            self.maybe_typeck_results.replace(self.tcx.typeck_body(body));
-        let body = self.tcx.hir().body(body);
-        self.visit_body(body);
+            self.maybe_typeck_results.replace(self.tcx.typeck_body(body_id));
+        self.visit_body(self.tcx.hir().body(body_id));
         self.maybe_typeck_results = old_maybe_typeck_results;
     }
 
-    fn visit_generic_arg(&mut self, generic_arg: &'tcx hir::GenericArg<'tcx>) {
-        match generic_arg {
-            hir::GenericArg::Type(t) => self.visit_ty(t),
-            hir::GenericArg::Infer(inf) => self.visit_infer(inf),
-            hir::GenericArg::Lifetime(_) | hir::GenericArg::Const(_) => {}
-        }
-    }
-
     fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) {
         self.span = hir_ty.span;
         if let Some(typeck_results) = self.maybe_typeck_results {
@@ -1163,19 +1134,19 @@ impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
                     return;
                 }
             } else {
-                // We don't do anything for const infers here.
+                // FIXME: check types of const infers here.
             }
         } else {
-            bug!("visit_infer without typeck_results");
+            span_bug!(self.span, "`hir::InferArg` outside of a body");
         }
         intravisit::walk_inf(self, inf);
     }
 
     fn visit_trait_ref(&mut self, trait_ref: &'tcx hir::TraitRef<'tcx>) {
         self.span = trait_ref.path.span;
-        if self.maybe_typeck_results.is_none() {
-            // Avoid calling `hir_trait_to_predicates` in bodies, it will ICE.
-            // The traits' privacy in bodies is already checked as a part of trait object types.
+        if self.maybe_typeck_results.is_some() {
+            // Privacy of traits in bodies is checked as a part of trait object types.
+        } else {
             let bounds = rustc_hir_analysis::hir_trait_to_predicates(
                 self.tcx,
                 trait_ref,
@@ -1223,7 +1194,10 @@ impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
             hir::ExprKind::MethodCall(segment, ..) => {
                 // Method calls have to be checked specially.
                 self.span = segment.ident.span;
-                if let Some(def_id) = self.typeck_results().type_dependent_def_id(expr.hir_id) {
+                let typeck_results = self
+                    .maybe_typeck_results
+                    .unwrap_or_else(|| span_bug!(self.span, "`hir::Expr` outside of a body"));
+                if let Some(def_id) = typeck_results.type_dependent_def_id(expr.hir_id) {
                     if self.visit(self.tcx.type_of(def_id).instantiate_identity()).is_break() {
                         return;
                     }
@@ -1251,9 +1225,13 @@ impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
                 Res::Def(kind, def_id) => Some((kind, def_id)),
                 _ => None,
             },
-            hir::QPath::TypeRelative(..) | hir::QPath::LangItem(..) => self
-                .maybe_typeck_results
-                .and_then(|typeck_results| typeck_results.type_dependent_def(id)),
+            hir::QPath::TypeRelative(..) | hir::QPath::LangItem(..) => {
+                match self.maybe_typeck_results {
+                    Some(typeck_results) => typeck_results.type_dependent_def(id),
+                    // FIXME: Check type-relative associated types in signatures.
+                    None => None,
+                }
+            }
         };
         let def = def.filter(|(kind, _)| {
             matches!(
@@ -1307,15 +1285,6 @@ impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
 
         intravisit::walk_local(self, local);
     }
-
-    // Check types in item interfaces.
-    fn visit_item(&mut self, item: &'tcx hir::Item<'tcx>) {
-        let orig_current_item = mem::replace(&mut self.current_item, item.owner_id.def_id);
-        let old_maybe_typeck_results = self.maybe_typeck_results.take();
-        intravisit::walk_item(self, item);
-        self.maybe_typeck_results = old_maybe_typeck_results;
-        self.current_item = orig_current_item;
-    }
 }
 
 impl<'tcx> DefIdVisitor<'tcx> for TypePrivacyVisitor<'tcx> {
@@ -1785,13 +1754,8 @@ fn check_mod_privacy(tcx: TyCtxt<'_>, module_def_id: LocalModDefId) {
 
     // Check privacy of explicitly written types and traits as well as
     // inferred types of expressions and patterns.
-    let mut visitor = TypePrivacyVisitor {
-        tcx,
-        maybe_typeck_results: None,
-        current_item: module_def_id.to_local_def_id(),
-        span,
-    };
-    intravisit::walk_mod(&mut visitor, module, hir_id);
+    let mut visitor = TypePrivacyVisitor { tcx, module_def_id, maybe_typeck_results: None, span };
+    tcx.hir().visit_item_likes_in_module(module_def_id, &mut visitor);
 }
 
 fn effective_visibilities(tcx: TyCtxt<'_>, (): ()) -> &EffectiveVisibilities {

tests/ui/privacy/private-type-in-interface.rs

@@ -26,6 +26,7 @@ type A = <m::Alias as m::Trait>::X; //~ ERROR type `Priv` is private
 trait Tr2<T> {}
 impl<T> Tr2<T> for u8 {}
 fn g() -> impl Tr2<m::Alias> { 0 } //~ ERROR type `Priv` is private
+                                   //~| ERROR type `Priv` is private
 fn g_ext() -> impl Tr2<ext::Alias> { 0 } //~ ERROR type `ext::Priv` is private
-
+                                         //~| ERROR type `ext::Priv` is private
 fn main() {}

tests/ui/privacy/private-type-in-interface.stderr

@@ -46,11 +46,23 @@ error: type `Priv` is private
 LL | fn g() -> impl Tr2<m::Alias> { 0 }
    |           ^^^^^^^^^^^^^^^^^^ private type
 
+error: type `Priv` is private
+  --> $DIR/private-type-in-interface.rs:28:16
+   |
+LL | fn g() -> impl Tr2<m::Alias> { 0 }
+   |                ^^^^^^^^^^^^^ private type
+
 error: type `ext::Priv` is private
-  --> $DIR/private-type-in-interface.rs:29:15
+  --> $DIR/private-type-in-interface.rs:30:15
    |
 LL | fn g_ext() -> impl Tr2<ext::Alias> { 0 }
    |               ^^^^^^^^^^^^^^^^^^^^ private type
 
-error: aborting due to 9 previous errors
+error: type `ext::Priv` is private
+  --> $DIR/private-type-in-interface.rs:30:20
+   |
+LL | fn g_ext() -> impl Tr2<ext::Alias> { 0 }
+   |                    ^^^^^^^^^^^^^^^ private type
+
+error: aborting due to 11 previous errors