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: behavior of pointer-like format arguments is undocumented #28865

Open
mvdan opened this issue Nov 19, 2018 · 8 comments
Open

fmt: behavior of pointer-like format arguments is undocumented #28865

mvdan opened this issue Nov 19, 2018 · 8 comments
Assignees
Labels

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Nov 19, 2018

Funcs and channels have one thing in common - they're all pointers underneath. This means one can use %p to print them, much like actual pointers.

It's documented that %b, %d, %o, %x and %X verbs also work with pointers, so those can also be used for both funcs and channels too.

But nowhere is it documented that channels and functions are treated as pointers when formatting. The closest thing is this piece:

The default format for %v is:
[...]
chan:                    %p

I think we should extend the documentation to clarify a number of things:

  • Funcs and channels are printed exactly like pointers
  • The default format for %v of a function is also %p
  • Maps are printed like pointers only with %p

That last point is especially tricky. For example, fmt.Printf("%p", map[bool]bool{true: false} will correctly print an address, but the same with %b or %d will fail as fmt recurses into the key and value elements, which aren't numbers.

I encountered this while fixing some bugs in vet, and I was surprised by how lightly fmt documents all this. A few of the behaviors surprised me too. I'll send a docs CL draft soon.

/cc @robpike @alandonovan @martisch

@mvdan mvdan added the Documentation label Nov 19, 2018
@mvdan
Copy link
Member Author

@mvdan mvdan commented Nov 19, 2018

This is even more confusing - here are some vet printf test cases:

// Can't print a function.
Printf("%d", someFunction) // want "Printf format %d arg someFunction is a func value, not called"
Printf("%v", someFunction) // want "Printf format %v arg someFunction is a func value, not called"
Println(someFunction)      // want "Println arg someFunction is a func value, not called"
Printf("%p", someFunction) // ok: maybe someone wants to see the pointer
Printf("%T", someFunction) // ok: maybe someone wants to see the type

So fmt allows printing a func via %d and %v, but vet does not. One of them is wrong. I'd hope it's fmt, in which case we need to clarify the documentation and fix that bug.

@mvdan mvdan self-assigned this Nov 19, 2018
@mvdan
Copy link
Member Author

@mvdan mvdan commented Nov 19, 2018

Will wait for some confirmation to send a CL, since I'm not 100% sure that vet is right and fmt is wrong about fmt.Printf("%d", someFunc). But I'm fairly positive, as vet has been running as part of go test for a while now. So if anyone depended on that behavior, they would have raised an issue by now.

@robpike
Copy link
Contributor

@robpike robpike commented Nov 19, 2018

Why do you trust vet over fmt? Vet simulates fmt; the simulation is wrong.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Nov 19, 2018

I'd hope that both fmt and vet implementations follow the fmt docs, which are ambiguous when it comes to formatting functions.

Generally I'd trust the fmt implementation over vet's, but vet has had this check since 2015 (43a7a9c) and we've enforced it via go test since 1.10, almost a year ago. So my point is that we could probably change fmt instead of changing vet, since go test should already have been failing on any programs disagreeing with vet.

That aside - @robpike, could you clarify if fmt's treatment of pointer-like types is as initially designed? If it is correct, then I'll just clarify the fmt docs and update vet to comply.

@robpike
Copy link
Contributor

@robpike robpike commented Nov 20, 2018

Well, it's tricky. Both are right in this case, I believe.
Fmt allows printing a function because you might need it for debugging.
Vet reports that printing a function without calling it is a likely mistake.

I wouldn't change either. When vet wasn't in the path to a build, this wasn't a problem. It's not clear it's a problem now, as in production code there is no likely story for printing a function, but it's certainly reasonable to print just about anything when debugging.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Nov 20, 2018

Well, it's tricky. Both are right in this case, I believe.

This issue is about clarifying the fmt docs to be clear and precise, in which case either fmt's implementation or vet's would be wrong :)

Fmt allows printing a function because you might need it for debugging.
Vet reports that printing a function without calling it is a likely mistake.

I think there's an important distinction to make here. A verb like %x seems reasonable to debug a function pointer. On the other hand, with verbs like %d and %v, I'd agree with vet that it's very likely a bug.

Isn't %x enough to debug function pointers? If so, we could change the fmt documentation to align with vet's warnings. If we do that, then for consistency I'd probably also make channels only be printable with %p, where %v would still default to %p.

If on the other hand we think that changing fmt's behavior is unnacceptable, I think we're left with a hard decision. Leaving fmt's documentation ambiguous is bad, and losing the useful vet warnings is bad.

@robpike
Copy link
Contributor

@robpike robpike commented Nov 20, 2018

Suggest what the documentation should say. I'd rather not change fmt's behavior; every time we do that it touches far more code than we expect and/or gets rolled back.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Dec 2, 2018

That's a good point; we already got bit by that recently :)

Perhaps all of these little fmt quirks could be fixed in a future v2 of the fmt package.

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
2 participants
You can’t perform that action at this time.