]> git.lizzy.rs Git - rust.git/blobdiff - src/librustc_privacy/lib.rs
privacy: Fix regression in impl reachability
[rust.git] / src / librustc_privacy / lib.rs
index 0cd8c9da57e29875f0a08594bae2a60b87df8a19..2f0d53f1d81bd5146f037ee5018e97beb34a047a 100644 (file)
@@ -23,6 +23,7 @@
 use rustc::ty::{self, TyCtxt, Ty, TraitRef, TypeFoldable, GenericParamDefKind};
 use rustc::ty::fold::TypeVisitor;
 use rustc::ty::query::Providers;
+use rustc::ty::subst::Substs;
 use rustc::util::nodemap::NodeSet;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::sync::Lrc;
@@ -31,8 +32,7 @@
 use syntax::symbol::keywords;
 use syntax_pos::Span;
 
-use std::{cmp, fmt};
-use std::mem::replace;
+use std::{cmp, fmt, mem};
 use std::marker::PhantomData;
 
 mod diagnostics;
 
 /// Implemented to visit all `DefId`s in a type.
 /// Visiting `DefId`s is useful because visibilities and reachabilities are attached to them.
+/// The idea is to visit "all components of a type", as documented in
+/// https://github.com/rust-lang/rfcs/blob/master/text/2145-type-privacy.md#how-to-determine-visibility-of-a-type
+/// Default type visitor (`TypeVisitor`) does most of the job, but it has some shortcomings.
+/// First, it doesn't have overridable `fn visit_trait_ref`, so we have to catch trait def-ids
+/// manually. Second, it doesn't visit some type components like signatures of fn types, or traits
+/// 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 visit_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> bool;
 
@@ -79,33 +86,39 @@ impl<'a, 'tcx, V> DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
     where V: DefIdVisitor<'a, 'tcx> + ?Sized
 {
     fn visit_trait(&mut self, trait_ref: TraitRef<'tcx>) -> bool {
-        self.def_id_visitor.visit_def_id(trait_ref.def_id, "trait", &trait_ref) ||
-        trait_ref.visit_with(self)
+        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)
     }
 
     fn visit_predicates(&mut self, predicates: Lrc<ty::GenericPredicates<'tcx>>) -> bool {
-        for (predicate, _) in &predicates.predicates {
-            let trait_ref = match predicate {
+        let ty::GenericPredicates { parent: _, predicates } = &*predicates;
+        for (predicate, _span) in predicates {
+            match predicate {
                 ty::Predicate::Trait(poly_predicate) => {
-                    poly_predicate.skip_binder().trait_ref
+                    let ty::TraitPredicate { trait_ref } = *poly_predicate.skip_binder();
+                    if self.visit_trait(trait_ref) {
+                        return true;
+                    }
                 }
                 ty::Predicate::Projection(poly_predicate) => {
-                    if poly_predicate.skip_binder().ty.visit_with(self) {
+                    let ty::ProjectionPredicate { projection_ty, ty } =
+                        *poly_predicate.skip_binder();
+                    if ty.visit_with(self) {
+                        return true;
+                    }
+                    if self.visit_trait(projection_ty.trait_ref(self.def_id_visitor.tcx())) {
                         return true;
                     }
-                    poly_predicate.skip_binder().projection_ty.trait_ref(self.def_id_visitor.tcx())
                 }
                 ty::Predicate::TypeOutlives(poly_predicate) => {
-                    if poly_predicate.skip_binder().0.visit_with(self) {
+                    let ty::OutlivesPredicate(ty, _region) = *poly_predicate.skip_binder();
+                    if ty.visit_with(self) {
                         return true;
                     }
-                    continue;
                 }
-                ty::Predicate::RegionOutlives(..) => continue,
+                ty::Predicate::RegionOutlives(..) => {},
                 _ => bug!("unexpected predicate: {:?}", predicate),
-            };
-            if self.visit_trait(trait_ref) {
-                return true;
             }
         }
         false
@@ -117,6 +130,7 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
 {
     fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
         let tcx = self.def_id_visitor.tcx();
+        // Substs are not visited here because they are visited below in `super_visit_with`.
         match ty.sty {
             ty::Adt(&ty::AdtDef { did: def_id, .. }, ..) |
             ty::Foreign(def_id) |
@@ -126,14 +140,18 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
                 if self.def_id_visitor.visit_def_id(def_id, "type", ty) {
                     return true;
                 }
-                // Default type visitor doesn't visit fn signatures.
+                // 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.
                 if let ty::FnDef(..) = ty.sty {
                     if tcx.fn_sig(def_id).visit_with(self) {
                         return true;
                     }
                 }
-                // Inherent static methods don't have self type in substs,
-                // we have to check it additionally.
+                // Inherent static methods don't have self type in substs.
+                // Something like `fn() {my_method}` type of the method
+                // `impl Pub<Priv> { pub fn my_method() {} }` is considered a private type,
+                // so we need to visit the self type additionally.
                 if let Some(assoc_item) = tcx.opt_associated_item(def_id) {
                     if let ty::ImplContainer(impl_def_id) = assoc_item.container {
                         if tcx.type_of(impl_def_id).visit_with(self) {
@@ -151,17 +169,19 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
                     // free type aliases, but this isn't done yet.
                     return false;
                 }
-                // This will also visit substs, so we don't need to recurse.
+                // This will also visit substs if necessary, so we don't need to recurse.
                 return self.visit_trait(proj.trait_ref(tcx));
             }
             ty::Dynamic(predicates, ..) => {
                 for predicate in *predicates.skip_binder() {
-                    let def_id = match *predicate {
-                        ty::ExistentialPredicate::Trait(trait_ref) => trait_ref.def_id,
-                        ty::ExistentialPredicate::Projection(proj) => proj.trait_ref(tcx).def_id,
-                        ty::ExistentialPredicate::AutoTrait(def_id) => def_id,
+                    let trait_ref = match *predicate {
+                        ty::ExistentialPredicate::Trait(trait_ref) => trait_ref,
+                        ty::ExistentialPredicate::Projection(proj) => proj.trait_ref(tcx),
+                        ty::ExistentialPredicate::AutoTrait(def_id) =>
+                            ty::ExistentialTraitRef { def_id, substs: Substs::empty() },
                     };
-                    if self.def_id_visitor.visit_def_id(def_id, "type", ty) {
+                    let ty::ExistentialTraitRef { def_id, substs: _ } = trait_ref;
+                    if self.def_id_visitor.visit_def_id(def_id, "trait", &trait_ref) {
                         return true;
                     }
                 }
@@ -169,6 +189,9 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
             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.
                     if self.visit_predicates(tcx.predicates_of(def_id)) {
                         return true;
                     }
@@ -185,7 +208,7 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
                 bug!("unexpected type: {:?}", ty),
         }
 
-        ty.super_visit_with(self)
+        self.def_id_visitor.recurse() && ty.super_visit_with(self)
     }
 }
 
@@ -249,24 +272,22 @@ fn def_id_visibility<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
             };
             (ty::Visibility::from_hir(vis, node_id, tcx), vis.span, vis.node.descr())
         }
-        None => (tcx.visibility(def_id), tcx.def_span(def_id), "private"),
+        None => {
+            let vis = tcx.visibility(def_id);
+            let descr = if vis == ty::Visibility::Public { "public" } else { "private" };
+            (vis, tcx.def_span(def_id), descr)
+        }
     }
 }
 
 // Set the correct `TypeckTables` for the given `item_id` (or an empty table if
 // there is no `TypeckTables` for the item).
-fn update_tables<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                           item_id: ast::NodeId,
-                           tables: &mut &'a ty::TypeckTables<'tcx>,
-                           empty_tables: &'a ty::TypeckTables<'tcx>)
-                           -> &'a ty::TypeckTables<'tcx> {
-    let def_id = tcx.hir().local_def_id(item_id);
-
-    if tcx.has_typeck_tables(def_id) {
-        replace(tables, tcx.typeck_tables_of(def_id))
-    } else {
-        replace(tables, empty_tables)
-    }
+fn item_tables<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
+                         node_id: ast::NodeId,
+                         empty_tables: &'a ty::TypeckTables<'tcx>)
+                         -> &'a ty::TypeckTables<'tcx> {
+    let def_id = tcx.hir().local_def_id(node_id);
+    if tcx.has_typeck_tables(def_id) { tcx.typeck_tables_of(def_id) } else { empty_tables }
 }
 
 fn min<'a, 'tcx>(vis1: ty::Visibility, vis2: ty::Visibility, tcx: TyCtxt<'a, 'tcx, 'tcx>)
@@ -298,24 +319,54 @@ fn visit_vis(&mut self, vis: &'tcx hir::Visibility) {
 /// Visitor used to determine impl visibility and reachability.
 ////////////////////////////////////////////////////////////////////////////////
 
-struct FindMin<'a, 'tcx, M: Min> {
+struct FindMin<'a, 'tcx, VL: VisibilityLike> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
     access_levels: &'a AccessLevels,
-    min: M,
+    min: VL,
+}
+
+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 visit_def_id(&mut self, def_id: DefId, _kind: &str, _descr: &dyn fmt::Display) -> bool {
+        self.min = VL::new_min(self, def_id);
+        false
+    }
 }
 
-trait Min: Sized {
+trait VisibilityLike: Sized {
     const MAX: Self;
+    const RECURSE: bool = true;
     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
+    // 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 {
+        let mut find = FindMin { tcx, access_levels, min: Self::MAX };
+        let def_id = tcx.hir().local_def_id(node_id);
+        find.visit(tcx.type_of(def_id));
+        if let Some(trait_ref) = tcx.impl_trait_ref(def_id) {
+            find.visit_trait(trait_ref);
+        }
+        find.min
+    }
 }
-impl Min for ty::Visibility {
+impl VisibilityLike for ty::Visibility {
     const MAX: Self = ty::Visibility::Public;
     fn new_min<'a, 'tcx>(find: &FindMin<'a, 'tcx, Self>, def_id: DefId) -> Self {
         min(def_id_visibility(find.tcx, def_id).0, find.min, find.tcx)
     }
 }
-impl Min for Option<AccessLevel> {
+impl VisibilityLike for Option<AccessLevel> {
     const MAX: Self = Some(AccessLevel::Public);
+    // Type inference is very smart sometimes.
+    // 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;
     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()
@@ -325,26 +376,6 @@ fn new_min<'a, 'tcx>(find: &FindMin<'a, 'tcx, Self>, def_id: DefId) -> Self {
     }
 }
 
-impl<'a, 'tcx, M: Min> DefIdVisitor<'a, 'tcx> for FindMin<'a, 'tcx, M> {
-    fn tcx(&self) -> TyCtxt<'a, 'tcx, 'tcx> { self.tcx }
-    fn recurse_into_assoc_tys(&self) -> bool { false }
-    fn visit_def_id(&mut self, def_id: DefId, _kind: &str, _descr: &dyn fmt::Display) -> bool {
-        self.min = M::new_min(self, def_id);
-        false
-    }
-}
-
-fn impl_min<'a, 'tcx, M: Min>(tcx: TyCtxt<'a, 'tcx, 'tcx>, access_levels: &'a AccessLevels,
-                              node_id: ast::NodeId) -> M {
-    let mut find = FindMin { tcx, access_levels, min: M::MAX };
-    let def_id = tcx.hir().local_def_id(node_id);
-    find.visit(tcx.type_of(def_id));
-    if let Some(trait_ref) = tcx.impl_trait_ref(def_id) {
-        find.visit_trait(trait_ref);
-    }
-    find.min
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 /// The embargo visitor, used to determine the exports of the ast
 ////////////////////////////////////////////////////////////////////////////////
@@ -404,7 +435,7 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
     fn visit_item(&mut self, item: &'tcx hir::Item) {
         let inherited_item_level = match item.node {
             hir::ItemKind::Impl(..) =>
-                impl_min::<Option<AccessLevel>>(self.tcx, &self.access_levels, item.id),
+                Option::<AccessLevel>::of_impl(item.id, self.tcx, &self.access_levels),
             // Foreign modules inherit level from parents.
             hir::ItemKind::ForeignMod(..) => self.prev_level,
             // Other `pub` items inherit levels from parents.
@@ -575,7 +606,7 @@ fn visit_item(&mut self, item: &'tcx hir::Item) {
             }
         }
 
-        let orig_level = replace(&mut self.prev_level, item_level);
+        let orig_level = mem::replace(&mut self.prev_level, item_level);
         intravisit::walk_item(self, item);
         self.prev_level = orig_level;
     }
@@ -584,7 +615,7 @@ fn visit_block(&mut self, b: &'tcx hir::Block) {
         // Blocks can have public items, for example impls, but they always
         // start as completely private regardless of publicity of a function,
         // constant, type, field, etc., in which this block resides.
-        let orig_level = replace(&mut self.prev_level, None);
+        let orig_level = mem::replace(&mut self.prev_level, None);
         intravisit::walk_block(self, b);
         self.prev_level = orig_level;
     }
@@ -732,28 +763,31 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
     }
 
     fn visit_nested_body(&mut self, body: hir::BodyId) {
-        let orig_tables = replace(&mut self.tables, self.tcx.body_tables(body));
+        let orig_tables = mem::replace(&mut self.tables, self.tcx.body_tables(body));
         let body = self.tcx.hir().body(body);
         self.visit_body(body);
         self.tables = orig_tables;
     }
 
     fn visit_item(&mut self, item: &'tcx hir::Item) {
-        let orig_current_item = replace(&mut self.current_item, item.id);
-        let orig_tables = update_tables(self.tcx, item.id, &mut self.tables, self.empty_tables);
+        let orig_current_item = mem::replace(&mut self.current_item, item.id);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, item.id, self.empty_tables));
         intravisit::walk_item(self, item);
         self.current_item = orig_current_item;
         self.tables = orig_tables;
     }
 
     fn visit_trait_item(&mut self, ti: &'tcx hir::TraitItem) {
-        let orig_tables = update_tables(self.tcx, ti.id, &mut self.tables, self.empty_tables);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, ti.id, self.empty_tables));
         intravisit::walk_trait_item(self, ti);
         self.tables = orig_tables;
     }
 
     fn visit_impl_item(&mut self, ii: &'tcx hir::ImplItem) {
-        let orig_tables = update_tables(self.tcx, ii.id, &mut self.tables, self.empty_tables);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, ii.id, self.empty_tables));
         intravisit::walk_impl_item(self, ii);
         self.tables = orig_tables;
     }
