go/analysis/passes/printf: suppress errors 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 <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

go/analysis/passes/printf/printf.go

@@ -24,6 +24,7 @@ import (
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/fmtstr"
 	"golang.org/x/tools/internal/typeparams"
+	"golang.org/x/tools/internal/versions"
 )
 
 func init() {
@@ -107,12 +108,12 @@ func (f *isWrapper) String() string {
 	}
 }
 
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass) (any, error) {
 	res := &Result{
 		funcs: make(map[*types.Func]Kind),
 	}
 	findPrintfLike(pass, res)
-	checkCall(pass)
+	checkCalls(pass)
 	return res, nil
 }
 
@@ -181,7 +182,7 @@ func maybePrintfWrapper(info *types.Info, decl ast.Decl) *printfWrapper {
 }
 
 // findPrintfLike scans the entire package to find printf-like functions.
-func findPrintfLike(pass *analysis.Pass, res *Result) (interface{}, error) {
+func findPrintfLike(pass *analysis.Pass, res *Result) (any, error) {
 	// Gather potential wrappers and call graph between them.
 	byObj := make(map[*types.Func]*printfWrapper)
 	var wrappers []*printfWrapper
@@ -408,20 +409,29 @@ func stringConstantExpr(pass *analysis.Pass, expr ast.Expr) (string, bool) {
 	return "", false
 }
 
-// checkCall triggers the print-specific checks if the call invokes a print function.
-func checkCall(pass *analysis.Pass) {
+// checkCalls triggers the print-specific checks for calls that invoke a print
+// function.
+func checkCalls(pass *analysis.Pass) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{
+		(*ast.File)(nil),
 		(*ast.CallExpr)(nil),
 	}
+
+	var fileVersion string // for selectively suppressing checks; "" if unknown.
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
-		call := n.(*ast.CallExpr)
-		fn, kind := printfNameAndKind(pass, call)
-		switch kind {
-		case KindPrintf, KindErrorf:
-			checkPrintf(pass, kind, call, fn.FullName())
-		case KindPrint:
-			checkPrint(pass, call, fn.FullName())
+		switch n := n.(type) {
+		case *ast.File:
+			fileVersion = versions.Lang(versions.FileVersion(pass.TypesInfo, n))
+
+		case *ast.CallExpr:
+			fn, kind := printfNameAndKind(pass, n)
+			switch kind {
+			case KindPrintf, KindErrorf:
+				checkPrintf(pass, fileVersion, kind, n, fn.FullName())
+			case KindPrint:
+				checkPrint(pass, n, fn.FullName())
+			}
 		}
 	})
 }
@@ -484,7 +494,7 @@ func isFormatter(typ types.Type) bool {
 }
 
 // checkPrintf checks a call to a formatted print routine such as Printf.
-func checkPrintf(pass *analysis.Pass, kind Kind, call *ast.CallExpr, name string) {
+func checkPrintf(pass *analysis.Pass, fileVersion string, kind Kind, call *ast.CallExpr, name string) {
 	idx := formatStringIndex(pass, call)
 	if idx < 0 || idx >= len(call.Args) {
 		return
@@ -498,7 +508,17 @@ func checkPrintf(pass *analysis.Pass, kind Kind, call *ast.CallExpr, name string
 		// non-constant format string and no arguments:
 		// if msg contains "%", misformatting occurs.
 		// Report the problem and suggest a fix: fmt.Printf("%s", msg).
-		if !suppressNonconstants && idx == len(call.Args)-1 {
+		//
+		// However, as described in golang/go#71485, this analysis can produce a
+		// significant number of diagnostics in existing code, and the bugs it
+		// finds are sometimes unlikely or inconsequential, and may not be worth
+		// fixing for some users. Gating on language version allows us to avoid
+		// breaking existing tests and CI scripts.
+		if !suppressNonconstants &&
+			idx == len(call.Args)-1 &&
+			fileVersion != "" && // fail open
+			versions.AtLeast(fileVersion, "go1.24") {
+
 			pass.Report(analysis.Diagnostic{
 				Pos: formatArg.Pos(),
 				End: formatArg.End(),

go/analysis/passes/printf/printf_test.go

@@ -5,17 +5,33 @@
 package printf_test
 
 import (
+	"path/filepath"
 	"testing"
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/printf"
+	"golang.org/x/tools/internal/testenv"
+	"golang.org/x/tools/internal/testfiles"
 )
 
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
 	printf.Analyzer.Flags.Set("funcs", "Warn,Warnf")
 
 	analysistest.Run(t, testdata, printf.Analyzer,
-		"a", "b", "nofmt", "typeparams", "issue68744", "issue70572")
-	analysistest.RunWithSuggestedFixes(t, testdata, printf.Analyzer, "fix")
+		"a", "b", "nofmt", "nonconst", "typeparams", "issue68744", "issue70572")
+}
+
+func TestNonConstantFmtString_Go123(t *testing.T) {
+	testenv.NeedsGo1Point(t, 23)
+
+	dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "nonconst_go123.txtar"))
+	analysistest.RunWithSuggestedFixes(t, dir, printf.Analyzer, "example.com/nonconst")
+}
+
+func TestNonConstantFmtString_Go124(t *testing.T) {
+	testenv.NeedsGo1Point(t, 24)
+
+	dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "nonconst_go124.txtar"))
+	analysistest.RunWithSuggestedFixes(t, dir, printf.Analyzer, "example.com/nonconst")
 }

go/analysis/passes/printf/testdata/nonconst_go123.txtar

@@ -0,0 +1,61 @@
+This test checks for the correct suppression (or activation) of the
+non-constant format string check (golang/go#60529), in a go1.23 module.
+
+See golang/go#71485 for details.
+
+-- go.mod --
+module example.com/nonconst
+
+go 1.23
+
+-- nonconst.go --
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+func _(s string) {
+	fmt.Printf(s)
+	fmt.Printf(s, "arg")
+	fmt.Fprintf(os.Stderr, s)
+	log.Printf(s)
+}
+
+-- nonconst_go124.go --
+//go:build go1.24
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+// With Go 1.24, the analyzer should be activated, as this is a go1.24 file.
+func _(s string) {
+	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`
+}
+
+-- nonconst_go124.go.golden --
+//go:build go1.24
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+// With Go 1.24, the analyzer should be activated, as this is a go1.24 file.
+func _(s string) {
+	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/printf/testdata/nonconst_go124.txtar

@@ -0,0 +1,59 @@
+This test checks for the correct suppression (or activation) of the
+non-constant format string check (golang/go#60529), in a go1.24 module.
+
+See golang/go#71485 for details.
+
+-- go.mod --
+module example.com/nonconst
+
+go 1.24
+
+-- nonconst.go --
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+func _(s string) {
+	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`
+}
+
+-- nonconst.go.golden --
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+func _(s string) {
+	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`
+}
+
+-- nonconst_go123.go --
+//go:build go1.23
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+// The analyzer should be silent, as this is a go1.23 file.
+func _(s string) {
+	fmt.Printf(s)
+	fmt.Printf(s, "arg")
+	fmt.Fprintf(os.Stderr, s)
+	log.Printf(s)
+}

go/analysis/passes/printf/testdata/src/nonconst/nonconst.go

@@ -0,0 +1,23 @@
+// 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 handling of non-constant
+// format strings (golang/go#60529).
+
+package nonconst
+
+import (
+	"fmt"
+	"log"
+	"os"
+)
+
+// As the language version is empty here, and the new check is gated on go1.24,
+// diagnostics are suppressed here.
+func nonConstantFormat(s string) {
+	fmt.Printf(s)
+	fmt.Printf(s, "arg")
+	fmt.Fprintf(os.Stderr, s)
+	log.Printf(s)
+}