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: "used as value" syntax error reports the entire expression #23385

Closed
mvdan opened this issue Jan 9, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@mvdan
Copy link
Member

commented Jan 9, 2018

What version of Go are you using (go version)?

go version devel +9044f018ec Tue Jan 9 01:53:38 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://play.golang.org/p/v7w9KCIbiDx

What did you expect to see?

syntax error: 0 = 1 used as value

What did you see instead?

syntax error: true || 0 = 1 used as value

I encountered this in a real program, where I typed = instead of ==. The overall boolean expression was much larger, so the error message wasn't very useful. I had to read the whole thing to manually spot where the mistake was.

I think that this error should instead point at what subexpression was invalid. That is, assuming that the syntax tree resulting from the bad program above has enough information to do that, which I hope it does. /cc @griesemer

@griesemer griesemer self-assigned this Jan 9, 2018

@griesemer griesemer added this to the Go1.11 milestone Jan 9, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

I agree that the error message is misleading. That said, it is not as wrong as it seems: Syntactically, this is not an expression but an assignment: (true || 0) = 1 . After parsing (and before type-checking) this is valid. But because it's used in an if statement, and because there's no ; following it, the if statement wants to see a condition (expression), but it sees a statement. It is correct to refer to the entire statement in the error message.

For the reference, gccgo complains with:

x.go:4:19: error: expected ';' after statement in if expression
  if true || 0 = 1 {
                   ^
x.go:4:15: error: invalid left hand side of assignment
  if true || 0 = 1 {
               ^
x.go:4:10: error: incompatible types in binary expression
  if true || 0 = 1 {
          ^

Leaving open for now; perhaps there's a better way to report this.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

I thought about that possibility for a bit before posting, but convinced myself that it wasn't the case because then the left part of an assignment would be an invalid node type. I should have played with other input programs for the compiler to check anyway - that was my bad.

I believe that "bad node type" could be detected before type checking. For example, speaking in AST terms, one could say that a binary expression can never be the left part of an assignment. I do not know whether this distinction could be part of the syntax package, or be considered a syntax error. But intuitively and practically, it seems to me like it could happen.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

To clarify - my thinking is that then the error for the example above would be something like cannot assign to "true || 0". It would allow identifiers, selectors, indexes, slices, stars, and other expression types that one can assign to.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

@mvdan Understood - such an error message for assignments may be helpful on its own, and it that case we might not get this error here. There's a lot of information that could be extracted from the AST before type-checking; it's a bit of a balancing act as to what errors the parser should report that are not pure syntax errors (syntactically, a + b = 0 is valid).

Independent of that (error for assignments), if the code would have been if true = 1 { ... we'd still have the same problem here: It would say true = 1 used as value albeit it would be perhaps less confusing (or more, depending on experience). The real question here is: What's more clearly expressing the programmer's intent, the fact that they wrote a statement here (and thus there's a semicolon missing and this is supposed to be an initialization statement); or the fact that they didn't write a semicolon (and thus this should have used an == instead of an = and this is supposed to be a condition)? It's a design decision. gccgo opted for the former and complains about a missing statementsemicolon; gc opted for the latter and complains about the statement used as value.

Perhaps the best solution is to do all three: Look for funny looking assignments (1), and in the error message here refer to the statement used as value (2), and the possibility of a missing semicolon (3).

Will play with this.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 11, 2018

Change https://golang.org/cl/87316 mentions this issue: cmd/compile/internal/syntax: better error msg for some 'if' statements

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

@mvdan I opted for simply highlighting (in the error message) the fact that we have an assignment here. See pending CL.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 19, 2018

Change https://golang.org/cl/88336 mentions this issue: cmd/compile/internal/syntax: added test cases for recent fixes

@gopherbot gopherbot closed this in be9a177 Feb 12, 2018

gopherbot pushed a commit that referenced this issue Feb 12, 2018

cmd/compile/internal/syntax: added test cases for recent fixes
R=go1.11.

Now that we have a syntax error test harness, we can add the
proper tests for the recent parser fixes.

For #20800.
For #20789.
For #23385.
For #23434.

A test for #20789 already exists in test/fixedbugs, but this
is the better location for that test. But leaving the existing
one where it is as well.

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

@golang golang locked and limited conversation to collaborators Feb 12, 2019

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.