]> Cypherpunks repositories - gostls13.git/commitdiff
go/parser: better error synchronization
authorRobert Griesemer <gri@golang.org>
Wed, 7 Mar 2012 20:24:20 +0000 (12:24 -0800)
committerRobert Griesemer <gri@golang.org>
Wed, 7 Mar 2012 20:24:20 +0000 (12:24 -0800)
gofmt reports now a single, accurate error for
the test case of issue 3106.

Also: Added test harness for general error
checking and two test cases for now.

Fixes #3106.

R=rsc, bradfitz
CC=golang-dev
https://golang.org/cl/5755062

src/pkg/go/parser/error_test.go [new file with mode: 0644]
src/pkg/go/parser/parser.go
src/pkg/go/parser/testdata/commas.src [new file with mode: 0644]
src/pkg/go/parser/testdata/issue3106.src [new file with mode: 0644]

diff --git a/src/pkg/go/parser/error_test.go b/src/pkg/go/parser/error_test.go
new file mode 100644 (file)
index 0000000..0bfa38a
--- /dev/null
@@ -0,0 +1,163 @@
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file implements a parser test harness. The files in the testdata
+// directory are parsed and the errors reported are compared against the
+// error messages expected in the test files. The test files must end in
+// .src rather than .go so that they are not disturbed by gofmt runs.
+//
+// Expected errors are indicated in the test files by putting a comment
+// of the form /* ERROR "rx" */ immediately following an offending token.
+// The harness will verify that an error matching the regular expression
+// rx is reported at that source position.
+//
+// For instance, the following test file indicates that a "not declared"
+// error should be reported for the undeclared variable x:
+//
+//     package p
+//     func f() {
+//             _ = x /* ERROR "not declared" */ + 1
+//     }
+
+package parser
+
+import (
+       "go/scanner"
+       "go/token"
+       "io/ioutil"
+       "path/filepath"
+       "regexp"
+       "strings"
+       "testing"
+)
+
+const testdata = "testdata"
+
+func getFile(filename string) (file *token.File) {
+       fset.Iterate(func(f *token.File) bool {
+               if f.Name() == filename {
+                       file = f
+                       return false // end iteration
+               }
+               return true
+       })
+       return file
+}
+
+func getPos(filename string, offset int) token.Pos {
+       if f := getFile(filename); f != nil {
+               return f.Pos(offset)
+       }
+       return token.NoPos
+}
+
+// ERROR comments must be of the form /* ERROR "rx" */ and rx is
+// a regular expression that matches the expected error message.
+//
+var errRx = regexp.MustCompile(`^/\* *ERROR *"([^"]*)" *\*/$`)
+
+// expectedErrors collects the regular expressions of ERROR comments found
+// in files and returns them as a map of error positions to error messages.
+//
+func expectedErrors(t *testing.T, filename string, src []byte) map[token.Pos]string {
+       errors := make(map[token.Pos]string)
+
+       var s scanner.Scanner
+       // file was parsed already - do not add it again to the file
+       // set otherwise the position information returned here will
+       // not match the position information collected by the parser
+       s.Init(getFile(filename), src, nil, scanner.ScanComments)
+       var prev token.Pos // position of last non-comment, non-semicolon token
+
+       for {
+               pos, tok, lit := s.Scan()
+               switch tok {
+               case token.EOF:
+                       return errors
+               case token.COMMENT:
+                       s := errRx.FindStringSubmatch(lit)
+                       if len(s) == 2 {
+                               errors[prev] = string(s[1])
+                       }
+               default:
+                       prev = pos
+               }
+       }
+
+       panic("unreachable")
+}
+
+// compareErrors compares the map of expected error messages with the list
+// of found errors and reports discrepancies.
+//
+func compareErrors(t *testing.T, expected map[token.Pos]string, found scanner.ErrorList) {
+       for _, error := range found {
+               // error.Pos is a token.Position, but we want
+               // a token.Pos so we can do a map lookup
+               pos := getPos(error.Pos.Filename, error.Pos.Offset)
+               if msg, found := expected[pos]; found {
+                       // we expect a message at pos; check if it matches
+                       rx, err := regexp.Compile(msg)
+                       if err != nil {
+                               t.Errorf("%s: %v", error.Pos, err)
+                               continue
+                       }
+                       if match := rx.MatchString(error.Msg); !match {
+                               t.Errorf("%s: %q does not match %q", error.Pos, error.Msg, msg)
+                               continue
+                       }
+                       // we have a match - eliminate this error
+                       delete(expected, pos)
+               } else {
+                       // To keep in mind when analyzing failed test output:
+                       // If the same error position occurs multiple times in errors,
+                       // this message will be triggered (because the first error at
+                       // the position removes this position from the expected errors).
+                       t.Errorf("%s: unexpected error: %s", error.Pos, error.Msg)
+               }
+       }
+
+       // there should be no expected errors left
+       if len(expected) > 0 {
+               t.Errorf("%d errors not reported:", len(expected))
+               for pos, msg := range expected {
+                       t.Errorf("%s: %s\n", fset.Position(pos), msg)
+               }
+       }
+}
+
+func checkErrors(t *testing.T, filename string) {
+       src, err := ioutil.ReadFile(filename)
+       if err != nil {
+               t.Error(err)
+               return
+       }
+
+       _, err = ParseFile(fset, filename, src, DeclarationErrors)
+       found, ok := err.(scanner.ErrorList)
+       if err != nil && !ok {
+               t.Error(err)
+               return
+       }
+
+       // we are expecting the following errors
+       // (collect these after parsing a file so that it is found in the file set)
+       expected := expectedErrors(t, filename, src)
+
+       // verify errors returned by the parser
+       compareErrors(t, expected, found)
+}
+
+func TestErrors(t *testing.T) {
+       list, err := ioutil.ReadDir(testdata)
+       if err != nil {
+               t.Fatal(err)
+       }
+       for _, fi := range list {
+               name := fi.Name()
+               if !fi.IsDir() && !strings.HasPrefix(name, ".") && strings.HasSuffix(name, ".src") {
+                       checkErrors(t, filepath.Join(testdata, name))
+               }
+       }
+}
index c39ea5c2a369c357f5bd090a4e307e644bbe457f..4fb9ae398e0aec1248890c17484b957ada55a2dc 100644 (file)
@@ -371,8 +371,16 @@ func (p *parser) expectClosing(tok token.Token, context string) token.Pos {
 }
 
 func (p *parser) expectSemi() {
+       // semicolon is optional before a closing ')' or '}'
        if p.tok != token.RPAREN && p.tok != token.RBRACE {
-               p.expect(token.SEMICOLON)
+               if p.tok == token.SEMICOLON {
+                       p.next()
+               } else {
+                       p.errorExpected(p.pos, "';'")
+                       for !isStmtSync(p.tok) {
+                               p.next() // make progress
+                       }
+               }
        }
 }
 
@@ -394,6 +402,31 @@ func assert(cond bool, msg string) {
        }
 }
 
+// isStmtSync reports whether tok starts a new statement.
+// Used for synchronization after an error.
+//
+func isStmtSync(tok token.Token) bool {
+       switch tok {
+       case token.BREAK, token.CONST, token.CONTINUE, token.DEFER,
+               token.FALLTHROUGH, token.FOR, token.GO, token.GOTO,
+               token.IF, token.RETURN, token.SELECT, token.SWITCH,
+               token.TYPE, token.VAR, token.EOF:
+               return true
+       }
+       return false
+}
+
+// isDeclSync reports whether tok starts a new declaration.
+// Used for synchronization after an error.
+//
+func isDeclSync(tok token.Token) bool {
+       switch tok {
+       case token.CONST, token.TYPE, token.VAR, token.EOF:
+               return true
+       }
+       return false
+}
+
 // ----------------------------------------------------------------------------
 // Identifiers
 
@@ -534,9 +567,11 @@ func (p *parser) makeIdentList(list []ast.Expr) []*ast.Ident {
        for i, x := range list {
                ident, isIdent := x.(*ast.Ident)
                if !isIdent {
-                       pos := x.Pos()
-                       p.errorExpected(pos, "identifier")
-                       ident = &ast.Ident{NamePos: pos, Name: "_"}
+                       if _, isBad := x.(*ast.BadExpr); !isBad {
+                               // only report error if it's a new one
+                               p.errorExpected(x.Pos(), "identifier")
+                       }
+                       ident = &ast.Ident{NamePos: x.Pos(), Name: "_"}
                }
                idents[i] = ident
        }
@@ -1003,19 +1038,21 @@ func (p *parser) parseOperand(lhs bool) ast.Expr {
 
        case token.FUNC:
                return p.parseFuncTypeOrLit()
+       }
 
-       default:
-               if typ := p.tryIdentOrType(true); typ != nil {
-                       // could be type for composite literal or conversion
-                       _, isIdent := typ.(*ast.Ident)
-                       assert(!isIdent, "type cannot be identifier")
-                       return typ
-               }
+       if typ := p.tryIdentOrType(true); typ != nil {
+               // could be type for composite literal or conversion
+               _, isIdent := typ.(*ast.Ident)
+               assert(!isIdent, "type cannot be identifier")
+               return typ
        }
 
+       // we have an error
        pos := p.pos
        p.errorExpected(pos, "operand")
-       p.next() // make progress
+       if !isStmtSync(p.tok) {
+               p.next() // make progress
+       }
        return &ast.BadExpr{From: pos, To: p.pos}
 }
 
@@ -1274,8 +1311,8 @@ L:
                                x = p.parseTypeAssertion(p.checkExpr(x))
                        default:
                                pos := p.pos
-                               p.next() // make progress
                                p.errorExpected(pos, "selector or type assertion")
+                               p.next() // make progress
                                x = &ast.BadExpr{From: pos, To: p.pos}
                        }
                case token.LBRACK:
@@ -1483,7 +1520,10 @@ func (p *parser) parseCallExpr() *ast.CallExpr {
        if call, isCall := x.(*ast.CallExpr); isCall {
                return call
        }
-       p.errorExpected(x.Pos(), "function/method call")
+       if _, isBad := x.(*ast.BadExpr); !isBad {
+               // only report error if it's a new one
+               p.errorExpected(x.Pos(), "function/method call")
+       }
        return nil
 }
 
@@ -1874,7 +1914,7 @@ func (p *parser) parseStmt() (s ast.Stmt) {
 
        switch p.tok {
        case token.CONST, token.TYPE, token.VAR:
-               s = &ast.DeclStmt{Decl: p.parseDecl()}
+               s = &ast.DeclStmt{Decl: p.parseDecl(isStmtSync)}
        case
                // tokens that may start an expression
                token.IDENT, token.INT, token.FLOAT, token.IMAG, token.CHAR, token.STRING, token.FUNC, token.LPAREN, // operands
@@ -1916,7 +1956,9 @@ func (p *parser) parseStmt() (s ast.Stmt) {
                // no statement found
                pos := p.pos
                p.errorExpected(pos, "statement")
-               p.next() // make progress
+               for !isStmtSync(p.tok) {
+                       p.next() // make progress
+               }
                s = &ast.BadStmt{From: pos, To: p.pos}
        }
 
@@ -2107,8 +2149,13 @@ func (p *parser) parseReceiver(scope *ast.Scope) *ast.FieldList {
        recv := par.List[0]
        base := deref(recv.Type)
        if _, isIdent := base.(*ast.Ident); !isIdent {
-               p.errorExpected(base.Pos(), "(unqualified) identifier")
-               par.List = []*ast.Field{{Type: &ast.BadExpr{From: recv.Pos(), To: recv.End()}}}
+               if _, isBad := base.(*ast.BadExpr); !isBad {
+                       // only report error if it's a new one
+                       p.errorExpected(base.Pos(), "(unqualified) identifier")
+               }
+               par.List = []*ast.Field{
+                       {Type: &ast.BadExpr{From: recv.Pos(), To: recv.End()}},
+               }
        }
 
        return par
@@ -2164,7 +2211,7 @@ func (p *parser) parseFuncDecl() *ast.FuncDecl {
        return decl
 }
 
-func (p *parser) parseDecl() ast.Decl {
+func (p *parser) parseDecl(isSync func(token.Token) bool) ast.Decl {
        if p.trace {
                defer un(trace(p, "Declaration"))
        }
@@ -2186,9 +2233,10 @@ func (p *parser) parseDecl() ast.Decl {
        default:
                pos := p.pos
                p.errorExpected(pos, "declaration")
-               p.next() // make progress
-               decl := &ast.BadDecl{From: pos, To: p.pos}
-               return decl
+               for !isSync(p.tok) {
+                       p.next() // make progress
+               }
+               return &ast.BadDecl{From: pos, To: p.pos}
        }
 
        return p.parseGenDecl(p.tok, f)
@@ -2227,7 +2275,7 @@ func (p *parser) parseFile() *ast.File {
                if p.mode&ImportsOnly == 0 {
                        // rest of package body
                        for p.tok != token.EOF {
-                               decls = append(decls, p.parseDecl())
+                               decls = append(decls, p.parseDecl(isDeclSync))
                        }
                }
        }
diff --git a/src/pkg/go/parser/testdata/commas.src b/src/pkg/go/parser/testdata/commas.src
new file mode 100644 (file)
index 0000000..af6e706
--- /dev/null
@@ -0,0 +1,19 @@
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test case for error messages/parser synchronization
+// after missing commas.
+
+package p
+
+var _ = []int{
+       0 /* ERROR "missing ','" */
+}
+
+var _ = []int{
+       0,
+       1,
+       2,
+       3 /* ERROR "missing ','" */
+}
diff --git a/src/pkg/go/parser/testdata/issue3106.src b/src/pkg/go/parser/testdata/issue3106.src
new file mode 100644 (file)
index 0000000..82796c8
--- /dev/null
@@ -0,0 +1,46 @@
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test case for issue 3106: Better synchronization of
+// parser after certain syntax errors.
+
+package main
+
+func f() {
+       var m Mutex
+       c := MakeCond(&m)
+       percent := 0
+       const step = 10
+       for i := 0; i < 5; i++ {
+               go func() {
+                       for {
+                               // Emulates some useful work.
+                               time.Sleep(1e8)
+                               m.Lock()
+                               defer
+                               if /* ERROR "expected operand, found 'if'" */ percent == 100 {
+                                       m.Unlock()
+                                       break
+                               }
+                               percent++
+                               if percent % step == 0 {
+                                       //c.Signal()
+                               }
+                               m.Unlock()
+                       }
+               }()
+       }
+       for {
+               m.Lock()
+               if percent == 0 || percent % step != 0 {
+                       c.Wait()
+               }
+               fmt.Print(",")
+               if percent == 100 {
+                       m.Unlock()
+                       break
+               }
+               m.Unlock()
+       }
+}