groundwork for RFC 1422, improve `PrivateItemsInPublicInterfacesVisitor` by jseyfried · Pull Request #32674 · rust-lang/rust

jseyfried · 2016-04-02T03:21:01Z

This PR lays groundwork for RFC 1422 (cc #32409) and improves PrivateItemsInPublicInterfacesVisitor. More specifically, it

Refactors away hir::Visibility::inherit_from, the semantics of which are obsolete.
Makes hir::Visibility non-Copy so that we will be able to add new variants to represent pub(restricted) (for example, Visibility::Restricted(Path)).
Adds a new Copy type ty::Visibility that represents a visibility value, i.e. a characterization of where an item is accessible. This is able to represent pub(restricted) visibilities.
Improves PrivateItemsInPublicInterfacesVisitor so that it checks for items in an interface that are less visible than the interface. This fixesPrivate structure can escape from its module through impl for another private structure #30079 but doesn't change any other behavior.

r? @nikomatsakis

jseyfried · 2016-04-02T03:22:49Z

cc @petrochenkov

jseyfried · 2016-04-02T03:27:57Z

required_visibility and min_visibility generalize is_quiet and is_public (respectively)

is_quiet is kind of a hack, I intended to split this visitor into two visitors (for checking T and Tr in impl Tr for T {....} and for everything else) and also harmonize it with some stuff in EmbargoVisitor...
Well, I guess I'll still do it, someday.

I think there's enough similarity to justify having just one visitor. Regardless, hack or not it generalizes well :)

petrochenkov · 2016-04-02T09:29:00Z

//< doesn't work with rustdoc

That's unfortunate, I'll make these preceding /// comments then

petrochenkov · 2016-04-02T10:40:15Z

Reviewed, LGTM.

petrochenkov · 2016-04-02T11:06:46Z

This is also a plugin-[breaking-change] due to the changes to AST
cc #31645

jseyfried · 2016-04-02T11:36:24Z

@petrochenkov thanks!

This is also a plugin-[breaking-change] due to the changes to AST

Ok, I'll change this to not break the AST and then do a separate PR that future proofs the AST wrt 1422 (i.e. makes ast::Visibility non-Copy and adds variants for pub(restricted)).

bors · 2016-04-02T15:52:10Z

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

jseyfried · 2016-04-02T20:44:07Z

I addressed @petrochenkov's comments, changed this PR to not break the AST, and rebased.

nikomatsakis · 2016-04-05T18:36:03Z

@bors r+

bors · 2016-04-05T18:36:04Z

📌 Commit bb47000 has been approved by nikomatsakis

…lity in `typeck` and in `privacy::PrivacyVisitor`.

jseyfried · 2016-04-07T05:52:33Z

rebased
@bors r=nikomatsakis

bors · 2016-04-07T05:52:33Z

📌 Commit dcd4621 has been approved by nikomatsakis

bors · 2016-04-07T14:42:47Z

⌛ Testing commit dcd4621 with merge a9fd0c5...

@nikomatsakis

…tsakis Lay groundwork for RFC 1422 and improve `PrivateItemsInPublicInterfacesVisitor` This PR lays groundwork for RFC 1422 (cc rust-lang#32409) and improves `PrivateItemsInPublicInterfacesVisitor`. More specifically, it - Refactors away `hir::Visibility::inherit_from`, the semantics of which are obsolete. - Makes `hir::Visibility` non-`Copy` so that we will be able to add new variants to represent `pub(restricted)` (for example, `Visibility::Restricted(Path)`). - Adds a new `Copy` type `ty::Visibility` that represents a visibility value, i.e. a characterization of where an item is accessible. This is able to represent `pub(restricted)` visibilities. - Improves `PrivateItemsInPublicInterfacesVisitor` so that it checks for items in an interface that are less visible than the interface. This fixesrust-lang#30079 but doesn't change any other behavior. r? @nikomatsakis

bors · 2016-04-07T14:54:37Z

⛄ The build was interrupted to prioritize another pull request.

Rollup of 7 pull requests - Successful merges: #32674, #32699, #32711, #32745, #32748, #32757, #32789 - Failed merges:

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.

rust-highfive assigned nikomatsakis Apr 2, 2016

jseyfried reviewed Apr 2, 2016
View reviewed changes

jseyfried force-pushed the 1422_groundwork branch from 3d547ee to da8d0bcCompare April 2, 2016 04:34

petrochenkov reviewed Apr 2, 2016
View reviewed changes

jseyfried force-pushed the 1422_groundwork branch 2 times, most recently from 622d906 to bb47000Compare April 2, 2016 20:43

jseyfried added 5 commits April 7, 2016 04:23

Refactor away hir::Visibility::inherit_from
ffbbf24

Make hir::Visibility non-copyable and add ty::Visibility
bb66d91

Use the node id from the Restricted variant when checking accessibi…
0d34c5d
…lity in `typeck` and in `privacy::PrivacyVisitor`.

Improve PrivateItemsInPublicInterfacesVisitor
3520402

Add test forrust-lang#30079
dcd4621

jseyfried force-pushed the 1422_groundwork branch from bb47000 to dcd4621Compare April 7, 2016 04:34

Manishearth mentioned this pull request Apr 7, 2016
Rollup of 7 pull requests #32794
Merged

bors added a commit that referenced this pull request Apr 7, 2016
Auto merge of#32794- Manishearth:rollup, r=Manishearth
470ca1c
Rollup of 7 pull requests - Successful merges: #32674, #32699, #32711, #32745, #32748, #32757, #32789 - Failed merges:

bors merged commit dcd4621 into rust-lang:masterApr 7, 2016

petrochenkov mentioned this pull request Jun 28, 2016
Tracking issue for private_in_public compatibility lint. #34537
Closed
5 tasks

jseyfried deleted the 1422_groundwork branch December 20, 2016 09:25

jseyfried changed the title ~~Lay groundwork for RFC 1422 and improve PrivateItemsInPublicInterfacesVisitor~~groundwork for RFC 1422, improve PrivateItemsInPublicInterfacesVisitorFeb 17, 2017

petrochenkov mentioned this pull request Dec 26, 2018
privacy: Use common DefId visiting infrastructure for all privacy visitors #56878
Merged

Uh oh!

Uh oh!

Conversation

jseyfried commented Apr 2, 2016

Uh oh!

jseyfried commented Apr 2, 2016

Uh oh!

jseyfriedApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

petrochenkovApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

petrochenkovApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Apr 2, 2016

Uh oh!

petrochenkov commented Apr 2, 2016

Uh oh!

jseyfried commented Apr 2, 2016

Uh oh!

bors commented Apr 2, 2016

Uh oh!

jseyfried commented Apr 2, 2016

Uh oh!

nikomatsakis commented Apr 5, 2016

Uh oh!

bors commented Apr 5, 2016

Uh oh!

jseyfried commented Apr 7, 2016

Uh oh!

bors commented Apr 7, 2016

Uh oh!

bors commented Apr 7, 2016

Uh oh!

bors commented Apr 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants