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.

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

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

Merged
merged 6 commits into from
Nov 3, 2015

Conversation

petrochenkov
Copy link
Contributor

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 #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

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)

@petrochenkov
Copy link
Contributor Author

Updated with comments and formatting

@alexcrichton
Copy link
Member

Carrying over from a collapsed comment

Should the checks in ItemImpl with a trait be public_ty || public_trait? I thought the methods are public/exported if either half of that is public? 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!

@petrochenkov
Copy link
Contributor Author

@alexcrichton
I've answered the comments. I edited some of the answers, so the versions received by mail may be incorrect.

In `middle::reachable` mark default impl of a trait method as reachable if this trait method is used from inlinable code
@petrochenkov
Copy link
Contributor Author

@alexcrichton
Updated with a fix for middle::reachable and a better explanation for impls in rustc_privacy

middle::reachable now marks trait items (and, consequently, their default impls) as reachable if they are used from inlinable code, so they are not required to always be exported.

r?

@petrochenkov
Copy link
Contributor Author

Updated with fixed FIXMEs

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.

@alexcrichton
Copy link
Member

Thanks @petrochenkov! Just one last nit and otherwise r=me

@petrochenkov
Copy link
Contributor Author

@alexcrichton
Updated

@alexcrichton
Copy link
Member

@bors: r+ 2a6f51b8bf8bb8ed6178e262698462cab4a694f3

Thanks again @petrochenkov!

@petrochenkov
Copy link
Contributor Author

Oh, no, I fixed a typo after r+

@alexcrichton
Copy link
Member

@bors: r+ ab7b345

@bors
Copy link
Contributor

bors commented Nov 2, 2015

⌛ Testing commit ab7b345 with merge e2bb53c...

bors added a commit that referenced this pull request Nov 2, 2015
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 #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
@bors bors merged commit ab7b345 into rust-lang:master Nov 3, 2015
bors added a commit that referenced this pull request Nov 6, 2015
Handle them in `middle::reachable` instead (no optimizations so far, just drop all trait impl items into the reachable set, as before). Addresses the concerns from #29291 (comment)
\+ In `middle::reachable` don't treat impls of `Drop` specially, they are subsumed by the general impl treatment.
\+ Add some tests checking reachability of trait methods written in UFCS form
\+ Minor refactoring in the second commit

r? @alexcrichton
@petrochenkov petrochenkov deleted the privacy branch November 22, 2015 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants