-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: miscompilation of comparison between type parameter and interface #51522
Comments
More comprehensive test case, that also exercises non-empty interfaces:
|
CC @randall77 |
It seems plausible that this could lead to subtle bugs in real code, for example: |
Do we understand the underlying issue here? If so, do we have a plan to fix it? |
We chatted about this on Monday and decided to flag this as an error in the compiler for 1.18, with a hope for a more fundamental fix in 1.19 |
From a conversation with @eliben, it looks like he's counting on @mdempsky or @randall77 to take a look. From Eli – "We decided to flag it as an error because we don't have a good undestanding of how to compare interface values with generic values at this time" Should be done by release time. |
Change https://go.dev/cl/391315 mentions this issue: |
Acceptance of https://go.dev/cl/391315 will change this from a release blocker to a bug for Go 1.19. A better temp. solution might be to report this issue in the compiler proper; this CL is just here as a last resort. |
I sent https://go.dev/cl/391374 that fixes some of the cases and give an error for a case that is harder to handle. I'll let @randall77 and @mdempsky decide which approach to take. |
Change https://go.dev/cl/391374 mentions this issue: |
Sorry, I've been away for a few days. |
Change https://go.dev/cl/391475 mentions this issue: |
Change https://go.dev/cl/391594 mentions this issue: |
At this point in stenciling, we have shape types, not raw type parameters. The code was correct in the other part of this function. Update #51522 Change-Id: Ife495160a2be5f6af5400363c3efb68dda518b5f Reviewed-on: https://go-review.googlesource.com/c/go/+/391475 Trust: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reopening for backport to 1.18 release branch. There are 2 CLs, 391475 and 391594. |
Change https://go.dev/cl/391794 mentions this issue: |
Change https://go.dev/cl/391795 mentions this issue: |
…e arg is a type param At this point in stenciling, we have shape types, not raw type parameters. The code was correct in the other part of this function. Update #51522 Change-Id: Ife495160a2be5f6af5400363c3efb68dda518b5f Reviewed-on: https://go-review.googlesource.com/c/go/+/391475 Trust: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 8cf1169) Reviewed-on: https://go-review.googlesource.com/c/go/+/391794 Trust: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
…pe parameters Both the thing we're switching on, as well as the cases we're switching for. Convert anything containing a type parameter to interface{} before the comparison happens. Fixes #51522 Change-Id: I97ba9429ed332cb7d4240cb60f46d42226dcfa5f Reviewed-on: https://go-review.googlesource.com/c/go/+/391594 Trust: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> (cherry picked from commit 2e46a0a) Reviewed-on: https://go-review.googlesource.com/c/go/+/391795 Trust: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
The program below should run quietly, but instead it prints:
Marking as a release blocker, because this seems like a miscompilation bug that users could plausibly actually run into. But I think it might be reasonable to punt fixing it to a point release.
A possible workaround here is to replace each use of
t
withany(t)
./cc @griesemer @ianlancetaylor @randall77
The text was updated successfully, but these errors were encountered: