Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: incorrect syntax error cascade #22164

Closed
rsc opened this issue Oct 6, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@rsc
Copy link
Contributor

commented Oct 6, 2017

Here's a bad program, boiled down from a larger case:

package p

func f() {
	x := f(g()
	y := 1
}

func g() {
}

func h() {
	x := f(g()
}

I forgot the closing paren on the first line of f's function body, and just for illustration I did it again in h.

The current compiler diagnoses those two problems but also gets confused by the beginning of g:

$ go tool compile /tmp/x.go
/tmp/x.go:4:12: syntax error: unexpected newline, expecting comma or )
/tmp/x.go:8:10: syntax error: unexpected { at end of statement
/tmp/x.go:12:12: syntax error: unexpected newline, expecting comma or )

In my bigger program there were more lines like the error at x.go:8. I'm not sure exactly why I had more but this example has only one, but one should be enough to illustrate. When I got this output I fixed the problem at (the equivalent of) line 4 and then moved on to line 8 and was a bit confused about where the syntax error was. It took me a moment to realize that there wasn't a syntax error on line 8 and that the parser was still confused about line 4. (But the error report at 12 is real, so it's not like it got totally lost.)

The old yacc parser correctly identified 4 and 12 without getting confused by 8:

$ go1.4 tool 6g /tmp/x.go
/tmp/x.go:4: syntax error: unexpected semicolon or newline, expecting )
/tmp/x.go:12: syntax error: unexpected semicolon or newline, expecting )

I think that the old yacc parser was getting back on its feet either because it assumed the newline started a new statement or because it assumed the closing braces matched so that f's closing brace reset things. I'm not sure of the exact confusion the new parser is in - at first I thought the error was what you'd get if 'func g() { }' appeared inside a function body, but that doesn't seem to be the case.

In passing, I will note:

$ gofmt /tmp/x.go
/tmp/x.go:4:12: missing ',' before newline in argument list
/tmp/x.go:5:4: missing ',' in argument list
/tmp/x.go:6:1: expected operand, found '}'
/tmp/x.go:13:3: missing ',' in argument list

Gofmt emits three errors inside f, doesn't get confused by g, and then reports the wrong line for the mistake in h. Overall the compiler is doing much better - one line in f, one line in g, one line (and the right one) in h - so thanks for those improvements there. It would just be even better for the compiler not to report g.

/cc @griesemer @mdempsky

@rsc rsc added this to the Go1.10 milestone Oct 6, 2017

@griesemer griesemer self-assigned this Oct 6, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

gofmt uses go/parser while the compiler uses the significantly improved parser in cmd/compile/internal/syntax. That explains the difference between gofmt and the compiler.

I'll look into better error recovery for this case. across function boundaries.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 13, 2017

Change https://golang.org/cl/70490 mentions this issue: cmd/compile/internal/syntax: cleanups around parser tracing

gopherbot pushed a commit that referenced this issue Oct 16, 2017

cmd/compile/internal/syntax: cleanups around parser tracing
These changes affect the parser only when the internal trace
constant is set.

- factored our printing code used for tracing
- streamlined advance function and added trace output

The parser's trace output now more clearly prints what tokens
are skipped and which is the next token in case of an error.

Example trace:

    4: . . . . . . . . . . call (
    4: . . . . . . . . . . . expr (
    4: . . . . . . . . . . . . unaryExpr (
    4: . . . . . . . . . . . . . pexpr (
    4: . . . . . . . . . . . . . . operand name (
    4: . . . . . . . . . . . . . . )
    4: . . . . . . . . . . . . . . call (
    4: . . . . . . . . . . . . . . )
    4: . . . . . . . . . . . . . )
    4: . . . . . . . . . . . . )
    4: . . . . . . . . . . . )
    4: . . . . . . . . . . . syntax error: expecting comma or )
    4: . . . . . . . . . . . skip ;
    6: . . . . . . . . . . . skip name
    6: . . . . . . . . . . . skip :=
    6: . . . . . . . . . . . skip literal
    6: . . . . . . . . . . . skip ;
    7: . . . . . . . . . . . skip }
    7: . . . . . . . . . . . skip ;
    9: . . . . . . . . . . . skip func
    9: . . . . . . . . . . . skip name
    9: . . . . . . . . . . . skip (
    9: . . . . . . . . . . . next )
    9: . . . . . . . . . . )

For #22164.

Change-Id: I4a233696b1f989ee3287472172afaf92cf424565
Reviewed-on: https://go-review.googlesource.com/70490
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Oct 17, 2017

Change https://golang.org/cl/71250 mentions this issue: cmd/compile/internal/syntax: better recovery after missing closing parentheses

@gopherbot gopherbot closed this in 0b2cb89 Oct 17, 2017

@golang golang locked and limited conversation to collaborators Oct 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.