Auto merge of #61331 - estebank:fn-arg-parse-recovery, r=varkor

bors · bors · commit c57ed9d9478d · 2019-06-03T05:40:53.000Z
Recover gracefully from argument with missing type or param name
diff --git a/src/libsyntax/parse/diagnostics.rs b/src/libsyntax/parse/diagnostics.rs
@@ -1,7 +1,6 @@
-use crate::ast;
 use crate::ast::{
-    BlockCheckMode, BinOpKind, Expr, ExprKind, Item, ItemKind, Pat, PatKind, PathSegment, QSelf,
-    Ty, TyKind, VariantData,
+    self, Arg, BinOpKind, BindingMode, BlockCheckMode, Expr, ExprKind, Ident, Item, ItemKind,
+    Mutability, Pat, PatKind, PathSegment, QSelf, Ty, TyKind, VariantData,
 };
 use crate::parse::{SeqSep, token, PResult, Parser};
 use crate::parse::parser::{BlockMode, PathStyle, SemiColonMode, TokenType, TokenExpectType};
@@ -12,9 +11,25 @@ use crate::symbol::{kw, sym};
 use crate::ThinVec;
 use crate::util::parser::AssocOp;
 use errors::{Applicability, DiagnosticBuilder, DiagnosticId};
+use rustc_data_structures::fx::FxHashSet;
 use syntax_pos::{Span, DUMMY_SP, MultiSpan};
 use log::{debug, trace};
 
+/// Creates a placeholder argument.
+crate fn dummy_arg(ident: Ident) -> Arg {
+    let pat = P(Pat {
+        id: ast::DUMMY_NODE_ID,
+        node: PatKind::Ident(BindingMode::ByValue(Mutability::Immutable), ident, None),
+        span: ident.span,
+    });
+    let ty = Ty {
+        node: TyKind::Err,
+        span: ident.span,
+        id: ast::DUMMY_NODE_ID
+    };
+    Arg { ty: P(ty), pat: pat, id: ast::DUMMY_NODE_ID, source: ast::ArgSource::Normal }
+}
+
 pub enum Error {
     FileNotFoundForModule {
         mod_name: String,
@@ -1092,12 +1107,12 @@ impl<'a> Parser<'a> {
         pat: P<ast::Pat>,
         require_name: bool,
         is_trait_item: bool,
-    ) {
+    ) -> Option<Ident> {
         // If we find a pattern followed by an identifier, it could be an (incorrect)
         // C-style parameter declaration.
         if self.check_ident() && self.look_ahead(1, |t| {
             *t == token::Comma || *t == token::CloseDelim(token::Paren)
-        }) {
+        }) { // `fn foo(String s) {}`
             let ident = self.parse_ident().unwrap();
             let span = pat.span.with_hi(ident.span.hi());
 
@@ -1107,18 +1122,30 @@ impl<'a> Parser<'a> {
                 String::from("<identifier>: <type>"),
                 Applicability::HasPlaceholders,
             );
-        } else if require_name && is_trait_item {
-            if let PatKind::Ident(_, ident, _) = pat.node {
+            return Some(ident);
+        } else if let PatKind::Ident(_, ident, _) = pat.node {
+            if require_name && (
+                is_trait_item ||
+                self.token == token::Comma ||
+                self.token == token::CloseDelim(token::Paren)
+            ) { // `fn foo(a, b) {}` or `fn foo(usize, usize) {}`
                 err.span_suggestion(
                     pat.span,
-                    "explicitly ignore parameter",
+                    "if this was a parameter name, give it a type",
+                    format!("{}: TypeName", ident),
+                    Applicability::HasPlaceholders,
+                );
+                err.span_suggestion(
+                    pat.span,
+                    "if this is a type, explicitly ignore the parameter name",
                     format!("_: {}", ident),
                     Applicability::MachineApplicable,
                 );
+                err.note("anonymous parameters are removed in the 2018 edition (see RFC 1685)");
+                return Some(ident);
             }
-
-            err.note("anonymous parameters are removed in the 2018 edition (see RFC 1685)");
         }
+        None
     }
 
     crate fn recover_arg_parse(&mut self) -> PResult<'a, (P<ast::Pat>, P<ast::Ty>)> {
@@ -1205,4 +1232,31 @@ impl<'a> Parser<'a> {
         err.span_label(span, "expected expression");
         err
     }
+
+    /// Replace duplicated recovered arguments with `_` pattern to avoid unecessary errors.
+    ///
+    /// This is necessary because at this point we don't know whether we parsed a function with
+    /// anonymous arguments or a function with names but no types. In order to minimize
+    /// unecessary errors, we assume the arguments are in the shape of `fn foo(a, b, c)` where
+    /// the arguments are *names* (so we don't emit errors about not being able to find `b` in
+    /// the local scope), but if we find the same name multiple times, like in `fn foo(i8, i8)`,
+    /// we deduplicate them to not complain about duplicated argument names.
+    crate fn deduplicate_recovered_arg_names(&self, fn_inputs: &mut Vec<Arg>) {
+        let mut seen_inputs = FxHashSet::default();
+        for input in fn_inputs.iter_mut() {
+            let opt_ident = if let (PatKind::Ident(_, ident, _), TyKind::Err) = (
+                &input.pat.node, &input.ty.node,
+            ) {
+                Some(*ident)
+            } else {
+                None
+            };
+            if let Some(ident) = opt_ident {
+                if seen_inputs.contains(&ident) {
+                    input.pat.node = PatKind::Wild;
+                }
+                seen_inputs.insert(ident);
+            }
+        }
+    }
 }
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
@@ -47,7 +47,7 @@ use crate::parse::PResult;
 use crate::ThinVec;
 use crate::tokenstream::{self, DelimSpan, TokenTree, TokenStream, TreeAndJoint};
 use crate::symbol::{kw, sym, Symbol};
-use crate::parse::diagnostics::Error;
+use crate::parse::diagnostics::{Error, dummy_arg};
 
 use errors::{Applicability, DiagnosticBuilder, DiagnosticId, FatalError};
 use rustc_target::spec::abi::{self, Abi};
@@ -451,22 +451,6 @@ impl From<P<Expr>> for LhsExpr {
     }
 }
 
