golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.14k stars 17.56k forks source link

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

Closed losinggeneration closed 1 year ago

losinggeneration commented 1 year ago

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: 6e5c26084f9f3bc910181854a4ff20851188e222

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 commented 1 year ago

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

bcmills commented 1 year ago

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 commented 1 year ago

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.

dmitshur commented 1 year ago

CC @CAFxX, @neild, @Sajmani.

ianlancetaylor commented 1 year ago

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