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

privacy: Use common DefId visiting infrastructure for all privacy visitors #56878

Merged
merged 2 commits into from
Jan 1, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 16, 2018

One repeating pattern in privacy checking is going through a type, visiting all DefIds inside it and doing something with them.
This is the case because visibilities and reachabilities are attached to DefIds.

Previously various privacy visitors visited types slightly differently using their own methods, with most recently written TypePrivacyVisitor being the "gold standard".
This mostly worked okay, but differences could manifest in overly conservative reachability analysis, some errors being reported twice, some private-in-public lints (not errors) being wrongly reported or not reported.

This PR does something that I wanted to do since #32674 (comment) - factoring out the common visiting logic!
Now all the common logic is contained in struct DefIdVisitorSkeleton, with specific privacy visitors deciding only what to do with visited DefIds (via trait DefIdVisitor).

A bunch of cleanups is also applied in the process.
This area is somewhat tricky due to lots of easily miss-able details, but thankfully it's was well covered by tests in #46083 and previous PRs, so I'm relatively sure in the refactoring correctness.

Fixes #56837 (comment) in particular.
Also this will help with implementing #48054.

@petrochenkov
Copy link
Contributor Author

r? @arielb1

@arielb1
Copy link
Contributor

arielb1 commented Dec 16, 2018

I'll take a more detailed look tomorrow.

@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 17, 2018
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Dec 22, 2018

So, I started to factor out the common "visit all def-ids in a type" functionality and it turned out a bit trickier than expected, I'll update the PR today hopefully.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@petrochenkov petrochenkov changed the title privacy: Check non-principal components of trait object types privacy: Use common DefId visiting infra for all privacy visitors Dec 26, 2018
@petrochenkov petrochenkov changed the title privacy: Use common DefId visiting infra for all privacy visitors privacy: Use common DefId visiting infrastructure for all privacy visitors Dec 26, 2018
@petrochenkov
Copy link
Contributor Author

The PR is completely rewritten, see the new description.

cc people who reviewed previous PRs in this area (#46083 and friends)
@eddyb @nikomatsakis

let mut me = self;
(&mut me).read_to_end(buf)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unused function newly detected due to improved reachability analysis (which is based on privacy data).

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2018
@petrochenkov
Copy link
Contributor Author

Updated.

/// The idea is to visit "all components of a type", as documented in
/// https://github.com/rust-lang/rfcs/blob/master/text/2145-type-privacy.md#how-to-determine-visibility-of-a-type
/// Default type visitor (`TypeVisitor`) does most of the job, but it has some shortcomings.
/// First, it doesn't have overridable `fn visit_trait_ref`, so we have to catch trait def-ids
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it would be good to support this in TypeVisitor itself, but I'm not sure this PR is the best place for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 31, 2018
@arielb1
Copy link
Contributor

arielb1 commented Dec 31, 2018

r=me with over-approximation comment

@petrochenkov
Copy link
Contributor Author

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Dec 31, 2018

📌 Commit 60d1fa7 has been approved by arielb1

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2018
@bors
Copy link
Contributor

bors commented Dec 31, 2018

⌛ Testing commit 60d1fa7 with merge fe6a54d...

bors added a commit that referenced this pull request Dec 31, 2018
privacy: Use common `DefId` visiting infrastructure for all privacy visitors

One repeating pattern in privacy checking is going through a type, visiting all `DefId`s inside it and doing something with them.
This is the case because visibilities and reachabilities are attached to `DefId`s.

Previously various privacy visitors visited types slightly differently using their own methods, with most recently written `TypePrivacyVisitor` being the "gold standard".
This mostly worked okay, but differences could manifest in overly conservative reachability analysis, some errors being reported twice, some private-in-public lints (not errors) being wrongly reported or not reported.

This PR does something that I wanted to do since #32674 (comment) - factoring out the common visiting logic!
Now all the common logic is contained in `struct DefIdVisitorSkeleton`, with specific privacy visitors deciding only what to do with visited `DefId`s (via `trait DefIdVisitor`).

A bunch of cleanups is also applied in the process.
This area is somewhat tricky due to lots of easily miss-able details, but thankfully it's was well covered by tests in #46083 and previous PRs, so I'm relatively sure in the refactoring correctness.

Fixes #56837 (comment) in particular.
Also this will help with implementing #48054.
@bors
Copy link
Contributor

bors commented Jan 1, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing fe6a54d to master...

@bors bors merged commit 60d1fa7 into rust-lang:master Jan 1, 2019
bors added a commit that referenced this pull request Jan 6, 2019
privacy: Fix regression in impl reachability

Rollback to pre-#56878 logic of determining reachability.
`reachability(impl Trait<Substs> for Type<Substs>) = reachability(Trait & Type)`, substs are ignored.

Fixes #57264
@@ -709,6 +709,9 @@ define_print! {

define_print! {
('tcx) ty::ExistentialTraitRef<'tcx>, (self, f, cx) {
display {
cx.parameterized(f, self.substs, self.def_id, &[])
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong. The substs are missing the Self type, which is why the debug {...} implementation below does through all of that trouble of creating a TraitRef.

I think the correct fix is moving the code from debug {...} to display {...} and changing debug {...} to contain just self.print_display(f, cx).

Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019
privacy: Fix private-in-public check for existential types

Fixes rust-lang#53546 (regression from rust-lang#56878)

r? @arielb1
Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019
privacy: Fix private-in-public check for existential types

Fixes rust-lang#53546 (regression from rust-lang#56878)

r? @arielb1
@petrochenkov petrochenkov deleted the privdyn branch June 5, 2019 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants