sync: Map - reflect.DeepEqual regression between Go 1.23 and 1.24 #71702

cipherboy · 2025-02-13T02:31:24Z

Go version

go version go1.24.0 linux/amd64

Output of `go env` in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE='on'
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/cipherboy/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/cipherboy/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build670466362=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/cipherboy/GitHub/cipherboy/openbao/go.mod'
GOMODCACHE='/home/cipherboy/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/cipherboy/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/cipherboy/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

// You can edit this code!
// Click here and start typing.
package main

import (
	"fmt"
	"reflect"
	"sync"
)

type obj struct {
	configMap sync.Map
}

func main() {
	x := new(obj)
	y := new(obj)

	x.configMap.Delete("key")
	y.configMap.Delete("key")

	fmt.Println(reflect.DeepEqual(x, y))
}

Go 1.24: https://go.dev/play/p/Dsu_70d--CL
Go 1.23: https://go.dev/play/p/Dsu_70d--CL?v=goprev

What did you see happen?

Calling sync.Map.Delete(...) on two empty maps results in a false value from reflect.DeepEqual when comparing those maps.

Before delete, this is memory is:

(gdb) p x.configMap
$1 = {_ = {<No data fields>}, m = {inited = {_ = {<No data fields>}, v = 0}, initMu = {
      state = 0, sema = 0}, root = {_ = 0xc0001a8100, _ = {<No data fields>}, v = 0x0}, 
    keyHash = {void (void *, uintptr, uintptr)} 0xc0001a8108, valEqual = {void (void *, 
    void *, bool)} 0xc0001a8110, seed = 0}}
(gdb) p y.configMap
$2 = {_ = {<No data fields>}, m = {inited = {_ = {<No data fields>}, v = 0}, initMu = {
      state = 0, sema = 0}, root = {_ = 0xc0001a8130, _ = {<No data fields>}, v = 0x0}, 
    keyHash = {void (void *, uintptr, uintptr)} 0xc0001a8138, valEqual = {void (void *, 
    void *, bool)} 0xc0001a8140, seed = 0}}

Post delete:

(gdb) p x.configMap
$2 = {_ = {<No data fields>}, m = {inited = {_ = {<No data fields>}, v = 1}, initMu = {
      state = 0, sema = 0}, root = {_ = 0xc0000b8100, _ = {<No data fields>}, 
      v = 0xc0000b40a0}, keyHash = {void (void *, uintptr, uintptr)} 0xc0000b8108, 
    valEqual = {void (void *, void *, bool)} 0xc0000b8110, seed = 13177593922945986167}}
(gdb) p y.configMap
$3 = {_ = {<No data fields>}, m = {inited = {_ = {<No data fields>}, v = 1}, initMu = {
      state = 0, sema = 0}, root = {_ = 0xc0000b8130, _ = {<No data fields>}, 
      v = 0xc0000b4140}, keyHash = {void (void *, uintptr, uintptr)} 0xc0000b8138, 
    valEqual = {void (void *, void *, bool)} 0xc0000b8140, seed = 1278365537727921764}}

Note that seed is now initialized.

What did you expect to see?

On Go 1.23, this previously was true, i.e., the maps were equal, due to the simpler implementation.

(gdb) p x.configMap
$1 = {mu = {state = 0, sema = 0}, read = {_ = 0xc00007c028, _ = {<No data fields>}, 
    v = 0x0}, dirty = 0x0, misses = 0}
(gdb) p y.configMap
$2 = {mu = {state = 0, sema = 0}, read = {_ = 0xc00007c048, _ = {<No data fields>}, 
    v = 0x0}, dirty = 0x0, misses = 0}

In particular, reflect appears to be recursing into the internal HashTrieMap implementation from the experiment now, with seed differing post-Delete.

We can work around this by using an alternative DeepEqual implementation in the test suite, but it is still an unfortunate issue.

The text was updated successfully, but these errors were encountered:

There's a new sync.Map implementation controllable via GOEXPERIMENT value. In the test use cases, we likely don't care about the strict reflect equality so we can swap to an alternative comparison implementation. Other uses of reflect.DeepEqual(...) at runtime don't appear to operate over sync.Map instances and so aren't affected. See also: https://tip.golang.org/doc/go1.24#syncpkgsync See also: golang/go#71702 Signed-off-by: Alexander Scheel <ascheel@gitlab.com>

gabyhelp · 2025-02-13T02:43:59Z

Related Issues

Related Code Changes

reflect: document DeepEqual, fix case of maps with NaN keys

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

There's a new sync.Map implementation controllable via GOEXPERIMENT value. In the test use cases, we likely don't care about the strict reflect equality so we can swap to an alternative comparison implementation. Other uses of reflect.DeepEqual(...) at runtime don't appear to operate over sync.Map instances and so aren't affected. See also: https://tip.golang.org/doc/go1.24#syncpkgsync See also: golang/go#71702 Signed-off-by: Alexander Scheel <ascheel@gitlab.com>

seankhliao · 2025-02-13T08:01:17Z

The internal representations of any struct aren't subject to stability guarantees.
while unfortunate I don't think this is likely to change.

