gofmt/go/parser: strengthen syntax checks

- don't allow parenthesized receiver base types or anonymous fields
- fixed a couple of other omissions
- adjusted gofmt test script
- removed several TODOs

R=rsc
CC=golang-dev
https://golang.org/cl/1897043

diff --git a/src/cmd/gofmt/test.sh b/src/cmd/gofmt/test.sh
index d2b7752c726889d3538d6cd2eb5affa98316f577..a8309421a7dbd84f360cba7f5273bab17d745400 100755 (executable)
--- a/src/cmd/gofmt/test.sh
+++ b/src/cmd/gofmt/test.sh
@@ -41,7 +41,7 @@ apply1() {
        bug106.go | bug121.go | bug125.go | bug133.go | bug160.go | \
        bug163.go | bug166.go | bug169.go | bug217.go | bug222.go | \
        bug226.go | bug228.go | bug248.go | bug274.go | bug280.go | \
-       bug282.go | bug287.go ) return ;;
+       bug282.go | bug287.go | bug298.go | bug299.go | bug300.go ) return ;;
        esac
        # the following directories are skipped because they contain test
        # cases for syntax errors and thus won't parse in the first place:

diff --git a/src/pkg/go/parser/parser.go b/src/pkg/go/parser/parser.go
index 56096013c1abd1a12049eb491737c65cc6e98ef1..a492e738f710bf71498d57102258e91e338c9466 100644 (file)
--- a/src/pkg/go/parser/parser.go
+++ b/src/pkg/go/parser/parser.go
@@ -508,19 +508,8 @@ func (p *parser) parseFieldDecl() *ast.Field {
 
        doc := p.leadComment
 
-       // a list of identifiers looks like a list of type names
-       var list vector.Vector
-       for {
-               // TODO(gri): do not allow ()'s here
-               list.Push(p.parseType())
-               if p.tok != token.COMMA {
-                       break
-               }
-               p.next()
-       }
-
-       // if we had a list of identifiers, it must be followed by a type
-       typ := p.tryType()
+       // fields
+       list, typ := p.parseVarList(false)
 
        // optional tag
        var tag *ast.BasicLit
@@ -533,15 +522,14 @@ func (p *parser) parseFieldDecl() *ast.Field {
        var idents []*ast.Ident
        if typ != nil {
                // IdentifierList Type
-               idents = p.makeIdentList(&list)
+               idents = p.makeIdentList(list)
        } else {
-               // Type (anonymous field)
-               if len(list) == 1 {
-                       // TODO(gri): check that this looks like a type
-                       typ = list.At(0).(ast.Expr)
-               } else {
-                       p.errorExpected(p.pos, "anonymous field")
-                       typ = &ast.BadExpr{p.pos}
+               // ["*"] TypeName (AnonymousField)
+               typ = (*list)[0].(ast.Expr) // we always have at least one element
+               if len(*list) > 1 || !isTypeName(deref(typ)) {
+                       pos := typ.Pos()
+                       p.errorExpected(pos, "anonymous field")
+                       typ = &ast.BadExpr{pos}
                }
        }
 
@@ -559,7 +547,10 @@ func (p *parser) parseStructType() *ast.StructType {
        pos := p.expect(token.STRUCT)
        lbrace := p.expect(token.LBRACE)
        var list vector.Vector
-       for p.tok == token.IDENT || p.tok == token.MUL {
+       for p.tok == token.IDENT || p.tok == token.MUL || p.tok == token.LPAREN {
+               // a field declaration cannot start with a '(' but we accept
+               // it here for more robust parsing and better error messages
+               // (parseFieldDecl will check and complain if necessary)
                list.Push(p.parseFieldDecl())
        }
        rbrace := p.expect(token.RBRACE)
@@ -589,8 +580,8 @@ func (p *parser) parsePointerType() *ast.StarExpr {
 }
 
 
-func (p *parser) tryParameterType(ellipsisOk bool) ast.Expr {
-       if ellipsisOk && p.tok == token.ELLIPSIS {
+func (p *parser) tryVarType(isParam bool) ast.Expr {
+       if isParam && p.tok == token.ELLIPSIS {
                pos := p.pos
                p.next()
                typ := p.tryType() // don't use parseType so we can provide better error message
@@ -607,8 +598,8 @@ func (p *parser) tryParameterType(ellipsisOk bool) ast.Expr {
 }
 
 
-func (p *parser) parseParameterType(ellipsisOk bool) ast.Expr {
-       typ := p.tryParameterType(ellipsisOk)
+func (p *parser) parseVarType(isParam bool) ast.Expr {
+       typ := p.tryVarType(isParam)
        if typ == nil {
                p.errorExpected(p.pos, "type")
                p.next() // make progress
@@ -618,16 +609,19 @@ func (p *parser) parseParameterType(ellipsisOk bool) ast.Expr {
 }
 
 
-func (p *parser) parseParameterDecl(ellipsisOk bool) (*vector.Vector, ast.Expr) {
+func (p *parser) parseVarList(isParam bool) (*vector.Vector, ast.Expr) {
        if p.trace {
-               defer un(trace(p, "ParameterDecl"))
+               defer un(trace(p, "VarList"))
        }
 
        // a list of identifiers looks like a list of type names
        var list vector.Vector
        for {
-               // TODO(gri): do not allow ()'s here
-               list.Push(p.parseParameterType(ellipsisOk))
+               // parseVarType accepts any type (including parenthesized ones)
+               // even though the syntax does not permit them here: we
+               // accept them all for more robust parsing and complain
+               // afterwards
+               list.Push(p.parseVarType(isParam))
                if p.tok != token.COMMA {
                        break
                }
@@ -635,7 +629,7 @@ func (p *parser) parseParameterDecl(ellipsisOk bool) (*vector.Vector, ast.Expr)
        }
 
        // if we had a list of identifiers, it must be followed by a type
-       typ := p.tryParameterType(ellipsisOk)
+       typ := p.tryVarType(isParam)
 
        return &list, typ
 }
@@ -646,7 +640,7 @@ func (p *parser) parseParameterList(ellipsisOk bool) []*ast.Field {
                defer un(trace(p, "ParameterList"))
        }
 
-       list, typ := p.parseParameterDecl(ellipsisOk)
+       list, typ := p.parseVarList(ellipsisOk)
        if typ != nil {
                // IdentifierList Type
                idents := p.makeIdentList(list)
@@ -658,7 +652,7 @@ func (p *parser) parseParameterList(ellipsisOk bool) []*ast.Field {
 
                for p.tok != token.RPAREN && p.tok != token.EOF {
                        idents := p.parseIdentList(ast.Var)
-                       typ := p.parseParameterType(ellipsisOk)
+                       typ := p.parseVarType(ellipsisOk)
                        list.Push(&ast.Field{nil, idents, typ, nil, nil})
                        if p.tok != token.COMMA {
                                break
@@ -1119,21 +1113,16 @@ func (p *parser) parseCompositeLit(typ ast.Expr) ast.Expr {
 }
 
 
-// TODO(gri): Consider different approach to checking syntax after parsing:
-//            Provide a arguments (set of flags) to parsing functions
-//            restricting what they are supposed to accept depending
-//            on context.
-
 // checkExpr checks that x is an expression (and not a type).
 func (p *parser) checkExpr(x ast.Expr) ast.Expr {
-       // TODO(gri): should provide predicate in AST nodes
-       switch t := x.(type) {
+       switch t := unparen(x).(type) {
        case *ast.BadExpr:
        case *ast.Ident:
        case *ast.BasicLit:
        case *ast.FuncLit:
        case *ast.CompositeLit:
        case *ast.ParenExpr:
+               panic("unreachable")
        case *ast.SelectorExpr:
        case *ast.IndexExpr:
        case *ast.SliceExpr:
@@ -1161,16 +1150,14 @@ func (p *parser) checkExpr(x ast.Expr) ast.Expr {
 }
 
 
-// isTypeName returns true iff x is type name.
+// isTypeName returns true iff x is a (qualified) TypeName.
 func isTypeName(x ast.Expr) bool {
-       // TODO(gri): should provide predicate in AST nodes
        switch t := x.(type) {
        case *ast.BadExpr:
        case *ast.Ident:
-       case *ast.ParenExpr:
-               return isTypeName(t.X) // TODO(gri): should (TypeName) be illegal?
        case *ast.SelectorExpr:
-               return isTypeName(t.X)
+               _, isIdent := t.X.(*ast.Ident)
+               return isIdent
        default:
                return false // all other nodes are not type names
        }
@@ -1178,16 +1165,14 @@ func isTypeName(x ast.Expr) bool {
 }
 
 
-// isCompositeLitType returns true iff x is a legal composite literal type.
-func isCompositeLitType(x ast.Expr) bool {
-       // TODO(gri): should provide predicate in AST nodes
+// isLiteralType returns true iff x is a legal composite literal type.
+func isLiteralType(x ast.Expr) bool {
        switch t := x.(type) {
        case *ast.BadExpr:
        case *ast.Ident:
-       case *ast.ParenExpr:
-               return isCompositeLitType(t.X)
        case *ast.SelectorExpr:
-               return isTypeName(t.X)
+               _, isIdent := t.X.(*ast.Ident)
+               return isIdent
        case *ast.ArrayType:
        case *ast.StructType:
        case *ast.MapType:
@@ -1198,12 +1183,31 @@ func isCompositeLitType(x ast.Expr) bool {
 }
 
 
+// If x is of the form *T, deref returns T, otherwise it returns x.
+func deref(x ast.Expr) ast.Expr {
+       if p, isPtr := x.(*ast.StarExpr); isPtr {
+               x = p.X
+       }
+       return x
+}
+
+
+// If x is of the form (T), unparen returns unparen(T), otherwise it returns x.
+func unparen(x ast.Expr) ast.Expr {
+       if p, isParen := x.(*ast.ParenExpr); isParen {
+               x = unparen(p.X)
+       }
+       return x
+}
+
+
 // checkExprOrType checks that x is an expression or a type
 // (and not a raw type such as [...]T).
 //
 func (p *parser) checkExprOrType(x ast.Expr) ast.Expr {
-       // TODO(gri): should provide predicate in AST nodes
-       switch t := x.(type) {
+       switch t := unparen(x).(type) {
+       case *ast.ParenExpr:
+               panic("unreachable")
        case *ast.UnaryExpr:
                if t.Op == token.RANGE {
                        // the range operator is only allowed at the top of a for statement
@@ -1238,7 +1242,7 @@ L:
                case token.LPAREN:
                        x = p.parseCallOrConversion(p.checkExprOrType(x))
                case token.LBRACE:
-                       if isCompositeLitType(x) && (p.exprLev >= 0 || !isTypeName(x)) {
+                       if isLiteralType(x) && (p.exprLev >= 0 || !isTypeName(x)) {
                                x = p.parseCompositeLit(x)
                        } else {
                                break L
@@ -1919,7 +1923,7 @@ func parseVarSpec(p *parser, doc *ast.CommentGroup) ast.Spec {
 
 func (p *parser) parseGenDecl(keyword token.Token, f parseSpecFunction) *ast.GenDecl {
        if p.trace {
-               defer un(trace(p, keyword.String()+"Decl"))
+               defer un(trace(p, "GenDecl("+keyword.String()+")"))
        }
 
        doc := p.leadComment
@@ -1960,17 +1964,15 @@ func (p *parser) parseReceiver(scope *ast.Scope) *ast.FieldList {
        if par.NumFields() != 1 {
                p.errorExpected(pos, "exactly one receiver")
                par.List = []*ast.Field{&ast.Field{Type: &ast.BadExpr{noPos}}}
+               return par
        }
 
+       // recv type must be of the form ["*"] identifier
        recv := par.List[0]
-
-       // recv type must be TypeName or *TypeName
-       base := recv.Type
-       if ptr, isPtr := base.(*ast.StarExpr); isPtr {
-               base = ptr.X
-       }
-       if !isTypeName(base) {
-               p.errorExpected(base.Pos(), "type name")
+       base := deref(recv.Type)
+       if _, isIdent := base.(*ast.Ident); !isIdent {
+               p.errorExpected(base.Pos(), "(unqualified) identifier")
+               par.List = []*ast.Field{&ast.Field{Type: &ast.BadExpr{recv.Pos()}}}
        }
 
        return par