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

[LexRiver]

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]

btw why not add fmt.Printlnf also?

See previous discussion under #46190

[seankhliao]

cc @matloob @timothy-king

[timothy-king]

#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]

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

[joe-mann]

#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]

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]

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]

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]

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]

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]

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.

[adonovan]

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]

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

I think this wording works.

[zpavlinovic]

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]

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

[gopherbot]

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