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

Jump to bottom

cmd/compile: missing/bad error for "s"+bool #46749

Closed
rsc opened this issue Jun 15, 2021 · 11 comments
Closed

cmd/compile: missing/bad error for "s"+bool #46749

rsc opened this issue Jun 15, 2021 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@rsc
Copy link
Contributor

rsc commented Jun 15, 2021 

% cat /tmp/x.go
package main

func main() {
	var b bool
	_ = "Hello, " + b
}
% go1.13 tool compile /tmp/x.go
/tmp/x.go:5:16: cannot convert "Hello, " (type untyped string) to type bool
/tmp/x.go:5:16: invalid operation: "Hello, " + b (mismatched types string and bool)
% go1.15 tool compile /tmp/x.go
/tmp/x.go:5:16: cannot use "Hello, " (type untyped string) as type bool
%

The Go 1.15 output is what is still printed today. Having two errors was definitely a bit annoying, but the fix should have been to delete the first one, not the second. I got this in a larger program as part of a larger expression, like:

for _, host := range validHosts {
	list = append(list, `blah blah`+host)
}

The error was cannot use "blah blah" (type untyped string) as bool.
I spent quite a while puzzling through the problem of why my string would need to be a bool.
The answer is that validHosts is a map[string]bool so host is a bool.
If the error had reported string+bool, the problem would have been far more obvious.

We should restore the second error and probably drop the first.
I think we could drop the first by making the implicit conversion of strings
just return plain string if the suggestion is bool.
Then the invalid operation or whatever other error will happen will be reported with more context.

Would be nice to fix for Go 1.17 but not a release blocker.

/cc @griesemer @mdempsky
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 15, 2021
@rsc rsc added this to the Go1.17 milestone Jun 15, 2021
@griesemer
Copy link
Contributor

Compiling this with -G=3 (using types2 for type checking) produces:

$ go tool compile -G=3 x.go
x.go:5:6: cannot convert "Hello, " (untyped string constant) to bool

which seems pretty good. So this will certainly be fixed for Go 1.18. Haven't looked into the compiler's type checker yet. @mdempsky ?

@rsc
Copy link
Contributor Author

rsc commented Jun 15, 2021

The -G=3 output has the same problem as the current regular output. It should say instead:

x.go:5:16: invalid operation: "Hello, " + b (mismatched types string and bool)

@cuonglm
Copy link
Member

cuonglm commented Jun 15, 2021

@rsc You want to make it specific to string+bool or general?

The "invalid operation" seems unclear to me, as the op "+" is defined for both string and bool.

@griesemer
Copy link
Contributor

@cuonglm I would expect that such a change should work in general (e.g. for "foo" + 1 we should get an error "mismatched types string and int").

@mdempsky
Copy link
Member

mdempsky commented Jun 15, 2021 •
edited

Having two errors was definitely a bit annoying, but the fix should have been to delete the first one, not the second.

Ack, the second error there was more helpful. I don't think it was specifically removed. I refactored a lot of the untyped constant handling in typecheck a while back (I think in that time period) to behave more consistently, and in particular to avoid printing multiple errors.

The "invalid operation" seems unclear to me, as the op "+" is defined for both string and bool.

The invalid operation is trying to add two mismatched types. (Also, "+" isn't applicable to bool, but the point still stands if it was int or something.)

I think this basically similar to when we made && and || print better diagnostics for operands that are impossible to coerce to a common type. We should figure out how to generalize that to more operations.

Edit: I suspect we may just want defaultlit2 to never report errors on its own. Instead, leave it for the caller to detect that their types are still mismatched and report a better error.

@mdempsky
Copy link
Member

Related: #41500

@cuonglm
Copy link
Member

cuonglm commented Jun 15, 2021 •
edited

The invalid operation is trying to add two mismatched types. (Also, "+" isn't applicable to bool, but the point still stands if it was int or something.)

Yeah, my bad. I mean the op can
be applicable for both operands in general case.

@gopherbot
Copy link

Change https://golang.org/cl/328050 mentions this issue: cmd/compile: better error message for invalid untyped operation

@gopherbot
Copy link

Change https://golang.org/cl/328053 mentions this issue: [dev.typeparams] cmd/compile: make types2 reports better error for invalid untyped operation

gopherbot pushed a commit that referenced this issue Jun 17, 2021
@cuonglm
[dev.typeparams] cmd/compile: make types2 report better error for inv…
b14fd72 
…alid untyped operation

This ports the fix in CL 328050 for typecheck to types2.

The fix is not identical, due to code structure differences between
typecheck and types2, but the idea is the same. We only do the untyped
conversion when both operands can be mixed.

Updates #46749

Change-Id: Ib2c63ba0d5dd8bf02318b1bfdfe51dcaeeeb7f82
Reviewed-on: https://go-review.googlesource.com/c/go/+/328053
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/328869 mentions this issue: [dev.typeparams] go/types: report better error for invalid untyped operation

@rsc
Copy link
Contributor Author

rsc commented Jun 17, 2021

CL 328050 looks great. Thanks so much!

gopherbot pushed a commit that referenced this issue Jun 18, 2021
@cuonglm
[dev.typeparams] go/types: report better error for invalid untyped op…
2a79007 
…eration

This is port of CL 328053 for types2 to go/type.

The change is identical, but for some tweaks to the error positions in
tests.

Updates #46749

Change-Id: I8d34c5b1669e59e4ec7d91f81dcf655b2bfd89a5
Reviewed-on: https://go-review.googlesource.com/c/go/+/328869
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
5 participants
@mdempsky @rsc @cuonglm @griesemer @gopherbot