code review 4803047: go/parser: report illegal label declarations at ':' rat... (issue4803047)

[g...@golang.org]

Reviewers: rsc,

Message:
Hello r...@golang.org (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
go/parser: report illegal label declarations at ':' rather than guessing
the start

Also:
- Add parser.SpuriousError flag. If set, the parser reports all
(including
spurious) errors rather then at most one error per line.
- Add -e flag to gofmt and gotype: If set, gofmt and gotype report all
(including spurious) errors rather than at most one error per line.
- Updated the respective documentation.

Fixes issue 2088.

Please review this at http://codereview.appspot.com/4803047/

Affected files:
M src/cmd/gofmt/doc.go
M src/cmd/gofmt/gofmt.go
M src/cmd/gotype/doc.go
M src/cmd/gotype/gotype.go
M src/pkg/go/parser/interface.go
M src/pkg/go/parser/parser.go


Index: src/cmd/gofmt/doc.go
===================================================================
--- a/src/cmd/gofmt/doc.go
+++ b/src/cmd/gofmt/doc.go
@@ -14,11 +14,12 @@
gofmt [flags] [path ...]

The flags are:
-
-d
Do not print reformatted sources to standard output.
If a file's formatting is different than gofmt's, print diffs
to standard output.
+ -e
+ Print all (including spurious) errors.
-l
Do not print reformatted sources to standard output.
If a file's formatting is different from gofmt's, print its name
@@ -31,6 +32,8 @@
Do not print reformatted sources to standard output.
If a file's formatting is different from gofmt's, overwrite it
with gofmt's version.
+
+Formatting control flags:
-comments=true
Print comments; if false, all comments are elided from the output.
-spaces
@@ -40,6 +43,7 @@
-tabwidth=8
Tab width in spaces.

+
The rewrite rule specified with the -r flag must be a string of the form:

pattern -> replacement
Index: src/cmd/gofmt/gofmt.go
===================================================================
--- a/src/cmd/gofmt/gofmt.go
+++ b/src/cmd/gofmt/gofmt.go
@@ -29,6 +29,7 @@
rewriteRule = flag.String("r", "", "rewrite rule (e.g., 'α[β:len(α)] ->
α[β:]')")
simplifyAST = flag.Bool("s", false, "simplify code")
doDiff = flag.Bool("d", false, "display diffs instead of rewriting
files")
+ allErrors = flag.Bool("e", false, "print all (including spurious)
errors")

// layout control
comments = flag.Bool("comments", true, "print comments")
@@ -64,6 +65,9 @@
if *comments {
parserMode |= parser.ParseComments
}
+ if *allErrors {
+ parserMode |= parser.SpuriousErrors
+ }
}

func initPrinterMode() {
Index: src/cmd/gotype/doc.go
===================================================================
--- a/src/cmd/gotype/doc.go
+++ b/src/cmd/gotype/doc.go
@@ -24,18 +24,20 @@
gotype [flags] [path ...]

The flags are:
+ -e
+ Print all (including spurious) errors.
-p pkgName
- process only those files in package pkgName.
+ Process only those files in package pkgName.
-r
- recursively process subdirectories.
+ Recursively process subdirectories.
-v
- verbose mode.
+ Verbose mode.

Debugging flags:
+ -ast
+ Print AST (disables concurrent parsing).
-trace
- print parse trace (disables concurrent parsing).
- -ast
- print AST (disables concurrent parsing).
+ Print parse trace (disables concurrent parsing).


Examples
Index: src/cmd/gotype/gotype.go
===================================================================
--- a/src/cmd/gotype/gotype.go
+++ b/src/cmd/gotype/gotype.go
@@ -23,6 +23,7 @@
pkgName = flag.String("p", "", "process only those files in package
pkgName")
recursive = flag.Bool("r", false, "recursively process subdirectories")
verbose = flag.Bool("v", false, "verbose mode")
+ allErrors = flag.Bool("e", false, "print all (including spurious) errors")

// debugging support
printTrace = flag.Bool("trace", false, "print parse trace")
@@ -68,6 +69,9 @@

// parse entire file
mode := parser.DeclarationErrors
+ if *allErrors {
+ mode |= parser.SpuriousErrors
+ }
if *printTrace {
mode |= parser.Trace
}
Index: src/pkg/go/parser/interface.go
===================================================================
--- a/src/pkg/go/parser/interface.go
+++ b/src/pkg/go/parser/interface.go
@@ -50,7 +50,11 @@

func (p *parser) parseEOF() os.Error {
p.expect(token.EOF)
- return p.GetError(scanner.Sorted)
+ mode := scanner.Sorted
+ if p.mode&SpuriousErrors == 0 {
+ mode = scanner.NoMultiples
+ }
+ return p.GetError(mode)
}

// ParseExpr parses a Go expression and returns the corresponding
@@ -133,7 +137,7 @@

var p parser
p.init(fset, filename, data, mode)
- return p.parseFile(), p.GetError(scanner.NoMultiples) // parseFile()
reads to EOF
+ return p.parseFile(), p.parseEOF()
}

// ParseFiles calls ParseFile for each file in the filenames list and
returns
Index: src/pkg/go/parser/parser.go
===================================================================
--- a/src/pkg/go/parser/parser.go
+++ b/src/pkg/go/parser/parser.go
@@ -26,6 +26,7 @@
ParseComments // parse comments and add them to AST
Trace // print a trace of parsed productions
DeclarationErrors // report declaration errors
+ SpuriousErrors // report all (not just the first)
errors per line
)

// The parser structure holds the parser's internal state.
@@ -1408,7 +1409,13 @@
p.declare(stmt, nil, p.labelScope, ast.Lbl, label)
return stmt
}
- p.error(x[0].Pos(), "illegal label declaration")
+ // The label declaration typically starts at x[0].Pos(), but the label
+ // declaration may be erroneous due to a token after that position (and
+ // before the ':'). If SpuriousErrors is not set, the (only) error re-
+ // ported for the line is the illegal label error instead of the token
+ // before the ':' that caused the problem. Thus, use the (latest) colon
+ // position for error reporting.
+ p.error(colon, "illegal label declaration")
return &ast.BadStmt{x[0].Pos(), colon + 1}

case token.ARROW:

[Russ Cox]

The label fix looks fine.

The -e flag seems not useful.
Most users won't even know it exists, unless
you want the meme to propagate that
'if gofmt errors don't make sense, use gofmt -e'.
I used gofmt as an example. This comes up
in any tool that uses go/parser; when it happened
to me it was gotest. Goinstall will have the same
issue. I don't think every tool should have -e.

Instead of -e it seems worth the effort to just
do the right thing. Of course that requires
figuring out what the right thing is, which is
probably non-trivial, but it will help many more
users. How can we make this just work?

Russ

[Russ Cox]

LGTM

Just read your comments on issue 2088.
If the % error comes out first then the 1-per-line
is still okay, and as long as -e is only for
debugging, lgtm.

One of the nicer things that yacc does is let
you write a rule like

stmt: error ';'

meaning that if you find a syntax error while
trying to parse a statement, after printing the
error, don't immediately try to keep parsing.
Instead, just eat tokens until you get to a ';'
and then use that combination - whatever led
to the syntax error plus the discarded tokens
plus the ';' - as a stmt and keep going.
In practice it means that once you get a syntax
error you don't get another one until a ;.
I wonder if a heuristic like that would work
better than suppressing all but the first thing
printed on each line.

Russ

[Robert Griesemer]

On Thu, Jul 21, 2011 at 7:30 PM, Russ Cox <r...@golang.org> wrote:
> LGTM
>
> Just read your comments on issue 2088.
> If the % error comes out first then the 1-per-line
> is still okay, and as long as -e is only for
> debugging, lgtm.

The idea is definitively for the % error to come first, which it now
does. The -e flag is similar to the -e flag for 6g. It should not be
needed in general.

>
> One of the nicer things that yacc does is let
> you write a rule like
>
> stmt: error ';'
>
> meaning that if you find a syntax error while
> trying to parse a statement, after printing the
> error, don't immediately try to keep parsing.
> Instead, just eat tokens until you get to a ';'
> and then use that combination - whatever led
> to the syntax error plus the discarded tokens
> plus the ';' - as a stmt and keep going.
> In practice it means that once you get a syntax
> error you don't get another one until a ;.
> I wonder if a heuristic like that would work
> better than suppressing all but the first thing
> printed on each line.

One can something along the same lines here as well. N. Wirth
describes a similar heuristic for recursive descent parsers where one
has a list of "stop symbols" to which to read after a syntax error,
for more graceful recovery. In Go this is particularly easy because
all statements end in ";". I just took a cheap route for now (one
error per line) as it works reasonably well. I'll play with it some
time.

- Robert

>
> Russ
>

[g...@golang.org]

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

go/parser: report illegal label declarations at ':' rather than guessing
the start

Also:
- Add parser.SpuriousError flag. If set, the parser reports all
(including
spurious) errors rather then at most one error per line.
- Add -e flag to gofmt and gotype: If set, gofmt and gotype report all
(including spurious) errors rather than at most one error per line.
- Updated the respective documentation.

Fixes issue 2088.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4803047

http://codereview.appspot.com/4803047/