cipherboy · 2025-02-13T13:25:01Z

@seankhliao While I concur, note that this leaks via reflect.DeepEqual(...) and is thus unscoped runtime breakage beyond explicitly interrogating internal representations.

GitHub presently shows ~170k usages for reflect.DeepEqual outside of tests: https://github.com/search?q=reflect.DeepEqual+language%3AGo+-path%3A*_test.go&type=code -- though it is not knowable how many compare with a sync.Map.

The Go docs merely note: https://pkg.go.dev/reflect#DeepEqual

However, this idea is impossible to implement without some inconsistency.

But nothing about behavior changes across versions. It seems like there'd be value in making sync.Map behave as closely to a map as possible w.r.t. DeepEqual in particular and exclude the new seed field as they are expected to differ post-initialization.

My usage is all in tests, so I'm happy to keep closed if that's the consensus.

seankhliao · 2025-02-13T14:12:30Z

https://go.dev/doc/go1compat

Go carves out the addition of struct fields, these may or may not be compared equal with DeepEqual, regardless if it's exported or an internal detail.
As such, any use of DeepEqual is not guaranteed to work in future versions.
Previous example: #45891

cipherboy · 2025-02-13T14:32:00Z

I see. Thank you for the clarification.

Should #43993 be extended to forbid all use of reflect.DeepEqual on standard library objects?

* Update to Go 1.24.0 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix reflect comparisons due to sync.Map changes There's a new sync.Map implementation controllable via GOEXPERIMENT value. In the test use cases, we likely don't care about the strict reflect equality so we can swap to an alternative comparison implementation. Other uses of reflect.DeepEqual(...) at runtime don't appear to operate over sync.Map instances and so aren't affected. See also: https://tip.golang.org/doc/go1.24#syncpkgsync See also: golang/go#71702 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix certificate parsing, issuing bugs Go now forbids creating 512-bit RSA keys and uses x509.Certificate.Policies as the field for policy identifiers. See also: https://go.dev/doc/go1.24#cryptorsapkgcryptorsa See also: https://go.dev/doc/go1.24#cryptox509pkgcryptox509 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix additional uses of formatters with constants Signed-off-by: Alexander Scheel <ascheel@gitlab.com> --------- Signed-off-by: Alexander Scheel <ascheel@gitlab.com>

ianlancetaylor · 2025-02-13T21:47:26Z

I filed #71732 for one take on this.

* Update to Go 1.24.0 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix reflect comparisons due to sync.Map changes There's a new sync.Map implementation controllable via GOEXPERIMENT value. In the test use cases, we likely don't care about the strict reflect equality so we can swap to an alternative comparison implementation. Other uses of reflect.DeepEqual(...) at runtime don't appear to operate over sync.Map instances and so aren't affected. See also: https://tip.golang.org/doc/go1.24#syncpkgsync See also: golang/go#71702 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix certificate parsing, issuing bugs Go now forbids creating 512-bit RSA keys and uses x509.Certificate.Policies as the field for policy identifiers. See also: https://go.dev/doc/go1.24#cryptorsapkgcryptorsa See also: https://go.dev/doc/go1.24#cryptox509pkgcryptox509 Signed-off-by: Alexander Scheel <ascheel@gitlab.com> * Fix additional uses of formatters with constants Signed-off-by: Alexander Scheel <ascheel@gitlab.com> --------- Signed-off-by: Alexander Scheel <ascheel@gitlab.com>

gopherbot added the compiler/runtime label Feb 13, 2025

cipherboy mentioned this issue Feb 13, 2025

Update to Go 1.24.0 openbao/openbao#1000

Merged

gabyhelp added the BugReport label Feb 13, 2025

seankhliao closed this as not planned Feb 13, 2025

ianlancetaylor mentioned this issue Feb 13, 2025

proposal: cmd/vet: reject uses of reflect.DeepEqual on imported types with unexported fields #71732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: Map - reflect.DeepEqual regression between Go 1.23 and 1.24 #71702

sync: Map - reflect.DeepEqual regression between Go 1.23 and 1.24 #71702

cipherboy commented Feb 13, 2025 •

edited

Loading

gabyhelp commented Feb 13, 2025

seankhliao commented Feb 13, 2025

cipherboy commented Feb 13, 2025

seankhliao commented Feb 13, 2025

cipherboy commented Feb 13, 2025

ianlancetaylor commented Feb 13, 2025

sync: Map - reflect.DeepEqual regression between Go 1.23 and 1.24 #71702

sync: Map - reflect.DeepEqual regression between Go 1.23 and 1.24 #71702

Comments

cipherboy commented Feb 13, 2025 • edited Loading

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

gabyhelp commented Feb 13, 2025

seankhliao commented Feb 13, 2025

cipherboy commented Feb 13, 2025

seankhliao commented Feb 13, 2025

cipherboy commented Feb 13, 2025

ianlancetaylor commented Feb 13, 2025

cipherboy commented Feb 13, 2025 •

edited

Loading

Output of `go env` in your module/workspace: