Address review comments

Author: petrochenkov

src/librustc_privacy/lib.rs

@@ -51,8 +51,8 @@ mod diagnostics;
 /// in `impl Trait`, see individual commits in `DefIdVisitorSkeleton::visit_ty`.
 trait DefIdVisitor<'a, 'tcx: 'a> {
     fn tcx(&self) -> TyCtxt<'a, 'tcx, 'tcx>;
-    fn recurse(&self) -> bool { true }
-    fn recurse_into_assoc_tys(&self) -> bool { true }
+    fn shallow(&self) -> bool { false }
+    fn skip_assoc_tys(&self) -> bool { false }
     fn visit_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> bool;
 
     /// Not overridden, but used to actually visit types and traits.
@@ -88,7 +88,7 @@ impl<'a, 'tcx, V> DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
     fn visit_trait(&mut self, trait_ref: TraitRef<'tcx>) -> bool {
         let TraitRef { def_id, substs } = trait_ref;
         self.def_id_visitor.visit_def_id(def_id, "trait", &trait_ref) ||
-        self.def_id_visitor.recurse() && substs.visit_with(self)
+        (!self.def_id_visitor.shallow() && substs.visit_with(self))
     }
 
     fn visit_predicates(&mut self, predicates: Lrc<ty::GenericPredicates<'tcx>>) -> bool {
@@ -140,6 +140,9 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
                 if self.def_id_visitor.visit_def_id(def_id, "type", ty) {
                     return true;
                 }
+                if self.def_id_visitor.shallow() {
+                    return false;
+                }
                 // Default type visitor doesn't visit signatures of fn types.
                 // Something like `fn() -> Priv {my_func}` is considered a private type even if
                 // `my_func` is public, so we need to visit signatures.
@@ -161,7 +164,7 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
                 }
             }
             ty::Projection(proj) | ty::UnnormalizedProjection(proj) => {
-                if !self.def_id_visitor.recurse_into_assoc_tys() {
+                if self.def_id_visitor.skip_assoc_tys() {
                     // Visitors searching for minimal visibility/reachability want to
                     // conservatively approximate associated types like `<Type as Trait>::Alias`
                     // as visible/reachable even if both `Type` and `Trait` are private.
@@ -173,6 +176,8 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
                 return self.visit_trait(proj.trait_ref(tcx));
             }
             ty::Dynamic(predicates, ..) => {
+                // All traits in the list are considered the "primary" part of the type
+                // and are visited by shallow visitors.
                 for predicate in *predicates.skip_binder() {
                     let trait_ref = match *predicate {
                         ty::ExistentialPredicate::Trait(trait_ref) => trait_ref,
@@ -189,9 +194,13 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
             ty::Opaque(def_id, ..) => {
                 // Skip repeated `Opaque`s to avoid infinite recursion.
                 if self.visited_opaque_tys.insert(def_id) {
-                    // Default type visitor doesn't visit traits in `impl Trait`.
-                    // Something like `impl PrivTr` is considered a private type,
-                    // so we need to visit the traits additionally.
+                    // The intent is to treat `impl Trait1 + Trait2` identically to
+                    // `dyn Trait1 + Trait2`. Therefore we ignore def-id of the opaque type itself
+                    // (it either has no visibility, or its visibility is insignificant, like
+                    // visibilities of type aliases) and recurse into predicates instead to go
+                    // through the trait list (default type visitor doesn't visit those traits).
+                    // All traits in the list are considered the "primary" part of the type
+                    // and are visited by shallow visitors.
                     if self.visit_predicates(tcx.predicates_of(def_id)) {
                         return true;
                     }
@@ -208,7 +217,7 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
                 bug!("unexpected type: {:?}", ty),
         }
 
-        self.def_id_visitor.recurse() && ty.super_visit_with(self)
+        !self.def_id_visitor.shallow() && ty.super_visit_with(self)
     }
 }
 
@@ -327,8 +336,8 @@ struct FindMin<'a, 'tcx, VL: VisibilityLike> {
 
 impl<'a, 'tcx, VL: VisibilityLike> DefIdVisitor<'a, 'tcx> for FindMin<'a, 'tcx, VL> {
     fn tcx(&self) -> TyCtxt<'a, 'tcx, 'tcx> { self.tcx }
-    fn recurse(&self) -> bool { VL::RECURSE }
-    fn recurse_into_assoc_tys(&self) -> bool { false }
+    fn shallow(&self) -> bool { VL::SHALLOW }
+    fn skip_assoc_tys(&self) -> bool { true }
     fn visit_def_id(&mut self, def_id: DefId, _kind: &str, _descr: &dyn fmt::Display) -> bool {
         self.min = VL::new_min(self, def_id);
         false
@@ -337,10 +346,10 @@ impl<'a, 'tcx, VL: VisibilityLike> DefIdVisitor<'a, 'tcx> for FindMin<'a, 'tcx,
 
 trait VisibilityLike: Sized {
     const MAX: Self;
-    const RECURSE: bool = true;
+    const SHALLOW: bool = false;
     fn new_min<'a, 'tcx>(find: &FindMin<'a, 'tcx, Self>, def_id: DefId) -> Self;
 
-    // Returns an over-approximation (`recurse_into_assoc_tys` = false) of visibility due to
+    // Returns an over-approximation (`skip_assoc_tys` = true) of visibility due to
     // associated types for which we can't determine visibility precisely.
     fn of_impl<'a, 'tcx>(node_id: ast::NodeId, tcx: TyCtxt<'a, 'tcx, 'tcx>,
                          access_levels: &'a AccessLevels) -> Self {
@@ -365,8 +374,12 @@ impl VisibilityLike for Option<AccessLevel> {
     // It can make an impl reachable even some components of its type or trait are unreachable.
     // E.g. methods of `impl ReachableTrait<UnreachableTy> for ReachableTy<UnreachableTy> { ... }`
     // can be usable from other crates (#57264). So we skip substs when calculating reachability
-    // and consider an impl reachable if its "primary" type and trait are reachable.
-    const RECURSE: bool = false;
+    // and consider an impl reachable if its "shallow" type and trait are reachable.
+    //
+    // The assumption we make here is that type-inference won't let you use an impl without knowing
+    // both "shallow" version of its self type and "shallow" version of its trait if it exists
+    // (which require reaching the `DefId`s in them).
+    const SHALLOW: bool = true;
     fn new_min<'a, 'tcx>(find: &FindMin<'a, 'tcx, Self>, def_id: DefId) -> Self {
         cmp::min(if let Some(node_id) = find.tcx.hir().as_local_node_id(def_id) {
             find.access_levels.map.get(&node_id).cloned()