code review 5755062: go/parser: better error synchronization

go/parser: better error synchronization

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 issue 3106.

[gri, 2012-03-07 01:42:04]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-03-07 01:50:31]

What is the "single, accurate error" for 3106? I would expect it to be in
this CL somewhere?

On Tue, Mar 6, 2012 at 5:42 PM, <gri@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> go/parser: better error synchronization
>
> gofmt reports now a single, accurate error for
> the test case of issue 3106.
>
> Fixes issue 3106.
>
> Please review this at
http://codereview.appspot.com/**5755062/<http://codereview.appspot.com/5755062/>
>
> Affected files:
>  M src/pkg/go/parser/parser.go
>
>
> Index: src/pkg/go/parser/parser.go
> ==============================**==============================**=======
> --- a/src/pkg/go/parser/parser.go
> +++ b/src/pkg/go/parser/parser.go
> @@ -371,8 +371,16 @@
>  }
>
>  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
> +                       }
> +               }
>        }
>  }
>
> @@ -396,6 +404,31 @@
>        }
>  }
>
> +// 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
>
> @@ -536,9 +569,11 @@
>        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
>        }
> @@ -1005,19 +1040,21 @@
>
>        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}
>  }
>
> @@ -1276,8 +1313,8 @@
>                                x = p.parseTypeAssertion(p.**checkExpr(x))
>                        default:
>                                pos := p.pos
> +                               p.errorExpected(pos, "selector or type
> assertion")
>                                p.next() // make progress
> -                               p.errorExpected(pos, "selector or type
> assertion")
>                                x = &ast.BadExpr{From: pos, To: p.pos}
>                        }
>                case token.LBRACK:
> @@ -1485,7 +1522,10 @@
>        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
>  }
>
> @@ -1876,7 +1916,7 @@
>
>        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
> @@ -1918,7 +1958,9 @@
>                // 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}
>        }
>
> @@ -2109,8 +2151,13 @@
>        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
> @@ -2166,7 +2213,7 @@
>        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"))
>        }
> @@ -2188,9 +2235,10 @@
>        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)
> @@ -2229,7 +2277,7 @@
>                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))
>                        }
>                }
>        }
>
>
>

[rsc, 2012-03-07 03:04:20]

I agree.  It would be nice to have an error test case
for this and for the comma issue.

[gri, 2012-03-07 19:56:46]

Hello rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-03-07 20:04:52]

LGTM

on tests especially. not very familiar with parser code, but seems sane.

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test.go
File src/pkg/go/parser/error_test.go (right):

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test...
src/pkg/go/parser/error_test.go:58: var errRx = regexp.MustCompile(`^/\* *ERROR
*"([^"]*)" *\*/$`)
the * in " *" probably isn't necessary. you document the format as a single
space.

[rsc, 2012-03-07 20:06:40]

LGTM

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test.go
File src/pkg/go/parser/error_test.go (right):

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test...
src/pkg/go/parser/error_test.go:91: func eliminate(t *testing.T, expected
map[token.Pos]string, found scanner.ErrorList) {
doc comment here would help
make clear that eliminate edits expected,
complains about unexpected errors in found,
but leaves the complaining about missing errors
to the caller.

[gri, 2012-03-07 20:14:15]

PTAL

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test.go
File src/pkg/go/parser/error_test.go (right):

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test...
src/pkg/go/parser/error_test.go:58: var errRx = regexp.MustCompile(`^/\* *ERROR
*"([^"]*)" *\*/$`)
On 2012/03/07 20:04:52, bradfitz wrote:
> the * in " *" probably isn't necessary. you document the format as a single
> space.

left alone - no harm done

http://codereview.appspot.com/5755062/diff/10001/src/pkg/go/parser/error_test...
src/pkg/go/parser/error_test.go:91: func eliminate(t *testing.T, expected
map[token.Pos]string, found scanner.ErrorList) {
On 2012/03/07 20:06:41, rsc wrote:
> doc comment here would help
> make clear that eliminate edits expected,
> complains about unexpected errors in found,
> but leaves the complaining about missing errors
> to the caller.

Added comment and reorganized so that all complaining happens in this function.

[rsc, 2012-03-07 20:15:34]

LGTM

http://codereview.appspot.com/5755062/diff/11006/src/pkg/go/parser/error_test.go
File src/pkg/go/parser/error_test.go (right):

http://codereview.appspot.com/5755062/diff/11006/src/pkg/go/parser/error_test...
src/pkg/go/parser/error_test.go:117: t.Errorf("%s: no (multiple?) error
expected, but found: %s", error.Pos, error.Msg)
s/no (multiple?) error expected, but found/unexpected error/

[gri, 2012-03-07 20:24:23]

*** Submitted as http://code.google.com/p/go/source/detail?r=097e02a1815a ***

go/parser: better error synchronization

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 issue 3106.

R=rsc, bradfitz
CC=golang-dev
http://codereview.appspot.com/5755062