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: improve documentation about how verbs work when printing pointers #21409

Closed
dtjackson opened this Issue Aug 11, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@dtjackson

dtjackson commented Aug 11, 2017

Please answer these questions before submitting your issue. Thanks!

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

go1.9rc2 linux/amd64

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

GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

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

package main

import (
"fmt"
)

func main() {
i := 37
p := &i
fmt.Printf("Why: %d", p)
}

What did you expect to see?

I expected to see some sort of error / warning about the implicit conversion of a pointer type into an integer format verb %d. I expected the computer to help me catch the mistake of not derefrencing p to get the expected integer.

What did you see instead?

No error or warning, and the output contains the address of the pointer printed in base10.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Aug 11, 2017

Contributor

Looking at the the docs this seems like a bug in fmt.

Contributor

cznic commented Aug 11, 2017

Looking at the the docs this seems like a bug in fmt.

@ianlancetaylor ianlancetaylor changed the title from Warn on implicit conversion of pointer type to integer in Printf to cmd/vet: warn on using integer format to print pointer in Printf Aug 11, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Aug 11, 2017

Contributor

We can't change fmt, but we could change cmd/vet.

Contributor

ianlancetaylor commented Aug 11, 2017

We can't change fmt, but we could change cmd/vet.

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 11, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor
Contributor

ianlancetaylor commented Aug 11, 2017

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Aug 11, 2017

Contributor

We can't change fmt, but we could change cmd/vet.

Even when the behavior is not supported by the documentation?

Contributor

cznic commented Aug 11, 2017

We can't change fmt, but we could change cmd/vet.

Even when the behavior is not supported by the documentation?

@dtjackson

This comment has been minimized.

Show comment
Hide comment
@dtjackson

dtjackson Aug 11, 2017

If the behavior is deemed correct, can the documentation be adjusted to account for it?

dtjackson commented Aug 11, 2017

If the behavior is deemed correct, can the documentation be adjusted to account for it?

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Aug 11, 2017

Contributor

In some cases we can adjust the doc, but in a case like this I personally think that the chances of breaking a currently working program are too high.

Contributor

ianlancetaylor commented Aug 11, 2017

In some cases we can adjust the doc, but in a case like this I personally think that the chances of breaking a currently working program are too high.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Aug 11, 2017

Contributor

Sorry, I mean in some cases we can adjust the code (but I think this case is too risky).

We can always adjust the doc, and, yes, we should do so.

Contributor

ianlancetaylor commented Aug 11, 2017

Sorry, I mean in some cases we can adjust the code (but I think this case is too risky).

We can always adjust the doc, and, yes, we should do so.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 11, 2017

Contributor

I can't find it now but I seem to remember this was an actual change that went in at some point, and that the docs need to catch up to the change. I might just be confused though.

Contributor

robpike commented Aug 11, 2017

I can't find it now but I seem to remember this was an actual change that went in at some point, and that the docs need to catch up to the change. I might just be confused though.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Aug 12, 2017

Contributor

According to the documentation, section 'Format errors', passing a pointer to the %d verb is the case of invalid argument, %d is documented to accept only integers - and it should produce %!d(pointer=0xdeafbeef) per docs.

Can someone please explain to me why adjusting the docs instead of fixing the bug is considered? IMO
it breaks the Go 1 compatibility promise, while fixing the bug does not, by definition.

Also, for the %d verb to accept pointers looks to me like a C-ism and the OP readily demostrates why it's good that Go avoids them.

Contributor

cznic commented Aug 12, 2017

According to the documentation, section 'Format errors', passing a pointer to the %d verb is the case of invalid argument, %d is documented to accept only integers - and it should produce %!d(pointer=0xdeafbeef) per docs.

Can someone please explain to me why adjusting the docs instead of fixing the bug is considered? IMO
it breaks the Go 1 compatibility promise, while fixing the bug does not, by definition.

Also, for the %d verb to accept pointers looks to me like a C-ism and the OP readily demostrates why it's good that Go avoids them.

@martisch

This comment has been minimized.

Show comment
Hide comment
@martisch

martisch Aug 13, 2017

Member

AFAICT %d has been supported for pointers since go1.0:
https://github.com/golang/go/blob/release-branch.go1/src/pkg/fmt/print.go#L589

The verbs b, o, x, X are also supported on pointers and not listed explicitly in the pointer section.

We have refrained from changing other behavior in fmt after go1 to avoid breaking code even if it is inconsistent: e.g. byte vs uint8 printing for types and # not always taking into account padding.

This is i think rather a documentation bug than a bug in fmt as go1 code clearly intended these verbs to work on pointers but did not document it.

Therefore, i would suggest we document the support for other verbs than p on pointers more explicitly then removing the b, o, d, x, X verb support for pointers.

@robike maybe this is the change you were searching for:
fmt: honor integer radix formats (%d etc.) for pointers
https://codereview.appspot.com/6441152
#3936

this changed the behavior of %d on pointers for go1.1 but did not introduce the %d support for pointers.

Member

martisch commented Aug 13, 2017

AFAICT %d has been supported for pointers since go1.0:
https://github.com/golang/go/blob/release-branch.go1/src/pkg/fmt/print.go#L589

The verbs b, o, x, X are also supported on pointers and not listed explicitly in the pointer section.

We have refrained from changing other behavior in fmt after go1 to avoid breaking code even if it is inconsistent: e.g. byte vs uint8 printing for types and # not always taking into account padding.

This is i think rather a documentation bug than a bug in fmt as go1 code clearly intended these verbs to work on pointers but did not document it.

Therefore, i would suggest we document the support for other verbs than p on pointers more explicitly then removing the b, o, d, x, X verb support for pointers.

@robike maybe this is the change you were searching for:
fmt: honor integer radix formats (%d etc.) for pointers
https://codereview.appspot.com/6441152
#3936

this changed the behavior of %d on pointers for go1.1 but did not introduce the %d support for pointers.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Aug 13, 2017

Contributor

Thanks for the link. It makes clear that supporting %d and other verbs for pointers is intentional. IIUC now, it's undocumented behavior since Go 1. Programs relying on undocumented behavior are not protected by the Go 1 compatibility promise.

Contributor

cznic commented Aug 13, 2017

Thanks for the link. It makes clear that supporting %d and other verbs for pointers is intentional. IIUC now, it's undocumented behavior since Go 1. Programs relying on undocumented behavior are not protected by the Go 1 compatibility promise.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 14, 2017

Contributor

@martisch Yes, that's the one I was thinking of. I agree it's a documentation issue at this point.

Contributor

robpike commented Aug 14, 2017

@martisch Yes, that's the one I was thinking of. I agree it's a documentation issue at this point.

@robpike robpike self-assigned this Aug 14, 2017

@robpike robpike changed the title from cmd/vet: warn on using integer format to print pointer in Printf to fmt: improve documentation about how verbs work when printing pointers Aug 14, 2017

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Aug 28, 2017

Change https://golang.org/cl/59412 mentions this issue: fmt: document verbs %b %d %o %x %X for printing pointers

gopherbot commented Aug 28, 2017

Change https://golang.org/cl/59412 mentions this issue: fmt: document verbs %b %d %o %x %X for printing pointers

@gopherbot gopherbot closed this in a4140b7 Aug 28, 2017

@golang golang locked and limited conversation to collaborators Aug 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.