Auto merge of #29291 - petrochenkov:privacy, r=alexcrichton

The public set is expanded with trait items, impls and their items, foreign items, exported macros, variant fields, i.e. all the missing parts. Now it's a subset of the exported set.
This is needed for https://github.com/rust-lang/rust/pull/29083 because stability annotation pass uses the public set and all things listed above need to be annotated.
Rustdoc can now be migrated to the public set as well, I guess.

Exported set is now slightly more correct with regard to exported items in blocks - 1) blocks in foreign items are considered and 2) publicity is not inherited from the block's parent - if a function is public it doesn't mean structures defined in its body are public.

r? @alexcrichton or maybe someone else

7 files changed, 217 insertions, 143 deletions

diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs
index a89da9704d9..97ab9c2dfb7 100644
--- a/src/librustc/middle/reachable.rs
+++ b/src/librustc/middle/reachable.rs
@@ -125,16 +125,11 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ReachableContext<'a, 'tcx> {
             hir::ExprMethodCall(..) => {
                 let method_call = ty::MethodCall::expr(expr.id);
                 let def_id = self.tcx.tables.borrow().method_map[&method_call].def_id;
-                match self.tcx.impl_or_trait_item(def_id).container() {
-                    ty::ImplContainer(_) => {
-                        if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
-                            if self.def_id_represents_local_inlined_item(def_id) {
-                                self.worklist.push(node_id)
-                            }
-                            self.reachable_symbols.insert(node_id);
-                        }
+                if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
+                    if self.def_id_represents_local_inlined_item(def_id) {
+                        self.worklist.push(node_id)
                     }
-                    ty::TraitContainer(_) => {}
+                    self.reachable_symbols.insert(node_id);
                 }
             }
             _ => {}
@@ -228,14 +223,8 @@ impl<'a, 'tcx> ReachableContext<'a, 'tcx> {
                 continue
             }
 
-            match self.tcx.map.find(search_item) {
-                Some(ref item) => self.propagate_node(item, search_item),
-                None if search_item == ast::CRATE_NODE_ID => {}
-                None => {
-                    self.tcx.sess.bug(&format!("found unmapped ID in worklist: \
-                                               {}",
-                                              search_item))
-                }
+            if let Some(ref item) = self.tcx.map.find(search_item) {
+                self.propagate_node(item, search_item);
             }
         }
     }
diff --git a/src/librustc/middle/stability.rs b/src/librustc/middle/stability.rs
index 111506eb108..4cc374431f4 100644
--- a/src/librustc/middle/stability.rs
+++ b/src/librustc/middle/stability.rs
@@ -217,7 +217,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for Annotator<'a, 'tcx> {
 
     fn visit_impl_item(&mut self, ii: &hir::ImplItem) {
         self.annotate(ii.id, true, &ii.attrs, ii.span,
-                      |v| visit::walk_impl_item(v, ii), true);
+                      |v| visit::walk_impl_item(v, ii), false);
     }
 
     fn visit_variant(&mut self, var: &Variant, g: &'v Generics, item_id: NodeId) {
@@ -227,7 +227,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for Annotator<'a, 'tcx> {
 
     fn visit_struct_field(&mut self, s: &StructField) {
         self.annotate(s.node.id, true, &s.node.attrs, s.span,
-                      |v| visit::walk_struct_field(v, s), true);
+                      |v| visit::walk_struct_field(v, s), !s.node.kind.is_unnamed());
     }
 
     fn visit_foreign_item(&mut self, i: &hir::ForeignItem) {
diff --git a/src/librustc_front/hir.rs b/src/librustc_front/hir.rs
index 40dd4b66751..6004a1c6556 100644
--- a/src/librustc_front/hir.rs
+++ b/src/librustc_front/hir.rs
@@ -1148,6 +1148,12 @@ impl StructFieldKind {
             NamedField(..) => false,
         }
     }
+
+    pub fn visibility(&self) -> Visibility {
+        match *self {
+            NamedField(_, vis) | UnnamedField(vis) => vis
+        }
+    }
 }
 
 /// Fields and Ids of enum variants and structs
diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs
index fbcb735a233..4fbe3a68335 100644
--- a/src/librustc_privacy/lib.rs
+++ b/src/librustc_privacy/lib.rs
@@ -162,7 +162,7 @@ struct EmbargoVisitor<'a, 'tcx: 'a> {
     // This is a list of all exported items in the AST. An exported item is any
     // function/method/item which is usable by external crates. This essentially
     // means that the result is "public all the way down", but the "path down"
-    // may jump across private boundaries through reexport statements.
+    // may jump across private boundaries through reexport statements or type aliases.
     exported_items: ExportedItems,
 
     // This sets contains all the destination nodes which are publicly
@@ -172,168 +172,162 @@ struct EmbargoVisitor<'a, 'tcx: 'a> {
     // destination must also be exported.
     reexports: NodeSet,
 
+    // Items that are directly public without help of reexports or type aliases.
     // These two fields are closely related to one another in that they are only
-    // used for generation of the 'PublicItems' set, not for privacy checking at
-    // all
+    // used for generation of the `public_items` set, not for privacy checking at
+    // all. Invariant: at any moment public items are a subset of exported items.
     public_items: PublicItems,
     prev_public: bool,
 }
 
 impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> {
-    // There are checks inside of privacy which depend on knowing whether a
-    // trait should be exported or not. The two current consumers of this are:
-    //
-    //  1. Should default methods of a trait be exported?
-    //  2. Should the methods of an implementation of a trait be exported?
-    //
-    // The answer to both of these questions partly rely on whether the trait
-    // itself is exported or not. If the trait is somehow exported, then the
-    // answers to both questions must be yes. Right now this question involves
-    // more analysis than is currently done in rustc, so we conservatively
-    // answer "yes" so that all traits need to be exported.
-    fn exported_trait(&self, _id: ast::NodeId) -> bool {
-        true
+    // Returns tuple (is_public, is_exported) for a type
+    fn is_public_exported_ty(&self, ty: &hir::Ty) -> (bool, bool) {
+        if let hir::TyPath(..) = ty.node {
+            match self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def() {
+                def::DefPrimTy(..) | def::DefSelfTy(..) => (true, true),
+                def => {
+                    if let Some(node_id) = self.tcx.map.as_local_node_id(def.def_id()) {
+                        (self.public_items.contains(&node_id),
+                         self.exported_items.contains(&node_id))
+                    } else {
+                        (true, true)
+                    }
+                }
+            }
+        } else {
+            (true, true)
+        }
     }
-}
 
-impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
-    fn visit_item(&mut self, item: &hir::Item) {
-        let orig_all_pub = self.prev_public;
-        self.prev_public = orig_all_pub && item.vis == hir::Public;
+    // Returns tuple (is_public, is_exported) for a trait
+    fn is_public_exported_trait(&self, trait_ref: &hir::TraitRef) -> (bool, bool) {
+        let did = self.tcx.trait_ref_to_def_id(trait_ref);
+        if let Some(node_id) = self.tcx.map.as_local_node_id(did) {
+            (self.public_items.contains(&node_id), self.exported_items.contains(&node_id))
+        } else {
+            (true, true)
+        }
+    }
+
+    fn maybe_insert_id(&mut self, id: ast::NodeId) {
         if self.prev_public {
-            self.public_items.insert(item.id);
+            self.public_items.insert(id);
+        }
+        if self.prev_exported {
+            self.exported_items.insert(id);
         }
+    }
+}
 
+impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
+    fn visit_item(&mut self, item: &hir::Item) {
+        let orig_all_public = self.prev_public;
         let orig_all_exported = self.prev_exported;
         match item.node {
             // impls/extern blocks do not break the "public chain" because they
-            // cannot have visibility qualifiers on them anyway
+            // cannot have visibility qualifiers on them anyway. They are also not
+            // added to public/exported sets based on inherited publicity.
             hir::ItemImpl(..) | hir::ItemDefaultImpl(..) | hir::ItemForeignMod(..) => {}
 
-            // Traits are a little special in that even if they themselves are
-            // not public they may still be exported.
-            hir::ItemTrait(..) => {
-                self.prev_exported = self.exported_trait(item.id);
-            }
-
             // Private by default, hence we only retain the "public chain" if
             // `pub` is explicitly listed.
             _ => {
-                self.prev_exported =
-                    (orig_all_exported && item.vis == hir::Public) ||
-                     self.reexports.contains(&item.id);
+                self.prev_public = self.prev_public && item.vis == hir::Public;
+                self.prev_exported = (self.prev_exported && item.vis == hir::Public) ||
+                                     self.reexports.contains(&item.id);
+
+                self.maybe_insert_id(item.id);
             }
         }
 
-        let public_first = self.prev_exported &&
-                           self.exported_items.insert(item.id);
-
         match item.node {
             // Enum variants inherit from their parent, so if the enum is
-            // public all variants are public unless they're explicitly priv
-            hir::ItemEnum(ref def, _) if public_first => {
+            // public all variants are public
+            hir::ItemEnum(ref def, _) => {
                 for variant in &def.variants {
-                    self.exported_items.insert(variant.node.data.id());
-                    self.public_items.insert(variant.node.data.id());
+                    self.maybe_insert_id(variant.node.data.id());
+                    for field in variant.node.data.fields() {
+                        // Variant fields are always public
+                        self.maybe_insert_id(field.node.id);
+                    }
                 }
             }
 
-            // Implementations are a little tricky to determine what's exported
-            // out of them. Here's a few cases which are currently defined:
-            //
-            // * Impls for private types do not need to export their methods
-            //   (either public or private methods)
-            //
-            // * Impls for public types only have public methods exported
-            //
-            // * Public trait impls for public types must have all methods
-            //   exported.
-            //
-            // * Private trait impls for public types can be ignored
-            //
-            // * Public trait impls for private types have their methods
-            //   exported. I'm not entirely certain that this is the correct
-            //   thing to do, but I have seen use cases of where this will cause
-            //   undefined symbols at linkage time if this case is not handled.
-            //
-            // * Private trait impls for private types can be completely ignored
-            hir::ItemImpl(_, _, _, _, ref ty, ref impl_items) => {
-                let public_ty = match ty.node {
-                    hir::TyPath(..) => {
-                        match self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def() {
-                            def::DefPrimTy(..) => true,
-                            def::DefSelfTy(..) => true,
-                            def => {
-                                let did = def.def_id();
-                                if let Some(node_id) = self.tcx.map.as_local_node_id(did) {
-                                    self.exported_items.contains(&node_id)
-                                } else {
-                                    true
-                                }
-                            }
+            // Public items in inherent impls for public/exported types are public/exported
+            // Inherent impls themselves are not public/exported, they are nothing more than
+            // containers for other items
+            hir::ItemImpl(_, _, _, None, ref ty, ref impl_items) => {
+                let (public_ty, exported_ty) = self.is_public_exported_ty(&ty);
+
+                for impl_item in impl_items {
+                    if impl_item.vis == hir::Public {
+                        if public_ty {
+                            self.public_items.insert(impl_item.id);
+                        }
+                        if exported_ty {
+                            self.exported_items.insert(impl_item.id);
                         }
                     }
-                    _ => true,
-                };
-                let tr = self.tcx.impl_trait_ref(self.tcx.map.local_def_id(item.id));
-                let public_trait = tr.clone().map_or(false, |tr| {
-                    if let Some(node_id) = self.tcx.map.as_local_node_id(tr.def_id) {
-                        self.exported_items.contains(&node_id)
-                    } else {
-                        true
-                    }
-                });
+                }
+            }
 
-                if public_ty || public_trait {
-                    for impl_item in impl_items {
-                        match impl_item.node {
-                            hir::ConstImplItem(..) => {
-                                if (public_ty && impl_item.vis == hir::Public)
-                                    || tr.is_some() {
-                                    self.exported_items.insert(impl_item.id);
-                                }
-                            }
-                            hir::MethodImplItem(ref sig, _) => {
-                                let meth_public = match sig.explicit_self.node {
-                                    hir::SelfStatic => public_ty,
-                                    _ => true,
-                                } && impl_item.vis == hir::Public;
-                                if meth_public || tr.is_some() {
-                                    self.exported_items.insert(impl_item.id);
-                                }
-                            }
-                            hir::TypeImplItem(_) => {}
-                        }
+            // It's not known until monomorphization if a trait impl item should be reachable
+            // from external crates or not. So, we conservatively mark all of them exported and
+            // the reachability pass (middle::reachable) marks all exported items as reachable.
+            // For example of private trait impl for private type that should be reachable see
+            // src/test/auxiliary/issue-11225-3.rs
+            hir::ItemImpl(_, _, _, Some(ref trait_ref), ref ty, ref impl_items) => {
+                let (public_ty, _exported_ty) = self.is_public_exported_ty(&ty);
+                let (public_trait, _exported_trait) = self.is_public_exported_trait(trait_ref);
+
+                if public_ty && public_trait {
+                    self.public_items.insert(item.id);
+                }
+                self.exported_items.insert(item.id);
+
+                for impl_item in impl_items {
+                    if public_ty && public_trait {
+                        self.public_items.insert(impl_item.id);
                     }
+                    self.exported_items.insert(impl_item.id);
+                }
+            }
+
+            // Default trait impls are public/exported for public/exported traits
+            hir::ItemDefaultImpl(_, ref trait_ref) => {
+                let (public_trait, exported_trait) = self.is_public_exported_trait(trait_ref);
+
+                if public_trait {
+                    self.public_items.insert(item.id);
+                }
+                if exported_trait {
+                    self.exported_items.insert(item.id);
                 }
             }
 
-            // Default methods on traits are all public so long as the trait
-            // is public
-            hir::ItemTrait(_, _, _, ref trait_items) if public_first => {
+            // Default methods on traits are all public/exported so long as the trait
+            // is public/exported
+            hir::ItemTrait(_, _, _, ref trait_items) => {
                 for trait_item in trait_items {
-                    debug!("trait item {}", trait_item.id);
-                    self.exported_items.insert(trait_item.id);
+                    self.maybe_insert_id(trait_item.id);
                 }
             }
 
             // Struct constructors are public if the struct is all public.
-            hir::ItemStruct(ref def, _) if public_first => {
+            hir::ItemStruct(ref def, _) => {
                 if !def.is_struct() {
-                    self.exported_items.insert(def.id());
+                    self.maybe_insert_id(def.id());
                 }
-                // fields can be public or private, so lets check
                 for field in def.fields() {
-                    let vis = match field.node.kind {
-                        hir::NamedField(_, vis) | hir::UnnamedField(vis) => vis
-                    };
-                    if vis == hir::Public {
-                        self.public_items.insert(field.node.id);
+                    // Struct fields can be public or private, so lets check
+                    if field.node.kind.visibility() == hir::Public {
+                        self.maybe_insert_id(field.node.id);
                     }
                 }
             }
 
-            hir::ItemTy(ref ty, _) if public_first => {
+            hir::ItemTy(ref ty, _) if self.prev_exported => {
                 if let hir::TyPath(..) = ty.node {
                     match self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def() {
                         def::DefPrimTy(..) | def::DefSelfTy(..) | def::DefTyParam(..) => {},
@@ -347,19 +341,41 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
                 }
             }
 
+            hir::ItemForeignMod(ref foreign_mod) => {
+                for foreign_item in &foreign_mod.items {
+                    let public = self.prev_public && foreign_item.vis == hir::Public;
+                    let exported = (self.prev_exported && foreign_item.vis == hir::Public) ||
+                                   self.reexports.contains(&foreign_item.id);
+
+                    if public {
+                        self.public_items.insert(foreign_item.id);
+                    }
+                    if exported {
+                        self.exported_items.insert(foreign_item.id);
+                    }
+                }
+            }
+
             _ => {}
         }
 
         visit::walk_item(self, item);
 
+        self.prev_public = orig_all_public;
         self.prev_exported = orig_all_exported;
-        self.prev_public = orig_all_pub;
     }
 
-    fn visit_foreign_item(&mut self, a: &hir::ForeignItem) {
-        if (self.prev_exported && a.vis == hir::Public) || self.reexports.contains(&a.id) {
-            self.exported_items.insert(a.id);
-        }
+    fn visit_block(&mut self, b: &'v hir::Block) {
+        let orig_all_public = replace(&mut self.prev_public, false);
+        let orig_all_exported = replace(&mut self.prev_exported, false);
+
+        // Blocks can have exported and public items, for example impls, but they always
+        // start as non-public and non-exported regardless of publicity of a function,
+        // constant, type, field, etc. in which this block resides
+        visit::walk_block(self, b);
+
+        self.prev_public = orig_all_public;
+        self.prev_exported = orig_all_exported;
     }
 
     fn visit_mod(&mut self, m: &hir::Mod, _sp: Span, id: ast::NodeId) {
@@ -375,6 +391,10 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
         }
         visit::walk_mod(self, m)
     }
+
+    fn visit_macro_def(&mut self, md: &'v hir::MacroDef) {
+        self.maybe_insert_id(md.id);
+    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1460,11 +1480,13 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
         }
     }
 
-
     // we don't need to introspect into these at all: an
     // expression/block context can't possibly contain exported things.
     // (Making them no-ops stops us from traversing the whole AST without
     // having to be super careful about our `walk_...` calls above.)
+    // FIXME(#29524): Unfortunately this ^^^ is not true, blocks can contain
+    // exported items (e.g. impls) and actual code in rustc itself breaks
+    // if we don't traverse blocks in `EmbargoVisitor`
     fn visit_block(&mut self, _: &hir::Block) {}
     fn visit_expr(&mut self, _: &hir::Expr) {}
 }
@@ -1514,9 +1536,12 @@ pub fn check_crate(tcx: &ty::ctxt,
         prev_public: true,
     };
     loop {
-        let before = visitor.exported_items.len();
+        let before = (visitor.exported_items.len(), visitor.public_items.len(),
+                      visitor.reexports.len());
         visit::walk_crate(&mut visitor, krate);
-        if before == visitor.exported_items.len() {
+        let after = (visitor.exported_items.len(), visitor.public_items.len(),
+                     visitor.reexports.len());
+        if after == before {
             break
         }
     }
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
index a6d90e15156..e56e49c4f49 100644
--- a/src/libsyntax/ast.rs
+++ b/src/libsyntax/ast.rs
@@ -1726,6 +1726,12 @@ impl StructFieldKind {
             NamedField(..) => false,
         }
     }
+
+    pub fn visibility(&self) -> Visibility {
+        match *self {
+            NamedField(_, vis) | UnnamedField(vis) => vis
+        }
+    }
 }
 
 /// Fields and Ids of enum variants and structs
diff --git a/src/test/auxiliary/issue-11225-3.rs b/src/test/auxiliary/issue-11225-3.rs
new file mode 100644
index 00000000000..51d73925dff
--- /dev/null
+++ b/src/test/auxiliary/issue-11225-3.rs
@@ -0,0 +1,29 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+trait PrivateTrait {
+    fn private_trait_method(&self);
+}
+
+struct PrivateStruct;
+
+impl PrivateStruct {
+    fn private_inherent_method(&self) { }
+}
+
+impl PrivateTrait for PrivateStruct {
+    fn private_trait_method(&self) { }
+}
+
+#[inline]
+pub fn public_generic_function() {
+    PrivateStruct.private_trait_method();
+    PrivateStruct.private_inherent_method();
+}
diff --git a/src/test/run-pass/issue-11225-3.rs b/src/test/run-pass/issue-11225-3.rs
new file mode 100644
index 00000000000..046c145e70e
--- /dev/null
+++ b/src/test/run-pass/issue-11225-3.rs
@@ -0,0 +1,19 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:issue-11225-3.rs
+
+// pretty-expanded FIXME #23616
+
+extern crate issue_11225_3;
+
+pub fn main() {
+    issue_11225_3::public_generic_function();
+}