From 9f450b061cce9ade250237ffe62343132e90d69d Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 30 Jan 2025 16:52:13 +0000 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley --- go/analysis/passes/printf/printf.go | 48 ++++++++++----- go/analysis/passes/printf/printf_test.go | 20 +++++- .../printf/testdata/nonconst_go123.txtar | 61 +++++++++++++++++++ .../printf/testdata/nonconst_go124.txtar | 59 ++++++++++++++++++ .../passes/printf/testdata/src/fix/fix.go | 20 ------ .../printf/testdata/src/fix/fix.go.golden | 20 ------ .../printf/testdata/src/nonconst/nonconst.go | 23 +++++++ 7 files changed, 195 insertions(+), 56 deletions(-) create mode 100644 go/analysis/passes/printf/testdata/nonconst_go123.txtar create mode 100644 go/analysis/passes/printf/testdata/nonconst_go124.txtar delete mode 100644 go/analysis/passes/printf/testdata/src/fix/fix.go delete mode 100644 go/analysis/passes/printf/testdata/src/fix/fix.go.golden create mode 100644 go/analysis/passes/printf/testdata/src/nonconst/nonconst.go diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index b95e2fd6f1a..81600a283aa 100644 --- a/go/analysis/passes/printf/printf.go +++ b/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(), diff --git a/go/analysis/passes/printf/printf_test.go b/go/analysis/passes/printf/printf_test.go index 198cf6ec549..1ce9c28c103 100644 --- a/go/analysis/passes/printf/printf_test.go +++ b/go/analysis/passes/printf/printf_test.go @@ -5,10 +5,13 @@ 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) { @@ -16,6 +19,19 @@ func Test(t *testing.T) { 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") } diff --git a/go/analysis/passes/printf/testdata/nonconst_go123.txtar b/go/analysis/passes/printf/testdata/nonconst_go123.txtar new file mode 100644 index 00000000000..87982917d9e --- /dev/null +++ b/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` +} diff --git a/go/analysis/passes/printf/testdata/nonconst_go124.txtar b/go/analysis/passes/printf/testdata/nonconst_go124.txtar new file mode 100644 index 00000000000..34d944ce970 --- /dev/null +++ b/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) +} diff --git a/go/analysis/passes/printf/testdata/src/fix/fix.go b/go/analysis/passes/printf/testdata/src/fix/fix.go deleted file mode 100644 index f5c9f654165..00000000000 --- a/go/analysis/passes/printf/testdata/src/fix/fix.go +++ /dev/null @@ -1,20 +0,0 @@ -// 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 deleted file mode 100644 index 57e5bb7db91..00000000000 --- a/go/analysis/passes/printf/testdata/src/fix/fix.go.golden +++ /dev/null @@ -1,20 +0,0 @@ -// 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/printf/testdata/src/nonconst/nonconst.go b/go/analysis/passes/printf/testdata/src/nonconst/nonconst.go new file mode 100644 index 00000000000..40779123a52 --- /dev/null +++ b/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) +}