Rollup merge of rust-lang#76808 - LeSeulArtichaut:diagnose-functions-struct, r=jackh726

Dylan-DPC · web-flow · commit eb36bc666a4e · 2021-05-08T01:06:22.000+02:00
Improve diagnostics for functions in `struct` definitions Tries to implement rust-lang#76421. This is probably going to need unit tests, but I wanted to hear from review all the cases tests should cover. I'd like to follow up with the "mechanically applicable suggestion here that adds an impl block" step, but I'd need guidance. My idea for now would be to try to parse a function, and if that succeeds, create a dummy `ast::Item` impl block to then format it using `pprust`. Would that be a viable approach? Is there a better alternative? r? `@matklad` cc `@estebank﻿`
diff --git a/compiler/rustc_parse/src/parser/item.rs b/compiler/rustc_parse/src/parser/item.rs
@@ -1124,11 +1124,11 @@ impl<'a> Parser<'a> {
                 if !this.recover_nested_adt_item(kw::Enum)? {
                     return Ok((None, TrailingToken::None));
                 }
-                let ident = this.parse_ident()?;
+                let ident = this.parse_field_ident("enum", vlo)?;
 
                 let struct_def = if this.check(&token::OpenDelim(token::Brace)) {
                     // Parse a struct variant.
-                    let (fields, recovered) = this.parse_record_struct_body()?;
+                    let (fields, recovered) = this.parse_record_struct_body("struct")?;
                     VariantData::Struct(fields, recovered)
                 } else if this.check(&token::OpenDelim(token::Paren)) {
                     VariantData::Tuple(this.parse_tuple_struct_body()?, DUMMY_NODE_ID)
@@ -1182,15 +1182,15 @@ impl<'a> Parser<'a> {
                 VariantData::Unit(DUMMY_NODE_ID)
             } else {
                 // If we see: `struct Foo<T> where T: Copy { ... }`
-                let (fields, recovered) = self.parse_record_struct_body()?;
+                let (fields, recovered) = self.parse_record_struct_body("struct")?;
                 VariantData::Struct(fields, recovered)
             }
         // No `where` so: `struct Foo<T>;`
         } else if self.eat(&token::Semi) {
             VariantData::Unit(DUMMY_NODE_ID)
         // Record-style struct definition
         } else if self.token == token::OpenDelim(token::Brace) {
-            let (fields, recovered) = self.parse_record_struct_body()?;
+            let (fields, recovered) = self.parse_record_struct_body("struct")?;
             VariantData::Struct(fields, recovered)
         // Tuple-style struct definition with optional where-clause.
         } else if self.token == token::OpenDelim(token::Paren) {
@@ -1220,10 +1220,10 @@ impl<'a> Parser<'a> {
 
         let vdata = if self.token.is_keyword(kw::Where) {
             generics.where_clause = self.parse_where_clause()?;
-            let (fields, recovered) = self.parse_record_struct_body()?;
+            let (fields, recovered) = self.parse_record_struct_body("union")?;
             VariantData::Struct(fields, recovered)
         } else if self.token == token::OpenDelim(token::Brace) {
-            let (fields, recovered) = self.parse_record_struct_body()?;
+            let (fields, recovered) = self.parse_record_struct_body("union")?;
             VariantData::Struct(fields, recovered)
         } else {
             let token_str = super::token_descr(&self.token);
@@ -1236,12 +1236,15 @@ impl<'a> Parser<'a> {
         Ok((class_name, ItemKind::Union(vdata, generics)))
     }
 
-    fn parse_record_struct_body(&mut self) -> PResult<'a, (Vec<FieldDef>, /* recovered */ bool)> {
+    fn parse_record_struct_body(
+        &mut self,
+        adt_ty: &str,
+    ) -> PResult<'a, (Vec<FieldDef>, /* recovered */ bool)> {
         let mut fields = Vec::new();
         let mut recovered = false;
         if self.eat(&token::OpenDelim(token::Brace)) {
             while self.token != token::CloseDelim(token::Brace) {
-                let field = self.parse_field_def().map_err(|e| {
+                let field = self.parse_field_def(adt_ty).map_err(|e| {
                     self.consume_block(token::Brace, ConsumeClosingDelim::No);
                     recovered = true;
                     e
@@ -1294,24 +1297,25 @@ impl<'a> Parser<'a> {
     }
 
     /// Parses an element of a struct declaration.
-    fn parse_field_def(&mut self) -> PResult<'a, FieldDef> {
+    fn parse_field_def(&mut self, adt_ty: &str) -> PResult<'a, FieldDef> {
         let attrs = self.parse_outer_attributes()?;
         self.collect_tokens_trailing_token(attrs, ForceCollect::No, |this, attrs| {
             let lo = this.token.span;
             let vis = this.parse_visibility(FollowedByType::No)?;
-            Ok((this.parse_single_struct_field(lo, vis, attrs)?, TrailingToken::None))
+            Ok((this.parse_single_struct_field(adt_ty, lo, vis, attrs)?, TrailingToken::None))
         })
     }
 
     /// Parses a structure field declaration.
     fn parse_single_struct_field(
         &mut self,
+        adt_ty: &str,
         lo: Span,
         vis: Visibility,
         attrs: Vec<Attribute>,
     ) -> PResult<'a, FieldDef> {
         let mut seen_comma: bool = false;
-        let a_var = self.parse_name_and_ty(lo, vis, attrs)?;
+        let a_var = self.parse_name_and_ty(adt_ty, lo, vis, attrs)?;
         if self.token == token::Comma {
             seen_comma = true;
         }
@@ -1398,11 +1402,12 @@ impl<'a> Parser<'a> {
     /// Parses a structure field.
     fn parse_name_and_ty(
         &mut self,
+        adt_ty: &str,
         lo: Span,
         vis: Visibility,
         attrs: Vec<Attribute>,
     ) -> PResult<'a, FieldDef> {
-        let name = self.parse_ident_common(false)?;
+        let name = self.parse_field_ident(adt_ty, lo)?;
         self.expect(&token::Colon)?;
         let ty = self.parse_ty()?;
         Ok(FieldDef {
@@ -1416,6 +1421,29 @@ impl<'a> Parser<'a> {
         })
     }
 
+    /// Parses a field identifier. Specialized version of `parse_ident_common`
+    /// for better diagnostics and suggestions.
+    fn parse_field_ident(&mut self, adt_ty: &str, lo: Span) -> PResult<'a, Ident> {
+        let (ident, is_raw) = self.ident_or_err()?;
+        if !is_raw && ident.is_reserved() {
+            let err = if self.check_fn_front_matter(false) {
+                let _ = self.parse_fn(&mut Vec::new(), |_| true, lo);
+                let mut err = self.struct_span_err(
+                    lo.to(self.prev_token.span),
+                    &format!("functions are not allowed in {} definitions", adt_ty),
+                );
+                err.help("unlike in C++, Java, and C#, functions are declared in `impl` blocks");
+                err.help("see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information");
+                err
+            } else {
+                self.expected_ident_found()
+            };
+            return Err(err);
+        }
+        self.bump();
+        Ok(ident)
+    }
+
     /// Parses a declarative macro 2.0 definition.
     /// The `macro` keyword has already been parsed.
     /// ```
diff --git a/compiler/rustc_parse/src/parser/mod.rs b/compiler/rustc_parse/src/parser/mod.rs
@@ -522,27 +522,27 @@ impl<'a> Parser<'a> {
         self.parse_ident_common(true)
     }
 
+    fn ident_or_err(&mut self) -> PResult<'a, (Ident, /* is_raw */ bool)> {
+        self.token.ident().ok_or_else(|| match self.prev_token.kind {
+            TokenKind::DocComment(..) => {
+                self.span_fatal_err(self.prev_token.span, Error::UselessDocComment)
+            }
+            _ => self.expected_ident_found(),
+        })
+    }
+
     fn parse_ident_common(&mut self, recover: bool) -> PResult<'a, Ident> {
-        match self.token.ident() {
-            Some((ident, is_raw)) => {
-                if !is_raw && ident.is_reserved() {
-                    let mut err = self.expected_ident_found();
-                    if recover {
-                        err.emit();
-                    } else {
-                        return Err(err);
-                    }
-                }
-                self.bump();
-                Ok(ident)
+        let (ident, is_raw) = self.ident_or_err()?;
+        if !is_raw && ident.is_reserved() {
+            let mut err = self.expected_ident_found();
+            if recover {
+                err.emit();
+            } else {
+                return Err(err);
             }
-            _ => Err(match self.prev_token.kind {
-                TokenKind::DocComment(..) => {
-                    self.span_fatal_err(self.prev_token.span, Error::UselessDocComment)
-                }
-                _ => self.expected_ident_found(),
-            }),
         }
+        self.bump();
+        Ok(ident)
     }
 
     /// Checks if the next token is `tok`, and returns `true` if so.
diff --git a/src/test/ui/structs/struct-fn-in-definition.rs b/src/test/ui/structs/struct-fn-in-definition.rs
@@ -0,0 +1,33 @@
+// It might be intuitive for a user coming from languages like Java
+// to declare a method directly in a struct's definition. Make sure
+// rustc can give a helpful suggestion.
+// Suggested in issue #76421
+
+struct S {
+    field: usize,
+
+    fn foo() {}
+    //~^ ERROR functions are not allowed in struct definitions
+    //~| HELP unlike in C++, Java, and C#, functions are declared in `impl` blocks
+    //~| HELP see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+}
+
+union U {
+    variant: usize,
+
+    fn foo() {}
+    //~^ ERROR functions are not allowed in union definitions
+    //~| HELP unlike in C++, Java, and C#, functions are declared in `impl` blocks
+    //~| HELP see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+}
+
+enum E {
+    Variant,
+
+    fn foo() {}
+    //~^ ERROR functions are not allowed in enum definitions
+    //~| HELP unlike in C++, Java, and C#, functions are declared in `impl` blocks
+    //~| HELP see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+}
+
+fn main() {}
diff --git a/src/test/ui/structs/struct-fn-in-definition.stderr b/src/test/ui/structs/struct-fn-in-definition.stderr
@@ -0,0 +1,29 @@
+error: functions are not allowed in struct definitions
+  --> $DIR/struct-fn-in-definition.rs:9:5
+   |
+LL |     fn foo() {}
+   |     ^^^^^^^^^^^
+   |
+   = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
+   = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+
+error: functions are not allowed in union definitions
+  --> $DIR/struct-fn-in-definition.rs:18:5
+   |
+LL |     fn foo() {}
+   |     ^^^^^^^^^^^
+   |
+   = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
+   = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+
+error: functions are not allowed in enum definitions
+  --> $DIR/struct-fn-in-definition.rs:27:5
+   |
+LL |     fn foo() {}
+   |     ^^^^^^^^^^^
+   |
+   = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
+   = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information
+
+error: aborting due to 3 previous errors
+
