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 <matloob@golang.org>
diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go
index eb597f6..14f3a47 100644
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -895,43 +895,51 @@
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 @@
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 @@
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 6596209..0f3dc8d 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 @@
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
