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: GoStringer checks should test non-pointer types for pointer-based GoStringer receiver #21230

Closed
jdef opened this issue Jul 31, 2017 · 7 comments

Comments

@jdef
Copy link

@jdef jdef commented Jul 31, 2017

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

go1.8.1

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vagrant/mesos-go-workspace"
GORACE=""
GOROOT="/usr/local/go-1.8.1"
GOTOOLDIR="/usr/local/go-1.8.1/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build763882381=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

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

What did you expect to see?

The GoStringer implementation invoked for all cases.

What did you see instead?

The GoStringer implementation was only invoked for pointer types.

@cznic
Copy link
Contributor

@cznic cznic commented Jul 31, 2017

The method set of T does not include methods defined on *T. Working as intended.

@jdef
Copy link
Author

@jdef jdef commented Jul 31, 2017

fmt's Stringer evaluation has the same problem.

It's counter-intuitive because if one invokes String() on a non-pointer receiver golang upgrades the receiver to a pointer in order to invoke the String() func.

Stringer example: https://play.golang.org/p/FRGf4klCBB

@martisch
Copy link
Contributor

@martisch martisch commented Aug 1, 2017

I do not think this request can be seen in isolation to the fmt package but the argument would apply to many other interfaces in the same way. Only changing how fmt works would also be inconsistent with other interface checks in the std lib.

That said the language spec https://golang.org/ref/spec#Method_sets notes.

The method set of the corresponding pointer type *T is the set of all methods declared with receiver *T or T (that is, it also contains the method set of T).

I do not see this (if possible at all) being added the other way around to solve this on a language level for go1 as that looks like a major language change that is not backwards compatible to me.

@jdef
Copy link
Author

@jdef jdef commented Aug 1, 2017

Using T as a receiver is one possible solution, but it forces pass by value semantics, which may have other consequences. For example, this approach is not compatible with a type T struct { sync.Mutex; x int } that needs to generate a string from data that's guarded by a sync.Mutex: it basically forces a lock-copy which is not safe.

I wouldn't want inconsistencies in the std lib. But I do want a language and std lib that are intuitive and mostly gotcha-free. The suggestion to use T as a receiver could work for the playground toys I've included here. But it doesn't seem like a solution that would scale to a more complex code base.

@cznic
Copy link
Contributor

@cznic cznic commented Aug 1, 2017

There's nothing to do here. If the GoString method is defined on a pointer receiver, then for fmt.Printf to use it, you have to pass a pointer to the instance. That's the only option.

@martisch
Copy link
Contributor

@martisch martisch commented Aug 1, 2017

I can not see how we can obtain a more consistent behavior with the suggestion.
Automatically using the Stringer, GoStringer, Formatter for the pointer will not always be the correct method to use and might be in itself unexpected just because its special in fmt package.

Additionally i think we cant even obtain the pointer arbitrary values to call Stringer on them as the reflect package which fmt uses does not seem to support that. The suggestion would also be a backwards incompatible change which does not seem to warrant the breakage it might cause.

For the struct with mutex used by Stringer to work we can either pass the pointer to T to fmt or the mutex could be referenced by a pointer within the struct.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 30, 2018

I don't think there is anything we can do here. Please comment if you disagree.

@golang golang locked and limited conversation to collaborators Mar 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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