[go] cmd/compile: fix mis-compilation when switching over channels
3 views
Skip to first unread message
Cuong Manh Le (Gerrit)
Jun 24, 2024, 3:46:53 PM (9 days ago) Jun 24
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: fix mis-compilation when switching over channels
CL 418101 changes Unified IR writer to force mixed tag/case to have
common type, emitting the implicit conversion if any of the case values
are not assignble to the tag value's type.
However, the Go spec definition of equality is non-transitive for
channels stored in interfaces, causing incorrect behavior with channel
values comparison.
To fix it, just stripping these implicit conversions.
Fixes #67190
Change-Id: I9a29d9ce3c7978f0689e9502ba6f15660c763d16
Change diff
diff --git a/src/cmd/compile/internal/noder/reader.go b/src/cmd/compile/internal/noder/reader.go
index 97865bb..987488b 100644
--- a/src/cmd/compile/internal/noder/reader.go
+++ b/src/cmd/compile/internal/noder/reader.go
@@ -2012,6 +2012,21 @@
rtypes = append(rtypes, reflectdata.TypePtrAt(cas.Pos(), types.Types[types.TBOOL]))
}
}
+ } else {
+ // Unified IR writer ensures mixed tag/case always have common type, rely on the fact
+ // that `any(x) == any(y)` yields the same value as `x == y`, if they are not nil.
+ //
+ // However, the Go spec definition of equality is non-transitive for channels stored
+ // in interfaces (see issue #44051). So undoing the implicit conversions emitted by
+ // the writer to ensure the correct channel comparison (see issue #44051).
+ if conv, ok := tag.(*ir.ConvExpr); ok && conv.Implicit() && conv.Type().IsEmptyInterface() {
+ if x := conv.X; x.Type().IsChan() {
+ tag = x
+ for i, cas := range cases {
+ cases[i] = cas.(*ir.ConvExpr).X
+ }
+ }
+ }
}
}
diff --git a/test/fixedbugs/issue67190.go b/test/fixedbugs/issue67190.go
new file mode 100644
index 0000000..c19b248
--- /dev/null
+++ b/test/fixedbugs/issue67190.go
@@ -0,0 +1,24 @@
+// run
+
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+func main() {
+ ch1 := make(chan struct{})
+ var ch2 <-chan struct{} = ch1
+
+ switch ch1 {
+ case ch2:
+ default:
+ panic("bad narrow case")
+ }
+
+ switch ch2 {
+ case ch1:
+ default:
+ panic("bad narrow switch")
+ }
+}
Change information
Files:
- M src/cmd/compile/internal/noder/reader.go
- A test/fixedbugs/issue67190.go
Change size: S
Delta: 2 files changed, 39 insertions(+), 0 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
Jun 24, 2024, 3:47:20 PM (9 days ago) Jun 24
to goph...@pubsubhelper.golang.org, Matthew Dempsky, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Keith Randall and Matthew Dempsky
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
Jun 26, 2024, 10:25:49 AM (7 days ago) Jun 26
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Than McIntosh, Go LUCI, Matthew Dempsky, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cuong Manh Le, Keith Randall and Matthew Dempsky
Than McIntosh voted and added 3 comments![]( "Open in Gerrit")
Votes added by Than McIntosh
| Code-Review | +2 |
3 comments
Patchset-level comments
Commit Message
Line 11, Patchset 1 (Latest):
are not assignble to the tag value's type.Than McIntosh . unresolved
assignable
File src/cmd/compile/internal/noder/reader.go
Line 2020, Patchset 1 (Latest):
// in interfaces (see issue #44051). So undoing the implicit conversions emitted byThan McIntosh . unresolved
undo
Related details
Attention is currently required from:
- Cuong Manh Le
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jun 26, 2024, 11:25:10 AM (7 days ago) Jun 26
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Than McIntosh, Go LUCI, Matthew Dempsky, Keith Randall, golang-co...@googlegroups.com
Attention needed from Cuong Manh Le, Keith Randall and Matthew Dempsky
Jorropo added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Jorropo . resolved
thx, fixedbugs files looks good, idk about noder enough to have an opinion.
Than McIntosh (Gerrit)
Jul 2, 2024, 10:53:56 AM (yesterday) Jul 2
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Matthew Dempsky, Than McIntosh, Go LUCI, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cuong Manh Le and Keith Randall
Than McIntosh removed Matthew Dempsky from this change![]( "Open in Gerrit")
Deleted Reviewers:
- Matthew Dempsky
Related details
Attention is currently required from:
- Cuong Manh Le
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:40 AM (21 hours ago) 12:40 AM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase, Keith Randall and Matthew Dempsky
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Cuong Manh Le uploaded patch set #2 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:41 AM (21 hours ago) 12:41 AM
to goph...@pubsubhelper.golang.org, Cherry Mui, David Chase, Than McIntosh, Go LUCI, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Matthew Dempsky
Cuong Manh Le voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Commit Message
Line 11, Patchset 1:
are not assignble to the tag value's type.Than McIntosh . resolved
Cuong Manh Leassignable
Done
File src/cmd/compile/internal/noder/reader.go
Line 2020, Patchset 1:
// in interfaces (see issue #44051). So undoing the implicit conversions emitted byThan McIntosh . resolved
Cuong Manh Leundo
Done
Related details
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Than McIntosh (Gerrit)
8:33 AM (13 hours ago) 8:33 AM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Than McIntosh, Cherry Mui, David Chase, Go LUCI, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase, Keith Randall and Matthew Dempsky
Than McIntosh voted![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
11:52 AM (9 hours ago) 11:52 AM
to goph...@pubsubhelper.golang.org, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Matthew Dempsky
Cuong Manh Le voted Auto-Submit+1![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
11:54 AM (9 hours ago) 11:54 AM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase, Keith Randall and Matthew Dempsky
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Cuong Manh Le uploaded patch set #3 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
11:54 AM (9 hours ago) 11:54 AM
to goph...@pubsubhelper.golang.org, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Matthew Dempsky
Cuong Manh Le voted![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Matthew Dempsky
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
12:21 PM (9 hours ago) 12:21 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase and Keith Randall
Keith Randall added 2 comments![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/reader.go
Line 2016, Patchset 2:
// Unified IR writer ensures mixed tag/case always have common type, rely on the factKeith Randall . unresolved
I think I'd rather fix this in the writer than undo its work in the reader.
The writer might just need (line 1662)
// We need to keep comparisons of channel values from
// being wrapped in any(). See issue 67190.
if _, ok := tagType.Underlying().(*types2.Chan); ok { continue }
Line 2026, Patchset 2:
cases[i] = cas.(*ir.ConvExpr).XKeith Randall . unresolved
In particular, I'm worried about here. How do we know there isn't some other reason why the tag has an implicit conversion, but doesn't make each case also be an implicit conversion? (I couldn't find a case where that could happen, but still makes me nervous.)
Related details
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:31 PM (9 hours ago) 12:31 PM
to goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le added 2 comments![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/reader.go
Line 2016, Patchset 2:
// Unified IR writer ensures mixed tag/case always have common type, rely on the factKeith Randall . unresolved
I think I'd rather fix this in the writer than undo its work in the reader.
The writer might just need (line 1662)
// We need to keep comparisons of channel values from
// being wrapped in any(). See issue 67190.
if _, ok := tagType.Underlying().(*types2.Chan); ok { continue }
Cuong Manh Le
Hmm, might not enough, since we still do the implicit conversion at L1669, causing the fatal at L2268.
Line 2026, Patchset 2:
cases[i] = cas.(*ir.ConvExpr).XKeith Randall . unresolved
In particular, I'm worried about here. How do we know there isn't some other reason why the tag has an implicit conversion, but doesn't make each case also be an implicit conversion? (I couldn't find a case where that could happen, but still makes me nervous.)
Cuong Manh Le
Hmm,the writer always make the implicit comversion, so that mismatch won't happen.
Related details
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
12:38 PM (9 hours ago) 12:38 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Keith Randall added 1 comment![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/reader.go
Line 2016, Patchset 2:
// Unified IR writer ensures mixed tag/case always have common type, rely on the factKeith Randall . unresolved
Cuong Manh LeI think I'd rather fix this in the writer than undo its work in the reader.
The writer might just need (line 1662)
// We need to keep comparisons of channel values from
// being wrapped in any(). See issue 67190.
if _, ok := tagType.Underlying().(*types2.Chan); ok { continue }
Hmm, might not enough, since we still do the implicit conversion at L1669, causing the fatal at L2268.
Keith Randall
If we skip the assignment to tagType, then it will always be the type of tag, so the implicit conversion would always work (or could be skipped, that would be fine also).
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:54 PM (8 hours ago) 12:54 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Cuong Manh Le uploaded patch set #4 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:55 PM (8 hours ago) 12:55 PM
to goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
File src/cmd/compile/internal/noder/reader.go
Line 2016, Patchset 2:
// Unified IR writer ensures mixed tag/case always have common type, rely on the factKeith Randall . resolved
Cuong Manh LeI think I'd rather fix this in the writer than undo its work in the reader.
The writer might just need (line 1662)
// We need to keep comparisons of channel values from
// being wrapped in any(). See issue 67190.
if _, ok := tagType.Underlying().(*types2.Chan); ok { continue }
Keith RandallHmm, might not enough, since we still do the implicit conversion at L1669, causing the fatal at L2268.
If we skip the assignment to tagType, then it will always be the type of tag, so the implicit conversion would always work (or could be skipped, that would be fine also).
Cuong Manh Le
Done
Line 2026, Patchset 2:
cases[i] = cas.(*ir.ConvExpr).XKeith Randall . resolved
Cuong Manh LeIn particular, I'm worried about here. How do we know there isn't some other reason why the tag has an implicit conversion, but doesn't make each case also be an implicit conversion? (I couldn't find a case where that could happen, but still makes me nervous.)
Hmm,the writer always make the implicit comversion, so that mismatch won't happen.
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:56 PM (8 hours ago) 12:56 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase, Keith Randall and Keith Randall
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
12:56 PM (8 hours ago) 12:56 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le voted![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
1:01 PM (8 hours ago) 1:01 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Keith Randall added 1 comment![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5 (Latest):
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . unresolved
I think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
1:01 PM (8 hours ago) 1:01 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Keith Randall added 1 comment![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5 (Latest):
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . unresolved
I think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Keith Randall
Passes all.bash for me.
Cuong Manh Le (Gerrit)
1:06 PM (8 hours ago) 1:06 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le added 1 comment![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5 (Latest):
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . unresolved
Keith RandallI think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Passes all.bash for me.
Cuong Manh Le
Should we add this change in this CL or a follow up? A follow up CL seems to be better in case we need to revert.
Related details
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
1:20 PM (8 hours ago) 1:20 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Commit-Queue | +1 |
1 comment
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5 (Latest):
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . unresolved
Keith RandallI think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Cuong Manh LePasses all.bash for me.
Should we add this change in this CL or a follow up? A follow up CL seems to be better in case we need to revert.
Cuong Manh Le
Looking back at https://go-review.googlesource.com/c/go/+/418101, do we still have to worry that "unified IR can't wire up appropriate RTTI operands for the conversion" if the conversion is emitted by walk?
Related details
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
1:26 PM (8 hours ago) 1:26 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Keith Randall added 2 comments![]( "Open in Gerrit")
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5 (Latest):
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . unresolved
Keith RandallI think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Cuong Manh LePasses all.bash for me.
Should we add this change in this CL or a follow up? A follow up CL seems to be better in case we need to revert.
Keith Randall
Sure, a followup is fine. The plan would then be this CL for 1.23 and the followup for 1.24?
Line 1677, Patchset 5 (Latest):
if tagTypeIsChan {Keith Randall . unresolved
I don't think we need these guards - implictConvExpr will be a no-op because tagType will have never been modified.
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
1:35 PM (8 hours ago) 1:35 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Cuong Manh Le uploaded patch set #6 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
1:35 PM (8 hours ago) 1:35 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
File src/cmd/compile/internal/noder/writer.go
Line 1668, Patchset 5:
if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {Keith Randall . resolved
Keith RandallI think the wrapping in "any" is only needed if either casType or tagType is an interface. And I think it is more clearly ok to do so in that situation, as == in that situation is implemented by upconverting to an interface anyway.
So maybe we can just add && (types2.IsInterface(casType) || types2.IsInterface(tagType)) here.
Cuong Manh LePasses all.bash for me.
Keith RandallShould we add this change in this CL or a follow up? A follow up CL seems to be better in case we need to revert.
Sure, a followup is fine. The plan would then be this CL for 1.23 and the followup for 1.24?
Cuong Manh Le
The plan would then be this CL for 1.23 and the followup for 1.24?
Sounds good to me.
I don't think we need these guards - implictConvExpr will be a no-op because tagType will have never been modified.
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
1:38 PM (8 hours ago) 1:38 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase, Keith Randall and Keith Randall
Cuong Manh Le uploaded new patchset![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
1:38 PM (8 hours ago) 1:38 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Keith Randall, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, David Chase, Keith Randall and Keith Randall
Cuong Manh Le voted![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- David Chase
- Keith Randall
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
2:05 PM (7 hours ago) 2:05 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, Keith Randall, Than McIntosh, Cherry Mui, David Chase, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le, David Chase and Keith Randall
Keith Randall voted Code-Review+2![]( "Open in Gerrit")
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
- Keith Randall
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
2:06 PM (7 hours ago) 2:06 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Keith Randall, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Jorropo, golang-co...@googlegroups.com
Attention needed from Cherry Mui, Cuong Manh Le and David Chase
Keith Randall voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Cherry Mui
- Cuong Manh Le
- David Chase
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
2:07 PM (7 hours ago) 2:07 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Keith Randall, Keith Randall, Go LUCI, Than McIntosh, Cherry Mui, David Chase, Jorropo, golang-co...@googlegroups.com
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
Commit message:
cmd/compile: fix mis-compilation when switching over channels
CL 418101 changes Unified IR writer to force mixed tag/case to have
common type, emitting the implicit conversion if any of the case values
are not assignable to the tag value's type.
However, the Go spec definition of equality is non-transitive for
channels stored in interfaces, causing incorrect behavior with channel
values comparison.
To fix it, don't emit the implicit conversions if tag type is channel.
Fixes #67190
Change-Id: I9a29d9ce3c7978f0689e9502ba6f15660c763d16
Reviewed-on: https://go-review.googlesource.com/c/go/+/594575
Auto-Submit: Cuong Manh Le <cuong.m...@gmail.com>
Reviewed-by: Keith Randall <k...@google.com>
Reviewed-by: Keith Randall <k...@golang.org>
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <th...@google.com>
Files:
- M src/cmd/compile/internal/noder/writer.go
- A test/fixedbugs/issue67190.go
Change size: M
Delta: 2 files changed, 43 insertions(+), 7 deletions(-)
Branch: refs/heads/master
Submit Requirements:
Code-Review: +2 by Keith Randall, +1 by Keith Randall, +2 by Than McIntosh
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
2:09 PM (7 hours ago) 2:09 PM
to Keith Randall, Than McIntosh, Keith Randall, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Keith Randall, Keith Randall and Than McIntosh
Cuong Manh Le has uploaded the change for review![]( "Open in Gerrit")
Cuong Manh Le would like Keith Randall, Than McIntosh and Keith Randall to review this change.
Commit message
cmd/compile: fix mis-compilation when switching over channels
CL 418101 changes Unified IR writer to force mixed tag/case to have
common type, emitting the implicit conversion if any of the case values
are not assignable to the tag value's type.
However, the Go spec definition of equality is non-transitive for
channels stored in interfaces, causing incorrect behavior with channel
values comparison.
To fix it, don't emit the implicit conversions if tag type is channel.
Fixes #67190
Change-Id: I9a29d9ce3c7978f0689e9502ba6f15660c763d16
Reviewed-on: https://go-review.googlesource.com/c/go/+/594575
Auto-Submit: Cuong Manh Le <cuong.m...@gmail.com>
Reviewed-by: Keith Randall <k...@google.com>
Reviewed-by: Keith Randall <k...@golang.org>
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <th...@google.com>
Change diff
diff --git a/src/cmd/compile/internal/noder/writer.go b/src/cmd/compile/internal/noder/writer.go
index fe8f8f2..8fed138 100644
--- a/src/cmd/compile/internal/noder/writer.go
+++ b/src/cmd/compile/internal/noder/writer.go
@@ -1582,6 +1582,7 @@
w.stmt(stmt.Init)
var iface, tagType types2.Type
+ var tagTypeIsChan bool
if guard, ok := stmt.Tag.(*syntax.TypeSwitchGuard); w.Bool(ok) {
iface = w.p.typeOf(guard.X)
@@ -1603,6 +1604,7 @@
tv := w.p.typeAndValue(tag)
tagType = tv.Type
tagValue = tv.Value
+ _, tagTypeIsChan = tagType.Underlying().(*types2.Chan)
} else {
tagType = types2.Typ[types2.Bool]
tagValue = constant.MakeBool(true)
@@ -1655,12 +1657,18 @@
// have the same type. If there are any case values that can't be
// converted to the tag value's type, then convert everything to
// `any` instead.
- Outer:
- for _, clause := range stmt.Body {
- for _, cas := range syntax.UnpackListExpr(clause.Cases) {
- if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {
- tagType = types2.NewInterfaceType(nil, nil)
- break Outer
+ //
+ // Except that we need to keep comparisons of channel values from
+ // being wrapped in any(). See issue #67190.
+
+ if !tagTypeIsChan {
+ Outer:
+ for _, clause := range stmt.Body {
+ for _, cas := range syntax.UnpackListExpr(clause.Cases) {
+ if casType := w.p.typeOf(cas); !types2.AssignableTo(casType, tagType) {
+ tagType = types2.NewInterfaceType(nil, nil)
+ break Outer
+ }
}
}
}
@@ -1696,7 +1704,11 @@
w.Sync(pkgbits.SyncExprs)
w.Len(len(cases))
for _, cas := range cases {
- w.implicitConvExpr(tagType, cas)
+ typ := tagType
+ if tagTypeIsChan {
+ typ = nil
+ }
+ w.implicitConvExpr(typ, cas)
}
}
diff --git a/test/fixedbugs/issue67190.go b/test/fixedbugs/issue67190.go
new file mode 100644
index 0000000..c19b248
--- /dev/null
+++ b/test/fixedbugs/issue67190.go
@@ -0,0 +1,24 @@
+// run
+
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+func main() {
+ ch1 := make(chan struct{})
+ var ch2 <-chan struct{} = ch1
+
+ switch ch1 {
+ case ch2:
+ default:
+ panic("bad narrow case")
+ }
+
+ switch ch2 {
+ case ch1:
+ default:
+ panic("bad narrow switch")
+ }
+}
Change information
Files:
- M src/cmd/compile/internal/noder/writer.go
- A test/fixedbugs/issue67190.go
Change size: M
Delta: 2 files changed, 43 insertions(+), 7 deletions(-)
Related details
Attention is currently required from:
- Keith Randall
- Keith Randall
- Than McIntosh
Submit Requirements:
Code-Review
Matching-Subject-Prefix
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
2:10 PM (7 hours ago) 2:10 PM
to goph...@pubsubhelper.golang.org, Keith Randall, Than McIntosh, Keith Randall, golang-co...@googlegroups.com
Attention needed from Keith Randall, Keith Randall and Than McIntosh
Cuong Manh Le voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Keith Randall
- Keith Randall
- Than McIntosh
Submit Requirements:
Code-Review
Matching-Subject-Prefix
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
2:16 PM (7 hours ago) 2:16 PM
to Cuong Manh Le, goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Keith Randall, golang-co...@googlegroups.com
Attention needed from Cuong Manh Le, Keith Randall and Than McIntosh
Keith Randall added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Keith Randall . resolved
I don't think we need to move the CL over to the release branch. We haven't forked from main yet, so the release team will just bring everything over from main for the next rc/release.
Related details
Attention is currently required from:
- Cuong Manh Le
- Keith Randall
- Than McIntosh
Submit Requirements:
Code-Review
Matching-Subject-Prefix
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
2:20 PM (7 hours ago) 2:20 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Keith Randall, golang-co...@googlegroups.com
Attention needed from Keith Randall, Keith Randall and Than McIntosh
Cuong Manh Le voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cuong Manh Le
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Keith Randall . resolved
I don't think we need to move the CL over to the release branch. We haven't forked from main yet, so the release team will just bring everything over from main for the next rc/release.
Cuong Manh Le
Ops, thanks for letting me know.
I thought that after the freeze begun, we have to cherry pick CL to release branch.
Related details
Attention is currently required from:
- Keith Randall
- Keith Randall
- Than McIntosh
Submit Requirements:
Code-Review
Matching-Subject-Prefix
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cuong Manh Le (Gerrit)
2:20 PM (7 hours ago) 2:20 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Keith Randall, Than McIntosh, Keith Randall, golang-co...@googlegroups.com
Cuong Manh Le abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Review
Matching-Subject-Prefix
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages