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

cmd/vet: suggest fix for "fmt.Println call has possible formatting directive" #47872

Closed
LexRiver opened this issue Aug 21, 2021 · 16 comments
Closed
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@LexRiver
Copy link

I'm about the warning message

fmt.Println("variable=%v", v)
fmt.Println call has possible formatting directive %v

In my opinion it's counter-intuitive. "Off course it has, that is what I want!" -)
Seems like I'm not alone:
https://stackoverflow.com/questions/53961617/call-has-possible-formatting-directive

I think error message should be something like this

fmt.Println call has possible formatting directive %v. Did you mean fmt.Printf?

btw why not add fmt.Printlnf also?

@fzipp
Copy link
Contributor

fzipp commented Aug 21, 2021

btw why not add fmt.Printlnf also?

See previous discussion under #46190

@seankhliao seankhliao changed the title More details for warning message "fmt.Println call has possible formatting directive" cmd/vet: suggest fix for "fmt.Println call has possible formatting directive" Aug 23, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 23, 2021
@seankhliao
Copy link
Member

cc @matloob @timothy-king

@timothy-king
Copy link
Contributor

#46190 (comment) brings up good reasons not to suggest fmt.Printf when starting from fmt.Println. I am worried folks will accept suggested changes as is. So I am hesitant to suggest fmt.Printf as a replacement.

In my opinion it's counter-intuitive.

Is there an alternative wording that you think might be clear?

@LexRiver
Copy link
Author

Something like
Formatting directive %v found in unformattable method call fmt.Println

@joe-mann
Copy link

#46190 (comment) brings up good reasons not to suggest fmt.Printf when starting from fmt.Println. I am worried folks will accept suggested changes as is. So I am hesitant to suggest fmt.Printf as a replacement.

I agree with this, and would be reluctant to accept the proposal for that reason. However maybe there's a way forward where vet could be a little more selective about when to offer the suggestion, Did you mean fmt.Printf?.

I would suggest that something like this, where the apparent value to be formatted is an identifier, is likely to be a valid candidate for replacement with fmt.Printf:

v := "value"
fmt.Println("variable=%v", v)

Whereas this, where the apparent value to be formatted is a (string) literal, is more likely to be the intended usage:

fmt.Println("variable=%v", "value")

So maybe vet could print fmt.Println call has possible formatting directive %v. Did you mean fmt.Printf? for the former, but the existing fmt.Println call has possible formatting directive %v in the latter scenario, could be a useful enhancement?

@zpavlinovic
Copy link
Contributor

Branching on cases adds more complexity to the checker and it is not clear that it is a better solution than a single clear message. I will keep this open for a while in hope that we can come up with such a message.

@tvanriper
Copy link

Okay, just to throw a completely different idea into the mix on the odd chance it's better than I imagine ('cause I don't necessarily think this is a decent idea), why not perform the formatting, appending a new line to the result?

Clearly, the caller intends to do formatting. And the issue happens often enough that people seem to expect this behavior. So why not embrace it, while still remaining true to the 'ln' component of the function's name?

@timothy-king
Copy link
Contributor

I am somewhat coming around @LexRiver's suggestion in #47872 (comment) . My suggested wording:

Formatting directive %v in argument to fmt.Println. fmt.Println does not support formatting directives.

It is less likely to be misinterpreted. It is not as pithy though. This is about twice an long as the current message:

fmt.Println call has possible formatting directive %v

@adonovan @zpavlinovic thoughts?

@timothy-king
Copy link
Contributor

And the issue happens often enough that people seem to expect this behavior. So why not embrace it, while still remaining true to the 'ln' component of the function's name?

This would break compatibility promise with existing fmt.Println calls that contain formatting directives. For example does fmt.Sprint("%v", "lorem ipsum") return %vlorem ipsum (current behavior) or "lorem ipsum" (proposed behavior)? I do not see a reading of the documentation https://pkg.go.dev/fmt#Sprint that returns "lorem ipsum". We want to be very cautious of breaking these other usages of fmt.

@zpavlinovic
Copy link
Contributor

Okay, just to throw a completely different idea into the mix on the odd chance it's better than I imagine ('cause I don't necessarily think this is a decent idea), why not perform the formatting, appending a new line to the result?

Clearly, the caller intends to do formatting. And the issue happens often enough that people seem to expect this behavior. So why not embrace it, while still remaining true to the 'ln' component of the function's name?

This seems to be suggesting the change of fmt.Println. I have a hard time believing this will be accepted. Formatting functions typically require f suffix and that approach has been discussed before (#46190).

@zpavlinovic
Copy link
Contributor

I am somewhat coming around @LexRiver's suggestion in #47872 (comment) . My suggested wording:

Formatting directive %v in argument to fmt.Println. fmt.Println does not support formatting directives.

It is less likely to be misinterpreted. It is not as pithy though. This is about twice an long as the current message:

fmt.Println call has possible formatting directive %v

@adonovan @zpavlinovic thoughts?

Why not just

fmt.Println does not support formatting directives.

I get that it indirectly suggests what is wrong, but IMO it is less confusing than the current message.

Or something like

fmt.Println call has possible formatting directive %v: fmt.Println("variable=%v", v)

if it is not too hard to get the string representation of the offending Println call.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
@adonovan
Copy link
Member

adonovan commented Apr 23, 2023

Summarizing approaches suggested above:

  • Suggest an automated fix. Rejected because we can't reliably infer the intent.
  • Add fmt.Printlnf. Rejected.
  • "Perform the formatting". Rejected: this would be an incompatible change.
  • Treat this as a documentation problem. Diagnostics now contain URLs. We could display these. (But these may be too verbose for many users' tastes, and longer than some of the reworded messages suggested in this thread.)
  • Reword the message. My two cents: fmt.Println call contains %v, possibly intended as Printf formatting directive?

@timothy-king
Copy link
Contributor

Reword the message. My two cents: fmt.Println call contains %v, possibly intended as Printf formatting directive?

I think this wording works.

@zpavlinovic
Copy link
Contributor

Reword the message. My two cents: fmt.Println call contains %v, possibly intended as Printf formatting directive?

I think this wording works.

Looks good to me as well.

@gopherbot
Copy link

Change https://go.dev/cl/491075 mentions this issue: go/analysis/passes/printf: update directive diagnostic message

@gopherbot
Copy link

Change https://go.dev/cl/493617 mentions this issue: go/analysis/passes/printf: reshorten diagnostic about %s in Println call

gopherbot pushed a commit to golang/tools that referenced this issue May 8, 2023
Originally the message said:

    Println call has possible formatting directive %v

For golang/go#47872 it was extended to:

    Println call contains %v, possibly intended as Printf formatting directive

The only new information content in this message is the mention of Printf.
All the rest of the wording is just longer words.
This CL preserves the Printf mention but goes back to the more concise text:

    Println call has possible Printf formatting directive %v

Fixes golang/go#47872.

Change-Id: Id514644d0cdbb4c983797e756be5e4444230cc1c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/493617
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants