Add diagnostic for incorrect `pub (restriction)` #40627

estebank · 2017-03-18T04:26:33Z

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 · 2017-03-18T04:26:43Z

r? @nikomatsakis

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

estebank · 2017-03-18T04:27:43Z

cc @pnkfelix @jonathandturner

petrochenkov · 2017-03-18T09:07:07Z

src/libsyntax/parse/parser.rs

@@ -5102,6 +5103,29 @@ impl<'a> Parser<'a> {
                let vis = Visibility::Restricted { path: P(path), id: ast::DUMMY_NODE_ID };
                self.expect(&token::CloseDelim(token::Paren))?; // `)`
                return Ok(vis)
+            } else if self.look_ahead(0, |t| *t == token::Token::OpenDelim(token::Paren)) {


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

petrochenkov · 2017-03-18T09:09:30Z

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

sophiajt · 2017-03-19T21:05:09Z

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

pnkfelix · 2017-03-20T15:58:18Z

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 · 2017-03-20T17:45:33Z

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

nikomatsakis · 2017-03-20T18:35:19Z

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 · 2017-03-20T18:40:57Z

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

petrochenkov · 2017-03-20T18:44:45Z

src/libsyntax/parse/parser.rs

@@ -5066,18 +5066,25 @@ impl<'a> Parser<'a> {
    fn parse_struct_decl_field(&mut self) -> PResult<'a, StructField> {
        let attrs = self.parse_outer_attributes()?;
        let lo = self.span.lo;
-        let vis = self.parse_visibility()?;
+        let vis = self.parse_visibility(true)?;


This is false too.

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

Done and done.

petrochenkov · 2017-03-20T18:50:01Z

src/libsyntax/parse/parser.rs

@@ -5009,7 +5009,7 @@ impl<'a> Parser<'a> {
            |p| {
                let attrs = p.parse_outer_attributes()?;
                let lo = p.span.lo;
-                let mut vis = p.parse_visibility()?;
+                let mut vis = p.parse_visibility(true)?;
                let ty_is_interpolated =
                    p.token.is_interpolated() || p.look_ahead(1, |t| t.is_interpolated());
                let mut ty = p.parse_ty()?;


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.

estebank · 2017-03-22T03:17:37Z

Current output:

bors · 2017-03-22T04:40:08Z

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

petrochenkov · 2017-03-22T07:52:10Z

LGTM, but couple of tests fail after rebase.

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 · 2017-03-23T07:06:20Z

@petrochenkov fixed.

petrochenkov · 2017-03-23T07:55:04Z

@bors r+

bors · 2017-03-23T07:55:05Z

📌 Commit 769b95d has been approved by petrochenkov

…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.

Rollup of 5 pull requests - Successful merges: #40612, #40627, #40668, #40715, #40753 - Failed merges:

rust-highfive assigned nikomatsakis Mar 18, 2017

estebank force-pushed the pub-restricted branch from 2daec2f to 280fd71 Compare March 18, 2017 04:27

estebank mentioned this pull request Mar 18, 2017

Subpar error message on pub (ident) fn foo #40599

Closed

petrochenkov reviewed Mar 18, 2017

View reviewed changes

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

petrochenkov reviewed Mar 20, 2017

View reviewed changes

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

estebank force-pushed the pub-restricted branch from c93ccb5 to ce52345 Compare March 22, 2017 16:09

estebank force-pushed the pub-restricted branch from ce52345 to 769b95d Compare March 23, 2017 05:54

frewsxcv mentioned this pull request Mar 23, 2017

Rollup of 5 pull requests #40758

Merged

bors added a commit that referenced this pull request Mar 23, 2017

Auto merge of #40758 - frewsxcv:rollup, r=frewsxcv

d558037

Rollup of 5 pull requests - Successful merges: #40612, #40627, #40668, #40715, #40753 - Failed merges:

bors merged commit 769b95d into rust-lang:master Mar 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diagnostic for incorrect `pub (restriction)` #40627

Add diagnostic for incorrect `pub (restriction)` #40627

estebank commented Mar 18, 2017 •

edited

Loading

rust-highfive commented Mar 18, 2017

estebank commented Mar 18, 2017 •

edited

Loading

petrochenkov Mar 18, 2017

petrochenkov commented Mar 18, 2017

sophiajt commented Mar 19, 2017

pnkfelix commented Mar 20, 2017

estebank commented Mar 20, 2017

nikomatsakis commented Mar 20, 2017

nikomatsakis commented Mar 20, 2017

petrochenkov Mar 20, 2017

nikomatsakis Mar 20, 2017

estebank Mar 22, 2017

petrochenkov Mar 20, 2017

estebank Mar 22, 2017

estebank commented Mar 22, 2017

bors commented Mar 22, 2017

petrochenkov commented Mar 22, 2017

estebank commented Mar 23, 2017

petrochenkov commented Mar 23, 2017

bors commented Mar 23, 2017

Add diagnostic for incorrect pub (restriction) #40627

Add diagnostic for incorrect pub (restriction) #40627

Conversation

estebank commented Mar 18, 2017 • edited Loading

rust-highfive commented Mar 18, 2017

estebank commented Mar 18, 2017 • edited Loading

petrochenkov Mar 18, 2017

Choose a reason for hiding this comment

petrochenkov commented Mar 18, 2017

sophiajt commented Mar 19, 2017

pnkfelix commented Mar 20, 2017

estebank commented Mar 20, 2017

nikomatsakis commented Mar 20, 2017

nikomatsakis commented Mar 20, 2017

petrochenkov Mar 20, 2017

Choose a reason for hiding this comment

nikomatsakis Mar 20, 2017

Choose a reason for hiding this comment

estebank Mar 22, 2017

Choose a reason for hiding this comment

petrochenkov Mar 20, 2017

Choose a reason for hiding this comment

estebank Mar 22, 2017

Choose a reason for hiding this comment

estebank commented Mar 22, 2017

bors commented Mar 22, 2017

petrochenkov commented Mar 22, 2017

estebank commented Mar 23, 2017

petrochenkov commented Mar 23, 2017

bors commented Mar 23, 2017

Add diagnostic for incorrect `pub (restriction)` #40627

Add diagnostic for incorrect `pub (restriction)` #40627

estebank commented Mar 18, 2017 •

edited

Loading

estebank commented Mar 18, 2017 •

edited

Loading