flag: panic in flag.PrintDefaults when zero value violates invariants #16694
Comments
|
I have also reproduced this with 1.7rc3 on linux/amd64, but I assume you probably don't care about earlier versions that much. |
bradfitz
changed the title
|
I'm inclined to just document this restriction. |
|
CL https://golang.org/cl/27003 mentions this issue. |
|
On Aug 14, 2016, at 20:37, Ian Lance Taylor notifications@github.com wrote:
–M |
|
The problem fixed by that commit: when the default value of a flag is the zero value, don't report the default value in the usage message. I don't want to unwind that commit (it's too late to unwind it for the 1.7 release in any case) and I definitely don't want to start catching a panic in that code. It's a subtle property in this case, but as you know in general we strongly encourage Go types to have valid zero values (https://golang.org/doc/effective_go.html#allocation_new). |
|
I argue that this is contrary to the spirit of the Go 1 guarantee, in that I had code that previously worked and did not depend on any unusual behaviour of the language or the library, and now my code breaks. My code isn't widely-used, so perhaps that doesn't matter. Granted the wording of the guarantee allows for undefined cases in the API. But since the API is defined to take a value implementing I understand this can't be fixed for the 1.7 release, but I claim it's still a bug, and that documentation isn't an adequate fix in the long term. |
|
I agree that it's a grey area. In practice we can not say that we will not break any Go program ever. That would make it impossible to ever make progress. In the discussion of issue #14058 we eventually agreed that we would proceed with that change even though it broke existing code. This problem is a consequence of that decision. |
|
CL https://golang.org/cl/27634 mentions this issue. |
Update #16694. Change-Id: Id6be1535d8a146b3dac3bee429ce407a51272032 Reviewed-on: https://go-review.googlesource.com/27634 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rob Pike <r@golang.org>
Summary: The Go flag package instantiats a zero of the flag type for rendering purposes (since 1.6 or 1.7, I think), and that leads to a nil indirection in the String implementation. Check for the nil first to avoid this. The Go team closed my golang/go#16694 as WAI, though I still argue this was a spurious violation of the compatibility guarantee. So we work around it. Reviewers: #core_team, zarko Reviewed By: #core_team, zarko Differential Revision: https://phabricator-dot-kythe-repo.appspot.com/D1242
(version details below)
Repro: https://play.golang.org/p/UU4V3BQDHA
Note that the playground is currently running an older version that doesn't have this problem. Download the code and use "go run" with a recent 1.7 release candidate (≥ rc3 has the break, as far as I can tell).
I believe this was introduced by 3659645
Summary: Given a user-defined type implementing the flag.Getter interface, register a value of the type using flag.Var. In some cases a panic occurs when flag.PrintDefaults tries to print a value for it. This appears to happen because the package is reflectively constructing an empty value of the implementing type—and that value may violate invariants of the original value.
I discovered this because I have a custom flag type whose package provides a constructor to ensure the value is always correct. The reflection bypasses this, of course, and constructs a value that crashes when its methods are called.
I can work around this by making sure my methods "work" when the invariants are violated, but this is a regression from go1.6 and arguably should be fixed. It's too subtle for a documentation patch.
$ go version
go version go1.7rc6 darwin/amd64
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/usr/local/share/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
The text was updated successfully, but these errors were encountered: