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

fmt: Shouldn't %q (and %s) treat nil as a valid value? #39886

Open
musiphil opened this issue Jun 26, 2020 · 9 comments
Open

fmt: Shouldn't %q (and %s) treat nil as a valid value? #39886

musiphil opened this issue Jun 26, 2020 · 9 comments
Milestone

Comments

@musiphil
Copy link

@musiphil musiphil commented Jun 26, 2020

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

$ go version
go version go1.14.4 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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="$HOME/.cache/go-build"
GOENV="$HOME/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="$HOME/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/google-golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
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=/tmp/go-build196177420=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

func printError(err error) {
	fmt.Printf("%v, %s, %q\n", err, err, err)
}

func main() {
	printError(nil)
	printError(io.EOF)
}

What did you expect to see?

<nil>, <nil>, <nil>
EOF, EOF, "EOF"

What did you see instead?

<nil>, %!s(<nil>), %!q(<nil>)
EOF, EOF, "EOF"

What's the rationale?

It's often useful to print error values enclosed with quotes in test failures:

if (err != nil) != tc.wantErr {
  t.Fatalf("Foo(...) returned %q, wantErr: %t", err, tc.wantErr) // problematic when err == nil
}

but %q (as well as %s) treats a nil value as a format error and prints %!q(<nil>) instead of plain <nil>.
Writing \"%v\" instead of %q avoids the ugly error output, but it doesn't handle escaping well, and it's not desirable to have even "<nil>" enclosed with quotes.
This leaves the plain %v as the only easy option (unless you are willing to have another if).

Wouldn't it be nice to have %q (and %s by extension) print just <nil>, as with %v, if the argument is nil?
%q and %s already handle an error argument as a string (through the Error method) if the argument is not nil, but having an error output for a nil argument limits its usefulness.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 26, 2020

@robpike
Copy link
Contributor

@robpike robpike commented Jun 27, 2020

I'm not convinced either way. It seems odd to me that a correct output from %q would not include quotes, but "<nil>" is also clearly wrong.

It may be wisest to do nothing for most types and do something special only for the error type, where people are used to seeing nil in test output and such.

But it's a fiddly thing to do.

@musiphil
Copy link
Author

@musiphil musiphil commented Jun 27, 2020

< and > in <nil> (kind of) serve as quotes in the nil case. As you said, using the "..." syntax (even "") would be clearly wrong, but <nil> signals that it's not even a valid string but just the plain nil.

As for the work, wouldn't it suffice to add 'q' and 's' in printArg?

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 27, 2020

I've also run into this issue in the past, and my initial suspicion was that was an issue in fmt package that needed to be fixed. I've spent more time investigating and considering what can be done, but in the end, I couldn't convince myself that there were any favorable changes that should be done in fmt itself. (Of course, it's possible I missed something that can be done.)

I instead modified the code to use %v in cases where the error might be nil, and reserved my use of %s (or %q) to cases where I've checked that it's not.

@dmitshur dmitshur added this to the Backlog milestone Jun 27, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 27, 2020

@robpike The catch is that since error is an interface type, a nil error gets converted to the empty interface as nil when calling fmt.Printf and friends. By the time that the fmt package sees the value, it's just a nil value of type interface{}, and the fact that it was once an error has been lost.

@robpike
Copy link
Contributor

@robpike robpike commented Jun 27, 2020

I'm not opposed to this change, and I'm also not enthusiastically in favor. My main concern at this point is the effect it will have on tests, as it could change golden output. It might be more disruptive than it's worth, but we can try it in the cycle if you like. As stated above, it's very easy to do. The challenge lies elsewhere.

@robpike
Copy link
Contributor

@robpike robpike commented Jun 27, 2020

@ianlancetaylor The package knows it's an error as it needs to check for the Error method, so it could be done there. But it's simpler and easier to explain if the change, should it happen, is done at the top of printArg.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 27, 2020

@robpike I may be missing something but I think that in this case there is no way that the fmt functions can tell that the value was originally an error. https://play.golang.org/p/8xDEPCAqoFa

@robpike
Copy link
Contributor

@robpike robpike commented Jun 27, 2020

@ianlancetaylor I see what you mean. Let's make the conversation more confusing: https://play.golang.org/p/pVnlOIX1ArU

In any case, as I said most recently, it's probably best to do this for all types in printArg if we do it at all. I'd like some discussion about that, though, because it might be disruptive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.