go/analysis/passes/printf: report non-constant format, no args
Calls such as fmt.Printf(s), where s is non-constant and there
are no arguments to format, are invariably a mistake. This
CL causes the printf checker to report them, and to offer
a suggested fix of fmt.Printf("%s", s).
Also:
- tests
- docs
- fixes to existing violations in x/tools (3 bugs, 2 merely bad form).
- an ignore-tagged main file for the printf checker.
Fixes golang/go#60529
Change-Id: Ic7cc4f3e7487bc1549948fa03b5b0b27959948e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/585795
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/cmd/goyacc/yacc.go b/cmd/goyacc/yacc.go
index 5a8ede0..bc63954 100644
--- a/cmd/goyacc/yacc.go
+++ b/cmd/goyacc/yacc.go
@@ -606,7 +606,7 @@
}
j = chfind(2, tokname)
if j >= NTBASE {
- lerrorf(ruleline, "nonterminal "+nontrst[j-NTBASE].name+" illegal after %%prec")
+ lerrorf(ruleline, "nonterminal %s illegal after %%prec", nontrst[j-NTBASE].name)
}
levprd[nprod] = toklev[j]
t = gettok()
@@ -1565,7 +1565,7 @@
}
if pempty[i] != OK {
fatfl = 0
- errorf("nonterminal " + nontrst[i].name + " never derives any token string")
+ errorf("nonterminal %s never derives any token string", nontrst[i].name)
}
}
diff --git a/go/analysis/passes/printf/doc.go b/go/analysis/passes/printf/doc.go
index 85da834..eebf402 100644
--- a/go/analysis/passes/printf/doc.go
+++ b/go/analysis/passes/printf/doc.go
@@ -45,6 +45,18 @@
//
// log.Print("%d", 123) // log.Print call has possible formatting directive %d
//
+// Conversely, it also reports calls to Printf-like functions with a
+// non-constant format string and no other arguments:
+//
+// fmt.Printf(message) // non-constant format string in call to fmt.Printf
+//
+// Such calls may have been intended for the function's Print-like
+// counterpart: if the value of message happens to contain "%",
+// misformatting will occur. In this case, the checker additionally
+// suggests a fix to turn the call into:
+//
+// fmt.Printf("%s", message)
+//
// # Inferred printf wrappers
//
// Functions that delegate their arguments to fmt.Printf are
diff --git a/go/analysis/passes/printf/main.go b/go/analysis/passes/printf/main.go
new file mode 100644
index 0000000..2a0fb7a
--- /dev/null
+++ b/go/analysis/passes/printf/main.go
@@ -0,0 +1,20 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build ignore
+
+// The printf command applies the printf checker to the specified
+// packages of Go source code.
+//
+// Run with:
+//
+// $ go run ./go/analysis/passes/printf/main.go -- packages...
+package main
+
+import (
+ "golang.org/x/tools/go/analysis/passes/printf"
+ "golang.org/x/tools/go/analysis/singlechecker"
+)
+
+func main() { singlechecker.Main(printf.Analyzer) }
diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go
index 3235019..b3453f8 100644
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -372,64 +372,29 @@
"(testing.TB).Skipf": true,
}
-// formatString returns the format string argument and its index within
-// the given printf-like call expression.
-//
-// The last parameter before variadic arguments is assumed to be
-// a format string.
-//
-// The first string literal or string constant is assumed to be a format string
-// if the call's signature cannot be determined.
-//
-// If it cannot find any format string parameter, it returns ("", -1).
-func formatString(pass *analysis.Pass, call *ast.CallExpr) (format string, idx int) {
+// formatStringIndex returns the index of the format string (the last
+// non-variadic parameter) within the given printf-like call
+// expression, or -1 if unknown.
+func formatStringIndex(pass *analysis.Pass, call *ast.CallExpr) int {
typ := pass.TypesInfo.Types[call.Fun].Type
- if typ != nil {
- if sig, ok := typ.(*types.Signature); ok {
- if !sig.Variadic() {
- // Skip checking non-variadic functions.
- return "", -1
- }
- idx := sig.Params().Len() - 2
- if idx < 0 {
- // Skip checking variadic functions without
- // fixed arguments.
- return "", -1
- }
- s, ok := stringConstantArg(pass, call, idx)
- if !ok {
- // The last argument before variadic args isn't a string.
- return "", -1
- }
- return s, idx
- }
+ if typ == nil {
+ return -1 // missing type
}
-
- // Cannot determine call's signature. Fall back to scanning for the first
- // string constant in the call.
- for idx := range call.Args {
- if s, ok := stringConstantArg(pass, call, idx); ok {
- return s, idx
- }
- if pass.TypesInfo.Types[call.Args[idx]].Type == types.Typ[types.String] {
- // Skip checking a call with a non-constant format
- // string argument, since its contents are unavailable
- // for validation.
- return "", -1
- }
+ sig, ok := typ.(*types.Signature)
+ if !ok {
+ return -1 // ill-typed
}
- return "", -1
-}
-
-// stringConstantArg returns call's string constant argument at the index idx.
-//
-// ("", false) is returned if call's argument at the index idx isn't a string
-// constant.
-func stringConstantArg(pass *analysis.Pass, call *ast.CallExpr, idx int) (string, bool) {
- if idx >= len(call.Args) {
- return "", false
+ if !sig.Variadic() {
+ // Skip checking non-variadic functions.
+ return -1
}
- return stringConstantExpr(pass, call.Args[idx])
+ idx := sig.Params().Len() - 2
+ if idx < 0 {
+ // Skip checking variadic functions without
+ // fixed arguments.
+ return -1
+ }
+ return idx
}
// stringConstantExpr returns expression's string constant value.
@@ -536,10 +501,34 @@
// 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) {
- format, idx := formatString(pass, call)
- if idx < 0 {
- if false {
- pass.Reportf(call.Lparen, "can't check non-constant format in call to %s", fn.FullName())
+ idx := formatStringIndex(pass, call)
+ if idx < 0 || idx >= len(call.Args) {
+ return
+ }
+ formatArg := call.Args[idx]
+ format, ok := stringConstantExpr(pass, formatArg)
+ if !ok {
+ // Format string argument is non-constant.
+
+ // It is a common mistake to call fmt.Printf(msg) with a
+ // non-constant format string and no arguments:
+ // if msg contains "%", misformatting occurs.
+ // Report the problem and suggest a fix: fmt.Printf("%s", msg).
+ if idx == len(call.Args)-1 {
+ pass.Report(analysis.Diagnostic{
+ Pos: formatArg.Pos(),
+ End: formatArg.End(),
+ Message: fmt.Sprintf("non-constant format string in call to %s",
+ fn.FullName()),
+ SuggestedFixes: []analysis.SuggestedFix{{
+ Message: `Insert "%s" format string`,
+ TextEdits: []analysis.TextEdit{{
+ Pos: formatArg.Pos(),
+ End: formatArg.Pos(),
+ NewText: []byte(`"%s", `),
+ }},
+ }},
+ })
}
return
}
diff --git a/go/analysis/passes/printf/printf_test.go b/go/analysis/passes/printf/printf_test.go
index 853d861..3506fec 100644
--- a/go/analysis/passes/printf/printf_test.go
+++ b/go/analysis/passes/printf/printf_test.go
@@ -16,4 +16,5 @@
printf.Analyzer.Flags.Set("funcs", "Warn,Warnf")
analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt", "typeparams")
+ analysistest.RunWithSuggestedFixes(t, testdata, printf.Analyzer, "fix")
}
diff --git a/go/analysis/passes/printf/testdata/src/fix/fix.go b/go/analysis/passes/printf/testdata/src/fix/fix.go
new file mode 100644
index 0000000..f5c9f65
--- /dev/null
+++ b/go/analysis/passes/printf/testdata/src/fix/fix.go
@@ -0,0 +1,20 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests of the printf checker's suggested fixes.
+
+package fix
+
+import (
+ "fmt"
+ "log"
+ "os"
+)
+
+func nonConstantFormat(s string) { // #60529
+ fmt.Printf(s) // want `non-constant format string in call to fmt.Printf`
+ fmt.Printf(s, "arg")
+ fmt.Fprintf(os.Stderr, s) // want `non-constant format string in call to fmt.Fprintf`
+ log.Printf(s) // want `non-constant format string in call to log.Printf`
+}
diff --git a/go/analysis/passes/printf/testdata/src/fix/fix.go.golden b/go/analysis/passes/printf/testdata/src/fix/fix.go.golden
new file mode 100644
index 0000000..57e5bb7
--- /dev/null
+++ b/go/analysis/passes/printf/testdata/src/fix/fix.go.golden
@@ -0,0 +1,20 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests of the printf checker's suggested fixes.
+
+package fix
+
+import (
+ "fmt"
+ "log"
+ "os"
+)
+
+func nonConstantFormat(s string) { // #60529
+ fmt.Printf("%s", s) // want `non-constant format string in call to fmt.Printf`
+ fmt.Printf(s, "arg")
+ fmt.Fprintf(os.Stderr, "%s", s) // want `non-constant format string in call to fmt.Fprintf`
+ log.Printf("%s", s) // want `non-constant format string in call to log.Printf`
+}
diff --git a/go/analysis/passes/tests/tests.go b/go/analysis/passes/tests/tests.go
index f5e760c..5b45982 100644
--- a/go/analysis/passes/tests/tests.go
+++ b/go/analysis/passes/tests/tests.go
@@ -6,7 +6,6 @@
import (
_ "embed"
- "fmt"
"go/ast"
"go/token"
"go/types"
@@ -184,13 +183,13 @@
i := mismatched[0]
expr := call.Args[i]
t := pass.TypesInfo.Types[expr].Type
- pass.ReportRangef(expr, fmt.Sprintf("mismatched type in call to (*testing.F).Add: %v, fuzz target expects %v", t, params.At(i+1).Type()))
+ pass.ReportRangef(expr, "mismatched type in call to (*testing.F).Add: %v, fuzz target expects %v", t, params.At(i+1).Type())
} else if len(mismatched) > 1 {
var gotArgs, wantArgs []types.Type
for i := 0; i < len(call.Args); i++ {
gotArgs, wantArgs = append(gotArgs, pass.TypesInfo.Types[call.Args[i]].Type), append(wantArgs, params.At(i+1).Type())
}
- pass.ReportRangef(call, fmt.Sprintf("mismatched types in call to (*testing.F).Add: %v, fuzz target expects %v", gotArgs, wantArgs))
+ pass.ReportRangef(call, "mismatched types in call to (*testing.F).Add: %v, fuzz target expects %v", gotArgs, wantArgs)
}
}
return true
@@ -244,7 +243,7 @@
}
}
}
- pass.ReportRangef(exprRange, "fuzzing arguments can only have the following types: "+formatAcceptedFuzzType())
+ pass.ReportRangef(exprRange, "fuzzing arguments can only have the following types: %s", formatAcceptedFuzzType())
ok = false
}
}
