From 9b80d3d3db126bda3eb976778cca4eb03a5a229b Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 25 Aug 2022 15:28:52 -0700 Subject: [PATCH] go/parser: remove validation of expression syntax, leave to type checker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the code that verifies that an expression is a type or non-type expression. For one, it cannot be done perfectly accurate (e.g., consider *p which could be an indirection or a pointer type), it also unnecessarily slows down parsing. It's simpler to leave the verification to the type checker which has all the information needed. Remove short compiler tests that tested the expression/type property. Adjust a couple of go/types tests which now trigger because the parser doesn't complain anymore. Change file for benchmark from "parser.go" to "../printer/nodes.go" to avoid a moving target when benchmarking. The parser may be marginally faster when tested on nodes.go: name old time/op new time/op delta ParseOnly-12 1.35ms ± 0% 1.31ms ± 0% ~ (p=0.100 n=3+3) name old speed new speed delta ParseOnly-12 39.9MB/s ± 0% 41.0MB/s ± 0% ~ (p=0.100 n=3+3) For #54511. Change-Id: I9a32c24c2c6e843c3d1af4587651c352f378b490 Reviewed-on: https://go-review.googlesource.com/c/go/+/425716 Reviewed-by: Robert Griesemer Run-TryBot: Robert Griesemer TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan Auto-Submit: Robert Griesemer --- src/go/parser/interface.go | 2 +- src/go/parser/parser.go | 134 ++++-------------- src/go/parser/performance_test.go | 4 +- src/go/parser/short_test.go | 19 --- src/go/types/testdata/check/stmt0.go | 2 +- src/go/types/testdata/fixedbugs/issue42987.go | 4 +- 6 files changed, 30 insertions(+), 135 deletions(-) diff --git a/src/go/parser/interface.go b/src/go/parser/interface.go index d911c8e1d0a..73cb16272e8 100644 --- a/src/go/parser/interface.go +++ b/src/go/parser/interface.go @@ -214,7 +214,7 @@ func ParseExprFrom(fset *token.FileSet, filename string, src any, mode Mode) (ex // parse expr p.init(fset, filename, text, mode) - expr = p.parseRhsOrType() + expr = p.parseRhs() // If a semicolon was inserted, consume it; // report an error if there's more tokens. diff --git a/src/go/parser/parser.go b/src/go/parser/parser.go index 3ac350d8f8c..3d4d83c4a41 100644 --- a/src/go/parser/parser.go +++ b/src/go/parser/parser.go @@ -470,10 +470,10 @@ func (p *parser) parseExprList() (list []ast.Expr) { defer un(trace(p, "ExpressionList")) } - list = append(list, p.checkExpr(p.parseExpr())) + list = append(list, p.parseExpr()) for p.tok == token.COMMA { p.next() - list = append(list, p.checkExpr(p.parseExpr())) + list = append(list, p.parseExpr()) } return @@ -581,10 +581,10 @@ func (p *parser) parseArrayFieldOrTypeInstance(x *ast.Ident) (*ast.Ident, ast.Ex var args []ast.Expr if p.tok != token.RBRACK { p.exprLev++ - args = append(args, p.parseRhsOrType()) + args = append(args, p.parseRhs()) for p.tok == token.COMMA { p.next() - args = append(args, p.parseRhsOrType()) + args = append(args, p.parseRhs()) } p.exprLev-- } @@ -1399,7 +1399,7 @@ func (p *parser) parseOperand() ast.Expr { lparen := p.pos p.next() p.exprLev++ - x := p.parseRhsOrType() // types may be parenthesized: (some type) + x := p.parseRhs() // types may be parenthesized: (some type) p.exprLev-- rparen := p.expect(token.RPAREN) return &ast.ParenExpr{Lparen: lparen, X: x, Rparen: rparen} @@ -1478,7 +1478,7 @@ func (p *parser) parseIndexOrSliceOrInstance(x ast.Expr) ast.Expr { if p.tok != token.COLON { // We can't know if we have an index expression or a type instantiation; // so even if we see a (named) type we are not going to be in type context. - index[0] = p.parseRhsOrType() + index[0] = p.parseRhs() } ncolons := 0 switch p.tok { @@ -1544,7 +1544,7 @@ func (p *parser) parseCallOrConversion(fun ast.Expr) *ast.CallExpr { var list []ast.Expr var ellipsis token.Pos for p.tok != token.RPAREN && p.tok != token.EOF && !ellipsis.IsValid() { - list = append(list, p.parseRhsOrType()) // builtins may expect a type: make(some type, ...) + list = append(list, p.parseRhs()) // builtins may expect a type: make(some type, ...) if p.tok == token.ELLIPSIS { ellipsis = p.pos p.next() @@ -1569,7 +1569,7 @@ func (p *parser) parseValue() ast.Expr { return p.parseLiteralValue(nil) } - x := p.checkExpr(p.parseExpr()) + x := p.parseExpr() return x } @@ -1621,38 +1621,6 @@ func (p *parser) parseLiteralValue(typ ast.Expr) ast.Expr { return &ast.CompositeLit{Type: typ, Lbrace: lbrace, Elts: elts, Rbrace: rbrace} } -// checkExpr checks that x is an expression (and not a type). -func (p *parser) checkExpr(x ast.Expr) ast.Expr { - switch 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.IndexListExpr: - case *ast.SliceExpr: - case *ast.TypeAssertExpr: - // If t.Type == nil we have a type assertion of the form - // y.(type), which is only allowed in type switch expressions. - // It's hard to exclude those but for the case where we are in - // a type switch. Instead be lenient and test this in the type - // checker. - case *ast.CallExpr: - case *ast.StarExpr: - case *ast.UnaryExpr: - case *ast.BinaryExpr: - default: - // all other nodes are not proper expressions - p.errorExpected(x.Pos(), "expression") - x = &ast.BadExpr{From: x.Pos(), To: p.safePos(x.End())} - } - 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 { @@ -1661,23 +1629,6 @@ func unparen(x ast.Expr) ast.Expr { 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 { - switch t := unparen(x).(type) { - case *ast.ParenExpr: - panic("unreachable") - case *ast.ArrayType: - if len, isEllipsis := t.Len.(*ast.Ellipsis); isEllipsis { - p.error(len.Pos(), "expected array length, found '...'") - x = &ast.BadExpr{From: x.Pos(), To: p.safePos(x.End())} - } - } - - // all other nodes are expressions or types - return x -} - func (p *parser) parsePrimaryExpr(x ast.Expr) ast.Expr { if p.trace { defer un(trace(p, "PrimaryExpr")) @@ -1698,9 +1649,9 @@ func (p *parser) parsePrimaryExpr(x ast.Expr) ast.Expr { p.next() switch p.tok { case token.IDENT: - x = p.parseSelector(p.checkExprOrType(x)) + x = p.parseSelector(x) case token.LPAREN: - x = p.parseTypeAssertion(p.checkExpr(x)) + x = p.parseTypeAssertion(x) default: pos := p.pos p.errorExpected(pos, "selector or type assertion") @@ -1716,9 +1667,9 @@ func (p *parser) parsePrimaryExpr(x ast.Expr) ast.Expr { x = &ast.SelectorExpr{X: x, Sel: sel} } case token.LBRACK: - x = p.parseIndexOrSliceOrInstance(p.checkExpr(x)) + x = p.parseIndexOrSliceOrInstance(x) case token.LPAREN: - x = p.parseCallOrConversion(p.checkExprOrType(x)) + x = p.parseCallOrConversion(x) case token.LBRACE: // operand may have returned a parenthesized complit // type; accept it but complain if we have a complit @@ -1763,7 +1714,7 @@ func (p *parser) parseUnaryExpr() ast.Expr { pos, op := p.pos, p.tok p.next() x := p.parseUnaryExpr() - return &ast.UnaryExpr{OpPos: pos, Op: op, X: p.checkExpr(x)} + return &ast.UnaryExpr{OpPos: pos, Op: op, X: x} case token.ARROW: // channel type or receive expression @@ -1809,14 +1760,14 @@ func (p *parser) parseUnaryExpr() ast.Expr { } // <-(expr) - return &ast.UnaryExpr{OpPos: arrow, Op: token.ARROW, X: p.checkExpr(x)} + return &ast.UnaryExpr{OpPos: arrow, Op: token.ARROW, X: x} case token.MUL: // pointer type or unary "*" expression pos := p.pos p.next() x := p.parseUnaryExpr() - return &ast.StarExpr{Star: pos, X: p.checkExprOrType(x)} + return &ast.StarExpr{Star: pos, X: x} } return p.parsePrimaryExpr(nil) @@ -1832,10 +1783,9 @@ func (p *parser) tokPrec() (token.Token, int) { // parseBinaryExpr parses a (possibly) binary expression. // If x is non-nil, it is used as the left operand. -// If check is true, operands are checked to be valid expressions. // // TODO(rfindley): parseBinaryExpr has become overloaded. Consider refactoring. -func (p *parser) parseBinaryExpr(x ast.Expr, prec1 int, check bool) ast.Expr { +func (p *parser) parseBinaryExpr(x ast.Expr, prec1 int) ast.Expr { if p.trace { defer un(trace(p, "BinaryExpr")) } @@ -1855,38 +1805,24 @@ func (p *parser) parseBinaryExpr(x ast.Expr, prec1 int, check bool) ast.Expr { return x } pos := p.expect(op) - y := p.parseBinaryExpr(nil, oprec+1, check) - if check { - x = p.checkExpr(x) - y = p.checkExpr(y) - } + y := p.parseBinaryExpr(nil, oprec+1) x = &ast.BinaryExpr{X: x, OpPos: pos, Op: op, Y: y} } } -// The result may be a type or even a raw type ([...]int). Callers must -// check the result (using checkExpr or checkExprOrType), depending on -// context. +// The result may be a type or even a raw type ([...]int). func (p *parser) parseExpr() ast.Expr { if p.trace { defer un(trace(p, "Expression")) } - return p.parseBinaryExpr(nil, token.LowestPrec+1, true) + return p.parseBinaryExpr(nil, token.LowestPrec+1) } func (p *parser) parseRhs() ast.Expr { old := p.inRhs p.inRhs = true - x := p.checkExpr(p.parseExpr()) - p.inRhs = old - return x -} - -func (p *parser) parseRhsOrType() ast.Expr { - old := p.inRhs - p.inRhs = true - x := p.checkExprOrType(p.parseExpr()) + x := p.parseExpr() p.inRhs = old return x } @@ -1991,7 +1927,7 @@ func (p *parser) checkAssignStmt(as *ast.AssignStmt) { } func (p *parser) parseCallExpr(callType string) *ast.CallExpr { - x := p.parseRhsOrType() // could be a conversion: (some type)(x) + x := p.parseRhs() // could be a conversion: (some type)(x) if call, isCall := x.(*ast.CallExpr); isCall { return call } @@ -2068,7 +2004,7 @@ func (p *parser) makeExpr(s ast.Stmt, want string) ast.Expr { return nil } if es, isExpr := s.(*ast.ExprStmt); isExpr { - return p.checkExpr(es.X) + return es.X } found := "simple statement" if _, isAss := s.(*ast.AssignStmt); isAss { @@ -2173,21 +2109,7 @@ func (p *parser) parseIfStmt() *ast.IfStmt { return &ast.IfStmt{If: pos, Init: init, Cond: cond, Body: body, Else: else_} } -func (p *parser) parseTypeList() (list []ast.Expr) { - if p.trace { - defer un(trace(p, "TypeList")) - } - - list = append(list, p.parseType()) - for p.tok == token.COMMA { - p.next() - list = append(list, p.parseType()) - } - - return -} - -func (p *parser) parseCaseClause(typeSwitch bool) *ast.CaseClause { +func (p *parser) parseCaseClause() *ast.CaseClause { if p.trace { defer un(trace(p, "CaseClause")) } @@ -2196,11 +2118,7 @@ func (p *parser) parseCaseClause(typeSwitch bool) *ast.CaseClause { var list []ast.Expr if p.tok == token.CASE { p.next() - if typeSwitch { - list = p.parseTypeList() - } else { - list = p.parseList(true) - } + list = p.parseList(true) } else { p.expect(token.DEFAULT) } @@ -2278,7 +2196,7 @@ func (p *parser) parseSwitchStmt() ast.Stmt { lbrace := p.expect(token.LBRACE) var list []ast.Stmt for p.tok == token.CASE || p.tok == token.DEFAULT { - list = append(list, p.parseCaseClause(typeSwitch)) + list = append(list, p.parseCaseClause()) } rbrace := p.expect(token.RBRACE) p.expectSemi() @@ -2643,7 +2561,7 @@ func (p *parser) parseTypeSpec(doc *ast.CommentGroup, _ token.Pos, _ token.Token // to parser.expr, and pass in name to parsePrimaryExpr. p.exprLev++ lhs := p.parsePrimaryExpr(x) - x = p.parseBinaryExpr(lhs, token.LowestPrec+1, false) + x = p.parseBinaryExpr(lhs, token.LowestPrec+1) p.exprLev-- } // Analyze expression x. If we can split x into a type parameter diff --git a/src/go/parser/performance_test.go b/src/go/parser/performance_test.go index 1249f35d39d..1308f212dc0 100644 --- a/src/go/parser/performance_test.go +++ b/src/go/parser/performance_test.go @@ -10,9 +10,7 @@ import ( "testing" ) -// TODO(rfindley): use a testdata file or file from another package here, to -// avoid a moving target. -var src = readFile("parser.go") +var src = readFile("../printer/nodes.go") func readFile(filename string) []byte { data, err := os.ReadFile(filename) diff --git a/src/go/parser/short_test.go b/src/go/parser/short_test.go index 86779e7e7e9..2d9016aaddc 100644 --- a/src/go/parser/short_test.go +++ b/src/go/parser/short_test.go @@ -141,18 +141,6 @@ var invalids = []string{ `package p; func f() { switch t = /* ERROR "expected ':=', found '='" */ t.(type) {} };`, `package p; func f() { switch t /* ERROR "expected switch expression" */ , t = t.(type) {} };`, `package p; func f() { switch t /* ERROR "expected switch expression" */ = t.(type), t {} };`, - `package p; var a = [ /* ERROR "expected expression" */ 1]int;`, - `package p; var a = [ /* ERROR "expected expression" */ ...]int;`, - `package p; var a = struct /* ERROR "expected expression" */ {}`, - `package p; var a = func /* ERROR "expected expression" */ ();`, - `package p; var a = interface /* ERROR "expected expression" */ {}`, - `package p; var a = [ /* ERROR "expected expression" */ ]int`, - `package p; var a = map /* ERROR "expected expression" */ [int]int`, - `package p; var a = chan /* ERROR "expected expression" */ int;`, - `package p; var a = []int{[ /* ERROR "expected expression" */ ]int};`, - `package p; var a = ( /* ERROR "expected expression" */ []int);`, - `package p; var a = <- /* ERROR "expected expression" */ chan int;`, - `package p; func f() { select { case _ <- chan /* ERROR "expected expression" */ int: } };`, `package p; func f() { _ = (<-<- /* ERROR "expected 'chan'" */ chan int)(nil) };`, `package p; func f() { _ = (<-chan<-chan<-chan<-chan<-chan<- /* ERROR "expected channel type" */ int)(nil) };`, `package p; func f() { var t []int; t /* ERROR "expected identifier on left side of :=" */ [0] := 0 };`, @@ -183,13 +171,6 @@ var invalids = []string{ `package p; type _ struct{ *( /* ERROR "cannot parenthesize embedded type" */ int) }`, `package p; type _ struct{ *( /* ERROR "cannot parenthesize embedded type" */ []byte) }`, - // TODO(rfindley): this error should be positioned on the ':' - `package p; var a = a[[]int:[ /* ERROR "expected expression" */ ]int];`, - - // TODO(rfindley): the compiler error is better here: "cannot parenthesize embedded type" - // TODO(rfindley): confirm that parenthesized types should now be accepted. - // `package p; type I1 interface{}; type I2 interface{ (/* ERROR "expected '}', found '\('" */ I1) }`, - // issue 8656 `package p; func f() (a b string /* ERROR "missing ','" */ , ok bool)`, diff --git a/src/go/types/testdata/check/stmt0.go b/src/go/types/testdata/check/stmt0.go index d8790b9616d..0caebcf544f 100644 --- a/src/go/types/testdata/check/stmt0.go +++ b/src/go/types/testdata/check/stmt0.go @@ -728,7 +728,7 @@ func typeswitches() { case int: println(x) println(x / 0 /* ERROR "invalid operation: division by zero" */) - case 1 /* ERROR "expected type, found 1" */: + case 1 /* ERROR "1 is not a type" */: } } } diff --git a/src/go/types/testdata/fixedbugs/issue42987.go b/src/go/types/testdata/fixedbugs/issue42987.go index 6060ec84bde..f58c63f8a36 100644 --- a/src/go/types/testdata/fixedbugs/issue42987.go +++ b/src/go/types/testdata/fixedbugs/issue42987.go @@ -5,6 +5,4 @@ // Check that there is only one error (no follow-on errors). package p -// TODO(rFindley) This is a parser error, but in types2 it is a type checking -// error. We could probably do without this check in the parser. -var _ = [... /* ERROR expected array length, found '...' */ ]byte("foo") +var _ = [ ... /* ERROR invalid use of \[...\] array */ ]byte("foo") \ No newline at end of file