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: confusing error message for 'a == x && b = y' #36858

Closed
josharian opened this issue Jan 29, 2020 · 12 comments
Closed

cmd/compile: confusing error message for 'a == x && b = y' #36858

josharian opened this issue Jan 29, 2020 · 12 comments
Assignees
Labels
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Jan 29, 2020

package p

func f(a, b string) {
	if a == "a" && b = "b" {
	}
}
$ go tool compile x.go
x.go:4:25: syntax error: assignment a == "a" && b = "b" used as value

The conjunction isn't an assignment; the second conjunct is.

@josharian josharian added this to the Backlog milestone Jan 29, 2020
@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Jan 29, 2020

go/types says x.go:4:5: expected boolean expression, found assignment (missing parentheses around composite literal?). Note that column 5 is the beginning of the conjunction.

@alain91

This comment has been minimized.

Copy link

@alain91 alain91 commented Feb 2, 2020

I would like to have a look on this issue.
Could you confirm, the error message with go/types seems more relevant and would be the solution to use in go tool compile

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Feb 2, 2020

I thought the original error message was pretty good. It is just being applied too broadly. It should read something like:

syntax error: assignment b = "b" used as value

And have its location be that of b = “b”, not of the entire conjunction.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Feb 4, 2020

It seems that the parser think the LHS of assignment is a == "a" && b and the RHS is b.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Feb 4, 2020

@josharian This is probably a simple fix for binary op:

diff --git a/src/cmd/compile/internal/syntax/parser.go b/src/cmd/compile/internal/syntax/parser.go
index f3c2c60ec8..ae26c4a396 100644
--- a/src/cmd/compile/internal/syntax/parser.go
+++ b/src/cmd/compile/internal/syntax/parser.go
@@ -1888,7 +1888,12 @@ done:
                // further confusion.
                str := String(s)
                if as, ok := s.(*AssignStmt); ok && as.Op == 0 {
-                       str = "assignment " + str
+                       prefix := "assignment "
+                       if lhs, ok := as.Lhs.(*Operation); ok && lhs.Y != nil {
+                               as.Lhs = lhs.Y
+                               str = String(as)
+                       }
+                       str = prefix + str
                }
                p.syntaxError(fmt.Sprintf("%s used as value", str))
        }

not sure it's applied for unary op or not 🤔

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Feb 4, 2020

I don’t know much about the parser. Maybe mail that as a CL and discuss with mdempsky or gri? In any case it is easier to discuss diffs in a CL.

@alain91

This comment has been minimized.

Copy link

@alain91 alain91 commented Feb 4, 2020

It seems that "help wanted" is no more required. I let Cuonlm finalize the fix.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Feb 5, 2020

Related #23385

Should we fix this @griesemer ?

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 6, 2020

@cuonglm It would be nice to fix this as it is probably a common typo. But at the same time it's subtle to get it right w/o going overboard. Your tentative patch seems reasonable. How about just highlighting the LHS and RHS of the assignment better with parentheses? For instance if the error message were:

syntax error: assignment (a == "a" && b) = "b" used as value

That is, we put parentheses around expressions if they are complex? (Needless to say that this is not even a valid assignment, but I don't think it's worthwhile spending code on figuring this out in the parser.)

Even simpler, maybe it is good enough to take the current approach and just syntaxErrorAt and use the position of the = (which is the assignment's Pos()). I would probably start with that and see how it goes.

@griesemer griesemer modified the milestones: Backlog, Go1.15 Feb 6, 2020
@griesemer griesemer self-assigned this Feb 6, 2020
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 6, 2020

@cuonglm I assigned myself to this just so I can keep track of it. Feel free to go ahead for now.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 6, 2020

Change https://golang.org/cl/218337 mentions this issue: cmd/compile/internal/syntax: better error when an assignment is used in value context

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 6, 2020

@cuonglm Since I was already investigating this a bit more, I just sent out at CL.

@gopherbot gopherbot closed this in ffc0573 Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.