From 599df16551ea12605ca50e182237e7c50a29af40 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 25 Oct 2015 04:42:31 +0300 Subject: [PATCH 1/6] rustc_privacy: Expand public node set, build exported node set more correctly --- src/librustc_front/hir.rs | 6 + src/librustc_privacy/lib.rs | 229 +++++++++++++++++++++--------------- src/libsyntax/ast.rs | 6 + 3 files changed, 149 insertions(+), 92 deletions(-) diff --git a/src/librustc_front/hir.rs b/src/librustc_front/hir.rs index e62eadaa4387d..88b64f6359ed5 100644 --- a/src/librustc_front/hir.rs +++ b/src/librustc_front/hir.rs @@ -1159,6 +1159,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 608558ac2bdb9..76ff96d629821 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,9 +172,12 @@ 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. Public items are mostly a subset of exported items with exception of + // fields and exported macros - they are public, but not exported. + // FIXME: Make fields and exported macros exported as well (requires fixing resulting ICEs) public_items: PublicItems, prev_public: bool, } @@ -194,58 +197,103 @@ impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> { fn exported_trait(&self, _id: ast::NodeId) -> bool { true } + + 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) + } + } + + 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_public = self.prev_public && item.vis == hir::Public; self.prev_exported = self.exported_trait(item.id); + + self.maybe_insert_id(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 + if self.prev_public { self.public_items.insert(field.node.id); } + // FIXME: Make fields exported (requires fixing resulting ICEs) + // if self.prev_exported { self.exported_items.insert(field.node.id); } + } + } + } + + // * Inherent impls for public types only have public methods exported + // * Inherent impls for private types do not need to export their methods + // * Inherent impls themselves are not 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); } + } } } // 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. // @@ -257,83 +305,51 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { // 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 - } - }); + 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 { - 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(_) => {} - } - } + if public_ty && public_trait { self.public_items.insert(item.id); } + if exported_trait { self.exported_items.insert(item.id); } + + for impl_item in impl_items { + if public_ty && public_trait { self.public_items.insert(impl_item.id); } + if exported_trait { self.exported_items.insert(impl_item.id); } } } + // Default trait impls are exported for public 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 => { + 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 { + if self.prev_public { self.public_items.insert(field.node.id); } + // FIXME: Make fields exported (requires fixing resulting ICEs) + // if self.prev_exported { self.exported_items.insert(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 +363,37 @@ 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 +409,12 @@ 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.public_items.insert(md.id); + // FIXME: Make exported macros exported (requires fixing resulting ICEs) + // self.exported_items.insert(md.id); + } } //////////////////////////////////////////////////////////////////////////////// @@ -1447,11 +1487,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: 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) {} } @@ -1501,9 +1543,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 a45f5c9048c65..76c4088609bc2 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -1737,6 +1737,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 From 43b4373a23c88557f74c3e3f8b9d8761cc078a18 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 25 Oct 2015 05:30:26 +0300 Subject: [PATCH 2/6] Tweak stability to not require annotations on impl items and unnamed fields --- src/librustc/middle/stability.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/middle/stability.rs b/src/librustc/middle/stability.rs index 89c0f4f2251e5..91f3f80414c60 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) { From 8cef018d52fe71824498a4c8f9e1ece074083586 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 27 Oct 2015 21:56:48 +0300 Subject: [PATCH 3/6] Comments and formatting --- src/librustc_privacy/lib.rs | 58 ++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 76ff96d629821..774bc9df1141e 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -198,6 +198,7 @@ impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> { 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() { @@ -216,6 +217,7 @@ impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> { } } + // 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) { @@ -226,8 +228,12 @@ impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> { } 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); } + if self.prev_public { + self.public_items.insert(id); + } + if self.prev_exported { + self.exported_items.insert(id); + } } } @@ -269,7 +275,9 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { self.maybe_insert_id(variant.node.data.id()); for field in variant.node.data.fields() { // Variant fields are always public - if self.prev_public { self.public_items.insert(field.node.id); } + if self.prev_public { + self.public_items.insert(field.node.id); + } // FIXME: Make fields exported (requires fixing resulting ICEs) // if self.prev_exported { self.exported_items.insert(field.node.id); } } @@ -285,8 +293,12 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { 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); } + if public_ty { + self.public_items.insert(impl_item.id); + } + if exported_ty { + self.exported_items.insert(impl_item.id); + } } } } @@ -309,12 +321,20 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { 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); } - if exported_trait { self.exported_items.insert(item.id); } + if public_ty && public_trait { + self.public_items.insert(item.id); + } + if exported_trait { + self.exported_items.insert(item.id); + } for impl_item in impl_items { - if public_ty && public_trait { self.public_items.insert(impl_item.id); } - if exported_trait { self.exported_items.insert(impl_item.id); } + if public_ty && public_trait { + self.public_items.insert(impl_item.id); + } + if exported_trait { + self.exported_items.insert(impl_item.id); + } } } @@ -322,8 +342,12 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { 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); } + 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 @@ -342,7 +366,9 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { for field in def.fields() { // Struct fields can be public or private, so lets check if field.node.kind.visibility() == hir::Public { - if self.prev_public { self.public_items.insert(field.node.id); } + if self.prev_public { + self.public_items.insert(field.node.id); + } // FIXME: Make fields exported (requires fixing resulting ICEs) // if self.prev_exported { self.exported_items.insert(field.node.id); } } @@ -369,8 +395,12 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { 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); } + if public { + self.public_items.insert(foreign_item.id); + } + if exported { + self.exported_items.insert(foreign_item.id); + } } } From 243a524d060dcaf0b0381a8d5dea0a11b1474dfd Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 28 Oct 2015 03:38:22 +0300 Subject: [PATCH 4/6] Fix for middle::reachable + better comments and tests In `middle::reachable` mark default impl of a trait method as reachable if this trait method is used from inlinable code --- src/librustc/middle/reachable.rs | 13 ++---- src/librustc_privacy/lib.rs | 64 ++++++----------------------- src/test/auxiliary/issue-11225-3.rs | 29 +++++++++++++ src/test/run-pass/issue-11225-3.rs | 19 +++++++++ 4 files changed, 65 insertions(+), 60 deletions(-) create mode 100644 src/test/auxiliary/issue-11225-3.rs create mode 100644 src/test/run-pass/issue-11225-3.rs diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index a89da9704d9fd..d5c8e501ae359 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); } } _ => {} diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 774bc9df1141e..f13b14e64d700 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -183,21 +183,6 @@ struct EmbargoVisitor<'a, 'tcx: 'a> { } 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 { @@ -247,15 +232,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { // 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_public = self.prev_public && item.vis == hir::Public; - self.prev_exported = self.exported_trait(item.id); - - self.maybe_insert_id(item.id); - } - // Private by default, hence we only retain the "public chain" if // `pub` is explicitly listed. _ => { @@ -284,9 +260,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { } } - // * Inherent impls for public types only have public methods exported - // * Inherent impls for private types do not need to export their methods - // * Inherent impls themselves are not exported, they are nothing more than + // 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); @@ -303,42 +278,29 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { } } - // Implementations are a little tricky to determine what's exported - // out of them. Here's a few cases which are currently defined: - // - // * 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 + // 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 shoud 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); + let (public_trait, _exported_trait) = self.is_public_exported_trait(trait_ref); if public_ty && public_trait { self.public_items.insert(item.id); } - if exported_trait { - self.exported_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); } - if exported_trait { - self.exported_items.insert(impl_item.id); - } + self.exported_items.insert(impl_item.id); } } - // Default trait impls are exported for public traits + // 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); @@ -350,8 +312,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { } } - // Default methods on traits are all public so long as the trait - // is public + // 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 { self.maybe_insert_id(trait_item.id); diff --git a/src/test/auxiliary/issue-11225-3.rs b/src/test/auxiliary/issue-11225-3.rs new file mode 100644 index 0000000000000..51d73925dff21 --- /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 0000000000000..046c145e70e78 --- /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(); +} From 61cbc84480e4d3bf525836cef715ad2a904452f4 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 29 Oct 2015 21:54:55 +0300 Subject: [PATCH 5/6] Make fields and macro defs exported --- src/librustc/middle/reachable.rs | 10 ++-------- src/librustc_privacy/lib.rs | 20 ++++---------------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/librustc/middle/reachable.rs b/src/librustc/middle/reachable.rs index d5c8e501ae359..97ab9c2dfb7f9 100644 --- a/src/librustc/middle/reachable.rs +++ b/src/librustc/middle/reachable.rs @@ -223,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_privacy/lib.rs b/src/librustc_privacy/lib.rs index f13b14e64d700..aeed1c6f7940b 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -175,9 +175,7 @@ struct EmbargoVisitor<'a, 'tcx: 'a> { // 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 `public_items` set, not for privacy checking at - // all. Public items are mostly a subset of exported items with exception of - // fields and exported macros - they are public, but not exported. - // FIXME: Make fields and exported macros exported as well (requires fixing resulting ICEs) + // all. Invariant: at any moment public items are a subset of exported items. public_items: PublicItems, prev_public: bool, } @@ -251,11 +249,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { self.maybe_insert_id(variant.node.data.id()); for field in variant.node.data.fields() { // Variant fields are always public - if self.prev_public { - self.public_items.insert(field.node.id); - } - // FIXME: Make fields exported (requires fixing resulting ICEs) - // if self.prev_exported { self.exported_items.insert(field.node.id); } + self.maybe_insert_id(field.node.id); } } } @@ -328,11 +322,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { for field in def.fields() { // Struct fields can be public or private, so lets check if field.node.kind.visibility() == hir::Public { - if self.prev_public { - self.public_items.insert(field.node.id); - } - // FIXME: Make fields exported (requires fixing resulting ICEs) - // if self.prev_exported { self.exported_items.insert(field.node.id); } + self.maybe_insert_id(field.node.id); } } } @@ -403,9 +393,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { } fn visit_macro_def(&mut self, md: &'v hir::MacroDef) { - self.public_items.insert(md.id); - // FIXME: Make exported macros exported (requires fixing resulting ICEs) - // self.exported_items.insert(md.id); + self.maybe_insert_id(md.id); } } From ab7b3456d00d13e52fa6631eb745bd1a472b5731 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Tue, 3 Nov 2015 01:58:41 +0300 Subject: [PATCH 6/6] Parens + issue number + typo --- src/librustc_privacy/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index aeed1c6f7940b..f939bab380853 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -234,7 +234,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { // `pub` is explicitly listed. _ => { self.prev_public = self.prev_public && item.vis == hir::Public; - self.prev_exported = self.prev_exported && 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); @@ -275,7 +275,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> { // 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 shoud be reachable see + // 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); @@ -344,7 +344,7 @@ 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 || + let exported = (self.prev_exported && foreign_item.vis == hir::Public) || self.reexports.contains(&foreign_item.id); if public { @@ -1471,7 +1471,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> { // 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: Unfortunately this ^^^ is not true, blocks can contain + // 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) {}