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/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 · 9 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mvdan
Copy link
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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 6, 2018
@robpike
Copy link
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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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 go/parser: lacks "unexpected X at end of statement" error cmd/go: test gives go/parser's syntax errors, which are sometimes worse than the compiler's Mar 7, 2018
@mvdan
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 12, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 12, 2018
@andybons andybons added this to the Unplanned milestone Mar 26, 2018
@mvdan
Copy link
Member Author

mvdan commented May 8, 2021

It occurred to me that this issue is likely relevant to @findleyr's recent work :)

@griesemer griesemer assigned findleyr and unassigned griesemer May 8, 2021
@griesemer
Copy link
Contributor

Yes, @findleyr is actively working on the parser and I suspect sooner or later the go/parser will produce much better error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants