Add diagnostic for incorrect pub (restriction)

estebank · estebank · commit 769b95dc9f92 · 2017-03-22T22:51:45.000-07:00
Given the following statement

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

Provide the following diagnostic:

```rust
error: incorrect restriction in `pub`
  --&gt; 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.
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
@@ -4626,7 +4626,7 @@ impl<'a> Parser<'a> {
 
         let mut attrs = self.parse_outer_attributes()?;
         let lo = self.span.lo;
-        let vis = self.parse_visibility()?;
+        let vis = self.parse_visibility(false)?;
         let defaultness = self.parse_defaultness()?;
         let (name, node) = if self.eat_keyword(keywords::Type) {
             let name = self.parse_ident()?;
@@ -4939,25 +4939,8 @@ impl<'a> Parser<'a> {
             |p| {
                 let attrs = p.parse_outer_attributes()?;
                 let lo = p.span.lo;
-                let mut vis = p.parse_visibility()?;
-                let ty_is_interpolated =
-                    p.token.is_interpolated() || p.look_ahead(1, |t| t.is_interpolated());
-                let mut ty = p.parse_ty()?;
-
-                // Handle `pub(path) type`, in which `vis` will be `pub` and `ty` will be `(path)`.
-                if vis == Visibility::Public && !ty_is_interpolated &&
-                   p.token != token::Comma && p.token != token::CloseDelim(token::Paren) {
-                    ty = if let TyKind::Paren(ref path_ty) = ty.node {
-                        if let TyKind::Path(None, ref path) = path_ty.node {
-                            vis = Visibility::Restricted { path: P(path.clone()), id: path_ty.id };
-                            Some(p.parse_ty()?)
-                        } else {
-                            None
-                        }
-                    } else {
-                        None
-                    }.unwrap_or(ty);
-                }
+                let vis = p.parse_visibility(true)?;
+                let ty = p.parse_ty()?;
                 Ok(StructField {
                     span: mk_sp(lo, p.span.hi),
                     vis: vis,
@@ -4996,18 +4979,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(false)?;
         self.parse_single_struct_field(lo, vis, attrs)
     }
 
-    // Parse `pub`, `pub(crate)` and `pub(in path)` plus shortcuts
-    // `pub(self)` for `pub(in self)` and `pub(super)` for `pub(in super)`.
-    fn parse_visibility(&mut self) -> PResult<'a, Visibility> {
+    /// Parse `pub`, `pub(crate)` and `pub(in path)` plus shortcuts `pub(self)` for `pub(in self)`
+    /// and `pub(super)` for `pub(in super)`.  If the following element can't be a tuple (i.e. it's
+    /// a function definition, it's not a tuple struct field) and the contents within the parens
+    /// isn't valid, emit a proper diagnostic.
+    fn parse_visibility(&mut self, can_take_tuple: bool) -> PResult<'a, Visibility> {
         if !self.eat_keyword(keywords::Pub) {
             return Ok(Visibility::Inherited)
         }
 
         if self.check(&token::OpenDelim(token::Paren)) {
+            let start_span = self.span;
+            // We don't `self.bump()` the `(` yet because this might be a struct definition where
+            // `()` or a tuple might be allowed. For example, `struct Struct(pub (), pub (usize));`.
+            // Because of this, we only `bump` the `(` if we're assured it is appropriate to do so
+            // by the following tokens.
             if self.look_ahead(1, |t| t.is_keyword(keywords::Crate)) {
                 // `pub(crate)`
                 self.bump(); // `(`
@@ -5032,6 +5022,28 @@ 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 !can_take_tuple {  // Provide this diagnostic if this is not a tuple struct
+                // `pub(something) fn ...` or `struct X { pub(something) y: Z }`
+                self.bump(); // `(`
+                let msg = "incorrect visibility restriction";
+                let suggestion = r##"some possible 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"##;
+                let path = self.parse_path(PathStyle::Mod)?;
+                let path_span = self.prev_span;
+                let help_msg = format!("to make this visible only to module `{}`, add `in` before \
+                                       the path:",
+                                       path);
+                self.expect(&token::CloseDelim(token::Paren))?;  // `)`
+                let sp = Span {
+                    lo: start_span.lo,
+                    hi: self.prev_span.hi,
+                    expn_id: start_span.expn_id,
+                };
+                let mut err = self.span_fatal_help(sp, &msg, &suggestion);
+                err.span_suggestion(path_span, &help_msg, format!("in {}", path));
+                err.emit();  // emit diagnostic, but continue with public visibility
             }
         }
 
@@ -5508,7 +5520,7 @@ impl<'a> Parser<'a> {
 
         let lo = self.span.lo;
 
-        let visibility = self.parse_visibility()?;
+        let visibility = self.parse_visibility(false)?;
 
         if self.eat_keyword(keywords::Use) {
             // USE ITEM
@@ -5787,7 +5799,7 @@ impl<'a> Parser<'a> {
     fn parse_foreign_item(&mut self) -> PResult<'a, Option<ForeignItem>> {
         let attrs = self.parse_outer_attributes()?;
         let lo = self.span.lo;
-        let visibility = self.parse_visibility()?;
+        let visibility = self.parse_visibility(false)?;
 
         if self.check_keyword(keywords::Static) {
             // FOREIGN STATIC ITEM
diff --git a/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test.rs b/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test.rs
@@ -10,7 +10,8 @@
 
 mod foo {
     type T = ();
-    struct S1(pub(foo) (), pub(T), pub(crate) (), pub(((), T)));
-    struct S2(pub((foo)) ()); //~ ERROR expected `,`, found `(`
-                              //~| ERROR expected one of `;` or `where`, found `(`
+    struct S1(pub(in foo) (), pub(T), pub(crate) (), pub(((), T)));
+    struct S2(pub((foo)) ());
+    //~^ ERROR expected `,`, found `(`
+    //~| ERROR expected one of `;` or `where`, found `(`
 }
diff --git a/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test2.rs b/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test2.rs
@@ -11,9 +11,10 @@
 macro_rules! define_struct {
     ($t:ty) => {
         struct S1(pub $t);
-        struct S2(pub (foo) ());
-        struct S3(pub $t ()); //~ ERROR expected `,`, found `(`
-                              //~| ERROR expected one of `;` or `where`, found `(`
+        struct S2(pub (in foo) ());
+        struct S3(pub $t ());
+        //~^ ERROR expected `,`, found `(`
+        //~| ERROR expected one of `;` or `where`, found `(`
     }
 }
 
diff --git a/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test3.rs b/src/test/compile-fail/privacy/restricted/tuple-struct-fields/test3.rs
@@ -11,9 +11,10 @@
 macro_rules! define_struct {
     ($t:ty) => {
         struct S1(pub($t));
-        struct S2(pub (foo) ());
-        struct S3(pub($t) ()); //~ ERROR expected `,`, found `(`
-                               //~| ERROR expected one of `;` or `where`, found `(`
+        struct S2(pub (in foo) ());
+        struct S3(pub($t) ());
+        //~^ ERROR expected `,`, found `(`
+        //~| ERROR expected one of `;` or `where`, found `(`
     }
 }
 
diff --git a/src/test/ui/pub/pub-restricted-error-fn.rs b/src/test/ui/pub/pub-restricted-error-fn.rs
@@ -0,0 +1,13 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(pub_restricted)]
+
+pub(crate) () fn foo() {}
diff --git a/src/test/ui/pub/pub-restricted-error-fn.stderr b/src/test/ui/pub/pub-restricted-error-fn.stderr
@@ -0,0 +1,8 @@
+error: unmatched visibility `pub`
+  --> $DIR/pub-restricted-error-fn.rs:13:10
+   |
+13 | pub(crate) () fn foo() {}
+   |          ^
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/pub/pub-restricted-error.rs b/src/test/ui/pub/pub-restricted-error.rs
@@ -0,0 +1,19 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(pub_restricted)]
+
+struct Bar(pub(()));
+
+struct Foo {
+    pub(crate) () foo: usize,
+}
+
+
diff --git a/src/test/ui/pub/pub-restricted-error.stderr b/src/test/ui/pub/pub-restricted-error.stderr
@@ -0,0 +1,8 @@
+error: expected identifier, found `(`
+  --> $DIR/pub-restricted-error.rs:16:16
+   |
+16 |     pub(crate) () foo: usize,
+   |                ^
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/pub/pub-restricted-non-path.rs b/src/test/ui/pub/pub-restricted-non-path.rs
@@ -0,0 +1,15 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(pub_restricted)]
+
+pub (.) fn afn() {}
+
+fn main() {}
diff --git a/src/test/ui/pub/pub-restricted-non-path.stderr b/src/test/ui/pub/pub-restricted-non-path.stderr
@@ -0,0 +1,8 @@
+error: expected identifier, found `.`
+  --> $DIR/pub-restricted-non-path.rs:13:6
+   |
+13 | pub (.) fn afn() {}
+   |      ^
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/pub/pub-restricted.rs b/src/test/ui/pub/pub-restricted.rs
@@ -0,0 +1,37 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(pub_restricted)]
+
+mod a {}
+
+pub (a) fn afn() {}
+pub (b) fn bfn() {}
+pub fn privfn() {}
+mod x {
+    mod y {
+        pub (in x) fn foo() {}
+        pub (super) fn bar() {}
+        pub (crate) fn qux() {}
+    }
+}
+
+mod y {
+    struct Foo {
+        pub (crate) c: usize,
+        pub (super) s: usize,
+        valid_private: usize,
+        pub (in y) valid_in_x: usize,
+        pub (a) invalid: usize,
+        pub (in x) non_parent_invalid: usize,
+    }
+}
+
+fn main() {}
diff --git a/src/test/ui/pub/pub-restricted.stderr b/src/test/ui/pub/pub-restricted.stderr
@@ -0,0 +1,47 @@
+error: incorrect visibility restriction
+  --> $DIR/pub-restricted.rs:15:5
+   |
+15 | pub (a) fn afn() {}
+   |     ^^^
+   |
+   = help: some possible 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() {}
+
+error: incorrect visibility restriction
+  --> $DIR/pub-restricted.rs:16:5
+   |
+16 | pub (b) fn bfn() {}
+   |     ^^^
+   |
+   = help: some possible 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 `b`, add `in` before the path:
+   | pub (in b) fn bfn() {}
+
+error: incorrect visibility restriction
+  --> $DIR/pub-restricted.rs:32:13
+   |
+32 |         pub (a) invalid: usize,
+   |             ^^^
+   |
+   = help: some possible 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) invalid: usize,
+
+error: visibilities can only be restricted to ancestor modules
+  --> $DIR/pub-restricted.rs:33:17
+   |
+33 |         pub (in x) non_parent_invalid: usize,
+   |                 ^
+
+error: aborting due to 4 previous errors
+

Original file line number	Diff line number	Diff line change
`@@ -11,9 +11,10 @@`
`11`	`11`	`macro_rules! define_struct {`
`12`	`12`	`($t:ty) => {`
`13`	`13`	`struct S1(pub $t);`
`14`		`- struct S2(pub (foo) ());`
`15`		- struct S3(pub $t ()); //~ ERROR expected `,`, found `(`
`16`		- //~\| ERROR expected one of `;` or `where`, found `(`
	`14`	`+ struct S2(pub (in foo) ());`
	`15`	`+ struct S3(pub $t ());`
	`16`	+ //~^ ERROR expected `,`, found `(`
	`17`	+ //~\| ERROR expected one of `;` or `where`, found `(`
`17`	`18`	`}`
`18`	`19`	`}`
`19`	`20`