@@ -864,8 +898,8 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
     }
 
     fn visit_nested_body(&mut self, body: hir::BodyId) {
-        let orig_tables = replace(&mut self.tables, self.tcx.body_tables(body));
-        let orig_in_body = replace(&mut self.in_body, true);
+        let orig_tables = mem::replace(&mut self.tables, self.tcx.body_tables(body));
+        let orig_in_body = mem::replace(&mut self.in_body, true);
         let body = self.tcx.hir().body(body);
         self.visit_body(body);
         self.tables = orig_tables;
@@ -1003,9 +1037,10 @@ fn visit_local(&mut self, local: &'tcx hir::Local) {
     // Check types in item interfaces.
     fn visit_item(&mut self, item: &'tcx hir::Item) {
         let orig_current_item =
-            replace(&mut self.current_item, self.tcx.hir().local_def_id(item.id));
-        let orig_in_body = replace(&mut self.in_body, false);
-        let orig_tables = update_tables(self.tcx, item.id, &mut self.tables, self.empty_tables);
+            mem::replace(&mut self.current_item, self.tcx.hir().local_def_id(item.id));
+        let orig_in_body = mem::replace(&mut self.in_body, false);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, item.id, self.empty_tables));
         intravisit::walk_item(self, item);
         self.tables = orig_tables;
         self.in_body = orig_in_body;
@@ -1013,13 +1048,15 @@ fn visit_item(&mut self, item: &'tcx hir::Item) {
     }
 
     fn visit_trait_item(&mut self, ti: &'tcx hir::TraitItem) {
-        let orig_tables = update_tables(self.tcx, ti.id, &mut self.tables, self.empty_tables);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, ti.id, self.empty_tables));
         intravisit::walk_trait_item(self, ti);
         self.tables = orig_tables;
     }
 
     fn visit_impl_item(&mut self, ii: &'tcx hir::ImplItem) {
-        let orig_tables = update_tables(self.tcx, ii.id, &mut self.tables, self.empty_tables);
+        let orig_tables =
+            mem::replace(&mut self.tables, item_tables(self.tcx, ii.id, self.empty_tables));
         intravisit::walk_impl_item(self, ii);
         self.tables = orig_tables;
     }
@@ -1576,7 +1613,7 @@ fn visit_item(&mut self, item: &'tcx hir::Item) {
             // A trait impl is public when both its type and its trait are public
             // Subitems of trait impls have inherited publicity.
             hir::ItemKind::Impl(.., ref trait_ref, _, ref impl_item_refs) => {
-                let impl_vis = impl_min::<ty::Visibility>(tcx, &Default::default(), item.id);
+                let impl_vis = ty::Visibility::of_impl(item.id, tcx, &Default::default());
                 self.check(item.id, impl_vis).generics().predicates();
                 for impl_item_ref in impl_item_refs {
                     let impl_item = tcx.hir().impl_item(impl_item_ref.id);