From a25a8d567b492f9a659e1e1838cc093d5bf278a0 Mon Sep 17 00:00:00 2001 From: "Matt T. Proud" Date: Wed, 8 Jul 2015 13:35:20 -0600 Subject: [PATCH] tools/cmd/vet: Create vet checks for examples. In spite of https://blog.golang.org/examples and http://golang.org/pkg/testing/#pkg-examples, a number of internal Go authors have found writing documentation examples to be problematic in the sense that the syntax is error-prone due to loose coupling with identifiers found in the source corpus. This commit introduces a suite of validations for documentation examples: Overall: - Correct suffices, if present - Niladic function argument and return signatures func Example() {} func ExampleF() {} - F exists func ExampleT() {} - T exists func ExampleT_M() {} - T exists - M exists within T Further, if the example is in `package foo_test`, vet attempts to resolve the respective lookups in `package foo`, if `package foo` exists (cf., `package stringutil_test`). Change-Id: Ifa13906363541ebf28325681b749b14b7f8b103d Reviewed-on: https://go-review.googlesource.com/11982 Reviewed-by: Andrew Gerrand --- cmd/vet/example.go | 120 +++++++++++++++++++ cmd/vet/main.go | 21 +++- cmd/vet/testdata/divergent/buf.go | 17 +++ cmd/vet/testdata/divergent/buf_test.go | 35 ++++++ cmd/vet/testdata/examples_test.go | 48 ++++++++ cmd/vet/testdata/incomplete/examples_test.go | 33 +++++ cmd/vet/vet_test.go | 73 ++++++++--- 7 files changed, 322 insertions(+), 25 deletions(-) create mode 100644 cmd/vet/example.go create mode 100644 cmd/vet/testdata/divergent/buf.go create mode 100644 cmd/vet/testdata/divergent/buf_test.go create mode 100644 cmd/vet/testdata/examples_test.go create mode 100644 cmd/vet/testdata/incomplete/examples_test.go diff --git a/cmd/vet/example.go b/cmd/vet/example.go new file mode 100644 index 0000000000..90a0aa04c5 --- /dev/null +++ b/cmd/vet/example.go @@ -0,0 +1,120 @@ +// Copyright 2015 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 main + +import ( + "go/ast" + "strings" + + "golang.org/x/tools/go/types" +) + +func init() { + register("example", + "check for common mistaken usages of documentation examples", + checkExample, + funcDecl) +} + +func isExampleSuffix(s string) bool { return strings.ToLower(s) == s } + +// checkExample walks the documentation example functions checking for common +// mistakes of misnamed functions, failure to map functions to existing +// identifiers, etc. +func checkExample(f *File, node ast.Node) { + if !f.IsTest() { + return + } + var ( + pkg = f.pkg + pkgName = pkg.typesPkg.Name() + scopes = []*types.Scope{pkg.typesPkg.Scope()} + lookup = func(name string) types.Object { + for _, scope := range scopes { + if o := scope.Lookup(name); o != nil { + return o + } + } + return nil + } + ) + if strings.HasSuffix(pkgName, "_test") { + // Treat 'package foo_test' as an alias for 'package foo'. + var ( + basePkg = strings.TrimSuffix(pkgName, "_test") + pkg = f.pkg + ) + for _, p := range pkg.typesPkg.Imports() { + if p.Name() == basePkg { + scopes = append(scopes, p.Scope()) + break + } + } + } + fn, ok := node.(*ast.FuncDecl) + if !ok { + // Ignore non-functions. + return + } + var ( + fnName = fn.Name.Name + report = func(format string, args ...interface{}) { f.Badf(node.Pos(), format, args...) } + ) + if fn.Recv != nil || !strings.HasPrefix(fnName, "Example") { + // Ignore methods and types not named "Example". + return + } + if params := fn.Type.Params; len(params.List) != 0 { + report("%s should be niladic", fnName) + } + if results := fn.Type.Results; results != nil && len(results.List) != 0 { + report("%s should return nothing", fnName) + } + if fnName == "Example" { + // Nothing more to do. + return + } + if filesRun && !includesNonTest { + // The coherence checks between a test and the package it tests + // will report false positives if no non-test files have + // been provided. + return + } + var ( + exName = strings.TrimPrefix(fnName, "Example") + elems = strings.SplitN(exName, "_", 3) + ident = elems[0] + obj = lookup(ident) + ) + if ident != "" && obj == nil { + // Check ExampleFoo and ExampleBadFoo. + report("%s refers to unknown identifier: %s", fnName, ident) + // Abort since obj is absent and no subsequent checks can be performed. + return + } + if elemCnt := strings.Count(exName, "_"); elemCnt == 0 { + // Nothing more to do. + return + } + mmbr := elems[1] + if ident == "" { + // Check Example_suffix and Example_BadSuffix. + if residual := strings.TrimPrefix(exName, "_"); !isExampleSuffix(residual) { + report("%s has malformed example suffix: %s", fnName, residual) + } + return + } + if !isExampleSuffix(mmbr) { + // Check ExampleFoo_Method and ExampleFoo_BadMethod. + if obj, _, _ := types.LookupFieldOrMethod(obj.Type(), true, obj.Pkg(), mmbr); obj == nil { + report("%s refers to unknown field or method: %s.%s", fnName, ident, mmbr) + } + } + if len(elems) == 3 && !isExampleSuffix(elems[2]) { + // Check ExampleFoo_Method_suffix and ExampleFoo_Method_Badsuffix. + report("%s has malformed example suffix: %s", fnName, elems[2]) + } + return +} diff --git a/cmd/vet/main.go b/cmd/vet/main.go index e4b68770b0..e659f14625 100644 --- a/cmd/vet/main.go +++ b/cmd/vet/main.go @@ -53,6 +53,12 @@ var experimental = map[string]bool{} // setTrueCount record how many flags are explicitly set to true. var setTrueCount int +var ( + dirsRun, filesRun bool + + includesNonTest bool +) + // A triState is a boolean that knows whether it has been set to either true or false. // It is used to identify if a flag appears; the standard boolean flag cannot // distinguish missing from unset. It also satisfies flag.Value. @@ -187,6 +193,8 @@ type File struct { checkers map[ast.Node][]func(*File, ast.Node) } +func (f *File) IsTest() bool { return strings.HasSuffix(f.name, "_test.go") } + func main() { flag.Usage = Usage flag.Parse() @@ -209,8 +217,6 @@ func main() { if flag.NArg() == 0 { Usage() } - dirs := false - files := false for _, name := range flag.Args() { // Is it a directory? fi, err := os.Stat(name) @@ -219,15 +225,18 @@ func main() { continue } if fi.IsDir() { - dirs = true + dirsRun = true } else { - files = true + filesRun = true + if !strings.HasSuffix(name, "_test.go") { + includesNonTest = true + } } } - if dirs && files { + if dirsRun && filesRun { Usage() } - if dirs { + if dirsRun { for _, name := range flag.Args() { walkDir(name) } diff --git a/cmd/vet/testdata/divergent/buf.go b/cmd/vet/testdata/divergent/buf.go new file mode 100644 index 0000000000..0efe0f838d --- /dev/null +++ b/cmd/vet/testdata/divergent/buf.go @@ -0,0 +1,17 @@ +// Test of examples with divergent packages. + +// Package buf ... +package buf + +// Buf is a ... +type Buf []byte + +// Append ... +func (*Buf) Append([]byte) {} + +func (Buf) Reset() {} + +func (Buf) Len() int { return 0 } + +// DefaultBuf is a ... +var DefaultBuf Buf diff --git a/cmd/vet/testdata/divergent/buf_test.go b/cmd/vet/testdata/divergent/buf_test.go new file mode 100644 index 0000000000..6b9cba3f01 --- /dev/null +++ b/cmd/vet/testdata/divergent/buf_test.go @@ -0,0 +1,35 @@ +// Test of examples with divergent packages. + +package buf_test + +func Example() {} // OK because is package-level. + +func Example_suffix() // OK because refers to suffix annotation. + +func Example_BadSuffix() // ERROR "Example_BadSuffix has malformed example suffix: BadSuffix" + +func ExampleBuf() // OK because refers to known top-level type. + +func ExampleBuf_Append() {} // OK because refers to known method. + +func ExampleBuf_Clear() {} // ERROR "ExampleBuf_Clear refers to unknown field or method: Buf.Clear" + +func ExampleBuf_suffix() {} // OK because refers to suffix annotation. + +func ExampleBuf_Append_Bad() {} // ERROR "ExampleBuf_Append_Bad has malformed example suffix: Bad" + +func ExampleBuf_Append_suffix() {} // OK because refers to known method with valid suffix. + +func ExampleDefaultBuf() {} // OK because refers to top-level identifier. + +func ExampleBuf_Reset() bool { return true } // ERROR "ExampleBuf_Reset should return nothing" + +func ExampleBuf_Len(i int) {} // ERROR "ExampleBuf_Len should be niladic" + +// "Puffer" is German for "Buffer". + +func ExamplePuffer() // ERROR "ExamplePuffer refers to unknown identifier: Puffer" + +func ExamplePuffer_Append() // ERROR "ExamplePuffer_Append refers to unknown identifier: Puffer" + +func ExamplePuffer_suffix() // ERROR "ExamplePuffer_suffix refers to unknown identifier: Puffer" diff --git a/cmd/vet/testdata/examples_test.go b/cmd/vet/testdata/examples_test.go new file mode 100644 index 0000000000..ca1dab722c --- /dev/null +++ b/cmd/vet/testdata/examples_test.go @@ -0,0 +1,48 @@ +// Test of examples. + +package testdata + +// Buf is a ... +type Buf []byte + +// Append ... +func (*Buf) Append([]byte) {} + +func (Buf) Reset() {} + +func (Buf) Len() int { return 0 } + +// DefaultBuf is a ... +var DefaultBuf Buf + +func Example() {} // OK because is package-level. + +func Example_suffix() // OK because refers to suffix annotation. + +func Example_BadSuffix() // ERROR "Example_BadSuffix has malformed example suffix: BadSuffix" + +func ExampleBuf() // OK because refers to known top-level type. + +func ExampleBuf_Append() {} // OK because refers to known method. + +func ExampleBuf_Clear() {} // ERROR "ExampleBuf_Clear refers to unknown field or method: Buf.Clear" + +func ExampleBuf_suffix() {} // OK because refers to suffix annotation. + +func ExampleBuf_Append_Bad() {} // ERROR "ExampleBuf_Append_Bad has malformed example suffix: Bad" + +func ExampleBuf_Append_suffix() {} // OK because refers to known method with valid suffix. + +func ExampleDefaultBuf() {} // OK because refers to top-level identifier. + +func ExampleBuf_Reset() bool { return true } // ERROR "ExampleBuf_Reset should return nothing" + +func ExampleBuf_Len(i int) {} // ERROR "ExampleBuf_Len should be niladic" + +// "Puffer" is German for "Buffer". + +func ExamplePuffer() // ERROR "ExamplePuffer refers to unknown identifier: Puffer" + +func ExamplePuffer_Append() // ERROR "ExamplePuffer_Append refers to unknown identifier: Puffer" + +func ExamplePuffer_suffix() // ERROR "ExamplePuffer_suffix refers to unknown identifier: Puffer" diff --git a/cmd/vet/testdata/incomplete/examples_test.go b/cmd/vet/testdata/incomplete/examples_test.go new file mode 100644 index 0000000000..445502b39e --- /dev/null +++ b/cmd/vet/testdata/incomplete/examples_test.go @@ -0,0 +1,33 @@ +// Test of examples. + +package testdata + +func Example() {} // OK because is package-level. + +func Example_suffix() // OK because refers to suffix annotation. + +func Example_BadSuffix() // OK because non-test package was excluded. No false positives wanted. + +func ExampleBuf() // OK because non-test package was excluded. No false positives wanted. + +func ExampleBuf_Append() {} // OK because non-test package was excluded. No false positives wanted. + +func ExampleBuf_Clear() {} // OK because non-test package was excluded. No false positives wanted. + +func ExampleBuf_suffix() {} // OK because refers to suffix annotation. + +func ExampleBuf_Append_Bad() {} // OK because non-test package was excluded. No false positives wanted. + +func ExampleBuf_Append_suffix() {} // OK because refers to known method with valid suffix. + +func ExampleBuf_Reset() bool { return true } // ERROR "ExampleBuf_Reset should return nothing" + +func ExampleBuf_Len(i int) {} // ERROR "ExampleBuf_Len should be niladic" + +// "Puffer" is German for "Buffer". + +func ExamplePuffer() // OK because non-test package was excluded. No false positives wanted. + +func ExamplePuffer_Append() // OK because non-test package was excluded. No false positives wanted. + +func ExamplePuffer_suffix() // OK because non-test package was excluded. No false positives wanted. diff --git a/cmd/vet/vet_test.go b/cmd/vet/vet_test.go index 6a09e3de4b..7508193659 100644 --- a/cmd/vet/vet_test.go +++ b/cmd/vet/vet_test.go @@ -22,23 +22,46 @@ const ( binary = "testvet.exe" ) +func CanRun(t *testing.T) bool { + // Plan 9 and Windows systems can't be guaranteed to have Perl and so can't run errchk. + switch runtime.GOOS { + case "plan9", "windows": + t.Skip("skipping test; no Perl on %q", runtime.GOOS) + return false + } + return true +} + +func Build(t *testing.T) { + // go build + cmd := exec.Command("go", "build", "-o", binary) + run(cmd, t) +} + +func Vet(t *testing.T, files []string) { + errchk := filepath.Join(runtime.GOROOT(), "test", "errchk") + flags := []string{ + "./" + binary, + "-printfuncs=Warn:1,Warnf:1", + "-test", // TODO: Delete once -shadow is part of -all. + } + cmd := exec.Command(errchk, append(flags, files...)...) + if !run(cmd, t) { + t.Fatal("vet command failed") + } +} + // Run this shell script, but do it in Go so it can be run by "go test". // go build -o testvet // $(GOROOT)/test/errchk ./testvet -shadow -printfuncs='Warn:1,Warnf:1' testdata/*.go testdata/*.s // rm testvet // + func TestVet(t *testing.T) { - // Plan 9 and Windows systems can't be guaranteed to have Perl and so can't run errchk. - switch runtime.GOOS { - case "plan9", "windows": - t.Skip("skipping test; no Perl on %q", runtime.GOOS) + if !CanRun(t) { + t.Skip("cannot run on this environment") } - - // go build - cmd := exec.Command("go", "build", "-o", binary) - run(cmd, t) - - // defer removal of vet + Build(t) defer os.Remove(binary) // errchk ./testvet @@ -51,16 +74,29 @@ func TestVet(t *testing.T) { t.Fatal(err) } files := append(gos, asms...) - errchk := filepath.Join(runtime.GOROOT(), "test", "errchk") - flags := []string{ - "./" + binary, - "-printfuncs=Warn:1,Warnf:1", - "-test", // TODO: Delete once -shadow is part of -all. + Vet(t, files) +} + +func TestDivergentPackagesExamples(t *testing.T) { + if !CanRun(t) { + t.Skip("cannot run on this environment") } - cmd = exec.Command(errchk, append(flags, files...)...) - if !run(cmd, t) { - t.Fatal("vet command failed") + Build(t) + defer os.Remove(binary) + + // errchk ./testvet + Vet(t, []string{"testdata/divergent/buf.go", "testdata/divergent/buf_test.go"}) +} + +func TestIncompleteExamples(t *testing.T) { + if !CanRun(t) { + t.Skip("cannot run on this environment") } + Build(t) + defer os.Remove(binary) + + // errchk ./testvet + Vet(t, []string{"testdata/incomplete/examples_test.go"}) } func run(c *exec.Cmd, t *testing.T) bool { @@ -83,7 +119,6 @@ func TestTags(t *testing.T) { cmd := exec.Command("go", "build", "-o", binary) run(cmd, t) - // defer removal of vet defer os.Remove(binary) args := []string{