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

context: Background and TODO may appear equal under some reflect-based notions of equivalence #60978

Closed
losinggeneration opened this issue Jun 23, 2023 · 5 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@losinggeneration
Copy link

losinggeneration commented Jun 23, 2023

What version of Go are you using (go version)?

$ go version

go version go1.21rc2 linux/amd64

Does this issue reproduce with the latest release?

This was introduced in 1.21rc1 & 1.21rc2. I bisected release-branch.go1.20 to release-branch.go1.21 and narrowed it down to this commit: 6e5c260

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/harley/Programs/bin'
GOCACHE='/home/harley/.cache/go-build'
GOENV='/home/harley/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/harley/Source/languages/go/ext/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/harley/Source/languages/go/ext'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/harley/Source/languages/go/src-1.21'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/harley/Source/languages/go/src-1.21/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21rc2'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/harley/Source/languages/go/src-1.21/src/go.mod'
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-build3178496071=/tmp/go-build -gno-record-gcc-switches'

What did you do?

// This is how some assertions libraries check for equality
// This particular implementation was taken from:
// https://github.com/golang/mock/blob/0cdccf5f55d777b12c1ac5a93f607cdd1dbf5296/gomock/matchers.go#L104
func equal(x, y any) bool {
	// In case, some value is nil
	if x == nil || y == nil {
		return reflect.DeepEqual(x, y)
	}

	xVal := reflect.ValueOf(x)
	yVal := reflect.ValueOf(y)

	if xVal.Type().AssignableTo(yVal.Type()) {
		x1ValConverted := xVal.Convert(yVal.Type())
		return reflect.DeepEqual(x1ValConverted.Interface(), yVal.Interface())
	}

	return false
}

func TestContext(t *testing.T) {
	background := context.Background()
	todo := context.TODO()

	// This probably should have never worked in <1.21
	// Mostly, this occurs when tests were carelessly written by declaring the
	// mock to expect context.TODO() but the call uses context.Background()
	// This is a trivial fix to make both use the same call either TODO or Background
	if !equal(background, todo) {
		t.Errorf("background: %T(%#v) is equel to TODO: %T(%#v)", background, background, todo, todo)
	}
}

https://go.dev/play/p/eZIlVe7s-Or

What did you expect to see?

Initially I thought it was a breaking change, but as I dug in, it really seems like it only worked as an artifact of undocumented behavior.

What did you see instead?

It's potentially worth pointing out the corrected behavior of this edge case in the release notes. While this works in versions before 1.21, but only worked because type emptyCtx int and that both Background & TODO were variables assigned new values of emptyCtx. This wasn't documented and generally not expected behavior.

@gopherbot
Copy link

Change https://go.dev/cl/505795 mentions this issue: doc/go1.21: context.Background and TODO may now appear equal

@bcmills
Copy link
Contributor

bcmills commented Jun 23, 2023

That equal function is a really weird way to define equality in Go. 😅

Notably, it does not match the language's definition of equality in https://go.dev/ref/spec#Comparison_operators.

@losinggeneration
Copy link
Author

That's absolutely true. I'm glad it was in a public repo to use as an example of how that library's equality could be (mis)used in previous versions of Go.

@adonovan adonovan changed the title affected/package: context context: Background and TODO may appear equal under some reflect-based notions of equivalence Jun 23, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 23, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jun 23, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Jun 23, 2023

CC @CAFxX, @neild, @Sajmani.

@ianlancetaylor
Copy link
Contributor

Note that I sent CL 505795 to update the release notes. I don't think there is anything else to do here.

@dmitshur dmitshur added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 24, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
Fixes golang#60978

Change-Id: I3e4bd366dc30ac435698b8f17170695330034683
Reviewed-on: https://go-review.googlesource.com/c/go/+/505795
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Sameer Ajmani <sameer@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants