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) + } + } + + // 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(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_pub = self.prev_public; - self.prev_public = orig_all_pub && item.vis == hir::Public; - if self.prev_public { - self.public_items.insert(item.id); - } - + 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 - } - } - } - } - _ => 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 - } - }); + // 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); - 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(_) => {} + 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); } } } } - // Default methods on traits are all public so long as the trait - // is public - hir::ItemTrait(_, _, _, ref trait_items) if public_first => { + // 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/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 or the MIT license +// , 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 or the MIT license +// , 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(); +}