Implement `pub(restricted)` privacy (RFC 1422) #32875

jseyfried · 2016-04-11T06:33:36Z

This implements pub(restricted) privacy from rust-lang/rfcs#1422 (cc #32409) behind a feature gate.

pub(restricted) paths currently cannot use re-exported modules both for simplicity of implementation and for future compatibility with rust-lang/rfcs#1560 (cc #31783).

r? @nikomatsakis

jseyfried · 2016-04-11T06:47:05Z

I didn't allow pub(restricted) for tuple struct fields, as suggested here.

Also, pub(restricted) imports cannot be used in non-lexical scopes (like private imports) so that pub(self) and the absence of a visibility modifier are equivalent (as suggested here). Once RFC 1560 is accepted and #32213 lands, all imports will be usable in non-lexical scopes (provided that they are visible enough, of course).

jseyfried · 2016-04-11T06:48:11Z

cc @petrochenkov @pnkfelix

petrochenkov · 2016-04-11T08:29:02Z

src/librustc_resolve/lib.rs

This comment is no longer true after d1ef356?

Indeed, fixed.

petrochenkov · 2016-04-11T19:18:28Z

src/libsyntax/visit.rs

Foreign items seem to be missing.

And HIR visibilities in librustc\hir\intravisit.rs are not visited.

Foreign items seem to be missing.

Good catch, I'll add them.

And HIR visibilities in librustc\hir\intravisit.rs are not visited.

There's no need to visit visibilities in the HIR currently -- do you think I should add a visit_vis method in intravisit anyway?

do you think I should a visit_vis method in intravisit

Yes, default visitor functions just traverse the tree exhaustively, providing, well, correct defaults.
As an example, overridden visit_paths will not visit paths in visibilities if intravisit doesn't traverse visibilities by default.

I've just noticed, default folders (fold.rs) have the same problems as visitors.
AST folders don't fold all visibilities and HIR folders don't fold visibilities at all.

jseyfried · 2016-04-13T06:32:05Z

I rebased and addressed @petrochenkov's comments.

bors · 2016-04-13T08:21:10Z

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

nikomatsakis · 2016-04-13T13:32:40Z

src/test/compile-fail/pub-restricted-2.rs

Meta note: Let's make a subdirectory for these tests.

Perhaps src/test/compile-fail/privacy? If we want to further designate tests aimed at the pub(restricted) RFC, then perhaps src/test/compile-fail/privacy/restricted?

nikomatsakis · 2016-04-13T14:06:32Z

This looks great. I suggest we move the privacy tests to a subdirectory. I want to start giving more structure to our compile-fail directory on new PRs. :) I'd also like to start maintaining a list that aims to show what is being tested (and, ideally, where). To that end, I drew up a list of stuff that I think we ought to test from this RFC and went through and checked off what you were already testing. The good news is that you got almost all of it! I'm going to write up my matrix here, maybe you can add tests for the remaining few items. In some cases, the tests are probably a bit redundant: I was just trying to think -- without reference to the impl -- of the various scenarios that might arise and try to be somewhat exhaustive about it.

So, here is my matrix. I've put an X for things that I thought you were testing, a - for cases that don't really apply, and left blank things that seem untested. I also added in an O in a few spots for the things I'd really like to see tests for. The rest I think you can add tests for if you think it makes sense.

pub(crate)                                              | X | X |
pub(super)                                              | X | X |
pub(other::path)                                        | X |   |
pub(bad::path)                                          | X | - |
pub(not::ancestor)                                      | X | - |
method calls                                            | X | X |
field access                                            | X | X |
struct literal where fields are private                 | X |   |
field lookup ignores fields with pub(foo)               | O | O |
method lookup ignores fields with pub(foo)              | O | O |
associated item notation (UFCS)                         |   |   |
  trait method (in scope, from parent)                  |   |   |
  inherent method (in scope, from parent)               |   |   |
  where trait is out of scope                           | O | O |
  where method is out of scope (inherent methods only)  | O | O |
reference of item in use statement                      | X | X |
reference of item in expression                         | X | X |
reference of item in type definition                    |   |   |
reference of trait in an impl                           |   |   |
reference of self-type in a trait impl                  |   |   |
reference of self-type in an inherent impl              |   |   |
feature-gate required                                   | X | - |
private type in public API:                             |   |   |
  fn arg                                                | X |   |
  other cases?                                          |   |   |

Eventually I'd like to have a system for tagging tests with what they test so we can easily construct matrices like this automatically to get some idea of test coverage. But we're not there yet.

r=me with suitable tests added.

jseyfried · 2016-04-14T03:59:29Z

@nikomatsakis
What are the columns? I'm guessing something like [ error emitted | no error emitted ].
Other than that, sounds good.

…y checking

reexports of private variants are handled correctly.

nikomatsakis · 2016-04-15T20:17:23Z

@jseyfried oh, I forgot to label those :) it was "intra-crate" and "cross-crate"

nikomatsakis · 2016-04-15T20:18:17Z

@jseyfried if you want me to review add'l tests, let me know, but if you feel like you've satisfied all the interesting points in that matrix, then r=me

jseyfried · 2016-04-16T08:45:02Z

Ok, makes sense. I'll add some more tests and r=you

