Skip to content

Add diagnostic for incorrect pub (restriction)#40627

Merged
bors merged 1 commit intorust-lang:masterfrom
estebank:pub-restricted
Mar 23, 2017
Merged

Add diagnostic for incorrect pub (restriction)#40627
bors merged 1 commit intorust-lang:masterfrom
estebank:pub-restricted

Conversation

@estebank
Copy link
Contributor

@estebank estebank commented Mar 18, 2017 •
edited
Loading

Given the following statement

pub (a) fn afn() {}

Provide the following diagnostic:

error: incorrect restriction in `pub`
  --> file.rs:15:1
   |
15 | pub (a) fn afn() {}
   |     ^^^
   |
   = help: some valid visibility restrictions are:
           `pub(crate)`: visible only on the current crate
           `pub(super)`: visible only in the current module's parent
           `pub(in path::to::module)`: visible only on the specified path
help: to make this visible only to module `a`, add `in` before the path:
   | pub (in a) fn afn() {}

Follow up to #40340, fix #40599, cc #32409.

@rust-highfive
Copy link
Contributor

rust-highfive commented Mar 18, 2017

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank
Copy link
Contributor Author

estebank commented Mar 18, 2017 •
edited
Loading

cc @pnkfelix @jonathandturner

@estebank estebank mentioned this pull request Mar 18, 2017
Closed
petrochenkov
petrochenkov reviewed Mar 18, 2017
View reviewed changes
src/libsyntax/parse/parser.rs Outdated
Copy link
Contributor

@petrochenkov petrochenkov Mar 18, 2017

Choose a reason for hiding this comment

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

Looks like this is always true, we have already checked for ( in the outer condition.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 18, 2017

Looks like struct S(pub ()); and struct S(pub (u8)), valid item declarations, are not treated correctly.

@estebank estebank force-pushed the pub-restricted branch 3 times, most recently from 19fd6b3 to f682709 Compare March 19, 2017 20:20
@sophiajt
Copy link
Contributor

sophiajt commented Mar 19, 2017

It's pretty chatty, but I admit it also looks very helpful. I'm good with it as a message.

@pnkfelix
Copy link
Contributor

pnkfelix commented Mar 20, 2017

regarding the chattiness, do we really need to keep these two lines:

    `pub`: visible on every module that imports this module
    ...
    `pub(self)` or ``: visible only in the current module

It seems like these cases are obvious enough, if one assumes that someone using parentheses after pub is looking for a non-trivial restriction, and removing them takes the help message from 6 lines to 4 lines...

@estebank
Copy link
Contributor Author

estebank commented Mar 20, 2017

@pnkfelix that's a reasonable. I'll modify it tonight.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 20, 2017

If we're not going to make it an exhaustive listing, then I would change the heading text from "valid restrictions are" to "some example restrictions are" or something like that.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 20, 2017

The code looks good to me, modulo tweaking the message. I agree it's chatty but also that it seems helpful.

petrochenkov
petrochenkov reviewed Mar 20, 2017
View reviewed changes
src/libsyntax/parse/parser.rs Outdated
Copy link
Contributor

@petrochenkov petrochenkov Mar 20, 2017

Choose a reason for hiding this comment

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

This is false too.

Copy link
Contributor

@nikomatsakis nikomatsakis Mar 20, 2017

Choose a reason for hiding this comment

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

Ah, that reminds me. Seems like we should have some tests exercising these paths (i.e., tests that would fail with this being true)...

Copy link
Contributor Author

@estebank estebank Mar 22, 2017

Choose a reason for hiding this comment

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

Done and done.

petrochenkov
petrochenkov reviewed Mar 20, 2017
View reviewed changes
src/libsyntax/parse/parser.rs Outdated
Copy link
Contributor

@petrochenkov petrochenkov Mar 20, 2017

Choose a reason for hiding this comment

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

It's not fully on-topic, but could you remove all this cruft remaining from the old visibility syntax, I've missed it somehow.
It should be "parse_visibility" -> "parse_ty" -> "return the result" now, no extra magic.

Copy link
Contributor Author

@estebank estebank Mar 22, 2017

Choose a reason for hiding this comment

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

Done.

@estebank
Copy link
Contributor Author

estebank commented Mar 22, 2017

Current output:

Image

@bors
Copy link
Collaborator

bors commented Mar 22, 2017

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

@estebank estebank force-pushed the pub-restricted branch 2 times, most recently from 4d89d2e to c93ccb5 Compare March 22, 2017 05:49
@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 22, 2017

LGTM, but couple of tests fail after rebase.

@estebank
Add diagnostic for incorrect pub (restriction)
769b95d 
Given the following statement

```rust
pub (a) fn afn() {}
```

Provide the following diagnostic:

```rust
error: incorrect restriction in `pub`
  --> file.rs:15:1
   |
15 | pub (a) fn afn() {}
   | ^^^^^^^
   |
   = help: some valid visibility restrictions are:
           `pub(crate)`: visible only on the current crate
           `pub(super)`: visible only in the current module's parent
           `pub(in path::to::module)`: visible only on the specified path
help: to make this visible only to module `a`, add `in` before the path:
   | pub (in a) fn afn() {}
```

Remove cruft from old `pub(path)` syntax.
@estebank
Copy link
Contributor Author

estebank commented Mar 23, 2017

@petrochenkov fixed.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 23, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 23, 2017

📌 Commit 769b95d has been approved by petrochenkov

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2017
@frewsxcv
Rollup merge of rust-lang#40627 - estebank:pub-restricted, r=petroche…
640dbbd 
…nkov

Add diagnostic for incorrect `pub (restriction)`

Given the following statement

```rust
pub (a) fn afn() {}
```

Provide the following diagnostic:

```rust
error: incorrect restriction in `pub`
  --> file.rs:15:1
   |
15 | pub (a) fn afn() {}
   |     ^^^
   |
   = help: some valid visibility restrictions are:
           `pub(crate)`: visible only on the current crate
           `pub(super)`: visible only in the current module's parent
           `pub(in path::to::module)`: visible only on the specified path
help: to make this visible only to module `a`, add `in` before the path:
   | pub (in a) fn afn() {}
```

Follow up to rust-lang#40340, fix rust-lang#40599, cc rust-lang#32409.
@frewsxcv frewsxcv mentioned this pull request Mar 23, 2017
Merged
bors added a commit that referenced this pull request Mar 23, 2017
@bors
Auto merge of #40758 - frewsxcv:rollup, r=frewsxcv
d558037 
Rollup of 5 pull requests

- Successful merges: #40612, #40627, #40668, #40715, #40753
- Failed merges:
@bors bors merged commit 769b95d into rust-lang:master Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikomatsakis nikomatsakis nikomatsakis left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@nikomatsakis nikomatsakis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Subpar error message on pub (ident) fn foo

7 participants

@estebank @rust-highfive @petrochenkov @sophiajt @pnkfelix @nikomatsakis @bors