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.

Sign up for GitHub

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: switching over channel value incorrectly does not match when the direction is narrower in the case #67190

Closed
Jorropo opened this issue May 5, 2024 · 12 comments
Closed

cmd/compile: switching over channel value incorrectly does not match when the direction is narrower in the case #67190

Jorropo opened this issue May 5, 2024 · 12 comments
Assignees
@cuonglm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@Jorropo
Copy link
Member

Jorropo commented May 5, 2024 •
edited
Loading

Go version

go version devel go1.23-619b419a4b Sun May 5 00:26:04 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/tmp/go-build'
GOENV='/home/hugo/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/hugo/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/hugo/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/hugo/k/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/home/hugo/k/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-619b419a4b Sun May 5 00:26:04 2024 +0000'
GODEBUG=''
GCCGO='/usr/bin/gccgo'
GOAMD64='v3'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build988502288=/tmp/go-build -gno-record-gcc-switches'

What did you do?

@LeaBit on discord gophers report this behavior:

package main

import "fmt"

func main() {
	ch1 := make(chan struct{})

	var ch2 <-chan struct{} = ch1

	switch ch1 {
	case ch2:
		fmt.Println("Narrow case: Same!")
	default:
		fmt.Println("Narrow case: Not same!")
	}

	switch ch2 {
	case ch1:
		fmt.Println("Narrow switch: Same!")
	default:
		fmt.Println("Narrow switch: Not same!")
	}

	fmt.Println("Equal:", ch1 == ch2)
}

What did you see happen?

Narrow case: Not same!
Narrow switch: Same!
Equal: true

What did you expect to see?

Narrow case: Same!
Narrow switch: Same!
Equal: true
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 5, 2024
@Jorropo Jorropo added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels May 5, 2024
@Jorropo Jorropo added this to the Backlog milestone May 5, 2024
@Jorropo Jorropo added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 5, 2024
@Jorropo
Copy link
Member Author

Jorropo commented May 5, 2024

cc @golang/compiler

@randall77
Copy link
Contributor

randall77 commented May 5, 2024 •
edited
Loading

The behavior changed from 1.19 to 1.20.
A bisect would be helpful to track down the cause.

@Jorropo Jorropo changed the title cmd/compile: switching over channel value incorrectly does not match when the direction is narower in the case cmd/compile: switching over channel value incorrectly does not match when the direction is narrower in the case May 5, 2024
@Jorropo
Copy link
Member Author

Jorropo commented May 5, 2024

I'll send a fix

@Jorropo
Copy link
Member Author

Jorropo commented May 6, 2024

Bisect results:

@Jorropo
Copy link
Member Author

Jorropo commented May 6, 2024 •
edited
Loading

Ok so the bug, this code assumes that the type of the case can't be assigned to the type of the tag it needs to compared with any and comparing with any first compare using the exact type tag (which is wrong because you can't assign <-chan T to chan T but you can == them).

ch1 := make(chan struct{})
var ch2 <-chan struct{} = ch1
fmt.Println("Equal:", ch1 == ch2)
fmt.Println("any Equal:", any(ch1) == any(ch2))

prints true and false, I don't know if this is a bug or intended.

@Jorropo
Copy link
Member Author

Jorropo commented May 6, 2024 •
edited
Loading

It looks like the any compare behavior is correct:

Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.

so TIL a == b has different semantics as any(a) == any(b)

https://gophers.slack.com/archives/C1C1YSQBT/p1714960380488909

@cuonglm
Copy link
Member

cuonglm commented May 6, 2024

Possibly duplicate of #44051

@Jorropo
Copy link
Member Author

Jorropo commented May 6, 2024

I don't think so, here I'm assuming the behavior seen in #44051 is correct and not a bug yet this is still broken.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/583395 mentions this issue: Revert "[dev.unified] cmd/compile/internal/noder: better switch statements"

@Jorropo
Copy link
Member Author

Jorropo commented May 6, 2024

Needs more changes in cmd/walk/switch.go to achieve what I want, it's tricky, someone else should make sure to do it right.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/594575 mentions this issue: cmd/compile: fix mis-compilation when switching over channels

@cuonglm cuonglm self-assigned this Jul 3, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Jul 3, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/596575 mentions this issue: cmd/compile: fix mis-compilation when switching over channels

Mchnan pushed a commit to Mchnan/go-sylixos that referenced this issue Jul 9, 2024
@cuonglm
cmd/compile: fix mis-compilation when switching over channels
45286eb 
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 golang#67190

Change-Id: I9a29d9ce3c7978f0689e9502ba6f15660c763d16
Reviewed-on: https://go-review.googlesource.com/c/go/+/594575
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cuonglm cuonglm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Done
Milestone
Backlog
Development

No branches or pull requests
4 participants
@cuonglm @randall77 @gopherbot @Jorropo