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

Type privacy polishing #46083

Merged
merged 5 commits into from
Dec 21, 2017
Merged

Type privacy polishing #46083

merged 5 commits into from
Dec 21, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 18, 2017

Various preparations before implementing rust-lang/rfcs#2145 containing final minor breaking changes (mostly for unstable code or code using allow(private_in_public)).
(Continuation of #42125, #44633 and #41332.)

It would be good to run crater on this.
r? @eddyb

@eddyb
Copy link
Member

eddyb commented Nov 18, 2017

@bors try

@bors
Copy link
Contributor

bors commented Nov 18, 2017

⌛ Trying commit 49c642c5fcda8a55fe0957b28298c3b602ced55d with merge 5ce31c1635dfa52f6071a39712fd3d50c57a5767...

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM

@eddyb
Copy link
Member

eddyb commented Nov 18, 2017

cc @nikomatsakis

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2017
@bors
Copy link
Contributor

bors commented Nov 18, 2017

☀️ Test successful - status-travis
State: approved= try=True

@petrochenkov
Copy link
Contributor Author

ping @aidanhs @Mark-Simulacrum
Crater run is needed.

@aidanhs
Copy link
Member

aidanhs commented Nov 21, 2017

Crater run started

@carols10cents
Copy link
Member

Ping @aidanhs -- has crater finished?

@aidanhs
Copy link
Member

aidanhs commented Nov 27, 2017

Unfortunately the build failed because the disk filled up (I'm not clear why, but it did), so I've restarted the run - it'll be done in another few days. Sorry!

@aidanhs
Copy link
Member

aidanhs commented Nov 30, 2017

Hi @petrochenkov (crater requester), @eddyb (reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-46083/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're interested)

@petrochenkov
Copy link
Contributor Author

Three legit regressions:

  • mioco-0.8.1 (root, private-in-public in associated types promoted to error)
  • tick-0.0.1 (root, private-in-public in associated types promoted to error)
  • razielgn.redis.fb38f9bf458c085e2d615c29488f1ec1fb09e723 (non-root, caused by mioco-0.8.1)

I'll send PRs.

@petrochenkov
Copy link
Contributor Author

Rebased and sent PRs to affected crates.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 1, 2017
@shepmaster shepmaster added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2017
@shepmaster
Copy link
Member

Ping from triage! It's been over 7 days since we heard from reviewer @eddyb. Please assign a new reviewer @rust-lang/compiler.

@eddyb
Copy link
Member

eddyb commented Dec 9, 2017

@shepmaster Oh, sorry, IMO this is good to go, I just wanted an extra confirmation.
r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Dec 9, 2017
@aidanhs
Copy link
Member

aidanhs commented Dec 14, 2017

Ping @nikomatsakis for review! (also on IRC)

@bors
Copy link
Contributor

bors commented Dec 15, 2017

☔ The latest upstream changes (presumably #46641) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

Argh my apologies @petrochenkov

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2017

📌 Commit 679165c has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r-

well, r=me, unmergeable.

@petrochenkov
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 21, 2017

📌 Commit c6209a3 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 21, 2017

⌛ Testing commit c6209a3 with merge a12706c...

bors added a commit that referenced this pull request Dec 21, 2017
Type privacy polishing

Various preparations before implementing rust-lang/rfcs#2145 containing final minor breaking changes (mostly for unstable code or code using `allow(private_in_public)`).
(Continuation of #42125, #44633 and #41332.)

It would be good to run crater on this.
r? @eddyb
@bors
Copy link
Contributor

bors commented Dec 21, 2017

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

@bors bors merged commit c6209a3 into rust-lang:master Dec 21, 2017
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.
@petrochenkov petrochenkov deleted the morepriv branch June 5, 2019 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants