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 |
|
The Kubernetes results definitely look like mostly/all bugs, but we should try more of the ecosystem to estimate the false positive rate. Thanks. |
|
Note that this corresponds to Staticcheck's SA1006, which might skew your estimates. |
|
We should drop the 'did you mean' but otherwise it looks like high-quality signal. |
|
Have all remaining concerns about this proposal been addressed? The proposal is to report calls to printf-like functions in which the format is a non-constant string and there are no arguments. In the ecosystem these are overwhelmingly misuses where the caller has prepared arguments for a print-like function, not a printf-like function. |
|
By the way, while we should definitely drop "did you mean" from the analyzer diagnostic text, it would be completely reasonable to make that code a SuggestedFix in the Diagnostic for people using the analyzer in IDEs. |
Go 1.24 now has a vet check about this that causes `go test` to fail: golang/go#60529
Go 1.24 now has a vet check about this that causes `go test` to fail: golang/go#60529
Go 1.24 now has a vet check about this that causes go test to fail: golang/go#60529
|
Change https://go.dev/cl/645595 mentions this issue: |
The new check added in golang/go#60529 reports errors for non-constant format strings with no arguments. These are almost always bugs, but are often mild or inconsequential, and can be numerous in existing code bases. To reduce friction from this change, gate the new check on the implied language version. For golang/go#71485 Change-Id: I4926da2809dd14ba70ae530cd1657119f5377ad5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/645595 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Robert Findley <rfindley@google.com>
|
Change https://go.dev/cl/645697 mentions this issue: |
Since [this commit in golang.org/x/tools] govet complains about calls to string formatting calls where there is a non-constant formatting string. See the [original issue] for more information. [this commit in golang.org/x/tools]: golang/tools@a5df6ad [original issue]: golang/go#60529
…rrors for non-const format strings The new check added in golang/go#60529 reports errors for non-constant format strings with no arguments. These are almost always bugs, but are often mild or inconsequential, and can be numerous in existing code bases. To reduce friction from this change, gate the new check on the implied language version. For golang/go#71485 Change-Id: I4926da2809dd14ba70ae530cd1657119f5377ad5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/645595 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Robert Findley <rfindley@google.com> (cherry picked from commit 9f450b0) Reviewed-on: https://go-review.googlesource.com/c/tools/+/645697
This is for addressing the change in go1.24 golang/go#60529, which suppress the error on non-constant format and no args.
This is a new go vet analysis pass that fires under go 1.24: $ go1.24rc2 vet ./... # github.com/go-chi/chi/v5 # [github.com/go-chi/chi/v5] ./mux_test.go:236:12: non-constant format string in call to (*testing.common).Fatalf ... The strings would be parsed as format strings which is not intended. See golang/go#60529 for details.
This is a new go vet analysis pass that fires under go 1.24: $ go1.24rc2 vet ./... # github.com/go-chi/chi/v5 # [github.com/go-chi/chi/v5] ./mux_test.go:236:12: non-constant format string in call to (*testing.common).Fatalf ... The strings would be parsed as format strings which is not intended. See golang/go#60529 for details.
The "go vet -printf ./..." command, when run with Go 1.24 (and a matching Go version number in the "go.mod" file), now detects format strings that are not constants, as described in golang/go#60529. We have one such non-constant format string in the newToken() function of the "netrc" package, so we adjust it now to use a standard format string with a specifier verb. This issue was reported in: git-lfs/git-lfs#5968 (comment)
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: