From 86c5873b4804d5d9d184ac147f98e9652a0fa258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Tue, 26 Feb 2019 13:40:04 +0100 Subject: [PATCH] [release-branch.go1.12] go/analysis/passes/printf: fix big.Int false positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's possible to use a type which implements fmt.Formatter without importing fmt directly, if the type is imported from another package such as math/big. On top of that, it's possible to use printf-like functions without importing fmt directly, such as using testing.T.Logf. These two scenarios combined can lead to the printf check not finding the fmt.Formatter type, since it's not a direct dependency of the root package. fmt must still be in the import graph somewhere, so we could search for it via types.Package.Imports. However, at that point it's simpler to just look for the Format method manually via go/types. Fixes #30399. Change-Id: Id78454bb6a51b3c5e1bcb1984a7fbfb4a29a5be0 Reviewed-on: https://go-review.googlesource.com/c/163817 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Alan Donovan (cherry picked from commit 589c23e65e65055d47b9ad4a99723bc389136265) Reviewed-on: https://go-review.googlesource.com/c/tools/+/164657 Run-TryBot: Alan Donovan Reviewed-by: Brad Fitzpatrick --- go/analysis/passes/printf/printf.go | 28 ++++++++++++------- go/analysis/passes/printf/printf_test.go | 2 +- .../passes/printf/testdata/src/nofmt/nofmt.go | 10 +++++++ go/analysis/passes/printf/types.go | 2 +- 4 files changed, 30 insertions(+), 12 deletions(-) create mode 100644 go/analysis/passes/printf/testdata/src/nofmt/nofmt.go diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index c0265aaf..b73ff5e0 100644 --- a/go/analysis/passes/printf/printf.go +++ b/go/analysis/passes/printf/printf.go @@ -453,15 +453,23 @@ func printfNameAndKind(pass *analysis.Pass, call *ast.CallExpr) (fn *types.Func, } // isFormatter reports whether t satisfies fmt.Formatter. -// Unlike fmt.Stringer, it's impossible to satisfy fmt.Formatter without importing fmt. -func isFormatter(pass *analysis.Pass, t types.Type) bool { - for _, imp := range pass.Pkg.Imports() { - if imp.Path() == "fmt" { - formatter := imp.Scope().Lookup("Formatter").Type().Underlying().(*types.Interface) - return types.Implements(t, formatter) - } +// The only interface method to look for is "Format(State, rune)". +func isFormatter(typ types.Type) bool { + obj, _, _ := types.LookupFieldOrMethod(typ, false, nil, "Format") + fn, ok := obj.(*types.Func) + if !ok { + return false } - return false + sig := fn.Type().(*types.Signature) + return sig.Params().Len() == 2 && + sig.Results().Len() == 0 && + isNamed(sig.Params().At(0).Type(), "fmt", "State") && + types.Identical(sig.Params().At(1).Type(), types.Typ[types.Rune]) +} + +func isNamed(T types.Type, pkgpath, name string) bool { + named, ok := T.(*types.Named) + return ok && named.Obj().Pkg().Path() == pkgpath && named.Obj().Name() == name } // formatState holds the parsed representation of a printf directive such as "%3.*[4]d". @@ -753,7 +761,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, state *formatState) (o formatter := false if state.argNum < len(call.Args) { if tv, ok := pass.TypesInfo.Types[call.Args[state.argNum]]; ok { - formatter = isFormatter(pass, tv.Type) + formatter = isFormatter(tv.Type) } } @@ -831,7 +839,7 @@ func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool { typ := pass.TypesInfo.Types[e].Type // It's unlikely to be a recursive stringer if it has a Format method. - if isFormatter(pass, typ) { + if isFormatter(typ) { return false } diff --git a/go/analysis/passes/printf/printf_test.go b/go/analysis/passes/printf/printf_test.go index 937bbe7a..68453269 100644 --- a/go/analysis/passes/printf/printf_test.go +++ b/go/analysis/passes/printf/printf_test.go @@ -10,5 +10,5 @@ import ( func Test(t *testing.T) { testdata := analysistest.TestData() printf.Analyzer.Flags.Set("funcs", "Warn,Warnf") - analysistest.Run(t, testdata, printf.Analyzer, "a", "b") + analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt") } diff --git a/go/analysis/passes/printf/testdata/src/nofmt/nofmt.go b/go/analysis/passes/printf/testdata/src/nofmt/nofmt.go new file mode 100644 index 00000000..c323796f --- /dev/null +++ b/go/analysis/passes/printf/testdata/src/nofmt/nofmt.go @@ -0,0 +1,10 @@ +package b + +import ( + "math/big" + "testing" +) + +func formatBigInt(t *testing.T) { + t.Logf("%d\n", big.NewInt(4)) +} diff --git a/go/analysis/passes/printf/types.go b/go/analysis/passes/printf/types.go index 2eb6dc74..12286fd5 100644 --- a/go/analysis/passes/printf/types.go +++ b/go/analysis/passes/printf/types.go @@ -38,7 +38,7 @@ func matchArgTypeInternal(pass *analysis.Pass, t printfArgType, typ types.Type, } } // If the type implements fmt.Formatter, we have nothing to check. - if isFormatter(pass, typ) { + if isFormatter(typ) { return true } // If we can use a string, might arg (dynamically) implement the Stringer or Error interface?