From dd8190d4c737fd889da7df899bc102ef1458299a Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 9 Oct 2018 10:52:57 -0400 Subject: [PATCH] go/analysis/passes/stdmethods: split check out of vet Also, rename to stdmethods and add more tests. Change-Id: I09b65899dc02a8062f3ec1d909c2eae45472e236 Reviewed-on: https://go-review.googlesource.com/c/140761 Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- .../method.go => stdmethods/stdmethods.go} | 108 +++++++++++------- .../passes/stdmethods/stdmethods_test.go | 13 +++ .../passes/stdmethods/testdata/src/a/a.go | 23 ++++ go/analysis/passes/vet/testdata/method.go | 22 ---- 4 files changed, 103 insertions(+), 63 deletions(-) rename go/analysis/passes/{vet/method.go => stdmethods/stdmethods.go} (63%) create mode 100644 go/analysis/passes/stdmethods/stdmethods_test.go create mode 100644 go/analysis/passes/stdmethods/testdata/src/a/a.go delete mode 100644 go/analysis/passes/vet/testdata/method.go diff --git a/go/analysis/passes/vet/method.go b/go/analysis/passes/stdmethods/stdmethods.go similarity index 63% rename from go/analysis/passes/vet/method.go rename to go/analysis/passes/stdmethods/stdmethods.go index 38efd9b0136..840a1b9dba5 100644 --- a/go/analysis/passes/vet/method.go +++ b/go/analysis/passes/stdmethods/stdmethods.go @@ -1,30 +1,47 @@ -// +build ignore - // Copyright 2010 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 the code to check canonical methods. - -package main +package stdmethods import ( + "bytes" "fmt" "go/ast" "go/printer" + "go/token" + "go/types" "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" ) -func init() { - register("methods", - "check that canonically named methods are canonically defined", - checkCanonicalMethod, - funcDecl, interfaceType) -} +var Analyzer = &analysis.Analyzer{ + Name: "stdmethods", + Doc: `check signature of methods of well-known interfaces -type MethodSig struct { - args []string - results []string +Sometimes a type may be intended to satisfy an interface but may fail to +do so because of a mistake in its method signature. +For example, the result of this WriteTo method should be (int64, error), +not error, to satisfy io.WriterTo: + + type myWriterTo struct{...} + func (myWriterTo) WriteTo(w io.Writer) error { ... } + +This check ensures that each method whose name matches one of several +well-known interface methods from the standard library has the correct +signature for that interface. + +Checked method names include: + Format GobEncode GobDecode MarshalJSON MarshalXML + Peek ReadByte ReadFrom ReadRune Scan Seek + UnmarshalJSON UnreadByte UnreadRune WriteByte + WriteTo +`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, } // canonicalMethods lists the input and output types for Go methods @@ -43,7 +60,7 @@ type MethodSig struct { // method doesn't have a fmt.ScanState as its first argument, // we let it go. But if it does have a fmt.ScanState, then the // rest has to match. -var canonicalMethods = map[string]MethodSig{ +var canonicalMethods = map[string]struct{ args, results []string }{ // "Flush": {{}, {"error"}}, // http.Flusher and jpeg.writer conflict "Format": {[]string{"=fmt.State", "rune"}, []string{}}, // fmt.Formatter "GobDecode": {[]string{"[]byte"}, []string{"error"}}, // gob.GobDecoder @@ -63,22 +80,31 @@ var canonicalMethods = map[string]MethodSig{ "WriteTo": {[]string{"=io.Writer"}, []string{"int64", "error"}}, // io.WriterTo } -func checkCanonicalMethod(f *File, node ast.Node) { - switch n := node.(type) { - case *ast.FuncDecl: - if n.Recv != nil { - canonicalMethod(f, n.Name, n.Type) - } - case *ast.InterfaceType: - for _, field := range n.Methods.List { - for _, id := range field.Names { - canonicalMethod(f, id, field.Type.(*ast.FuncType)) +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + (*ast.InterfaceType)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + switch n := n.(type) { + case *ast.FuncDecl: + if n.Recv != nil { + canonicalMethod(pass, n.Name, n.Type) + } + case *ast.InterfaceType: + for _, field := range n.Methods.List { + for _, id := range field.Names { + canonicalMethod(pass, id, field.Type.(*ast.FuncType)) + } } } - } + }) + return nil, nil } -func canonicalMethod(f *File, id *ast.Ident, t *ast.FuncType) { +func canonicalMethod(pass *analysis.Pass, id *ast.Ident, t *ast.FuncType) { // Expected input/output. expect, ok := canonicalMethods[id.Name] if !ok { @@ -93,12 +119,12 @@ func canonicalMethod(f *File, id *ast.Ident, t *ast.FuncType) { } // Do the =s (if any) all match? - if !f.matchParams(expect.args, args, "=") || !f.matchParams(expect.results, results, "=") { + if !matchParams(pass, expect.args, args, "=") || !matchParams(pass, expect.results, results, "=") { return } // Everything must match. - if !f.matchParams(expect.args, args, "") || !f.matchParams(expect.results, results, "") { + if !matchParams(pass, expect.args, args, "") || !matchParams(pass, expect.results, results, "") { expectFmt := id.Name + "(" + argjoin(expect.args) + ")" if len(expect.results) == 1 { expectFmt += " " + argjoin(expect.results) @@ -106,15 +132,15 @@ func canonicalMethod(f *File, id *ast.Ident, t *ast.FuncType) { expectFmt += " (" + argjoin(expect.results) + ")" } - f.b.Reset() - if err := printer.Fprint(&f.b, f.fset, t); err != nil { - fmt.Fprintf(&f.b, "<%s>", err) + var buf bytes.Buffer + if err := printer.Fprint(&buf, pass.Fset, t); err != nil { + fmt.Fprintf(&buf, "<%s>", err) } - actual := f.b.String() + actual := buf.String() actual = strings.TrimPrefix(actual, "func") actual = id.Name + actual - f.Badf(id.Pos(), "method %s should have signature %s", actual, expectFmt) + pass.Reportf(id.Pos(), "method %s should have signature %s", actual, expectFmt) } } @@ -148,7 +174,7 @@ func typeFlatten(l []*ast.Field) []ast.Expr { } // Does each type in expect with the given prefix match the corresponding type in actual? -func (f *File) matchParams(expect []string, actual []ast.Expr, prefix string) bool { +func matchParams(pass *analysis.Pass, expect []string, actual []ast.Expr, prefix string) bool { for i, x := range expect { if !strings.HasPrefix(x, prefix) { continue @@ -156,7 +182,7 @@ func (f *File) matchParams(expect []string, actual []ast.Expr, prefix string) bo if i >= len(actual) { return false } - if !f.matchParamType(x, actual[i]) { + if !matchParamType(pass.Fset, pass.Pkg, x, actual[i]) { return false } } @@ -167,15 +193,15 @@ func (f *File) matchParams(expect []string, actual []ast.Expr, prefix string) bo } // Does this one type match? -func (f *File) matchParamType(expect string, actual ast.Expr) bool { +func matchParamType(fset *token.FileSet, pkg *types.Package, expect string, actual ast.Expr) bool { expect = strings.TrimPrefix(expect, "=") // Strip package name if we're in that package. - if n := len(f.file.Name.Name); len(expect) > n && expect[:n] == f.file.Name.Name && expect[n] == '.' { + if n := len(pkg.Name()); len(expect) > n && expect[:n] == pkg.Name() && expect[n] == '.' { expect = expect[n+1:] } // Overkill but easy. - f.b.Reset() - printer.Fprint(&f.b, f.fset, actual) - return f.b.String() == expect + var buf bytes.Buffer + printer.Fprint(&buf, fset, actual) + return buf.String() == expect } diff --git a/go/analysis/passes/stdmethods/stdmethods_test.go b/go/analysis/passes/stdmethods/stdmethods_test.go new file mode 100644 index 00000000000..33a0f9a8309 --- /dev/null +++ b/go/analysis/passes/stdmethods/stdmethods_test.go @@ -0,0 +1,13 @@ +package stdmethods_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/stdmethods" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, stdmethods.Analyzer, "a") +} diff --git a/go/analysis/passes/stdmethods/testdata/src/a/a.go b/go/analysis/passes/stdmethods/testdata/src/a/a.go new file mode 100644 index 00000000000..a007bbfb8c7 --- /dev/null +++ b/go/analysis/passes/stdmethods/testdata/src/a/a.go @@ -0,0 +1,23 @@ +// Copyright 2010 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. + +package a + +import "fmt" + +type T int + +func (T) Scan(x fmt.ScanState, c byte) {} // want "should have signature Scan" + +func (T) Format(fmt.State, byte) {} // want `should have signature Format\(fmt.State, rune\)` + +type U int + +func (U) Format(byte) {} // no error: first parameter must be fmt.State to trigger check + +func (U) GobDecode() {} // want `should have signature GobDecode\(\[\]byte\) error` + +type I interface { + ReadByte() byte // want "should have signature ReadByte" +} diff --git a/go/analysis/passes/vet/testdata/method.go b/go/analysis/passes/vet/testdata/method.go deleted file mode 100644 index 52b500df272..00000000000 --- a/go/analysis/passes/vet/testdata/method.go +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2010 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 for the canonical method checker. - -// This file contains the code to check canonical methods. - -package testdata - -import ( - "fmt" -) - -type MethodTest int - -func (t *MethodTest) Scan(x fmt.ScanState, c byte) { // ERROR "should have signature Scan" -} - -type MethodTestInterface interface { - ReadByte() byte // ERROR "should have signature ReadByte" -}