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/go: test gives go/parser's syntax errors, which are sometimes worse than the compiler's #24278

Open
mvdan opened this Issue Mar 6, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@mvdan
Member

mvdan commented Mar 6, 2018

$ cat f3.go
package main

func f(x int) int {
        retrun x
}
$ go run f3.go
# command-line-arguments
./f3.go:4:9: syntax error: unexpected x at end of statement
$ gofmt f3.go
f3.go:4:9: expected ';', found x
f3.go:5:3: expected '}', found 'EOF'

I would usually not mind if go/parser is worse than the compiler's parser at giving useful errors. However, now that vet always runs with test, it's vet that reports syntax errors to me when I run go test. This particular error (the more confusing one) had me scratching my head for a minute, before I realised the typo.

Should this be fixed in go/parser, or should something else be done, such as using the compiler's syntax package to replace go/parser's syntax errors?

/cc @griesemer

@mvdan mvdan added the NeedsDecision label Mar 6, 2018

@robpike

This comment has been minimized.

Contributor

robpike commented Mar 6, 2018

I agree that the change to run vet always changes what errors one sees, and after all the work put into getting excellent errors from the compiler, it's a shame to go back to the less precise ones from go/parser.

But: vet is running in parallel with the compiler, so I think it makes sense to hold off printing errors from vet until the compiler has completed. If the compile fails, drop the noise from vet. That way we only see vet errors if something actually builds.

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 6, 2018

That makes sense. That's the sort of thing I was suggesting as an alternative to having to improve go/parser's errors.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Mar 7, 2018

I'd like to see the new syntax package taking the place of go/parser, eventually. Then all tools (including the compiler) will use the same front-end code, with identical behavior and errors. But I don't want to rush this process because once package syntax is exposed (not internal) we cannot change its API anymore. Also, all other go/* packages would have to be adjusted which - while not hard - a time-consuming and error-prone process.

But it should be fairly straight-forward to port back this specific error message if need be But @r's suggestion is a better one and solves the more general problem.

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 7, 2018

Great - I'll give Rob's fix a try. Thank you both for your input.

@griesemer after repurposing this issue to be about the go command, do you want to open another issue about improving go/parser's error?

@mvdan mvdan changed the title from go/parser: lacks "unexpected X at end of statement" error to cmd/go: test gives go/parser's syntax errors, which are sometimes worse than the compiler's Mar 7, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 7, 2018

Actually, I was wrong - my particular case had nothing to do with vet. My problem was coming from go test using go/parser directly in loadTestFuncs. Skipping this error is a bit tricky, since this happens much earlier than the build action. I'm no longer sure that a change in test is the right fix.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Mar 7, 2018

@mvdan I'll assign this to me and will look at the parser's error message. But this is not urgent.

@griesemer griesemer self-assigned this Mar 7, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 7, 2018

Indeed not urgent - also fine by me if it's not fixed at all, as long as go/parser is replaced eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment