From a576cf524670603350282ba181c0efbd384b0892 Mon Sep 17 00:00:00 2001 From: Sergey Glushchenko Date: Mon, 23 Mar 2020 19:11:59 +0000 Subject: [PATCH] go/analysis/passes/printf: warn against using %v verb in Error() methods go vet should warn against using %v verb in Error() methods just like it does for String(). Fixes golang/go#33884 Change-Id: I14e30aae316dc84adc62e2b2a0144cc157bb2698 GitHub-Last-Rev: 12240ac78d9c8afb12b014eff8f81ea0bf42471f GitHub-Pull-Request: golang/tools#214 Reviewed-on: https://go-review.googlesource.com/c/tools/+/223239 Reviewed-by: Michael Matloob --- go/analysis/passes/printf/printf.go | 54 +++++++++++-------- go/analysis/passes/printf/testdata/src/a/a.go | 53 ++++++++++++++++++ 2 files changed, 85 insertions(+), 22 deletions(-) diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go index eb597f63b5d..14f3a47610b 100644 --- a/go/analysis/passes/printf/printf.go +++ b/go/analysis/passes/printf/printf.go @@ -895,43 +895,51 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, state *formatState) (o pass.ReportRangef(call, "%s format %s has arg %s of wrong type %s", state.name, state.format, analysisutil.Format(pass.Fset, arg), typeString) return false } - if v.typ&argString != 0 && v.verb != 'T' && !bytes.Contains(state.flags, []byte{'#'}) && recursiveStringer(pass, arg) { - pass.ReportRangef(call, "%s format %s with arg %s causes recursive String method call", state.name, state.format, analysisutil.Format(pass.Fset, arg)) - return false + if v.typ&argString != 0 && v.verb != 'T' && !bytes.Contains(state.flags, []byte{'#'}) { + if methodName, ok := recursiveStringer(pass, arg); ok { + pass.ReportRangef(call, "%s format %s with arg %s causes recursive %s method call", state.name, state.format, analysisutil.Format(pass.Fset, arg), methodName) + return false + } } return true } // recursiveStringer reports whether the argument e is a potential -// recursive call to stringer, such as t and &t in these examples: +// recursive call to stringer or is an error, such as t and &t in these examples: // // func (t *T) String() string { printf("%s", t) } -// func (t T) String() string { printf("%s", t) } +// func (t T) Error() string { printf("%s", t) } // func (t T) String() string { printf("%s", &t) } -// -func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool { +func recursiveStringer(pass *analysis.Pass, e ast.Expr) (string, bool) { typ := pass.TypesInfo.Types[e].Type // It's unlikely to be a recursive stringer if it has a Format method. if isFormatter(typ) { - return false + return "", false } - // Does e allow e.String()? - obj, _, _ := types.LookupFieldOrMethod(typ, false, pass.Pkg, "String") - stringMethod, ok := obj.(*types.Func) - if !ok { - return false + // Does e allow e.String() or e.Error()? + strObj, _, _ := types.LookupFieldOrMethod(typ, false, pass.Pkg, "String") + strMethod, strOk := strObj.(*types.Func) + errObj, _, _ := types.LookupFieldOrMethod(typ, false, pass.Pkg, "Error") + errMethod, errOk := errObj.(*types.Func) + if !strOk && !errOk { + return "", false } - // Is the expression e within the body of that String method? - if stringMethod.Pkg() != pass.Pkg || !stringMethod.Scope().Contains(e.Pos()) { - return false + // Is the expression e within the body of that String or Error method? + var method *types.Func + if strOk && strMethod.Pkg() == pass.Pkg && strMethod.Scope().Contains(e.Pos()) { + method = strMethod + } else if errOk && errMethod.Pkg() == pass.Pkg && errMethod.Scope().Contains(e.Pos()) { + method = errMethod + } else { + return "", false } - sig := stringMethod.Type().(*types.Signature) + sig := method.Type().(*types.Signature) if !isStringer(sig) { - return false + return "", false } // Is it the receiver r, or &r? @@ -939,9 +947,11 @@ func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool { e = u.X // strip off & from &r } if id, ok := e.(*ast.Ident); ok { - return pass.TypesInfo.Uses[id] == sig.Recv() + if pass.TypesInfo.Uses[id] == sig.Recv() { + return method.Name(), true + } } - return false + return "", false } // isStringer reports whether the method signature matches the String() definition in fmt.Stringer. @@ -1065,8 +1075,8 @@ func checkPrint(pass *analysis.Pass, call *ast.CallExpr, fn *types.Func) { if isFunctionValue(pass, arg) { pass.ReportRangef(call, "%s arg %s is a func value, not called", fn.Name(), analysisutil.Format(pass.Fset, arg)) } - if recursiveStringer(pass, arg) { - pass.ReportRangef(call, "%s arg %s causes recursive call to String method", fn.Name(), analysisutil.Format(pass.Fset, arg)) + if methodName, ok := recursiveStringer(pass, arg); ok { + pass.ReportRangef(call, "%s arg %s causes recursive call to %s method", fn.Name(), analysisutil.Format(pass.Fset, arg), methodName) } } } diff --git a/go/analysis/passes/printf/testdata/src/a/a.go b/go/analysis/passes/printf/testdata/src/a/a.go index 6596209b285..0f3dc8df763 100644 --- a/go/analysis/passes/printf/testdata/src/a/a.go +++ b/go/analysis/passes/printf/testdata/src/a/a.go @@ -526,6 +526,59 @@ func (p *recursivePtrStringer) String() string { return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to String method" } +type recursiveError int + +func (s recursiveError) Error() string { + _ = fmt.Sprintf("%d", s) + _ = fmt.Sprintf("%#v", s) + _ = fmt.Sprintf("%v", s) // want "Sprintf format %v with arg s causes recursive Error method call" + _ = fmt.Sprintf("%v", &s) // want "Sprintf format %v with arg &s causes recursive Error method call" + _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error + return fmt.Sprintln(s) // want "Sprintln arg s causes recursive call to Error method" +} + +type recursivePtrError int + +func (p *recursivePtrError) Error() string { + _ = fmt.Sprintf("%v", *p) + _ = fmt.Sprint(&p) // ok; prints address + return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to Error method" +} + +type recursiveStringerAndError int + +func (s recursiveStringerAndError) String() string { + _ = fmt.Sprintf("%d", s) + _ = fmt.Sprintf("%#v", s) + _ = fmt.Sprintf("%v", s) // want "Sprintf format %v with arg s causes recursive String method call" + _ = fmt.Sprintf("%v", &s) // want "Sprintf format %v with arg &s causes recursive String method call" + _ = fmt.Sprintf("%T", s) // ok; does not recursively call String + return fmt.Sprintln(s) // want "Sprintln arg s causes recursive call to String method" +} + +func (s recursiveStringerAndError) Error() string { + _ = fmt.Sprintf("%d", s) + _ = fmt.Sprintf("%#v", s) + _ = fmt.Sprintf("%v", s) // want "Sprintf format %v with arg s causes recursive Error method call" + _ = fmt.Sprintf("%v", &s) // want "Sprintf format %v with arg &s causes recursive Error method call" + _ = fmt.Sprintf("%T", s) // ok; does not recursively call Error + return fmt.Sprintln(s) // want "Sprintln arg s causes recursive call to Error method" +} + +type recursivePtrStringerAndError int + +func (p *recursivePtrStringerAndError) String() string { + _ = fmt.Sprintf("%v", *p) + _ = fmt.Sprint(&p) // ok; prints address + return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to String method" +} + +func (p *recursivePtrStringerAndError) Error() string { + _ = fmt.Sprintf("%v", *p) + _ = fmt.Sprint(&p) // ok; prints address + return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to Error method" +} + // implements a String() method but with non-matching return types type nonStringerWrongReturn int