proposal: cmd/vet: report printf calls with non-const format and no args #60529
Comments
adonovan
added
help wanted
gopls/analysis
Ok, I imagined one (having found https://github.com/golang/tools/blob/master/gopls/internal/lsp/cache/load.go#L378): The code would be improved by calling |
|
The technique you're forbidding is used to generate messages in different languages: (In fact, the I realize in this case you're arguing for the case with zero arguments, and my example is a bit contrived that way, but still, to me it feels too useful a technique to be forbidden outright. And there are other ways to do this, of course. Yet, one person's bad code is another's useful technique. |
|
@robpike Sorry, I'm not sure I understand your example. The proposed checker wouldn't report anything for that code. |
|
I guess the question is whether it should be OK to use I am not sure if this rule should be applied to |
mknyszek
added
the
NeedsInvestigation
Sorry, editing mistake. Example updated to drop the user name. And yes, you could use Println instead, but @findleyr gets to the nub: Do we force (through vet) people to use a different printer when there are no arguments? That's really what this is about. |
If we know "v" contains a verb this is clearly a bug. Maybe it is okay to warn if v may conditionally contain a verb?
For the handful I have looked at (~5), I am a bit wishy-washy on whether these are buggy. This particular pattern seems common:
(Another example I have seen is when |
That's true, but in all the examples I've looked it, it requires knowledge of a non-local and undocumented invariant that the args are percent-free. I would usually run a measurement over the module mirror corpus and then analyze the rate of true and false positives, but a quick experiment on a single repo (kubernetes) turned up plenty of findings, nearly all bugs. Here's the new logic: tools$ git diff
diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go
index 3235019258..a0ecc5d2eb 100644
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -536,6 +536,18 @@ type formatState struct {
// checkPrintf checks a call to a formatted print routine such as Printf.
func checkPrintf(pass *analysis.Pass, kind Kind, call *ast.CallExpr, fn *types.Func) {
+ // Calls to fmt.Printf(msg), with a non-constant
+ // format string and no arguments, seem to be a common
+ // mistake.
+ if len(call.Args) == 1 && pass.TypesInfo.Types[call.Args[0]].Value == nil {
+ pass.Reportf(call.Lparen,
+ "non-constant format string in call to %s (did you mean %s(\"%%s\", %s)?)",
+ fn.FullName(),
+ fn.Name(),
+ analysisutil.Format(pass.Fset, call.Args[0]))
+ return
+ }
+And here are a random 25 of its 182 findings: https://go-mod-viewer.appspot.com/k8s.io/kubernetes@v1.29.3/pkg/controller/statefulset/stateful_set_test.go#L894: non-constant format string in call to (*testing.common).Errorf (did you mean Errorf("%s", onPolicy("Expected set to stay at two replicas"))?) All but one are true positives: bugs (sometimes more than one in the same call!). The one false positive was of the form: var s = "benign"
fmt.Printf(s)So that's a 96% success rate. |
adonovan
changed the title
|
I was looking into https://go-mod-viewer.appspot.com/k8s.io/kubernetes@v1.29.3/pkg/controller/statefulset/stateful_set_test.go#L891: non-constant format string in call to (*testing.common).Errorf (did you mean Errorf("%s", onPolicy("Could not get scaled down StatefulSet: %v", err))?) The diagnostic given on |
call.Args[0] could be a function call. We should also check that if call.Args[0]'s type is a tuple, that it is 1 element. |
The code assumes that that the result of applying %s to policy (i.e. calling the String method of StatefulSetPersistentVolumeClaimRetentionPolicy, a protocol message) is percent-free. In this particular test, that is true, but in general, the string of this type contains arbitrary characters, and would cause this formatting to fail. So I would describe this as a latent bug just waiting for someone to add a new row to the test table.
Yes, the lesson is that non-const formats are often a mistake for any value of len(Args), but they are nearly always a mistake for len(Args)=0. |
rsc
changed the title
|
Side note but please do not add "did you mean" to error messages. Report the problem, then stop. |
|
This proposal has been added to the active column of the proposals project |
A common mistake not caught by the existing printf vet checker is
Printf(v), where v is a variable, when the user intendedPrintf("%s", v). If the value of v contains an unexpected percent sign, the program has a bug.I feel like I see this mistake in Google Go readability reviews at least once a week, and the Google corpus shows up hundreds of violations with what looks like close to 100% precision:
https://source.corp.google.com/search?q=%5B.%5DPrintf%5C(%5Ba-z%5D*%5C)%20lang:go&sq=
(Apologies, link to Google internal service.)
Printf and similar functions are usually called with a literal format string, followed by zero or more arguments. Occasionally the format string argument is a variable, either because preceding logic chooses between two alternative formats, or because the call appears in a wrapper function that takes both the format and its arguments as parameters, but in both those cases the format is followed by other arguments. It's hard to imagine any good reason one would call printf with a variable format string and no arguments.
We should make the printf checker report this error.
@timothy-king @findleyr
The text was updated successfully, but these errors were encountered: