math/big: document that shallow copies of big.Int/Rat/Float are not supported

[QuantumGhost]

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

go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/qg/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/qg/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/02/nfvdwz45077g85khfsv07khm0000gn/T/go-build594965128=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

go playground
Run this code under 1.10 and 1.11 respectively.

package main

import (
	"fmt"
	"math/big"
)

const Int32Prime = 47

func main() {
	int32Prime := big.NewInt(Int32Prime)
	inverse := *int32Prime

	inverse.ModInverse(&inverse, big.NewInt(2))
	if int32(int32Prime.Int64()) != Int32Prime {
		fmt.Println("Changed!")
	} else {
		fmt.Println("Not Changed")
	}
}

go 1.10 prints Not Changed
go 1.11 prints Changed!

What did you expect to see?

The value of int32Prime should not be changed.

What did you see instead?

The value of int32Prime changed.

[randall77]

You're depending on the behavior of shallow copies of big.Int, which I think is not supported.

inverse := *int32Prime

If instead you do:

var inverse big.Int
inverse.Set(int32Prime)

It should work correctly.

@griesemer : Is this documented anywhere? I didn't see it in the math/big package docs.

[griesemer]

This is working as intended; and @randall77 's suggested change would be the correct approach here.

The ModInverse implementation changed; in the past it created a new underlying slice to represent the result of ModInverse, and thus didn't change int32Prime's value (which shared the same underlying array). Now it does, and consequently, int32Prime changes as well.

Shallow copies are definitively not supported. I'll add documentation.

[QuantumGhost]

Thanks @griesemer @randall77.
I think it's better to mention this change in the release note, as it may break someone's code.

[griesemer]

I don't think this warrants a note in the release notes - besides that we fixed a bug in ModInverse. None of the math/big code operates on big.Int values; all operations take pointers. The implementation of a big.Int is completely opaque, and nowhere does it say that one might safely shallow copy it: In general, making shallow copies of exported types is not a good idea unless explicitly permitted. This is true for every package, not just math/big.

[QuantumGhost]

The problem is here:

1. big.Int does not have a word on how it should be copied.
2. There is no method named Copy or Clone (or some other meaningful names) that may warn the users how big.Int should be copied.
3. The shallow copy code (accidentally) works in go 1.10.

https://golang.org/doc/go1compat#expectations also says nothing about the compatibility issue of shallow copy, which IMHO means that if making a shallow copy works, it should continue work in all future releases in Go 1.

Anyway, I'll be more cautious when making shallow copies of exported types.

[griesemer]

@QuantumGhost You're right that the documentation probably should say how to properly make a copy of a big.Int - after all it's a (mostly) abstract data type and should support this operation. Which I believe this issue should be about.

Shallow copy may have worked for the specific operation and arguments, but in general shallow copying won't work at all with big.Int (or any of big.Rat or big.Float). It's really a lucky coincidence that it did work.

But as I said before, unless you know the exact details of an abstract data type, making a shallow copy is usually a bad idea. If we had a reliable mechanism to disallow copies, we'd be using it.

[randall77]

Note that shallow copies have been broken forever, and in ways that have nothing to do with ModInverse. For instance:

package main

import (
	"fmt"
	"math/big"
)

func main() {
	x := big.NewInt(7)

	yc := *x // shallow copy
	y := &yc
	y.Add(y, big.NewInt(1))

	fmt.Printf("%d %d\n", x, y)
}

This prints 8 8, not 7 8, in all versions of Go.

[QuantumGhost]

Thanks for mentioning this @randall77.

[gopherbot]

Change https://golang.org/cl/145537 mentions this issue: math/big: shallow copies of Int/Rat/Float are not supported (documentation)

[dgryski]

Is this the sort of thing that go vet could check for, similar to how it checks for passing a sync.Mutex by value?

[griesemer]

@dgryski Yes. Or one could just embed a sync.Mutex into the Int/Rat/Float objects. I thought about it, but I'm not sure it's necessary.

[randall77]

FYI adding Lock and Unlock methods to Int, Rat, and Float tricks vet into reporting shallow copies as errors. With those changes all.bash still passes. So the stdlib never makes this mistake...