From 19496731f956472d344b6693e7b6badcafbbab1c Mon Sep 17 00:00:00 2001 From: Tim King Date: Thu, 22 Apr 2021 13:46:04 -0700 Subject: [PATCH] passes/stdmethods: warn when an Is, As, or Unwrap has the wrong signature. Extend stdmethods to warn when a type implementing error has an Is, As, or Unwrap method with a different signature than the one expected by the errors package. For golang/go#40356 Change-Id: Ia196bb83a685340d6dab216b87c8a4aa2846f785 Reviewed-on: https://go-review.googlesource.com/c/tools/+/312829 Run-TryBot: Tim King gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Michael Matloob Trust: Bryan C. Mills --- go/analysis/passes/stdmethods/stdmethods.go | 17 ++++++++++++++ .../passes/stdmethods/testdata/src/a/a.go | 22 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/go/analysis/passes/stdmethods/stdmethods.go b/go/analysis/passes/stdmethods/stdmethods.go index 856c6ae0d81..64a28ac0b97 100644 --- a/go/analysis/passes/stdmethods/stdmethods.go +++ b/go/analysis/passes/stdmethods/stdmethods.go @@ -61,10 +61,12 @@ var Analyzer = &analysis.Analyzer{ // we let it go. But if it does have a fmt.ScanState, then the // rest has to match. var canonicalMethods = map[string]struct{ args, results []string }{ + "As": {[]string{"interface{}"}, []string{"bool"}}, // errors.As // "Flush": {{}, {"error"}}, // http.Flusher and jpeg.writer conflict "Format": {[]string{"=fmt.State", "rune"}, []string{}}, // fmt.Formatter "GobDecode": {[]string{"[]byte"}, []string{"error"}}, // gob.GobDecoder "GobEncode": {[]string{}, []string{"[]byte", "error"}}, // gob.GobEncoder + "Is": {[]string{"error"}, []string{"bool"}}, // errors.Is "MarshalJSON": {[]string{}, []string{"[]byte", "error"}}, // json.Marshaler "MarshalXML": {[]string{"*xml.Encoder", "xml.StartElement"}, []string{"error"}}, // xml.Marshaler "ReadByte": {[]string{}, []string{"byte", "error"}}, // io.ByteReader @@ -76,6 +78,7 @@ var canonicalMethods = map[string]struct{ args, results []string }{ "UnmarshalXML": {[]string{"*xml.Decoder", "xml.StartElement"}, []string{"error"}}, // xml.Unmarshaler "UnreadByte": {[]string{}, []string{"error"}}, "UnreadRune": {[]string{}, []string{"error"}}, + "Unwrap": {[]string{}, []string{"error"}}, // errors.Unwrap "WriteByte": {[]string{"byte"}, []string{"error"}}, // jpeg.writer (matching bufio.Writer) "WriteTo": {[]string{"=io.Writer"}, []string{"int64", "error"}}, // io.WriterTo } @@ -123,6 +126,14 @@ func canonicalMethod(pass *analysis.Pass, id *ast.Ident) { return } + // Special case: Is, As and Unwrap only apply when type + // implements error. + if id.Name == "Is" || id.Name == "As" || id.Name == "Unwrap" { + if recv := sign.Recv(); recv == nil || !implementsError(recv.Type()) { + return + } + } + // Do the =s (if any) all match? if !matchParams(pass, expect.args, args, "=") || !matchParams(pass, expect.results, results, "=") { return @@ -185,3 +196,9 @@ func matchParamType(expect string, actual types.Type) bool { // Overkill but easy. return typeString(actual) == expect } + +var errorType = types.Universe.Lookup("error").Type().Underlying().(*types.Interface) + +func implementsError(actual types.Type) bool { + return types.Implements(actual, errorType) +} diff --git a/go/analysis/passes/stdmethods/testdata/src/a/a.go b/go/analysis/passes/stdmethods/testdata/src/a/a.go index f660d321f5e..7f9e9ae648c 100644 --- a/go/analysis/passes/stdmethods/testdata/src/a/a.go +++ b/go/analysis/passes/stdmethods/testdata/src/a/a.go @@ -36,3 +36,25 @@ func (T) WriteTo(w io.Writer, more, args int) {} // ok - clearly not io.WriterTo type I interface { ReadByte() byte // want `should have signature ReadByte\(\) \(byte, error\)` } + +type V int // V does not implement error. + +func (V) As() T { return 0 } // ok - V is not an error +func (V) Is() bool { return false } // ok - V is not an error +func (V) Unwrap() int { return 0 } // ok - V is not an error + +type E int + +func (E) Error() string { return "" } // E implements error. + +func (E) As() {} // want `method As\(\) should have signature As\(interface{}\) bool` +func (E) Is() {} // want `method Is\(\) should have signature Is\(error\) bool` +func (E) Unwrap() {} // want `method Unwrap\(\) should have signature Unwrap\(\) error` + +type F int + +func (F) Error() string { return "" } // Both F and *F implement error. + +func (*F) As() {} // want `method As\(\) should have signature As\(interface{}\) bool` +func (*F) Is() {} // want `method Is\(\) should have signature Is\(error\) bool` +func (*F) Unwrap() {} // want `method Unwrap\(\) should have signature Unwrap\(\) error`