-/// Creates a placeholder argument.
-fn dummy_arg(span: Span) -> Arg {
-    let ident = Ident::new(kw::Invalid, span);
-    let pat = P(Pat {
-        id: ast::DUMMY_NODE_ID,
-        node: PatKind::Ident(BindingMode::ByValue(Mutability::Immutable), ident, None),
-        span,
-    });
-    let ty = Ty {
-        node: TyKind::Err,
-        span,
-        id: ast::DUMMY_NODE_ID
-    };
-    Arg { ty: P(ty), pat: pat, id: ast::DUMMY_NODE_ID, source: ast::ArgSource::Normal }
-}
-
 #[derive(Copy, Clone, Debug)]
 crate enum TokenExpectType {
     Expect,
@@ -1528,8 +1512,17 @@ impl<'a> Parser<'a> {
             let pat = self.parse_pat(Some("argument name"))?;
 
             if let Err(mut err) = self.expect(&token::Colon) {
-                self.argument_without_type(&mut err, pat, require_name, is_trait_item);
-                return Err(err);
+                if let Some(ident) = self.argument_without_type(
+                    &mut err,
+                    pat,
+                    require_name,
+                    is_trait_item,
+                ) {
+                    err.emit();
+                    return Ok(dummy_arg(ident));
+                } else {
+                    return Err(err);
+                }
             }
 
             self.eat_incorrect_doc_comment("a method argument's type");
@@ -5432,7 +5425,7 @@ impl<'a> Parser<'a> {
                             p.eat_to_tokens(&[&token::Comma, &token::CloseDelim(token::Paren)]);
                             // Create a placeholder argument for proper arg count (issue #34264).
                             let span = lo.to(p.prev_span);
-                            Ok(Some(dummy_arg(span)))
+                            Ok(Some(dummy_arg(Ident::new(kw::Invalid, span))))
                         }
                     }
                 }
@@ -5585,7 +5578,7 @@ impl<'a> Parser<'a> {
 
         // Parse the rest of the function parameter list.
         let sep = SeqSep::trailing_allowed(token::Comma);
-        let (fn_inputs, recovered) = if let Some(self_arg) = self_arg {
+        let (mut fn_inputs, recovered) = if let Some(self_arg) = self_arg {
             if self.check(&token::CloseDelim(token::Paren)) {
                 (vec![self_arg], false)
             } else if self.eat(&token::Comma) {
@@ -5608,6 +5601,9 @@ impl<'a> Parser<'a> {
             // Parse closing paren and return type.
             self.expect(&token::CloseDelim(token::Paren))?;
         }
+        // Replace duplicated recovered arguments with `_` pattern to avoid unecessary errors.
+        self.deduplicate_recovered_arg_names(&mut fn_inputs);
+
         Ok(P(FnDecl {
             inputs: fn_inputs,
             output: self.parse_ret_ty(true)?,
diff --git a/src/test/ui/anon-params-denied-2018.rs b/src/test/ui/anon-params-denied-2018.rs
@@ -6,7 +6,13 @@ trait T {
     fn foo(i32); //~ expected one of `:` or `@`, found `)`
 
     fn bar_with_default_impl(String, String) {}
-    //~^ ERROR expected one of `:` or `@`, found `,`
+    //~^ ERROR expected one of `:`
+    //~| ERROR expected one of `:`
+
+    // do not complain about missing `b`
+    fn baz(a:usize, b, c: usize) -> usize { //~ ERROR expected one of `:`
+        a + b + c
+    }
 }
 
 fn main() {}
diff --git a/src/test/ui/anon-params-denied-2018.stderr b/src/test/ui/anon-params-denied-2018.stderr
@@ -2,21 +2,65 @@ error: expected one of `:` or `@`, found `)`
   --> $DIR/anon-params-denied-2018.rs:6:15
    |
 LL |     fn foo(i32);
-   |            ---^ expected one of `:` or `@` here
-   |            |
-   |            help: explicitly ignore parameter: `_: i32`
+   |               ^ expected one of `:` or `@` here
    |
    = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL |     fn foo(i32: TypeName);
+   |            ^^^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL |     fn foo(_: i32);
+   |            ^^^^^^
 
 error: expected one of `:` or `@`, found `,`
   --> $DIR/anon-params-denied-2018.rs:8:36
    |
 LL |     fn bar_with_default_impl(String, String) {}
-   |                              ------^ expected one of `:` or `@` here
-   |                              |
-   |                              help: explicitly ignore parameter: `_: String`
+   |                                    ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL |     fn bar_with_default_impl(String: TypeName, String) {}
+   |                              ^^^^^^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL |     fn bar_with_default_impl(_: String, String) {}
+   |                              ^^^^^^^^^
+
+error: expected one of `:` or `@`, found `)`
+  --> $DIR/anon-params-denied-2018.rs:8:44
+   |
+LL |     fn bar_with_default_impl(String, String) {}
+   |                                            ^ expected one of `:` or `@` here
    |
    = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL |     fn bar_with_default_impl(String, String: TypeName) {}
+   |                                      ^^^^^^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL |     fn bar_with_default_impl(String, _: String) {}
+   |                                      ^^^^^^^^^
+
+error: expected one of `:` or `@`, found `,`
+  --> $DIR/anon-params-denied-2018.rs:13:22
+   |
+LL |     fn baz(a:usize, b, c: usize) -> usize {
+   |                      ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL |     fn baz(a:usize, b: TypeName, c: usize) -> usize {
+   |                     ^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL |     fn baz(a:usize, _: b, c: usize) -> usize {
+   |                     ^^^^
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
 
diff --git a/src/test/ui/parser/inverted-parameters.rs b/src/test/ui/parser/inverted-parameters.rs
@@ -20,6 +20,8 @@ fn pattern((i32, i32) (a, b)) {}
 
 fn fizz(i32) {}
 //~^ ERROR expected one of `:` or `@`
+//~| HELP if this was a parameter name, give it a type
+//~| HELP if this is a type, explicitly ignore the parameter name
 
 fn missing_colon(quux S) {}
 //~^ ERROR expected one of `:` or `@`
diff --git a/src/test/ui/parser/inverted-parameters.stderr b/src/test/ui/parser/inverted-parameters.stderr
@@ -33,9 +33,19 @@ error: expected one of `:` or `@`, found `)`
    |
 LL | fn fizz(i32) {}
    |            ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL | fn fizz(i32: TypeName) {}
+   |         ^^^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL | fn fizz(_: i32) {}
+   |         ^^^^^^
 
 error: expected one of `:` or `@`, found `S`
-  --> $DIR/inverted-parameters.rs:24:23
+  --> $DIR/inverted-parameters.rs:26:23
    |
 LL | fn missing_colon(quux S) {}
    |                  -----^
diff --git a/src/test/ui/parser/omitted-arg-in-item-fn.stderr b/src/test/ui/parser/omitted-arg-in-item-fn.stderr
@@ -3,6 +3,16 @@ error: expected one of `:` or `@`, found `)`
    |
 LL | fn foo(x) {
    |         ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL | fn foo(x: TypeName) {
+   |        ^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL | fn foo(_: x) {
+   |        ^^^^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/span/issue-34264.stderr b/src/test/ui/span/issue-34264.stderr
@@ -9,12 +9,32 @@ error: expected one of `:` or `@`, found `)`
    |
 LL | fn foo(Option<i32>, String) {}
    |                           ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL | fn foo(Option<i32>, String: TypeName) {}
+   |                     ^^^^^^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL | fn foo(Option<i32>, _: String) {}
+   |                     ^^^^^^^^^
 
 error: expected one of `:` or `@`, found `,`
   --> $DIR/issue-34264.rs:3:9
    |
 LL | fn bar(x, y: usize) {}
    |         ^ expected one of `:` or `@` here
+   |
+   = note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
+help: if this was a parameter name, give it a type
+   |
+LL | fn bar(x: TypeName, y: usize) {}
+   |        ^^^^^^^^^^^
+help: if this is a type, explicitly ignore the parameter name
+   |
+LL | fn bar(_: x, y: usize) {}
+   |        ^^^^
 
 error[E0061]: this function takes 2 parameters but 3 parameters were supplied
   --> $DIR/issue-34264.rs:7:5
