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: compound object rules don't seem to apply recursively #28625

Open
mvdan opened this issue Nov 6, 2018 · 9 comments
Open

fmt: compound object rules don't seem to apply recursively #28625

mvdan opened this issue Nov 6, 2018 · 9 comments
Labels
Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Nov 6, 2018

https://golang.org/pkg/fmt/#hdr-Printing reads:

For compound objects, the elements are printed using these rules, recursively, laid out like this:

    struct:             {field0 field1 ...}
    array, slice:       [elem0 elem1 ...]
    maps:               map[key1:value1 key2:value2 ...]
    pointer to above:   &{}, &[], &map[]

It is specifically said that these rules apply recursively. However, take a look at this playground link: https://play.golang.org/p/CbrniH9q45I

It currently prints:

{x}
&{x}
{%!s(*main.T2=&{x})}
&{%!s(*main.T2=&{x})}

But I'd expect it to print:

{x}
&{x}
{&{x}}
&{&{x}}

Reading the docs carefully again, I can't find a reason why only top-level pointers would follow the "pointer to above" rule that's clearly documented.

It seems to me like either the code is wrong, or the docs need clarification. If only top-level pointers are supposed to follow the "pointer to above" rule, that should be made clear in the rules.

This issue is split from #27672. I initially thought this was a bug in vet, but then started wondering if this was a bug in fmt instead.

/cc @robpike @martisch @rogpeppe

@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 6, 2018
@martisch
Copy link
Contributor

martisch commented Nov 6, 2018

related: #27768 (comment)
I think we should point this out in the documentation.

@mvdan
Copy link
Member Author

mvdan commented Nov 6, 2018

You're right; I hadn't caught that issue as the title seemed unrelated.

Should we keep this issue open and amend the documentation?

@martisch
Copy link
Contributor

martisch commented Nov 6, 2018

I think the issues are slightly different (the other one started as %d not applying to pointers, which IIRC is WAI according to the documentation). So happy if this stays open and we can discuss if we should just update the documentation. With the other issue i wanted to reference the note that the implementation is deliberately not following pointers to arbitrary depth as there could be loops in pointer chains.

@deanveloper
Copy link

Those look like rules for %v, but your example uses %s

Output using %v: https://play.golang.org/p/G4sWTdsNIi0

{x}
&{x}
{0x40e150}
&{0x40e160}

Which makes a bit more sense. Since it's a pointer to a pointer, %v would print with &{%p}. Either way, I agree that the documentation should be more clear.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/147997 mentions this issue: cmd/vet: fix printf false negative with nested pointers

@mvdan
Copy link
Member Author

mvdan commented Nov 6, 2018

I don't think the rules only apply to %v - one can use them with %s just fine, as my playground program above shows.

@martisch
Copy link
Contributor

martisch commented Nov 6, 2018

As far as I understand the rules for compounds they should always apply. As far as I remember the implementation it will work for all verbs except for %T and %p as those are handled specially.

@robpike
Copy link
Contributor

robpike commented Nov 6, 2018

Every time we try to fix fmt to be more consistent or correct, we roll back the change because it breaks things. I suspect we should just leave it alone.

@mvdan
Copy link
Member Author

mvdan commented Nov 6, 2018

I don't imagine that clarifying the docs will break any program :)

@mvdan mvdan added Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 6, 2018
gopherbot pushed a commit that referenced this issue Nov 9, 2018
Pointers to compound objects (structs, slices, arrays, maps) are only
followed by fmt if the pointer is at the top level of an argument. This
is to minimise the chances of fmt running into loops.

However, vet did not follow this rule. It likely doesn't help that fmt
does not document that restriction well, which is being tracked in
 #28625.

Updates #27672.

Change-Id: Ie9bbd9b974eda5ab9a285986d207ef92fca4453e
Reviewed-on: https://go-review.googlesource.com/c/147997
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants