syntax: Refactor parsing of method declarations

Fix spans and expected token lists, fix #33413 + other cosmetic improvements
Add test for #33413
Convert between `Arg` and `ExplicitSelf` precisely
Simplify pretty-printing for methods

src/libsyntax/ast.rs

@@ -16,13 +16,12 @@
 pub use self::PathParameters::*;
 
 use attr::ThinAttributes;
-use codemap::{Span, Spanned, DUMMY_SP, ExpnId};
+use codemap::{mk_sp, respan, Span, Spanned, DUMMY_SP, ExpnId};
 use abi::Abi;
 use errors;
 use ext::base;
 use ext::tt::macro_parser;
-use parse::token::InternedString;
-use parse::token;
+use parse::token::{self, keywords, InternedString};
 use parse::lexer;
 use parse::lexer::comments::{doc_comment_style, strip_doc_comment_decoration};
 use print::pprust;
@@ -1674,7 +1673,25 @@ pub struct Arg {
     pub id: NodeId,
 }
 
+/// Represents the kind of 'self' associated with a method.
+/// String representation of `Ident` here is always "self", but hygiene contexts may differ.
+#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
+pub enum SelfKind {
+    /// No self
+    Static,
+    /// `self`, `mut self`
+    Value(Ident),
+    /// `&'lt self`, `&'lt mut self`
+    Region(Option<Lifetime>, Mutability, Ident),
+    /// `self: TYPE`, `mut self: TYPE`
+    Explicit(P<Ty>, Ident),
+}
+
+pub type ExplicitSelf = Spanned<SelfKind>;
+
 impl Arg {
+    #[unstable(feature = "rustc_private", issue = "27812")]
+    #[rustc_deprecated(since = "1.10.0", reason = "use `from_self` instead")]
     pub fn new_self(span: Span, mutability: Mutability, self_ident: Ident) -> Arg {
         let path = Spanned{span:span,node:self_ident};
         Arg {
@@ -1692,6 +1709,51 @@ pub fn new_self(span: Span, mutability: Mutability, self_ident: Ident) -> Arg {
             id: DUMMY_NODE_ID
         }
     }
+
+    pub fn to_self(&self) -> Option<ExplicitSelf> {
+        if let PatKind::Ident(_, ident, _) = self.pat.node {
+            if ident.node.name == keywords::SelfValue.name() {
+                return match self.ty.node {
+                    TyKind::Infer => Some(respan(self.pat.span, SelfKind::Value(ident.node))),
+                    TyKind::Rptr(lt, MutTy{ref ty, mutbl}) if ty.node == TyKind::Infer => {
+                        Some(respan(self.pat.span, SelfKind::Region(lt, mutbl, ident.node)))
+                    }
+                    _ => Some(respan(mk_sp(self.pat.span.lo, self.ty.span.hi),
+                                     SelfKind::Explicit(self.ty.clone(), ident.node))),
+                }
+            }
+        }
+        None
+    }
+
+    pub fn from_self(eself: ExplicitSelf, ident_sp: Span, mutbl: Mutability) -> Arg {
+        let pat = |ident, span| P(Pat {
+            id: DUMMY_NODE_ID,
+            node: PatKind::Ident(BindingMode::ByValue(mutbl), respan(ident_sp, ident), None),
+            span: span,
+        });
+        let infer_ty = P(Ty {
+            id: DUMMY_NODE_ID,
+            node: TyKind::Infer,
+            span: DUMMY_SP,
+        });
+        let arg = |ident, ty, span| Arg {
+            pat: pat(ident, span),
+            ty: ty,
+            id: DUMMY_NODE_ID,
+        };
+        match eself.node {
+            SelfKind::Static => panic!("bug: `Arg::from_self` is called \
+                                        with `SelfKind::Static` argument"),
+            SelfKind::Explicit(ty, ident) => arg(ident, ty, mk_sp(eself.span.lo, ident_sp.hi)),
+            SelfKind::Value(ident) => arg(ident, infer_ty, eself.span),
+            SelfKind::Region(lt, mutbl, ident) => arg(ident, P(Ty {
+                id: DUMMY_NODE_ID,
+                node: TyKind::Rptr(lt, MutTy { ty: infer_ty, mutbl: mutbl }),
+                span: DUMMY_SP,
+            }), eself.span),
+        }
+    }
 }
 
 /// Represents the header (not the body) of a function declaration
@@ -1772,21 +1834,6 @@ pub fn span(&self) -> Span {
     }
 }
 
-/// Represents the kind of 'self' associated with a method
-#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
-pub enum SelfKind {
-    /// No self
-    Static,
-    /// `self`
-    Value(Ident),
-    /// `&'lt self`, `&'lt mut self`
-    Region(Option<Lifetime>, Mutability, Ident),
-    /// `self: TYPE`
-    Explicit(P<Ty>, Ident),
-}
-
-pub type ExplicitSelf = Spanned<SelfKind>;
-
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
 pub struct Mod {
     /// A span from the first token past `{` to the last token until `}`.

src/libsyntax/parse/parser.rs

@@ -2036,7 +2036,7 @@ pub fn parse_lifetimes(&mut self, sep: token::Token) -> PResult<'a, Vec<ast::Lif
         }
     }
 
-    /// Parse mutability declaration (mut/const/imm)
+    /// Parse mutability (`mut` or nothing).
     pub fn parse_mutability(&mut self) -> PResult<'a, Mutability> {
         if self.eat_keyword(keywords::Mut) {
             Ok(Mutability::Mutable)
@@ -4616,184 +4616,142 @@ pub fn parse_fn_decl(&mut self, allow_variadic: bool) -> PResult<'a, P<FnDecl>>
         }))
     }
 
-    fn expect_self_ident(&mut self) -> PResult<'a, ast::Ident> {
-        match self.token {
-            token::Ident(id) if id.name == keywords::SelfValue.name() => {
-                self.bump();
-                // The hygiene context of `id` needs to be preserved here,
-                // so we can't just return `SelfValue.ident()`.
-                Ok(id)
-            },
-            _ => {
-                let token_str = self.this_token_to_string();
-                return Err(self.fatal(&format!("expected `self`, found `{}`",
-                                   token_str)))
-            }
-        }
-    }
-
-    /// Parse the argument list and result type of a function
-    /// that may have a self type.
+    /// Parse the parameter list and result type of a function that may have a `self` parameter.
     fn parse_fn_decl_with_self<F>(&mut self,
-                                  parse_arg_fn: F) -> PResult<'a, (ExplicitSelf, P<FnDecl>)> where
-        F: FnMut(&mut Parser<'a>) -> PResult<'a,  Arg>,
+                                  parse_arg_fn: F)
+                                  -> PResult<'a, (ExplicitSelf, P<FnDecl>)>
+        where F: FnMut(&mut Parser<'a>) -> PResult<'a,  Arg>,
     {
-        fn maybe_parse_borrowed_explicit_self<'b>(this: &mut Parser<'b>)
-                                                  -> PResult<'b, ast::SelfKind> {
-            // The following things are possible to see here:
-            //
-            //     fn(&mut self)
-            //     fn(&mut self)
-            //     fn(&'lt self)
-            //     fn(&'lt mut self)
-            //
-            // We already know that the current token is `&`.
-
-            if this.look_ahead(1, |t| t.is_keyword(keywords::SelfValue)) {
-                this.bump();
-                Ok(SelfKind::Region(None, Mutability::Immutable, this.expect_self_ident()?))
-            } else if this.look_ahead(1, |t| t.is_mutability()) &&
-                      this.look_ahead(2, |t| t.is_keyword(keywords::SelfValue)) {
-                this.bump();
-                let mutability = this.parse_mutability()?;
-                Ok(SelfKind::Region(None, mutability, this.expect_self_ident()?))
-            } else if this.look_ahead(1, |t| t.is_lifetime()) &&
-                      this.look_ahead(2, |t| t.is_keyword(keywords::SelfValue)) {
-                this.bump();
-                let lifetime = this.parse_lifetime()?;
-                let ident = this.expect_self_ident()?;
-                Ok(SelfKind::Region(Some(lifetime), Mutability::Immutable, ident))
-            } else if this.look_ahead(1, |t| t.is_lifetime()) &&
-                      this.look_ahead(2, |t| t.is_mutability()) &&
-                      this.look_ahead(3, |t| t.is_keyword(keywords::SelfValue)) {
-                this.bump();
-                let lifetime = this.parse_lifetime()?;
-                let mutability = this.parse_mutability()?;
-                Ok(SelfKind::Region(Some(lifetime), mutability, this.expect_self_ident()?))
-            } else {
-                Ok(SelfKind::Static)
-            }
-        }
+        let expect_ident = |this: &mut Self| match this.token {
+            token::Ident(ident) => { this.bump(); ident } // Preserve hygienic context.
+            _ => unreachable!()
+        };
 
         self.expect(&token::OpenDelim(token::Paren))?;
 
