math/big: SetString does not handle malformed input correctly

[Tom-Coates]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.5 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tomc/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.5/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.5/libexec/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT=""
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Using SetString to convert "4/3/2" to a *big.Rat claimed to succeed (which I think is the wrong behaviour) and returned 4/3 as the result (which is definitely the wrong behaviour)

https://play.golang.org/p/6iUww8dSlJ

What did you expect to see?

I expected the boolean ok in

_, ok := x.SetString("4/3/2")

to be false.

What did you see instead?

ok was true, and x was set to the rational number 4/3.

[quentinmit]

This seems to be a limitation of nat.scan, which is called by SetString to read the denominator. It stops scanning at the first non-digit character, without returning an error.

SetString does check for extra characters after a decimal number, so one could argue it should also check after parsing a rational number. That said, we are also in the unfortunate place where there might be someone relying on this behavior. I suspect the best we can do is to simply document that SetString ignores any characters after the fraction.

/cc @griesemer

[griesemer]

I would consider this a bug. For instance, Int.SetString checks that we have consumed all string characters. It looks like this is missing here, and the intent is definitively the same. The behavior should be consistent across all SetString methods.

[Tom-Coates]

Also the current behaviour is mathematically wrong. See

https://play.golang.org/p/kULH591JrR

The rational number determined by the string "4/3/2" should probably be 2/3
-- being (4/3)/2 -- or could conceivably be 8/3 -- being 4/(3/2) -- but
certainly shouldn't be 4/3. Similarly, the rational number determined by
the string "5/2.5" should probably be undefined -- because the floating
point value 2.5 probably shouldn't be considered to be exact -- or could
conceivably be 2, but certainly shouldn't be 5/2. And the rational number
determined by the string "6/5abc" should probably be undefined, but
currently is 6/5.

So I feel that this is a bug.

Tom Coates
Professor of Mathematics
Imperial College London

[griesemer]

@TomCoates8161 Agreed. The issue is that the parsing stops once a character is encountered that cannot possibly be part of a (simple) denominator. SetString should really return an error.

[gopherbot]

CL https://golang.org/cl/30472 mentions this issue.