syntax: unify and simplify fn signature parsing.

Centril · Centril · commit a7ba754b6c02 · 2019-10-07T06:01:38.000+02:00
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
@@ -11,7 +11,7 @@ mod stmt;
 mod generics;
 
 use crate::ast::{
-    self, DUMMY_NODE_ID, AttrStyle, Attribute, BindingMode, CrateSugar, FnDecl, Ident,
+    self, DUMMY_NODE_ID, AttrStyle, Attribute, BindingMode, CrateSugar, Ident,
     IsAsync, MacDelimiter, Mutability, Param, StrStyle, SelfKind, TyKind, Visibility,
     VisibilityKind, Unsafety,
 };
@@ -56,6 +56,17 @@ crate enum BlockMode {
     Ignore,
 }
 
+/// The parsing configuration used to parse a parameter list (see `parse_fn_params`).
+struct ParamCfg {
+    /// Is `self` is allowed as the first parameter?
+    is_self_allowed: bool,
+    /// Is `...` allowed as the tail of the parameter list?
+    allow_c_variadic: bool,
+    /// `is_name_required` decides if, per-parameter,
+    /// the parameter must have a pattern or just a type.
+    is_name_required: fn(&token::Token) -> bool,
+}
+
 /// Like `maybe_whole_expr`, but for things other than expressions.
 #[macro_export]
 macro_rules! maybe_whole {
@@ -1094,26 +1105,18 @@ impl<'a> Parser<'a> {
         res
     }
 
-    fn parse_fn_params(
-        &mut self,
-        named_params: bool,
-        allow_c_variadic: bool,
-    ) -> PResult<'a, Vec<Param>> {
+    /// Parses the parameter list of a function, including the `(` and `)` delimiters.
+    fn parse_fn_params(&mut self, mut cfg: ParamCfg) -> PResult<'a, Vec<Param>> {
         let sp = self.token.span;
-        let do_not_enforce_named_params_for_c_variadic = |token: &token::Token| {
-            match token.kind {
-                token::DotDotDot => false,
-                _ => named_params,
-            }
-        };
+        let is_trait_item = cfg.is_self_allowed;
         let mut c_variadic = false;
+        // Parse the arguments, starting out with `self` being possibly allowed...
         let (params, _) = self.parse_paren_comma_seq(|p| {
-            match p.parse_param_general(
-                false,
-                false,
-                allow_c_variadic,
-                do_not_enforce_named_params_for_c_variadic,
-            ) {
+            let param = p.parse_param_general(&cfg, is_trait_item);
+            // ...now that we've parsed the first argument, `self` is no longer allowed.
+            cfg.is_self_allowed = false;
+
+            match param {
                 Ok(param) => Ok(
                     if let TyKind::CVarArgs = param.ty.kind {
                         c_variadic = true;
@@ -1144,7 +1147,10 @@ impl<'a> Parser<'a> {
             }
         })?;
 
-        let params: Vec<_> = params.into_iter().filter_map(|x| x).collect();
+        let mut params: Vec<_> = params.into_iter().filter_map(|x| x).collect();
+
+        // Replace duplicated recovered params with `_` pattern to avoid unecessary errors.
+        self.deduplicate_recovered_params_names(&mut params);
 
         if c_variadic && params.len() <= 1 {
             self.span_err(
@@ -1156,79 +1162,53 @@ impl<'a> Parser<'a> {
         Ok(params)
     }
 
-    /// Parses the parameter list and result type of a function that may have a `self` parameter.
-    fn parse_fn_decl_with_self(
-        &mut self,
-        is_name_required: impl Copy + Fn(&token::Token) -> bool,
-    ) -> PResult<'a, P<FnDecl>> {
-        // Parse the arguments, starting out with `self` being allowed...
-        let mut is_self_allowed = true;
-        let (mut inputs, _): (Vec<_>, _) = self.parse_paren_comma_seq(|p| {
-            let res = p.parse_param_general(is_self_allowed, true, false, is_name_required);
-            // ...but now that we've parsed the first argument, `self` is no longer allowed.
-            is_self_allowed = false;
-            res
-        })?;
-
-        // Replace duplicated recovered params with `_` pattern to avoid unecessary errors.
-        self.deduplicate_recovered_params_names(&mut inputs);
-
-        Ok(P(FnDecl {
-            inputs,
-            output: self.parse_ret_ty(true)?,
-        }))
-    }
-
     /// Skips unexpected attributes and doc comments in this position and emits an appropriate
     /// error.
     /// This version of parse param doesn't necessarily require identifier names.
-    fn parse_param_general(
-        &mut self,
-        is_self_allowed: bool,
-        is_trait_item: bool,
-        allow_c_variadic: bool,
-        is_name_required: impl Fn(&token::Token) -> bool,
-    ) -> PResult<'a, Param> {
+    fn parse_param_general(&mut self, cfg: &ParamCfg, is_trait_item: bool) -> PResult<'a, Param> {
         let lo = self.token.span;
         let attrs = self.parse_outer_attributes()?;
 
         // Possibly parse `self`. Recover if we parsed it and it wasn't allowed here.
         if let Some(mut param) = self.parse_self_param()? {
             param.attrs = attrs.into();
-            return if is_self_allowed {
+            return if cfg.is_self_allowed {
                 Ok(param)
             } else {
                 self.recover_bad_self_param(param, is_trait_item)
             };
         }
 
-        let is_name_required = is_name_required(&self.token);
+        let is_name_required = match self.token.kind {
+            token::DotDotDot => false,
+            _ => (cfg.is_name_required)(&self.token),
+        };
         let (pat, ty) = if is_name_required || self.is_named_param() {
             debug!("parse_param_general parse_pat (is_name_required:{})", is_name_required);
 
             let pat = self.parse_fn_param_pat()?;
             if let Err(mut err) = self.expect(&token::Colon) {
-                if let Some(ident) = self.parameter_without_type(
+                return if let Some(ident) = self.parameter_without_type(
                     &mut err,
                     pat,
                     is_name_required,
-                    is_self_allowed,
+                    cfg.is_self_allowed,
                     is_trait_item,
                 ) {
                     err.emit();
-                    return Ok(dummy_arg(ident));
+                    Ok(dummy_arg(ident))
                 } else {
-                    return Err(err);
-                }
+                    Err(err)
+                };
             }
 
             self.eat_incorrect_doc_comment_for_param_type();
-            (pat, self.parse_ty_common(true, true, allow_c_variadic)?)
+            (pat, self.parse_ty_common(true, true, cfg.allow_c_variadic)?)
         } else {
             debug!("parse_param_general ident_to_pat");
             let parser_snapshot_before_ty = self.clone();
             self.eat_incorrect_doc_comment_for_param_type();
-            let mut ty = self.parse_ty_common(true, true, allow_c_variadic);
+            let mut ty = self.parse_ty_common(true, true, cfg.allow_c_variadic);
             if ty.is_ok() && self.token != token::Comma &&
                self.token != token::CloseDelim(token::Paren) {
                 // This wasn't actually a type, but a pattern looking like a type,
diff --git a/src/libsyntax/parse/parser/item.rs b/src/libsyntax/parse/parser/item.rs
@@ -1,4 +1,4 @@
-use super::{Parser, PResult, PathStyle, SemiColonMode, BlockMode};
+use super::{Parser, PResult, PathStyle, SemiColonMode, BlockMode, ParamCfg};
 
 use crate::maybe_whole;
 use crate::ptr::P;
@@ -805,14 +805,15 @@ impl<'a> Parser<'a> {
     /// of a method. The body is not parsed as that differs between `trait`s and `impl`s.
     fn parse_method_sig(
         &mut self,
-        is_name_required: impl Copy + Fn(&token::Token) -> bool,
+        is_name_required: fn(&token::Token) -> bool,
     ) -> PResult<'a, (Ident, MethodSig, Generics)> {
         let header = self.parse_fn_front_matter()?;
-        let (ident, mut generics) = self.parse_fn_header()?;
-        let decl = self.parse_fn_decl_with_self(is_name_required)?;
-        let sig = MethodSig { header, decl };
-        generics.where_clause = self.parse_where_clause()?;
-        Ok((ident, sig, generics))
+        let (ident, decl, generics) = self.parse_fn_sig(ParamCfg {
+            is_self_allowed: true,
+            allow_c_variadic: false,
+            is_name_required,
+        })?;
+        Ok((ident, MethodSig { header, decl }, generics))
     }
 
     /// Parses all the "front matter" for a `fn` declaration, up to
@@ -1200,36 +1201,34 @@ impl<'a> Parser<'a> {
         attrs: Vec<Attribute>,
         header: FnHeader,
     ) -> PResult<'a, Option<P<Item>>> {
-        let allow_c_variadic = header.abi == Abi::C && header.unsafety == Unsafety::Unsafe;
-        let (ident, decl, generics) = self.parse_fn_sig(allow_c_variadic)?;
+        let (ident, decl, generics) = self.parse_fn_sig(ParamCfg {
+            is_self_allowed: false,
+            allow_c_variadic: header.abi == Abi::C && header.unsafety == Unsafety::Unsafe,
+            is_name_required: |_| true,
+        })?;
         let (inner_attrs, body) = self.parse_inner_attrs_and_block()?;
         let kind = ItemKind::Fn(decl, header, generics, body);
         self.mk_item_with_info(attrs, lo, vis, (ident, kind, Some(inner_attrs)))
     }
 
     /// Parse the "signature", including the identifier, parameters, and generics of a function.
-    fn parse_fn_sig(
-        &mut self,
-        allow_c_variadic: bool,
-    ) -> PResult<'a, (Ident, P<FnDecl>, Generics)> {
-        let (ident, mut generics) = self.parse_fn_header()?;
-        let decl = self.parse_fn_decl(allow_c_variadic)?;
+    fn parse_fn_sig(&mut self, cfg: ParamCfg) -> PResult<'a, (Ident, P<FnDecl>, Generics)> {
+        let ident = self.parse_ident()?;
+        let mut generics = self.parse_generics()?;
+        let decl = self.parse_fn_decl(cfg, true)?;
         generics.where_clause = self.parse_where_clause()?;
         Ok((ident, decl, generics))
     }
 
-    /// Parses the name and optional generic types of a function header.
-    fn parse_fn_header(&mut self) -> PResult<'a, (Ident, Generics)> {
-        let id = self.parse_ident()?;
-        let generics = self.parse_generics()?;
-        Ok((id, generics))
-    }
-
     /// Parses the parameter list and result type of a function declaration.
-    fn parse_fn_decl(&mut self, allow_c_variadic: bool) -> PResult<'a, P<FnDecl>> {
+    pub(super) fn parse_fn_decl(
+        &mut self,
+        cfg: ParamCfg,
+        ret_allow_plus: bool,
+    ) -> PResult<'a, P<FnDecl>> {
         Ok(P(FnDecl {
-            inputs: self.parse_fn_params(true, allow_c_variadic)?,
-            output: self.parse_ret_ty(true)?,
+            inputs: self.parse_fn_params(cfg)?,
+            output: self.parse_ret_ty(ret_allow_plus)?,
         }))
     }
 
@@ -1353,7 +1352,11 @@ impl<'a> Parser<'a> {
         extern_sp: Span,
     ) -> PResult<'a, ForeignItem> {
         self.expect_keyword(kw::Fn)?;
-        let (ident, decl, generics) = self.parse_fn_sig(true)?;
+        let (ident, decl, generics) = self.parse_fn_sig(super::ParamCfg {
+            is_self_allowed: false,
+            allow_c_variadic: true,
+            is_name_required: |_| true,
+        })?;
         let span = lo.to(self.token.span);
         self.parse_semi_or_incorrect_foreign_fn_body(&ident, extern_sp)?;
         Ok(ast::ForeignItem {
diff --git a/src/libsyntax/parse/parser/ty.rs b/src/libsyntax/parse/parser/ty.rs
@@ -4,7 +4,7 @@ use crate::{maybe_whole, maybe_recover_from_interpolated_ty_qpath};
 use crate::ptr::P;
 use crate::ast::{self, Ty, TyKind, MutTy, BareFnTy, FunctionRetTy, GenericParam, Lifetime, Ident};
 use crate::ast::{TraitBoundModifier, TraitObjectSyntax, GenericBound, GenericBounds, PolyTraitRef};
-use crate::ast::{Mutability, AnonConst, FnDecl, Mac};
+use crate::ast::{Mutability, AnonConst, Mac};
 use crate::parse::token::{self, Token};
 use crate::source_map::Span;
 use crate::symbol::{kw};
@@ -288,12 +288,12 @@ impl<'a> Parser<'a> {
         };
 
         self.expect_keyword(kw::Fn)?;
-        let inputs = self.parse_fn_params(false, true)?;
-        let ret_ty = self.parse_ret_ty(false)?;
-        let decl = P(FnDecl {
-            inputs,
-            output: ret_ty,
-        });
+        let cfg = super::ParamCfg {
+            is_self_allowed: false,
+            allow_c_variadic: true,
+            is_name_required: |_| false,
+        };
+        let decl = self.parse_fn_decl(cfg, false)?;
         Ok(TyKind::BareFn(P(BareFnTy {
             abi,
             unsafety,
diff --git a/src/test/ui/parser/issue-33413.rs b/src/test/ui/parser/issue-33413.rs
@@ -3,6 +3,7 @@ struct S;
 impl S {
     fn f(*, a: u8) -> u8 {}
     //~^ ERROR expected parameter name, found `*`
+    //~| ERROR mismatched types
 }
 
 fn main() {}
diff --git a/src/test/ui/parser/issue-33413.stderr b/src/test/ui/parser/issue-33413.stderr
@@ -4,5 +4,17 @@ error: expected parameter name, found `*`
 LL |     fn f(*, a: u8) -> u8 {}
    |          ^ expected parameter name
 
-error: aborting due to previous error
+error[E0308]: mismatched types
+  --> $DIR/issue-33413.rs:4:23
+   |
+LL |     fn f(*, a: u8) -> u8 {}
+   |        -              ^^ expected u8, found ()
+   |        |
+   |        implicitly returns `()` as its body has no tail or `return` expression
+   |
+   = note: expected type `u8`
+              found type `()`
+
+error: aborting due to 2 previous errors
 
+For more information about this error, try `rustc --explain E0308`.

Original file line number	Diff line number	Diff line change
`@@ -3,6 +3,7 @@ struct S;`
`3`	`3`	`impl S {`
`4`	`4`	`fn f(*, a: u8) -> u8 {}`
`5`	`5`	//~^ ERROR expected parameter name, found `*`
	`6`	`+ //~\| ERROR mismatched types`
`6`	`7`	`}`
`7`	`8`
`8`	`9`	`fn main() {}`