petrochenkov · 2016-04-16T10:47:32Z

A note: visibilities do not expect type parameters in paths, but use parse_path for parsing, so paths with type parameters can be supplied to them with macros $p: path => pub($p), so visibilities need to be sanity-checked after expansion. I'll fix this as a part of #32875 (comment).

the feature `pub_restricted` is enabled.

jseyfried · 2016-04-16T22:54:18Z

@petrochenkov interesting, sounds good.

jseyfried · 2016-04-16T22:57:46Z

@bors r=nikomatsakis

bors · 2016-04-16T22:57:48Z

📌 Commit e14504a has been approved by nikomatsakis

bors · 2016-04-16T23:21:09Z

⌛ Testing commit e14504a with merge ae33aa7...

@nikomatsakis

Implement `pub(restricted)` privacy (RFC 1422) This implements `pub(restricted)` privacy from RFC 1422 (cc #32409) behind a feature gate. `pub(restricted)` paths currently cannot use re-exported modules both for simplicity of implementation and for future compatibility with RFC 1560 (cf #31783). r? @nikomatsakis

bors · 2016-04-17T01:31:10Z

@eddyb

resolve: Refactor away `DefModifiers` This refactors away `DefModifiers`, which is unneeded now that rust-lang#32875 has landed. r? @eddyb

rust-highfive assigned nikomatsakis Apr 11, 2016

jseyfried force-pushed the 1422_implementation branch 2 times, most recently from 84e1cdb to ef05df9 Compare April 11, 2016 08:22

petrochenkov reviewed Apr 11, 2016
View reviewed changes

jseyfried force-pushed the 1422_implementation branch from ef05df9 to c2ccd5b Compare April 11, 2016 08:34

petrochenkov reviewed Apr 11, 2016
View reviewed changes

jseyfried force-pushed the 1422_implementation branch from c2ccd5b to 358aed4 Compare April 13, 2016 02:27

nikomatsakis reviewed Apr 13, 2016
View reviewed changes

jseyfried added 9 commits April 14, 2016 04:48

Add a span to the Crate variant of ast::Visibility

4b189e6

Visit visibilities in the ast visitor's walk_* functions

2b2e4dd

Add Crate and Restricted variants to hir::Visibility

1e59424

Fix fallout in rustdoc

3bf9fc0

resolve: Use vis: ty::Visibility instead of is_public: bool

a4196cd

resolve: use the Restricted variant of ty::Visibility when privac…

a0c3ce3

…y checking

resolve: refactor away PRIVATE_VARIANT and ensure that restricted

0833a89

reexports of private variants are handled correctly.

Parse pub(restricted)

50ce605

Feature gate pub(restricted)

44c78ee

jseyfried force-pushed the 1422_implementation branch from d699383 to a05dd73 Compare April 14, 2016 05:33

Always report PRIVATE_IN_PUBLIC violations as hard errors when

daec3fe

the feature `pub_restricted` is enabled.

jseyfried force-pushed the 1422_implementation branch from a05dd73 to 70eb972 Compare April 16, 2016 22:49

Add tests for pub(restricted)

e14504a

jseyfried force-pushed the 1422_implementation branch from 70eb972 to e14504a Compare April 16, 2016 22:53

bors merged commit e14504a into rust-lang:master Apr 17, 2016

This was referenced Apr 17, 2016

syntax: Make parsing of paths whitespace agnostic + some other changes #33041

Merged

Consistent no_prelude attribute #32025

Closed

rustc: Implement custom panic runtimes #32900

Merged

[WIP] Implement RFC 1560. #32213

Closed

petrochenkov mentioned this pull request Apr 17, 2016

syntax: Parse import prefixes as paths #33044

Merged

jseyfried mentioned this pull request Apr 17, 2016

resolve: Refactor away DefModifiers #33045

Merged

nikomatsakis mentioned this pull request Mar 17, 2017

Tracking issue for pub(restricted) privacy (RFC #1422) #32409

Closed

6 tasks

Implement pub(restricted) privacy (RFC 1422) #32875

Implement pub(restricted) privacy (RFC 1422) #32875

Uh oh!

Conversation

jseyfried commented Apr 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseyfried commented Apr 11, 2016

Uh oh!

jseyfried commented Apr 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseyfried Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jseyfried commented Apr 13, 2016

Uh oh!

bors commented Apr 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Apr 13, 2016

Uh oh!

jseyfried commented Apr 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Apr 15, 2016

Uh oh!

nikomatsakis commented Apr 15, 2016

Uh oh!

jseyfried commented Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Apr 16, 2016

Uh oh!

jseyfried commented Apr 16, 2016

Uh oh!

jseyfried commented Apr 16, 2016

Uh oh!

bors commented Apr 16, 2016

Uh oh!

bors commented Apr 16, 2016

Uh oh!

bors commented Apr 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Implement `pub(restricted)` privacy (RFC 1422) #32875

Implement `pub(restricted)` privacy (RFC 1422) #32875

jseyfried commented Apr 11, 2016 •

edited

Loading

jseyfried Apr 14, 2016 •

edited

Loading

jseyfried commented Apr 14, 2016 •

edited

Loading

jseyfried commented Apr 16, 2016 •

edited

Loading