-        // A bit of complexity and lookahead is needed here in order to be
-        // backwards compatible.
-        let lo = self.span.lo;
-        let mut self_ident_lo = self.span.lo;
-        let mut self_ident_hi = self.span.hi;
-
-        let mut mutbl_self = Mutability::Immutable;
-        let explicit_self = match self.token {
+        // Parse optional self parameter of a method.
+        // Only a limited set of initial token sequences is considered self parameters, anything
+        // else is parsed as a normal function parameter list, so some lookahead is required.
+        let eself_lo = self.span.lo;
+        let mut eself_mutbl = Mutability::Immutable;
+        let (eself, eself_ident_sp) = match self.token {
             token::BinOp(token::And) => {
-                let eself = maybe_parse_borrowed_explicit_self(self)?;
-                self_ident_lo = self.last_span.lo;
-                self_ident_hi = self.last_span.hi;
-                eself
+                // &self
+                // &mut self
+                // &'lt self
+                // &'lt mut self
+                // &not_self
+                if self.look_ahead(1, |t| t.is_keyword(keywords::SelfValue)) {
+                    self.bump();
+                    (SelfKind::Region(None, Mutability::Immutable, expect_ident(self)),
+                        self.last_span)
+                } else if self.look_ahead(1, |t| t.is_keyword(keywords::Mut)) &&
+                          self.look_ahead(2, |t| t.is_keyword(keywords::SelfValue)) {
+                    self.bump();
+                    self.bump();
+                    (SelfKind::Region(None, Mutability::Mutable, expect_ident(self)),
+                        self.last_span)
+                } else if self.look_ahead(1, |t| t.is_lifetime()) &&
+                          self.look_ahead(2, |t| t.is_keyword(keywords::SelfValue)) {
+                    self.bump();
+                    let lt = self.parse_lifetime()?;
+                    (SelfKind::Region(Some(lt), Mutability::Immutable, expect_ident(self)),
+                        self.last_span)
+                } else if self.look_ahead(1, |t| t.is_lifetime()) &&
+                          self.look_ahead(2, |t| t.is_keyword(keywords::Mut)) &&
+                          self.look_ahead(3, |t| t.is_keyword(keywords::SelfValue)) {
+                    self.bump();
+                    let lt = self.parse_lifetime()?;
+                    self.bump();
+                    (SelfKind::Region(Some(lt), Mutability::Mutable, expect_ident(self)),
+                        self.last_span)
+                } else {
+                    (SelfKind::Static, codemap::DUMMY_SP)
+                }
             }
             token::BinOp(token::Star) => {
-                // Possibly "*self" or "*mut self" -- not supported. Try to avoid
-                // emitting cryptic "unexpected token" errors.
-                self.bump();
-                let _mutability = if self.token.is_mutability() {
-                    self.parse_mutability()?
-                } else {
-                    Mutability::Immutable
-                };
-                if self.token.is_keyword(keywords::SelfValue) {
-                    let span = self.span;
-                    self.span_err(span, "cannot pass self by raw pointer");
+                // *self
+                // *const self
+                // *mut self
+                // *not_self
+                // Emit special error for `self` cases.
+                if self.look_ahead(1, |t| t.is_keyword(keywords::SelfValue)) {
                     self.bump();
+                    self.span_err(self.span, "cannot pass `self` by raw pointer");
+                    (SelfKind::Value(expect_ident(self)), self.last_span)
+                } else if self.look_ahead(1, |t| t.is_mutability()) &&
+                          self.look_ahead(2, |t| t.is_keyword(keywords::SelfValue)) {
+                    self.bump();
+                    self.bump();
+                    self.span_err(self.span, "cannot pass `self` by raw pointer");
+                    (SelfKind::Value(expect_ident(self)), self.last_span)
+                } else {
+                    (SelfKind::Static, codemap::DUMMY_SP)
                 }
-                // error case, making bogus self ident:
-                SelfKind::Value(keywords::SelfValue.ident())
             }
             token::Ident(..) => {
                 if self.token.is_keyword(keywords::SelfValue) {
-                    let self_ident = self.expect_self_ident()?;
-
-                    // Determine whether this is the fully explicit form, `self:
-                    // TYPE`.
+                    // self
+                    // self: TYPE
+                    let eself_ident = expect_ident(self);
+                    let eself_ident_sp = self.last_span;
                     if self.eat(&token::Colon) {
-                        SelfKind::Explicit(self.parse_ty_sum()?, self_ident)
+                        (SelfKind::Explicit(self.parse_ty_sum()?, eself_ident), eself_ident_sp)
                     } else {
-                        SelfKind::Value(self_ident)
+                        (SelfKind::Value(eself_ident), eself_ident_sp)
                     }
-                } else if self.token.is_mutability() &&
+                } else if self.token.is_keyword(keywords::Mut) &&
                         self.look_ahead(1, |t| t.is_keyword(keywords::SelfValue)) {
-                    mutbl_self = self.parse_mutability()?;
-                    let self_ident = self.expect_self_ident()?;
-
-                    // Determine whether this is the fully explicit form,
-                    // `self: TYPE`.
+                    // mut self
+                    // mut self: TYPE
+                    eself_mutbl = Mutability::Mutable;
+                    self.bump();
+                    let eself_ident = expect_ident(self);
+                    let eself_ident_sp = self.last_span;
                     if self.eat(&token::Colon) {
-                        SelfKind::Explicit(self.parse_ty_sum()?, self_ident)
+                        (SelfKind::Explicit(self.parse_ty_sum()?, eself_ident), eself_ident_sp)
                     } else {
-                        SelfKind::Value(self_ident)
+                        (SelfKind::Value(eself_ident), eself_ident_sp)
                     }
                 } else {
-                    SelfKind::Static
+                    (SelfKind::Static, codemap::DUMMY_SP)
                 }
             }
-            _ => SelfKind::Static,
+            _ => (SelfKind::Static, codemap::DUMMY_SP)
         };
+        let mut eself = codemap::respan(mk_sp(eself_lo, self.last_span.hi), eself);
 
-        let explicit_self_sp = mk_sp(self_ident_lo, self_ident_hi);
-
-        // shared fall-through for the three cases below. borrowing prevents simply
-        // writing this as a closure
-        macro_rules! parse_remaining_arguments {
-            ($self_id:ident) =>
-            {
-            // If we parsed a self type, expect a comma before the argument list.
-            match self.token {
-                token::Comma => {
+        // Parse the rest of the function parameter list.
+        let sep = SeqSep::trailing_allowed(token::Comma);
+        let fn_inputs = match eself.node {
+            SelfKind::Static => {
+                eself.span = codemap::DUMMY_SP;
+                self.parse_seq_to_before_end(&token::CloseDelim(token::Paren), sep, parse_arg_fn)
+            }
+            SelfKind::Value(..) | SelfKind::Region(..) | SelfKind::Explicit(..) => {
+                if self.check(&token::CloseDelim(token::Paren)) {
+                    vec![Arg::from_self(eself.clone(), eself_ident_sp, eself_mutbl)]
+                } else if self.check(&token::Comma) {
                     self.bump();
-                    let sep = SeqSep::trailing_allowed(token::Comma);
-                    let mut fn_inputs = self.parse_seq_to_before_end(
-                        &token::CloseDelim(token::Paren),
-                        sep,
-                        parse_arg_fn
+                    let mut fn_inputs = vec![Arg::from_self(eself.clone(), eself_ident_sp,
+                                                            eself_mutbl)];
+                    fn_inputs.append(&mut self.parse_seq_to_before_end(
+                        &token::CloseDelim(token::Paren), sep, parse_arg_fn)
                     );
-                    fn_inputs.insert(0, Arg::new_self(explicit_self_sp, mutbl_self, $self_id));
                     fn_inputs
-                }
-                token::CloseDelim(token::Paren) => {
-                    vec!(Arg::new_self(explicit_self_sp, mutbl_self, $self_id))
-                }
-                _ => {
-                    let token_str = self.this_token_to_string();
-                    return Err(self.fatal(&format!("expected `,` or `)`, found `{}`",
-                                       token_str)))
+                } else {
+                    return self.unexpected();
                 }
             }
-            }
-        }
-
-        let fn_inputs = match explicit_self {
-            SelfKind::Static =>  {
-                let sep = SeqSep::trailing_allowed(token::Comma);
-                self.parse_seq_to_before_end(&token::CloseDelim(token::Paren), sep, parse_arg_fn)
-            }
-            SelfKind::Value(id) => parse_remaining_arguments!(id),
-            SelfKind::Region(_,_,id) => parse_remaining_arguments!(id),
-            SelfKind::Explicit(_,id) => parse_remaining_arguments!(id),
         };
 
-
+        // Parse closing paren and return type.
         self.expect(&token::CloseDelim(token::Paren))?;
-
-        let hi = self.span.hi;
-
-        let ret_ty = self.parse_ret_ty()?;
-
-        let fn_decl = P(FnDecl {
+        Ok((eself, P(FnDecl {
             inputs: fn_inputs,
-            output: ret_ty,
+            output: self.parse_ret_ty()?,
             variadic: false
-        });
-
-        Ok((spanned(lo, hi, explicit_self), fn_decl))
+        })))
     }
 
     // parse the |arg, arg| header on a lambda

src/libsyntax/print/pprust.rs

@@ -1522,7 +1522,7 @@ pub fn print_method_sig(&mut self,
                       m.abi,
                       Some(ident),
                       &m.generics,
-                      Some(&m.explicit_self.node),
+                      None,
                       vis)
     }
 
@@ -2656,36 +2656,9 @@ pub fn print_fn(&mut self,
     }
 
     pub fn print_fn_args(&mut self, decl: &ast::FnDecl,
-                         opt_explicit_self: Option<&ast::SelfKind>,
+                         _: Option<&ast::SelfKind>,
                          is_closure: bool) -> io::Result<()> {
-        // It is unfortunate to duplicate the commasep logic, but we want the
-        // self type and the args all in the same box.
-        self.rbox(0, Inconsistent)?;
-        let mut first = true;
-        if let Some(explicit_self) = opt_explicit_self {
-            let m = match *explicit_self {
-                ast::SelfKind::Static => ast::Mutability::Immutable,
-                _ => match decl.inputs[0].pat.node {
-                    PatKind::Ident(ast::BindingMode::ByValue(m), _, _) => m,
-                    _ => ast::Mutability::Immutable
-                }
-            };
-            first = !self.print_explicit_self(explicit_self, m)?;
-        }
-
-        // HACK(eddyb) ignore the separately printed self argument.
-        let args = if first {
-            &decl.inputs[..]
-        } else {
-            &decl.inputs[1..]
-        };
-
-        for arg in args {
-            if first { first = false; } else { self.word_space(",")?; }
-            self.print_arg(arg, is_closure)?;
-        }
-
-        self.end()
+        self.commasep(Inconsistent, &decl.inputs, |s, arg| s.print_arg(arg, is_closure))
     }
 
     pub fn print_fn_args_and_ret(&mut self, decl: &ast::FnDecl,
@@ -2956,18 +2929,24 @@ pub fn print_arg(&mut self, input: &ast::Arg, is_closure: bool) -> io::Result<()
         match input.ty.node {
             ast::TyKind::Infer if is_closure => self.print_pat(&input.pat)?,
             _ => {
-                match input.pat.node {
-                    PatKind::Ident(_, ref path1, _)
-                            if path1.node.name == keywords::Invalid.name() => {
-                        // Do nothing.
+                let (mutbl, invalid) = match input.pat.node {
+                    PatKind::Ident(ast::BindingMode::ByValue(mutbl), ident, _) |
+                    PatKind::Ident(ast::BindingMode::ByRef(mutbl), ident, _) => {
+                        (mutbl, ident.node.name == keywords::Invalid.name())
                     }
-                    _ => {
+                    _ => (ast::Mutability::Immutable, false)
+                };
+
+                if let Some(eself) = input.to_self() {
+                    self.print_explicit_self(&eself.node, mutbl)?;
+                } else {
+                    if !invalid {
                         self.print_pat(&input.pat)?;
                         word(&mut self.s, ":")?;
                         space(&mut self.s)?;
                     }
+                    self.print_type(&input.ty)?;
                 }
-                self.print_type(&input.ty)?;
             }
         }
         self.end()

src/libsyntax_ext/deriving/generic/mod.rs

@@ -864,9 +864,8 @@ fn create_method(&self,
         let self_arg = match explicit_self.node {
             ast::SelfKind::Static => None,
             // creating fresh self id
-            _ => Some(ast::Arg::new_self(trait_.span,
-                                         ast::Mutability::Immutable,
-                                         keywords::SelfValue.ident()))
+            _ => Some(ast::Arg::from_self(explicit_self.clone(), trait_.span,
+                                          ast::Mutability::Immutable)),
         };
         let args = {
             let args = arg_types.into_iter().map(|(name, ty)| {

src/test/parse-fail/issue-33413.rs

+// Copyright 2016 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.
+
+// compile-flags: -Z parse-only
+
+impl S {
+    fn f(*, a: u8) -> u8 {} //~ ERROR expected pattern, found `*`
+    //~^ ERROR expected one of `)`, `-`, `box`, `false`, `mut`, `ref`, or `true`, found `*`
+}

src/test/parse-fail/no-unsafe-self.rs

@@ -11,14 +11,16 @@
 // compile-flags: -Z parse-only -Z continue-parse-after-error
 
 trait A {
-    fn foo(*mut self); //~ ERROR cannot pass self by raw pointer
-    fn bar(*self); //~ ERROR cannot pass self by raw pointer
+    fn foo(*mut self); //~ ERROR cannot pass `self` by raw pointer
+    fn baz(*const self); //~ ERROR cannot pass `self` by raw pointer
+    fn bar(*self); //~ ERROR cannot pass `self` by raw pointer
 }
 
 struct X;
 impl A for X {
-    fn foo(*mut self) { } //~ ERROR cannot pass self by raw pointer
-    fn bar(*self) { } //~ ERROR cannot pass self by raw pointer
+    fn foo(*mut self) { } //~ ERROR cannot pass `self` by raw pointer
+    fn baz(*const self) { } //~ ERROR cannot pass `self` by raw pointer
+    fn bar(*self) { } //~ ERROR cannot pass `self` by raw pointer
 }
 
 fn main() { }