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

math/big: DeepEqual return false when comparing Rat with SetString("0") and SetInt64(0) #50944

Closed
shuLhan opened this issue Feb 1, 2022 · 1 comment
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@shuLhan
Copy link
Contributor

shuLhan commented Feb 1, 2022

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

$ go version
go version devel go1.18-c6abf3ba19 Tue Feb 1 13:40:29 2022 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/ms/go/bin"
GOCACHE="/home/ms/.cache/go-build"
GOENV="/home/ms/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ms/go/pkg/mod"
GONOPROXY="github.com/tokenomy"
GONOSUMDB="github.com/tokenomy"
GOOS="linux"
GOPATH="/home/ms/go"
GOPRIVATE="github.com/tokenomy"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ms/opt/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ms/opt/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.18-c6abf3ba19 Tue Feb 1 13:40:29 2022"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/home/ms/src/go/src/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build203756167=/tmp/go-build -gno-record-gcc-switches"

What did you do?

After CL 24430, reflect.DeepEqual no longer returns true when comparing a *Rat built with (*Rat).SetString("0") with one built with (*Rat).SetInt64(0).
These should be equivalent, but because (*Rat).SetString does not call norm() when returning the zero value, the result of reflect.DeepEqual will be false.

One could suggest that developers should use (*Rat).Cmp instead of relying on reflect.DeepEqual, but if a (*Rat) is part of a
larger struct that is being compared, this can be cumbersome.

Given the following test scenario,

(0) Given the struct of R, S, and T that contains *Rat.
(1) Do the calculcation on R that cause the value of *Rat
    become 0.
(2) Convert the result to text, for example to JSON.
(3) Consume the result as S (by UnmarshalText).
(4) Prepare the expected value of T, since we know what to test
    and the result, we set Rat value in T to 0 using SetInt64(0).
(5) Compare the value of S with T.

The expected result should be S = T, but due to SetString return without calling norm, the result of DeepEqual(S, T) is always false.

A code snippet to simulate this scenario,

r1 := big.Rat{}
r2 := big.Rat{}

r1.SetString("0")
r2.SetInt64(0)

fmt.Printf("r1: %v\n", r1)
fmt.Printf("r2: %v\n", r2)

if !reflect.DeepEqual(&r1, &r2) {
	log.Printf("r1 != r2")
}

Playground: https://go.dev/play/p/RyS70bghhue

What did you expect to see?

r1: {{false []} {false [1]}}
r2: {{false []} {false [1]}}

What did you see instead?

r1: {{false []} {false []}} 0/1
r2: {{false []} {false [1]}} 0/1
2021/11/17 01:07:39 r1 != r2
@gopherbot
Copy link

Change https://golang.org/cl/364434 mentions this issue: math/big: call norm when returning success from Rat SetString

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 4, 2022
@toothrot toothrot added this to the Backlog milestone Feb 4, 2022
@golang golang locked and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

3 participants