Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rustc_privacy: Expand public node set, build exported node set more correctly #29291

Merged
merged 6 commits into from
Nov 3, 2015
Merged

rustc_privacy: Expand public node set, build exported node set more correctly #29291

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
599df16
rustc_privacy: Expand public node set, build exported node set more c…
petrochenkov Oct 25, 2015
43b4373
Tweak stability to not require annotations on impl items and unnamed …
petrochenkov Oct 25, 2015
8cef018
Comments and formatting
petrochenkov Oct 27, 2015
243a524
Fix for middle::reachable + better comments and tests
petrochenkov Oct 28, 2015
61cbc84
Make fields and macro defs exported
petrochenkov Oct 29, 2015
ab7b345
Parens + issue number + typo
petrochenkov Nov 2, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/librustc/middle/stability.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
6 changes: 6 additions & 0 deletions src/librustc_front/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
229 changes: 137 additions & 92 deletions src/librustc_privacy/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
}
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this and the function below explaining the tuple return value? Just saying which bool is which is fine

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); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried by the number of FIXME annotations present here, especially in cases like this where the nodes were previously in the set of exported items. Could you elaborate on the ICEs you ran into? Could they be fixed as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields weren't previously in the exported set, i.e. it's not a regression.
I guess they are not encoded/decoded somewhere for cross-crate usage or something like this. I'll require time to investigate and it's not directly necessary for this PR of for the stability PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh fields, I missed that part!

I would still like to have a better handle on what all these mysterious ICEs are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ICE I see is due to absence of field ids from ctxt::map

}
}
}

// * 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.
//
Expand All @@ -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); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be public_ty || public_trait? I thought the methods are public/exported if either half of that is public?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hm, I guess exported is handled the same as it was before. I guess I don't quite understand the implication of this, what does it mean for a method in a trait impl to be a public item? The answer to that probably answers the && vs || question as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trait impls are public if both the trait and the type are public, in this case we can require stability annotations on them and document them in rustdoc.
I'm not sure trait impl methods are needed in the public set, given that stability annotations on them are going to be eradicated, maybe for rustdoc? But why not? At least it's not harmful.

Regarding the exported set, if the trait is exported, then impl is exported (with accordance to the comment above). This comment - // * Private trait impls for public types can be ignored. Besides, all traits are currently exported, so there's no distinction between exported_trait and exported_trait || exported_ty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustdoc uses the exported set currently, but only due to deficiencies of the public set, I guess.
I'll probably go ahead and patch it up when this PR lands. It makes sense to leave trait impl methods in the public set until then. I'll remove them if rustdoc doesn't need them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov

all impl methods need to be exported, because inference can be made to pick them. For example:

in crate foo

#![crate_type="rlib"]

pub trait Foo { fn poke(self); }
struct Bar;
impl Foo for Option<Bar> { fn poke(self) { } }

then in some other crate:

extern crate foo;

use foo::Foo;

fn main() {
    None.poke();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't happen with inherent impls for some reason:

Crate 1:

struct PrivateStruct;

impl PrivateStruct {
    fn private_inherent_method(&self) { }
}

pub fn public_generic_function<T>(t: T) {
    PrivateStruct.private_inherent_method();
}

Crate 2:

extern crate crate1;

pub fn main() {
    crate1::public_generic_function(1); // Ok!
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested, why do non-exported free functions used in public generic functions don't result in linker errors, but trait methods do? Because the set of external linker symbols is built somewhere before monomorphization? (Is middle::reachable related?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov

Option<Foo> is a private type, as outer crates can't refer to it. You could do the same thing with impl Trait<Foo> for ()

Your example is a different issue - the problem is that reachability checking does not handle private traits correctly (this is indeed the job of middle::reachable), so while public_generic_function contains a reference to <PrivateStruct as PrivateTrait>::private_inherent_method, the method is not exported (shouldn't these be monomorphized anyway?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the suspicion this line is at fault. The code works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arielb1
That line was indeed at fault, but it was mitigated by rustc_privacy. (The "undefined reference" in my example wasn't an actual error, it's from my temporary branch where "private trait for private type" impls are not exported)

By the way, does rustc have a function like get_impl_method_id_if_it_is_available_without_doing_monomorphization(MethodCall) -> Option<ImplOrTraitItem>?
Example:

trait Tr {
    fn method(&self) {} // ID_1 (default impl)
}

impl Tr for u8 {}
impl Tr for u16 {
    fn method(&self) {} // ID_2
}

fn generic<T: Tr>(arg: T) {
    0u8.method(); // concrete context, get_impl_method_id_... returns Some(ID_1)
    0u16.method(); // concrete context, get_impl_method_id_... returns Some(ID_2)
    arg.method(); // generic context, get_impl_method_id_... returns None
}

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(..) => {},
Expand All @@ -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 ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add parens around one of the clauses here? I always forget the precedence of && and ||...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

self.reexports.contains(&foreign_item.id);

if public { self.public_items.insert(foreign_item.id); }
if exported { self.exported_items.insert(foreign_item.id); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think stylistically in rustfmt it never formats if statements on one line, so to head off the churn could this go ahead and move to lines after braces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay (although I'm a fan of one-liners for short blocks)

}
}

_ => {}
}

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) {
Expand All @@ -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);
}
}

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these empty visit_block and visit_expr calls be removed due to this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not without auditing the rest of VisiblePrivateTypesVisitor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It contains some logic assuming that it is not... very recursive. (It is even mentioned in the comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it kinda odd to document inside code why code is wrong, can you instead open an issue with an instance of this and then reference it from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #29524

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add that here in the comment as well?

fn visit_block(&mut self, _: &hir::Block) {}
fn visit_expr(&mut self, _: &hir::Expr) {}
}
Expand Down Expand Up @@ -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
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/libsyntax/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down