Rollup merge of rust-lang#66183 - Centril:empty-vis-trait-decl, r=petrochenkov

*Syntactically* permit visibilities on trait items & enum variants

Fixes rust-lang#65041

Suppose we have `$vis trait_item` or `$vis enum_variant` and `$vis` is a `:vis` macro fragment. Before this PR, this would fail to parse. This is now instead allowed as per language team consensus in rust-lang#65041 (comment). (See added tests for elaboration.)

Moreover, we now also permit visibility modifiers on trait items & enum variants *syntactically* but reject them with semantic checks (in `ast_validation`):

```rust
#[cfg(FALSE)]
trait Foo { pub fn bar(); } // OK

#[cfg(FALSE)]
enum E { pub U } // OK
```

Author: Centril

src/librustc_parse/parser/diagnostics.rs

@@ -1189,26 +1189,6 @@ impl<'a> Parser<'a> {
         }
     }
 
-    /// Recovers from `pub` keyword in places where it seems _reasonable_ but isn't valid.
-    pub(super) fn eat_bad_pub(&mut self) {
-        // When `unclosed_delims` is populated, it means that the code being parsed is already
-        // quite malformed, which might mean that, for example, a pub struct definition could be
-        // parsed as being a trait item, which is invalid and this error would trigger
-        // unconditionally, resulting in misleading diagnostics. Because of this, we only attempt
-        // this nice to have recovery for code that is otherwise well formed.
-        if self.token.is_keyword(kw::Pub) && self.unclosed_delims.is_empty() {
-            match self.parse_visibility(false) {
-                Ok(vis) => {
-                    self.diagnostic()
-                        .struct_span_err(vis.span, "unnecessary visibility qualifier")
-                        .span_label(vis.span, "`pub` not permitted here")
-                        .emit();
-                }
-                Err(mut err) => err.emit(),
-            }
-        }
-    }
-
     /// Eats tokens until we can be relatively sure we reached the end of the
     /// statement. This is something of a best-effort heuristic.
     ///

src/librustc_parse/parser/item.rs

@@ -1,4 +1,4 @@
-use super::{Parser, PathStyle};
+use super::{Parser, PathStyle, FollowedByType};
 use super::diagnostics::{Error, dummy_arg, ConsumeClosingDelim};
 
 use crate::maybe_whole;
@@ -87,7 +87,7 @@ impl<'a> Parser<'a> {
 
         let lo = self.token.span;
 
-        let vis = self.parse_visibility(false)?;
+        let vis = self.parse_visibility(FollowedByType::No)?;
 
         if self.eat_keyword(kw::Use) {
             // USE ITEM
@@ -696,7 +696,7 @@ impl<'a> Parser<'a> {
         mut attrs: Vec<Attribute>,
     ) -> PResult<'a, ImplItem> {
         let lo = self.token.span;
-        let vis = self.parse_visibility(false)?;
+        let vis = self.parse_visibility(FollowedByType::No)?;
         let defaultness = self.parse_defaultness();
         let (name, kind, generics) = if self.eat_keyword(kw::Type) {
             let (name, ty, generics) = self.parse_type_alias()?;
@@ -883,7 +883,7 @@ impl<'a> Parser<'a> {
         mut attrs: Vec<Attribute>,
     ) -> PResult<'a, TraitItem> {
         let lo = self.token.span;
-        self.eat_bad_pub();
+        let vis = self.parse_visibility(FollowedByType::No)?;
         let (name, kind, generics) = if self.eat_keyword(kw::Type) {
             self.parse_trait_item_assoc_ty()?
         } else if self.is_const_item() {
@@ -899,6 +899,7 @@ impl<'a> Parser<'a> {
             id: DUMMY_NODE_ID,
             ident: name,
             attrs,
+            vis,
             generics,
             kind,
             span: lo.to(self.prev_span),
@@ -1129,7 +1130,7 @@ impl<'a> Parser<'a> {
 
         let attrs = self.parse_outer_attributes()?;
         let lo = self.token.span;
-        let visibility = self.parse_visibility(false)?;
+        let visibility = self.parse_visibility(FollowedByType::No)?;
 
         // FOREIGN STATIC ITEM
         // Treat `const` as `static` for error recovery, but don't add it to expected tokens.
@@ -1339,7 +1340,7 @@ impl<'a> Parser<'a> {
             let variant_attrs = self.parse_outer_attributes()?;
             let vlo = self.token.span;
 
-            self.eat_bad_pub();
+            let vis = self.parse_visibility(FollowedByType::No)?;
             let ident = self.parse_ident()?;
 
             let struct_def = if self.check(&token::OpenDelim(token::Brace)) {
@@ -1366,6 +1367,7 @@ impl<'a> Parser<'a> {
 
             let vr = ast::Variant {
                 ident,
+                vis,
                 id: DUMMY_NODE_ID,
                 attrs: variant_attrs,
                 data: struct_def,
@@ -1519,7 +1521,7 @@ impl<'a> Parser<'a> {
         self.parse_paren_comma_seq(|p| {
             let attrs = p.parse_outer_attributes()?;
             let lo = p.token.span;
-            let vis = p.parse_visibility(true)?;
+            let vis = p.parse_visibility(FollowedByType::Yes)?;
             let ty = p.parse_ty()?;
             Ok(StructField {
                 span: lo.to(ty.span),
@@ -1537,7 +1539,7 @@ impl<'a> Parser<'a> {
     fn parse_struct_decl_field(&mut self) -> PResult<'a, StructField> {
         let attrs = self.parse_outer_attributes()?;
         let lo = self.token.span;
-        let vis = self.parse_visibility(false)?;
+        let vis = self.parse_visibility(FollowedByType::No)?;
         self.parse_single_struct_field(lo, vis, attrs)
     }
 

src/librustc_parse/parser/mod.rs

@@ -348,6 +348,8 @@ impl SeqSep {
     }
 }
 
+pub enum FollowedByType { Yes, No }
+
 impl<'a> Parser<'a> {
     pub fn new(
         sess: &'a ParseSess,
@@ -1120,7 +1122,7 @@ impl<'a> Parser<'a> {
     /// If the following element can't be a tuple (i.e., it's a function definition), then
     /// it's not a tuple struct field), and the contents within the parentheses isn't valid,
     /// so emit a proper diagnostic.
-    pub fn parse_visibility(&mut self, can_take_tuple: bool) -> PResult<'a, Visibility> {
+    pub fn parse_visibility(&mut self, fbt: FollowedByType) -> PResult<'a, Visibility> {
         maybe_whole!(self, NtVis, |x| x);
 
         self.expected_tokens.push(TokenType::Keyword(kw::Crate));
@@ -1175,7 +1177,9 @@ impl<'a> Parser<'a> {
                     id: ast::DUMMY_NODE_ID,
                 };
                 return Ok(respan(lo.to(self.prev_span), vis));
-            } else if !can_take_tuple { // Provide this diagnostic if this is not a tuple struct.
+            } else if let FollowedByType::No = fbt {
+                // Provide this diagnostic if a type cannot follow;
+                // in particular, if this is not a tuple struct.
                 self.recover_incorrect_vis_restriction()?;
                 // Emit diagnostic, but continue with public visibility.
             }

src/librustc_passes/ast_validation.rs

@@ -529,6 +529,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
             }
             ItemKind::Enum(ref def, _) => {
                 for variant in &def.variants {
+                    self.invalid_visibility(&variant.vis, None);
                     for field in variant.data.fields() {
                         self.invalid_visibility(&field.vis, None);
                     }
@@ -751,6 +752,11 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
         }
         visit::walk_impl_item(self, ii);
     }
+
+    fn visit_trait_item(&mut self, ti: &'a TraitItem) {
+        self.invalid_visibility(&ti.vis, None);
+        visit::walk_trait_item(self, ti);
+    }
 }
 
 pub fn check_crate(session: &Session, krate: &Crate, lints: &mut lint::LintBuffer) -> bool {

src/libsyntax/ast.rs

@@ -982,12 +982,12 @@ pub struct Arm {
 /// Access of a named (e.g., `obj.foo`) or unnamed (e.g., `obj.0`) struct field.
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct Field {
+    pub attrs: ThinVec<Attribute>,
+    pub id: NodeId,
+    pub span: Span,
     pub ident: Ident,
     pub expr: P<Expr>,
-    pub span: Span,
     pub is_shorthand: bool,
-    pub attrs: ThinVec<Attribute>,
-    pub id: NodeId,
     pub is_placeholder: bool,
 }
 
@@ -1567,12 +1567,14 @@ pub struct FnSig {
 /// signature) or provided (meaning it has a default implementation).
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct TraitItem {
+    pub attrs: Vec<Attribute>,
     pub id: NodeId,
+    pub span: Span,
+    pub vis: Visibility,
     pub ident: Ident,
-    pub attrs: Vec<Attribute>,
+
     pub generics: Generics,
     pub kind: TraitItemKind,
-    pub span: Span,
     /// See `Item::tokens` for what this is.
     pub tokens: Option<TokenStream>,
 }
@@ -1588,14 +1590,15 @@ pub enum TraitItemKind {
 /// Represents anything within an `impl` block.
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct ImplItem {
+    pub attrs: Vec<Attribute>,
     pub id: NodeId,
-    pub ident: Ident,
+    pub span: Span,
     pub vis: Visibility,
+    pub ident: Ident,
+
     pub defaultness: Defaultness,
-    pub attrs: Vec<Attribute>,
     pub generics: Generics,
     pub kind: ImplItemKind,
-    pub span: Span,
     /// See `Item::tokens` for what this is.
     pub tokens: Option<TokenStream>,
 }
@@ -2174,22 +2177,24 @@ pub struct GlobalAsm {
 pub struct EnumDef {
     pub variants: Vec<Variant>,
 }
-
 /// Enum variant.
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct Variant {
-    /// Name of the variant.
-    pub ident: Ident,
     /// Attributes of the variant.
     pub attrs: Vec<Attribute>,
     /// Id of the variant (not the constructor, see `VariantData::ctor_id()`).
     pub id: NodeId,
+    /// Span
+    pub span: Span,
+    /// The visibility of the variant. Syntactically accepted but not semantically.
+    pub vis: Visibility,
+    /// Name of the variant.
+    pub ident: Ident,
+
     /// Fields and constructor id of the variant.
     pub data: VariantData,
     /// Explicit discriminant, e.g., `Foo = 1`.
     pub disr_expr: Option<AnonConst>,
-    /// Span
-    pub span: Span,
     /// Is a macro placeholder
     pub is_placeholder: bool,
 }
@@ -2368,12 +2373,13 @@ impl VisibilityKind {
 /// E.g., `bar: usize` as in `struct Foo { bar: usize }`.
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct StructField {
+    pub attrs: Vec<Attribute>,
+    pub id: NodeId,
     pub span: Span,
-    pub ident: Option<Ident>,
     pub vis: Visibility,
-    pub id: NodeId,
+    pub ident: Option<Ident>,
+
     pub ty: P<Ty>,
-    pub attrs: Vec<Attribute>,
     pub is_placeholder: bool,
 }
 
@@ -2417,12 +2423,13 @@ impl VariantData {
 /// The name might be a dummy name in case of anonymous items.
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct Item {
-    pub ident: Ident,
     pub attrs: Vec<Attribute>,
     pub id: NodeId,
-    pub kind: ItemKind,
-    pub vis: Visibility,
     pub span: Span,
+    pub vis: Visibility,
+    pub ident: Ident,
+
+    pub kind: ItemKind,
 
     /// Original tokens this item was parsed from. This isn't necessarily
     /// available for all items, although over time more and more items should
@@ -2579,12 +2586,13 @@ impl ItemKind {
 
 #[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
 pub struct ForeignItem {
-    pub ident: Ident,
     pub attrs: Vec<Attribute>,
-    pub kind: ForeignItemKind,
     pub id: NodeId,
     pub span: Span,
     pub vis: Visibility,
+    pub ident: Ident,
+
+    pub kind: ForeignItemKind,
 }
 
 /// An item within an `extern` block.

src/libsyntax/mut_visit.rs

@@ -472,16 +472,17 @@ pub fn noop_visit_foreign_mod<T: MutVisitor>(foreign_mod: &mut ForeignMod, vis:
     items.flat_map_in_place(|item| vis.flat_map_foreign_item(item));
 }
 
-pub fn noop_flat_map_variant<T: MutVisitor>(mut variant: Variant, vis: &mut T)
+pub fn noop_flat_map_variant<T: MutVisitor>(mut variant: Variant, visitor: &mut T)
     -> SmallVec<[Variant; 1]>
 {
-    let Variant { ident, attrs, id, data, disr_expr, span, is_placeholder: _ } = &mut variant;
-    vis.visit_ident(ident);
-    visit_attrs(attrs, vis);
-    vis.visit_id(id);
-    vis.visit_variant_data(data);
-    visit_opt(disr_expr, |disr_expr| vis.visit_anon_const(disr_expr));
-    vis.visit_span(span);
+    let Variant { ident, vis, attrs, id, data, disr_expr, span, is_placeholder: _ } = &mut variant;
+    visitor.visit_ident(ident);
+    visitor.visit_vis(vis);
+    visit_attrs(attrs, visitor);
+    visitor.visit_id(id);
+    visitor.visit_variant_data(data);
+    visit_opt(disr_expr, |disr_expr| visitor.visit_anon_const(disr_expr));
+    visitor.visit_span(span);
     smallvec![variant]
 }
 
@@ -916,32 +917,33 @@ pub fn noop_visit_item_kind<T: MutVisitor>(kind: &mut ItemKind, vis: &mut T) {
     }
 }
 
-pub fn noop_flat_map_trait_item<T: MutVisitor>(mut item: TraitItem, vis: &mut T)
+pub fn noop_flat_map_trait_item<T: MutVisitor>(mut item: TraitItem, visitor: &mut T)
     -> SmallVec<[TraitItem; 1]>
 {
-    let TraitItem { id, ident, attrs, generics, kind, span, tokens: _ } = &mut item;
-    vis.visit_id(id);
-    vis.visit_ident(ident);
-    visit_attrs(attrs, vis);
-    vis.visit_generics(generics);
+    let TraitItem { id, ident, vis, attrs, generics, kind, span, tokens: _ } = &mut item;
+    visitor.visit_id(id);
+    visitor.visit_ident(ident);
+    visitor.visit_vis(vis);
+    visit_attrs(attrs, visitor);
+    visitor.visit_generics(generics);
     match kind {
         TraitItemKind::Const(ty, default) => {
-            vis.visit_ty(ty);
-            visit_opt(default, |default| vis.visit_expr(default));
+            visitor.visit_ty(ty);
+            visit_opt(default, |default| visitor.visit_expr(default));
         }
         TraitItemKind::Method(sig, body) => {
-            visit_fn_sig(sig, vis);
-            visit_opt(body, |body| vis.visit_block(body));
+            visit_fn_sig(sig, visitor);
+            visit_opt(body, |body| visitor.visit_block(body));
         }
         TraitItemKind::Type(bounds, default) => {
-            visit_bounds(bounds, vis);
-            visit_opt(default, |default| vis.visit_ty(default));
+            visit_bounds(bounds, visitor);
+            visit_opt(default, |default| visitor.visit_ty(default));
         }
         TraitItemKind::Macro(mac) => {
-            vis.visit_mac(mac);
+            visitor.visit_mac(mac);
         }
     }
-    vis.visit_span(span);
+    visitor.visit_span(span);
 
     smallvec![item]
 }

src/libsyntax/print/pprust/tests.rs

@@ -50,6 +50,7 @@ fn test_variant_to_string() {
 
         let var = ast::Variant {
             ident,
+            vis: source_map::respan(syntax_pos::DUMMY_SP, ast::VisibilityKind::Inherited),
             attrs: Vec::new(),
             id: ast::DUMMY_NODE_ID,
             data: ast::VariantData::Unit(ast::DUMMY_NODE_ID),

src/libsyntax/visit.rs

@@ -310,6 +310,7 @@ pub fn walk_variant<'a, V: Visitor<'a>>(visitor: &mut V, variant: &'a Variant)
     where V: Visitor<'a>,
 {
     visitor.visit_ident(variant.ident);
+    visitor.visit_vis(&variant.vis);
     visitor.visit_variant_data(&variant.data);
     walk_list!(visitor, visit_anon_const, &variant.disr_expr);
     walk_list!(visitor, visit_attribute, &variant.attrs);
@@ -581,6 +582,7 @@ pub fn walk_fn<'a, V>(visitor: &mut V, kind: FnKind<'a>, declaration: &'a FnDecl
 }
 
 pub fn walk_trait_item<'a, V: Visitor<'a>>(visitor: &mut V, trait_item: &'a TraitItem) {
+    visitor.visit_vis(&trait_item.vis);
     visitor.visit_ident(trait_item.ident);
     walk_list!(visitor, visit_attribute, &trait_item.attrs);
     visitor.visit_generics(&trait_item.generics);