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 <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Tim King <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

cmd/goyacc/yacc.go

@@ -606,7 +606,7 @@ outer:
 				}
 				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 @@ more:
 		}
 		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)
 		}
 	}
 

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

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) }

go/analysis/passes/printf/printf.go

@@ -372,64 +372,29 @@ var isPrint = stringSet{
 	"(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 @@ 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) {
-	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
 	}

go/analysis/passes/printf/printf_test.go

@@ -16,4 +16,5 @@ func Test(t *testing.T) {
 	printf.Analyzer.Flags.Set("funcs", "Warn,Warnf")
 
 	analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt", "typeparams")
+	analysistest.RunWithSuggestedFixes(t, testdata, printf.Analyzer, "fix")
 }

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`
+}

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`
+}

go/analysis/passes/tests/tests.go

@@ -6,7 +6,6 @@ package tests
 
 import (
 	_ "embed"
-	"fmt"
 	"go/ast"
 	"go/token"
 	"go/types"
@@ -184,13 +183,13 @@ func checkAddCalls(pass *analysis.Pass, fn *ast.FuncDecl, params *types.Tuple) {
 				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 @@ func validateFuzzArgs(pass *analysis.Pass, params *types.Tuple, expr ast.Expr) b
 					}
 				}
 			}
-			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
 		}
 	}