-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: report printf calls with non-const format and no args #60529
Comments
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 |
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. |
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. |
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. |
The cmd/vet in Go 1.24 reports printf calls with non-const format and no args, causing one test in logp to fail. See golang/go#60529. ``` $ go install golang.org/dl/gotip@latest $ gotip download $ gotip test ./logp -count=1 logp/core_test.go:714:7: non-constant format string in call to github.com/elastic/elastic-agent-libs/logp.Info FAIL github.com/elastic/elastic-agent-libs/logp [build failed] ```
The cmd/vet in Go 1.24 reports printf calls with non-const format and no args, causing one test in logp to fail. See golang/go#60529. ``` $ go install golang.org/dl/gotip@latest $ gotip download $ gotip test ./logp -count=1 logp/core_test.go:714:7: non-constant format string in call to github.com/elastic/elastic-agent-libs/logp.Info FAIL github.com/elastic/elastic-agent-libs/logp [build failed] ```
The cmd/vet in Go 1.24 reports printf calls with non-const format and no args, causing one test in logp to fail. See golang/go#60529. ``` $ go install golang.org/dl/gotip@latest $ gotip download $ gotip test ./logp -count=1 logp/core_test.go:714:7: non-constant format string in call to github.com/elastic/elastic-agent-libs/logp.Info FAIL github.com/elastic/elastic-agent-libs/logp [build failed] ```
``` $ make GO=gotip build gotip version go version devel go1.24-4865aadc Fri Nov 22 05:22:24 2024 +0000 linux/amd64 gofmt (simplify) vet dump_region/main.go:59:14: non-constant format string in call to fmt.Printf pkg/check/privilege.go:146:51: non-constant format string in call to github.com/pingcap/tidb-tools/pkg/check.NewError pkg/check/table_structure.go:130:19: non-constant format string in call to github.com/pingcap/tidb-tools/pkg/check.NewError pkg/check/table_structure.go:136:19: non-constant format string in call to github.com/pingcap/tidb-tools/pkg/check.NewError importer/config.go:73:14: non-constant format string in call to fmt.Printf make: *** [Makefile:86: check] Error 1 ``` Related: golang/go#60529
This vet change is new to Go 1.24 and still needs to be mentioned in Go 1.24 release notes, right? Reopening with a release blocker label to track that. |
Change https://go.dev/cl/631682 mentions this issue: |
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
We were incorrectly using 'fmt.Printf', 'fmt.Errorf' and 't.Logf' with non-template strings/no arguments. The fix to this is replace these calls with the non-suffixed variants. There are many users of 'fmt.Fprint' - too many to do by hand - so this replacement was resolved using 'sed': sed 's/Fprintf/Fprint/g' -i $(ag fmt.Fprintf -l) We then manually fix the 25 cases where 'fmt.Fprintf' is actually warranted and manually replaced the errant users of 'fmt.Errorf' and 't.Logf'. We also rework 'internal/acceptance/clients/clients.go' slightly to make the code a bit clearer. PS: This is apparently going to be an issue in go 1.24 (specifically in 'go vet') [1] so this is not just golangci-lint being annoying. @pierreprinetti, that's directed at you ;) [1] golang/go#60529 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
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: