diff --git a/.travis.yml b/.travis.yml index 22a4c73..6e13d3a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,12 +3,36 @@ sudo: false matrix: include: - - go: "1.6" - - go: "1.7" - - go: "1.8" - - go: "1.9" - - go: "1.10" + - go: "1.11.x" + env: {GO111MODULE: "off"} + - go: "1.11.x" + env: {GO111MODULE: "on"} + - go: "1.12.x" + env: {GO111MODULE: "off"} + - go: "1.12.x" + env: {GO111MODULE: "on"} + - go: "1.13.x" + env: {GO111MODULE: "off"} + - go: "1.13.x" + env: {GO111MODULE: "on"} + - go: "1.14.x" + env: {GO111MODULE: "off"} + - go: "1.14.x" + env: {GO111MODULE: "on"} - go: "tip" + env: {GO111MODULE: "off"} + - go: "tip" + env: {GO111MODULE: "on"} + +install: | + if test -z "$(go env GOMOD)"; then + go get -d -t ./... && + ver=$(sed -n '/^require/s/.*[ -]//p' go.mod) && + ( + cd "$(go env GOPATH)/src/golang.org/x/tools" && + git checkout "$ver" + ) + fi script: - go test -race ./... diff --git a/README.md b/README.md index 64cf174..f763ee1 100644 --- a/README.md +++ b/README.md @@ -2,13 +2,13 @@ errcheck is a program for checking for unchecked errors in go programs. -[![Build Status](https://travis-ci.org/kisielk/errcheck.png?branch=master)](https://travis-ci.org/kisielk/errcheck) +[![Build Status](https://travis-ci.org/kisielk/errcheck.svg?branch=master)](https://travis-ci.org/kisielk/errcheck) ## Install go get -u github.com/kisielk/errcheck -errcheck requires Go 1.6 or newer and depends on the package go/loader from the golang.org/x/tools repository. +errcheck requires Go 1.11 or newer, and depends on the package go/packages from the golang.org/x/tools repository. ## Use @@ -46,16 +46,33 @@ be excluded. The file should contain one function signature per line. The format for function signatures is `package.FunctionName` while for methods it's `(package.Receiver).MethodName` for value receivers -and `(*package.Receiver).MethodName` for pointer receivers. +and `(*package.Receiver).MethodName` for pointer receivers. If the function name is followed by string of form `(TYPE)`, then +the the function call is excluded only if the type of the first argument is `TYPE`. It also accepts a special suffix +`(os.Stdout)` and `(os.Stderr)`, which excludes the function only when the first argument is a literal `os.Stdout` or `os.Stderr`. An example of an exclude file is: io/ioutil.ReadFile + io.Copy(*bytes.Buffer) + io.Copy(os.Stdout) + + // Sometimes we don't care if a HTTP request fails. (*net/http.Client).Do The exclude list is combined with an internal list for functions in the Go standard library that have an error return type but are documented to never return an error. +When using vendored dependencies, specify the full import path. For example: +* Your project's import path is `example.com/yourpkg` +* You've vendored `example.net/fmt2` as `vendor/example.net/fmt2` +* You want to exclude `fmt2.Println` from error checking + +In this case, add this line to your exclude file: +``` +example.com/yourpkg/vendor/example.net/fmt2.Println +``` + +Empty lines and lines starting with `//` are ignored. ### The deprecated method @@ -94,12 +111,9 @@ no arguments. ## Cgo -Currently errcheck is unable to check packages that `import "C"` due to limitations -in the importer. +Currently errcheck is unable to check packages that import "C" due to limitations in the importer when used with versions earlier than Go 1.11. -However, you can use errcheck on packages that depend on those which use cgo. In -order for this to work you need to `go install` the cgo dependencies before running -errcheck on the dependent packages. +However, you can use errcheck on packages that depend on those which use cgo. In order for this to work you need to go install the cgo dependencies before running errcheck on the dependent packages. See https://github.com/kisielk/errcheck/issues/16 for more details. diff --git a/go.mod b/go.mod index 56c9a97..abd540e 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,5 @@ -module "github.com/kisielk/errcheck" +module github.com/kisielk/errcheck -require ( - "github.com/kisielk/gotool" v1.0.0 - "golang.org/x/tools" v0.0.0-20180221164845-07fd8470d635 -) +go 1.14 + +require golang.org/x/tools v0.0.0-20200619180055-7c47624df98f diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..623727a --- /dev/null +++ b/go.sum @@ -0,0 +1,20 @@ +github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= +golang.org/x/mod v0.2.0 h1:KU7oHjnv3XNWfa5COkzUifxZmxp1TyI7ImMXqFxLwvQ= +golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= +golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= +golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20200619180055-7c47624df98f h1:tuwaIjfUa6eI6REiNueIxvNm1popyPUnqWga83S7U0o= +golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/errcheck/embedded_walker.go b/internal/errcheck/embedded_walker.go new file mode 100644 index 0000000..3b31925 --- /dev/null +++ b/internal/errcheck/embedded_walker.go @@ -0,0 +1,144 @@ +package errcheck + +import ( + "fmt" + "go/types" +) + +// walkThroughEmbeddedInterfaces returns a slice of Interfaces that +// we need to walk through in order to reach the actual definition, +// in an Interface, of the method selected by the given selection. +// +// false will be returned in the second return value if: +// - the right side of the selection is not a function +// - the actual definition of the function is not in an Interface +// +// The returned slice will contain all the interface types that need +// to be walked through to reach the actual definition. +// +// For example, say we have: +// +// type Inner interface {Method()} +// type Middle interface {Inner} +// type Outer interface {Middle} +// type T struct {Outer} +// type U struct {T} +// type V struct {U} +// +// And then the selector: +// +// V.Method +// +// We'll return [Outer, Middle, Inner] by first walking through the embedded structs +// until we reach the Outer interface, then descending through the embedded interfaces +// until we find the one that actually explicitly defines Method. +func walkThroughEmbeddedInterfaces(sel *types.Selection) ([]types.Type, bool) { + fn, ok := sel.Obj().(*types.Func) + if !ok { + return nil, false + } + + // Start off at the receiver. + currentT := sel.Recv() + + // First, we can walk through any Struct fields provided + // by the selection Index() method. We ignore the last + // index because it would give the method itself. + indexes := sel.Index() + for _, fieldIndex := range indexes[:len(indexes)-1] { + currentT = getTypeAtFieldIndex(currentT, fieldIndex) + } + + // Now currentT is either a type implementing the actual function, + // an Invalid type (if the receiver is a package), or an interface. + // + // If it's not an Interface, then we're done, as this function + // only cares about Interface-defined functions. + // + // If it is an Interface, we potentially need to continue digging until + // we find the Interface that actually explicitly defines the function. + interfaceT, ok := maybeUnname(currentT).(*types.Interface) + if !ok { + return nil, false + } + + // The first interface we pass through is this one we've found. We return the possibly + // wrapping types.Named because it is more useful to work with for callers. + result := []types.Type{currentT} + + // If this interface itself explicitly defines the given method + // then we're done digging. + for !explicitlyDefinesMethod(interfaceT, fn) { + // Otherwise, we find which of the embedded interfaces _does_ + // define the method, add it to our list, and loop. + namedInterfaceT, ok := getEmbeddedInterfaceDefiningMethod(interfaceT, fn) + if !ok { + // This should be impossible as long as we type-checked: either the + // interface or one of its embedded ones must implement the method... + panic(fmt.Sprintf("either %v or one of its embedded interfaces must implement %v", currentT, fn)) + } + result = append(result, namedInterfaceT) + interfaceT = namedInterfaceT.Underlying().(*types.Interface) + } + + return result, true +} + +func getTypeAtFieldIndex(startingAt types.Type, fieldIndex int) types.Type { + t := maybeUnname(maybeDereference(startingAt)) + s, ok := t.(*types.Struct) + if !ok { + panic(fmt.Sprintf("cannot get Field of a type that is not a struct, got a %T", t)) + } + + return s.Field(fieldIndex).Type() +} + +// getEmbeddedInterfaceDefiningMethod searches through any embedded interfaces of the +// passed interface searching for one that defines the given function. If found, the +// types.Named wrapping that interface will be returned along with true in the second value. +// +// If no such embedded interface is found, nil and false are returned. +func getEmbeddedInterfaceDefiningMethod(interfaceT *types.Interface, fn *types.Func) (*types.Named, bool) { + for i := 0; i < interfaceT.NumEmbeddeds(); i++ { + embedded := interfaceT.Embedded(i) + if definesMethod(embedded.Underlying().(*types.Interface), fn) { + return embedded, true + } + } + return nil, false +} + +func explicitlyDefinesMethod(interfaceT *types.Interface, fn *types.Func) bool { + for i := 0; i < interfaceT.NumExplicitMethods(); i++ { + if interfaceT.ExplicitMethod(i) == fn { + return true + } + } + return false +} + +func definesMethod(interfaceT *types.Interface, fn *types.Func) bool { + for i := 0; i < interfaceT.NumMethods(); i++ { + if interfaceT.Method(i) == fn { + return true + } + } + return false +} + +func maybeDereference(t types.Type) types.Type { + p, ok := t.(*types.Pointer) + if ok { + return p.Elem() + } + return t +} + +func maybeUnname(t types.Type) types.Type { + n, ok := t.(*types.Named) + if ok { + return n.Underlying() + } + return t +} diff --git a/internal/errcheck/embedded_walker_test.go b/internal/errcheck/embedded_walker_test.go new file mode 100644 index 0000000..51c13a2 --- /dev/null +++ b/internal/errcheck/embedded_walker_test.go @@ -0,0 +1,93 @@ +package errcheck + +import ( + "go/ast" + "go/parser" + "go/token" + "go/types" + "testing" +) + +const commonSrc = ` +package p + +type Inner struct {} +func (Inner) Method() + +type Outer struct {Inner} +type OuterP struct {*Inner} + +type InnerInterface interface { + Method() +} + +type OuterInterface interface {InnerInterface} +type MiddleInterfaceStruct struct {OuterInterface} +type OuterInterfaceStruct struct {MiddleInterfaceStruct} + +var c = ` + +type testCase struct { + selector string + expectedOk bool + expected []string +} + +func TestWalkThroughEmbeddedInterfaces(t *testing.T) { + cases := []testCase{ + testCase{"Inner{}.Method", false, nil}, + testCase{"(&Inner{}).Method", false, nil}, + testCase{"Outer{}.Method", false, nil}, + testCase{"InnerInterface.Method", true, []string{"test.InnerInterface"}}, + testCase{"OuterInterface.Method", true, []string{"test.OuterInterface", "test.InnerInterface"}}, + testCase{"OuterInterfaceStruct.Method", true, []string{"test.OuterInterface", "test.InnerInterface"}}, + } + + for _, c := range cases { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "test", commonSrc+c.selector, 0) + if err != nil { + t.Fatal(err) + } + + conf := types.Config{} + info := types.Info{ + Selections: make(map[*ast.SelectorExpr]*types.Selection), + } + _, err = conf.Check("test", fset, []*ast.File{f}, &info) + if err != nil { + t.Fatal(err) + } + ast.Inspect(f, func(n ast.Node) bool { + s, ok := n.(*ast.SelectorExpr) + if ok { + selection, ok := info.Selections[s] + if !ok { + t.Fatalf("no Selection!") + } + ts, ok := walkThroughEmbeddedInterfaces(selection) + if ok != c.expectedOk { + t.Errorf("expected ok %v got %v", c.expectedOk, ok) + return false + } + if !ok { + return false + } + + if len(ts) != len(c.expected) { + t.Fatalf("expected %d types, got %d", len(c.expected), len(ts)) + } + + for i, e := range c.expected { + if e != ts[i].String() { + t.Errorf("mismatch at index %d: expected %s got %s", i, e, ts[i]) + } + } + } + + return true + }) + + } + +} diff --git a/internal/errcheck/errcheck.go b/internal/errcheck/errcheck.go index b2b3fab..e1548ba 100644 --- a/internal/errcheck/errcheck.go +++ b/internal/errcheck/errcheck.go @@ -8,16 +8,17 @@ import ( "errors" "fmt" "go/ast" - "go/build" "go/token" "go/types" "os" + "os/exec" "regexp" + "runtime" "sort" "strings" "sync" - "golang.org/x/tools/go/loader" + "golang.org/x/tools/go/packages" ) var errorType *types.Interface @@ -108,6 +109,9 @@ type Checker struct { // If true, checking of _test.go files is disabled WithoutTests bool + // If true, checking of files with generated code is disabled + WithoutGeneratedCode bool + exclude map[string]bool } @@ -118,21 +122,47 @@ func NewChecker() *Checker { } func (c *Checker) SetExclude(l map[string]bool) { - // Default exclude for stdlib functions - c.exclude = map[string]bool{ - "math/rand.Read": true, - "(*math/rand.Rand).Read": true, - - "(*bytes.Buffer).Write": true, - "(*bytes.Buffer).WriteByte": true, - "(*bytes.Buffer).WriteRune": true, - "(*bytes.Buffer).WriteString": true, + c.exclude = map[string]bool{} - "(*strings.Builder).Write": true, - "(*strings.Builder).WriteByte": true, - "(*strings.Builder).WriteRune": true, - "(*strings.Builder).WriteString": true, + // Default exclude for stdlib functions + for _, exc := range []string{ + // bytes + "(*bytes.Buffer).Write", + "(*bytes.Buffer).WriteByte", + "(*bytes.Buffer).WriteRune", + "(*bytes.Buffer).WriteString", + + // fmt + "fmt.Errorf", + "fmt.Print", + "fmt.Printf", + "fmt.Println", + "fmt.Fprint(*bytes.Buffer)", + "fmt.Fprintf(*bytes.Buffer)", + "fmt.Fprintln(*bytes.Buffer)", + "fmt.Fprint(*strings.Builder)", + "fmt.Fprintf(*strings.Builder)", + "fmt.Fprintln(*strings.Builder)", + "fmt.Fprint(os.Stderr)", + "fmt.Fprintf(os.Stderr)", + "fmt.Fprintln(os.Stderr)", + + // math/rand + "math/rand.Read", + "(*math/rand.Rand).Read", + + // strings + "(*strings.Builder).Write", + "(*strings.Builder).WriteByte", + "(*strings.Builder).WriteRune", + "(*strings.Builder).WriteString", + + // hash + "(hash.Hash).Write", + } { + c.exclude[exc] = true } + for k := range l { c.exclude[k] = true } @@ -144,66 +174,112 @@ func (c *Checker) logf(msg string, args ...interface{}) { } } -func (c *Checker) load(paths ...string) (*loader.Program, error) { - ctx := build.Default - for _, tag := range c.Tags { - ctx.BuildTags = append(ctx.BuildTags, tag) - } - loadcfg := loader.Config{ - Build: &ctx, +// loadPackages is used for testing. +var loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { + return packages.Load(cfg, paths...) +} + +func (c *Checker) load(paths ...string) ([]*packages.Package, error) { + cfg := &packages.Config{ + Mode: packages.LoadAllSyntax, + Tests: !c.WithoutTests, + BuildFlags: []string{fmt.Sprintf("-tags=%s", strings.Join(c.Tags, " "))}, } - rest, err := loadcfg.FromArgs(paths, !c.WithoutTests) - if err != nil { - return nil, fmt.Errorf("could not parse arguments: %s", err) + return loadPackages(cfg, paths...) +} + +var generatedCodeRegexp = regexp.MustCompile("^// Code generated .* DO NOT EDIT\\.$") + +func (c *Checker) shouldSkipFile(file *ast.File) bool { + if !c.WithoutGeneratedCode { + return false } - if len(rest) > 0 { - return nil, fmt.Errorf("unhandled extra arguments: %v", rest) + + for _, cg := range file.Comments { + for _, comment := range cg.List { + if generatedCodeRegexp.MatchString(comment.Text) { + return true + } + } } - return loadcfg.Load() + return false } // CheckPackages checks packages for errors. func (c *Checker) CheckPackages(paths ...string) error { - program, err := c.load(paths...) + pkgs, err := c.load(paths...) if err != nil { - return fmt.Errorf("could not type check: %s", err) + return err + } + // Check for errors in the initial packages. + work := make(chan *packages.Package, len(pkgs)) + for _, pkg := range pkgs { + if len(pkg.Errors) > 0 { + return fmt.Errorf("errors while loading package %s: %v", pkg.ID, pkg.Errors) + } + work <- pkg + } + close(work) + + gomod, err := exec.Command("go", "env", "GOMOD").Output() + go111module := (err == nil) && strings.TrimSpace(string(gomod)) != "" + ignore := c.Ignore + if go111module { + ignore = make(map[string]*regexp.Regexp) + for pkg, re := range c.Ignore { + if nonVendoredPkg, ok := nonVendoredPkgPath(pkg); ok { + ignore[nonVendoredPkg] = re + } else { + ignore[pkg] = re + } + } } var wg sync.WaitGroup u := &UncheckedErrors{} - for _, pkgInfo := range program.InitialPackages() { - if pkgInfo.Pkg.Path() == "unsafe" { // not a real package - continue - } - + for i := 0; i < runtime.NumCPU(); i++ { wg.Add(1) - go func(pkgInfo *loader.PackageInfo) { + go func() { defer wg.Done() - c.logf("Checking %s", pkgInfo.Pkg.Path()) - - v := &visitor{ - prog: program, - pkg: pkgInfo, - ignore: c.Ignore, - blank: c.Blank, - asserts: c.Asserts, - lines: make(map[string][]string), - exclude: c.exclude, - errors: []UncheckedError{}, - } + for pkg := range work { + c.logf("Checking %s", pkg.Types.Path()) + + v := &visitor{ + pkg: pkg, + ignore: ignore, + blank: c.Blank, + asserts: c.Asserts, + lines: make(map[string][]string), + exclude: c.exclude, + go111module: go111module, + errors: []UncheckedError{}, + } - for _, astFile := range v.pkg.Files { - ast.Walk(v, astFile) + for _, astFile := range v.pkg.Syntax { + if c.shouldSkipFile(astFile) { + continue + } + ast.Walk(v, astFile) + } + u.Append(v.errors...) } - u.Append(v.errors...) - }(pkgInfo) + }() } wg.Wait() if u.Len() > 0 { + // Sort unchecked errors and remove duplicates. Duplicates may occur when a file + // containing an unchecked error belongs to > 1 package. sort.Sort(byName{u}) + uniq := u.Errors[:0] // compact in-place + for i, err := range u.Errors { + if i == 0 || err != u.Errors[i-1] { + uniq = append(uniq, err) + } + } + u.Errors = uniq return u } return nil @@ -211,42 +287,142 @@ func (c *Checker) CheckPackages(paths ...string) error { // visitor implements the errcheck algorithm type visitor struct { - prog *loader.Program - pkg *loader.PackageInfo - ignore map[string]*regexp.Regexp - blank bool - asserts bool - lines map[string][]string - exclude map[string]bool + pkg *packages.Package + ignore map[string]*regexp.Regexp + blank bool + asserts bool + lines map[string][]string + exclude map[string]bool + go111module bool errors []UncheckedError } -func (v *visitor) fullName(call *ast.CallExpr) (string, bool) { +// selectorAndFunc tries to get the selector and function from call expression. +// For example, given the call expression representing "a.b()", the selector +// is "a.b" and the function is "b" itself. +// +// The final return value will be true if it is able to do extract a selector +// from the call and look up the function object it refers to. +// +// If the call does not include a selector (like if it is a plain "f()" function call) +// then the final return value will be false. +func (v *visitor) selectorAndFunc(call *ast.CallExpr) (*ast.SelectorExpr, *types.Func, bool) { sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { - return "", false + return nil, nil, false } - fn, ok := v.pkg.ObjectOf(sel.Sel).(*types.Func) + + fn, ok := v.pkg.TypesInfo.ObjectOf(sel.Sel).(*types.Func) if !ok { // Shouldn't happen, but be paranoid - return "", false + return nil, nil, false } - // The name is fully qualified by the import path, possible type, - // function/method name and pointer receiver. - // + + return sel, fn, true + +} + +// fullName will return a package / receiver-type qualified name for a called function +// if the function is the result of a selector. Otherwise it will return +// the empty string. +// +// The name is fully qualified by the import path, possible type, +// function/method name and pointer receiver. +// +// For example, +// - for "fmt.Printf(...)" it will return "fmt.Printf" +// - for "base64.StdEncoding.Decode(...)" it will return "(*encoding/base64.Encoding).Decode" +// - for "myFunc()" it will return "" +func (v *visitor) fullName(call *ast.CallExpr) string { + _, fn, ok := v.selectorAndFunc(call) + if !ok { + return "" + } + // TODO(dh): vendored packages will have /vendor/ in their name, // thus not matching vendored standard library packages. If we // want to support vendored stdlib packages, we need to implement // FullName with our own logic. - return fn.FullName(), true + return fn.FullName() } -func (v *visitor) excludeCall(call *ast.CallExpr) bool { - if name, ok := v.fullName(call); ok { - return v.exclude[name] +// namesForExcludeCheck will return a list of fully-qualified function names +// from a function call that can be used to check against the exclusion list. +// +// If a function call is against a local function (like "myFunc()") then no +// names are returned. If the function is package-qualified (like "fmt.Printf()") +// then just that function's fullName is returned. +// +// Otherwise, we walk through all the potentially embeddded interfaces of the receiver +// the collect a list of type-qualified function names that we will check. +func (v *visitor) namesForExcludeCheck(call *ast.CallExpr) []string { + sel, fn, ok := v.selectorAndFunc(call) + if !ok { + return nil + } + + name := v.fullName(call) + if name == "" { + return nil + } + + // This will be missing for functions without a receiver (like fmt.Printf), + // so just fall back to the the function's fullName in that case. + selection, ok := v.pkg.TypesInfo.Selections[sel] + if !ok { + return []string{name} + } + + // This will return with ok false if the function isn't defined + // on an interface, so just fall back to the fullName. + ts, ok := walkThroughEmbeddedInterfaces(selection) + if !ok { + return []string{name} + } + + result := make([]string, len(ts)) + for i, t := range ts { + // Like in fullName, vendored packages will have /vendor/ in their name, + // thus not matching vendored standard library packages. If we + // want to support vendored stdlib packages, we need to implement + // additional logic here. + result[i] = fmt.Sprintf("(%s).%s", t.String(), fn.Name()) + } + return result +} + +// isBufferType checks if the expression type is a known in-memory buffer type. +func (v *visitor) argName(expr ast.Expr) string { + // Special-case literal "os.Stdout" and "os.Stderr" + if sel, ok := expr.(*ast.SelectorExpr); ok { + if obj := v.pkg.TypesInfo.ObjectOf(sel.Sel); obj != nil { + vr, ok := obj.(*types.Var) + if ok && vr.Pkg() != nil && vr.Pkg().Name() == "os" && (vr.Name() == "Stderr" || vr.Name() == "Stdout") { + return "os." + vr.Name() + } + } + } + t := v.pkg.TypesInfo.TypeOf(expr) + if t == nil { + return "" } + return t.String() +} +func (v *visitor) excludeCall(call *ast.CallExpr) bool { + var arg0 string + if len(call.Args) > 0 { + arg0 = v.argName(call.Args[0]) + } + for _, name := range v.namesForExcludeCheck(call) { + if v.exclude[name] { + return true + } + if arg0 != "" && v.exclude[name+"("+arg0+")"] { + return true + } + } return false } @@ -278,7 +454,7 @@ func (v *visitor) ignoreCall(call *ast.CallExpr) bool { return true } - if obj := v.pkg.Uses[id]; obj != nil { + if obj := v.pkg.TypesInfo.Uses[id]; obj != nil { if pkg := obj.Pkg(); pkg != nil { if re, ok := v.ignore[pkg.Path()]; ok { return re.MatchString(id.Name) @@ -286,9 +462,11 @@ func (v *visitor) ignoreCall(call *ast.CallExpr) bool { // if current package being considered is vendored, check to see if it should be ignored based // on the unvendored path. - if nonVendoredPkg, ok := nonVendoredPkgPath(pkg.Path()); ok { - if re, ok := v.ignore[nonVendoredPkg]; ok { - return re.MatchString(id.Name) + if !v.go111module { + if nonVendoredPkg, ok := nonVendoredPkgPath(pkg.Path()); ok { + if re, ok := v.ignore[nonVendoredPkg]; ok { + return re.MatchString(id.Name) + } } } } @@ -312,7 +490,7 @@ func nonVendoredPkgPath(pkgPath string) (string, bool) { // len(s) == number of return types of call // s[i] == true iff return type at position i from left is an error type func (v *visitor) errorsByArg(call *ast.CallExpr) []bool { - switch t := v.pkg.Types[call].Type.(type) { + switch t := v.pkg.TypesInfo.Types[call].Type.(type) { case *types.Named: // Single return return []bool{isErrorType(t)} @@ -354,7 +532,7 @@ func (v *visitor) callReturnsError(call *ast.CallExpr) bool { // isRecover returns true if the given CallExpr is a call to the built-in recover() function. func (v *visitor) isRecover(call *ast.CallExpr) bool { if fun, ok := call.Fun.(*ast.Ident); ok { - if _, ok := v.pkg.Uses[fun].(*types.Builtin); ok { + if _, ok := v.pkg.TypesInfo.Uses[fun].(*types.Builtin); ok { return fun.Name == "recover" } } @@ -362,7 +540,7 @@ func (v *visitor) isRecover(call *ast.CallExpr) bool { } func (v *visitor) addErrorAtPosition(position token.Pos, call *ast.CallExpr) { - pos := v.prog.Fset.Position(position) + pos := v.pkg.Fset.Position(position) lines, ok := v.lines[pos.Filename] if !ok { lines = readfile(pos.Filename) @@ -376,7 +554,7 @@ func (v *visitor) addErrorAtPosition(position token.Pos, call *ast.CallExpr) { var name string if call != nil { - name, _ = v.fullName(call) + name = v.fullName(call) } v.errors = append(v.errors, UncheckedError{pos, line, name}) diff --git a/internal/errcheck/errcheck_test.go b/internal/errcheck/errcheck_test.go index 99e12a0..26400e3 100644 --- a/internal/errcheck/errcheck_test.go +++ b/internal/errcheck/errcheck_test.go @@ -2,16 +2,13 @@ package errcheck import ( "fmt" - "go/build" - "go/parser" - "go/token" "io/ioutil" "os" "path" "regexp" - "runtime" - "strings" "testing" + + "golang.org/x/tools/go/packages" ) const testPackage = "github.com/kisielk/errcheck/testdata" @@ -40,28 +37,28 @@ func init() { blankMarkers = make(map[marker]bool) assertMarkers = make(map[marker]bool) - pkg, err := build.Import(testPackage, "", 0) - if err != nil { - panic("failed to import test package") + cfg := &packages.Config{ + Mode: packages.LoadSyntax, + Tests: true, } - fset := token.NewFileSet() - astPkg, err := parser.ParseDir(fset, pkg.Dir, nil, parser.ParseComments) + pkgs, err := packages.Load(cfg, testPackage) if err != nil { - panic("failed to parse test package") - } - - for _, file := range astPkg["main"].Files { - for _, comment := range file.Comments { - text := comment.Text() - pos := fset.Position(comment.Pos()) - m := marker{pos.Filename, pos.Line} - switch text { - case "UNCHECKED\n": - uncheckedMarkers[m] = true - case "BLANK\n": - blankMarkers[m] = true - case "ASSERT\n": - assertMarkers[m] = true + panic(fmt.Errorf("failed to import test package: %v", err)) + } + for _, pkg := range pkgs { + for _, file := range pkg.Syntax { + for _, comment := range file.Comments { + text := comment.Text() + pos := pkg.Fset.Position(comment.Pos()) + m := marker{pos.Filename, pos.Line} + switch text { + case "UNCHECKED\n": + uncheckedMarkers[m] = true + case "BLANK\n": + blankMarkers[m] = true + case "ASSERT\n": + assertMarkers[m] = true + } } } } @@ -89,11 +86,127 @@ func TestAll(t *testing.T) { test(t, CheckAsserts|CheckBlank) } +func TestBuildTags(t *testing.T) { + const ( + // uses "custom1" build tag and contains 1 unchecked error + testBuildCustom1Tag = ` +` + `// +build custom1 + +package custom + +import "fmt" + +func Print1() { + // returns an error that is not checked + fmt.Fprintln(nil) +}` + // uses "custom2" build tag and contains 1 unchecked error + testBuildCustom2Tag = ` +` + `// +build custom2 + +package custom + +import "fmt" + +func Print2() { + // returns an error that is not checked + fmt.Fprintln(nil) +}` + // included so that package is not empty when built without specifying tags + testDoc = ` +// Package custom contains code for testing build tags. +package custom +` + ) + + tmpGopath, err := ioutil.TempDir("", "testbuildtags") + if err != nil { + t.Fatalf("unable to create testbuildtags directory: %v", err) + } + testBuildTagsDir := path.Join(tmpGopath, "src", "github.com/testbuildtags") + if err := os.MkdirAll(testBuildTagsDir, 0755); err != nil { + t.Fatalf("MkdirAll failed: %v", err) + } + defer func() { + os.RemoveAll(tmpGopath) + }() + + if err := ioutil.WriteFile(path.Join(testBuildTagsDir, "go.mod"), []byte("module github.com/testbuildtags"), 0644); err != nil { + t.Fatalf("Failed to write testbuildtags go.mod: %v", err) + } + if err := ioutil.WriteFile(path.Join(testBuildTagsDir, "custom1.go"), []byte(testBuildCustom1Tag), 0644); err != nil { + t.Fatalf("Failed to write testbuildtags custom1: %v", err) + } + if err := ioutil.WriteFile(path.Join(testBuildTagsDir, "custom2.go"), []byte(testBuildCustom2Tag), 0644); err != nil { + t.Fatalf("Failed to write testbuildtags custom2: %v", err) + } + if err := ioutil.WriteFile(path.Join(testBuildTagsDir, "doc.go"), []byte(testDoc), 0644); err != nil { + t.Fatalf("Failed to write testbuildtags doc: %v", err) + } + + cases := []struct { + tags []string + numExpectedErrs int + }{ + // with no tags specified, main is ignored and there are no errors + { + tags: nil, + numExpectedErrs: 0, + }, + // specifying "custom1" tag includes file with 1 error + { + tags: []string{"custom1"}, + numExpectedErrs: 1, + }, + // specifying "custom1" and "custom2" tags includes 2 files with 1 error each + { + tags: []string{"custom1", "custom2"}, + numExpectedErrs: 2, + }, + } + + for i, currCase := range cases { + checker := NewChecker() + checker.Tags = currCase.tags + + loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { + cfg.Env = append(os.Environ(), + "GOPATH="+tmpGopath) + cfg.Dir = testBuildTagsDir + pkgs, err := packages.Load(cfg, paths...) + return pkgs, err + } + err := checker.CheckPackages("github.com/testbuildtags") + + if currCase.numExpectedErrs == 0 { + if err != nil { + t.Errorf("Case %d: expected no errors, but got: %v", i, err) + } + continue + } + + uerr, ok := err.(*UncheckedErrors) + if !ok { + t.Errorf("Case %d: wrong error type returned: %v", i, err) + continue + } + + if currCase.numExpectedErrs != len(uerr.Errors) { + t.Errorf("Case %d:\nExpected: %d errors\nActual: %d errors", i, currCase.numExpectedErrs, len(uerr.Errors)) + } + } +} + func TestWhitelist(t *testing.T) { } -const testVendorMain = ` +func TestIgnore(t *testing.T) { + const testVendorGoMod = `module github.com/testvendor + +require github.com/testlog v0.0.0 +` + const testVendorMain = ` package main import "github.com/testlog" @@ -102,26 +215,29 @@ const testVendorMain = ` // returns an error that is not checked testlog.Info() }` -const testLog = ` + const testLog = ` package testlog func Info() error { return nil }` -func TestIgnore(t *testing.T) { - if strings.HasPrefix(runtime.Version(), "go1.5") && os.Getenv("GO15VENDOREXPERIMENT") != "1" { - // skip tests if running in go1.5 and vendoring is not enabled - t.SkipNow() - } - - // copy testvendor directory into current directory for test - testVendorDir, err := ioutil.TempDir(".", "testvendor") + // copy testvendor directory into directory for test + tmpGopath, err := ioutil.TempDir("", "testvendor") if err != nil { t.Fatalf("unable to create testvendor directory: %v", err) } - defer os.RemoveAll(testVendorDir) + testVendorDir := path.Join(tmpGopath, "src", "github.com/testvendor") + if err := os.MkdirAll(testVendorDir, 0755); err != nil { + t.Fatalf("MkdirAll failed: %v", err) + } + defer func() { + os.RemoveAll(tmpGopath) + }() + if err := ioutil.WriteFile(path.Join(testVendorDir, "go.mod"), []byte(testVendorGoMod), 0755); err != nil { + t.Fatalf("Failed to write testvendor go.mod: %v", err) + } if err := ioutil.WriteFile(path.Join(testVendorDir, "main.go"), []byte(testVendorMain), 0755); err != nil { t.Fatalf("Failed to write testvendor main: %v", err) } @@ -144,7 +260,7 @@ func TestIgnore(t *testing.T) { // ignoring vendored import works { ignore: map[string]*regexp.Regexp{ - path.Join("github.com/kisielk/errcheck/internal/errcheck", testVendorDir, "vendor/github.com/testlog"): regexp.MustCompile("Info"), + path.Join("github.com/testvendor/vendor/github.com/testlog"): regexp.MustCompile("Info"), }, }, // non-vendored path ignores vendored import @@ -158,7 +274,111 @@ func TestIgnore(t *testing.T) { for i, currCase := range cases { checker := NewChecker() checker.Ignore = currCase.ignore - err := checker.CheckPackages(path.Join("github.com/kisielk/errcheck/internal/errcheck", testVendorDir)) + loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { + cfg.Env = append(os.Environ(), + "GOPATH="+tmpGopath, + "GOFLAGS=-mod=vendor") + cfg.Dir = testVendorDir + pkgs, err := packages.Load(cfg, paths...) + return pkgs, err + } + err := checker.CheckPackages("github.com/testvendor") + + if currCase.numExpectedErrs == 0 { + if err != nil { + t.Errorf("Case %d: expected no errors, but got: %v", i, err) + } + continue + } + + uerr, ok := err.(*UncheckedErrors) + if !ok { + t.Errorf("Case %d: wrong error type returned: %v", i, err) + continue + } + + if currCase.numExpectedErrs != len(uerr.Errors) { + t.Errorf("Case %d:\nExpected: %d errors\nActual: %d errors", i, currCase.numExpectedErrs, len(uerr.Errors)) + } + } +} + +func TestWithoutGeneratedCode(t *testing.T) { + const testVendorGoMod = `module github.com/testvendor + +require github.com/testlog v0.0.0 +` + const testVendorMain = ` + // Code generated by protoc-gen-go. DO NOT EDIT. + package main + + import "github.com/testlog" + + func main() { + // returns an error that is not checked + testlog.Info() + }` + const testLog = ` + package testlog + + func Info() error { + return nil + }` + + // copy testvendor directory into directory for test + tmpGopath, err := ioutil.TempDir("", "testvendor") + if err != nil { + t.Fatalf("unable to create testvendor directory: %v", err) + } + testVendorDir := path.Join(tmpGopath, "src", "github.com/testvendor") + if err := os.MkdirAll(testVendorDir, 0755); err != nil { + t.Fatalf("MkdirAll failed: %v", err) + } + defer func() { + os.RemoveAll(tmpGopath) + }() + + if err := ioutil.WriteFile(path.Join(testVendorDir, "go.mod"), []byte(testVendorGoMod), 0755); err != nil { + t.Fatalf("Failed to write testvendor go.mod: %v", err) + } + if err := ioutil.WriteFile(path.Join(testVendorDir, "main.go"), []byte(testVendorMain), 0755); err != nil { + t.Fatalf("Failed to write testvendor main: %v", err) + } + if err := os.MkdirAll(path.Join(testVendorDir, "vendor/github.com/testlog"), 0755); err != nil { + t.Fatalf("MkdirAll failed: %v", err) + } + if err := ioutil.WriteFile(path.Join(testVendorDir, "vendor/github.com/testlog/testlog.go"), []byte(testLog), 0755); err != nil { + t.Fatalf("Failed to write testlog: %v", err) + } + + cases := []struct { + withoutGeneratedCode bool + numExpectedErrs int + }{ + // basic case has one error + { + withoutGeneratedCode: false, + numExpectedErrs: 1, + }, + // ignoring generated code works + { + withoutGeneratedCode: true, + numExpectedErrs: 0, + }, + } + + for i, currCase := range cases { + checker := NewChecker() + checker.WithoutGeneratedCode = currCase.withoutGeneratedCode + loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { + cfg.Env = append(os.Environ(), + "GOPATH="+tmpGopath, + "GOFLAGS=-mod=vendor") + cfg.Dir = testVendorDir + pkgs, err := packages.Load(cfg, paths...) + return pkgs, err + } + err := checker.CheckPackages(path.Join("github.com/testvendor")) if currCase.numExpectedErrs == 0 { if err != nil { @@ -169,7 +389,7 @@ func TestIgnore(t *testing.T) { uerr, ok := err.(*UncheckedErrors) if !ok { - t.Errorf("Case %d: wrong error type returned", i) + t.Errorf("Case %d: wrong error type returned: %v", i, err) continue } @@ -187,10 +407,13 @@ func test(t *testing.T, f flags) { checker := NewChecker() checker.Asserts = asserts checker.Blank = blank + checker.SetExclude(map[string]bool{ + fmt.Sprintf("(%s.ErrorMakerInterface).MakeNilError", testPackage): true, + }) err := checker.CheckPackages(testPackage) uerr, ok := err.(*UncheckedErrors) if !ok { - t.Fatal("wrong error type returned") + t.Fatalf("wrong error type returned: %v", err) } numErrors := len(uncheckedMarkers) diff --git a/main.go b/main.go index 097bd49..da37140 100644 --- a/main.go +++ b/main.go @@ -2,8 +2,10 @@ package main import ( "bufio" + "bytes" "flag" "fmt" + "io/ioutil" "os" "path/filepath" "regexp" @@ -11,7 +13,6 @@ import ( "strings" "github.com/kisielk/errcheck/internal/errcheck" - "github.com/kisielk/gotool" ) const ( @@ -133,6 +134,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { flags.BoolVar(&checker.Blank, "blank", false, "if true, check for errors assigned to blank identifier") flags.BoolVar(&checker.Asserts, "asserts", false, "if true, check for ignored type assertion results") flags.BoolVar(&checker.WithoutTests, "ignoretests", false, "if true, checking of _test.go files is disabled") + flags.BoolVar(&checker.WithoutGeneratedCode, "ignoregenerated", false, "if true, checking of files with generated code is disabled") flags.BoolVar(&checker.Verbose, "verbose", false, "produce more verbose logging") flags.BoolVar(&abspath, "abspath", false, "print absolute paths to files") @@ -140,9 +142,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { tags := tagsFlag{} flags.Var(&tags, "tags", "space-separated list of build tags to include") ignorePkg := flags.String("ignorepkg", "", "comma-separated list of package paths to ignore") - ignore := ignoreFlag(map[string]*regexp.Regexp{ - "fmt": dotStar, - }) + ignore := ignoreFlag(map[string]*regexp.Regexp{}) flags.Var(ignore, "ignore", "[deprecated] comma-separated list of pairs of the form pkg:regex\n"+ " the regex is used to ignore names within pkg.") @@ -154,26 +154,17 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { } if excludeFile != "" { - exclude := make(map[string]bool) - fh, err := os.Open(excludeFile) + excludes, err := readExcludes(excludeFile) if err != nil { - fmt.Fprintf(os.Stderr, "Could not read exclude file: %s\n", err) + fmt.Fprintf(os.Stderr, "Could not read exclude file: %v\n", err) return nil, exitFatalError } - scanner := bufio.NewScanner(fh) - for scanner.Scan() { - name := scanner.Text() - exclude[name] = true - - if checker.Verbose { - fmt.Printf("Excluding %s\n", name) + if checker.Verbose { + for _, exclude := range excludes { + fmt.Printf("Excluding %v\n", exclude) } } - if err := scanner.Err(); err != nil { - fmt.Fprintf(os.Stderr, "Could not read exclude file: %s\n", err) - return nil, exitFatalError - } - checker.SetExclude(exclude) + checker.SetExclude(excludes) } checker.Tags = tags @@ -184,8 +175,37 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { } checker.Ignore = ignore - // ImportPaths normalizes paths and expands '...' - return gotool.ImportPaths(flags.Args()), exitCodeOk + paths := flags.Args() + if len(paths) == 0 { + paths = []string{"."} + } + return paths, exitCodeOk +} + +// readExcludes reads an excludes file, a newline delimited file that lists +// patterns for which to allow unchecked errors. +func readExcludes(path string) (map[string]bool, error) { + excludes := map[string]bool{} + + buf, err := ioutil.ReadFile(path) + if err != nil { + return nil, err + } + + scanner := bufio.NewScanner(bytes.NewReader(buf)) + for scanner.Scan() { + name := scanner.Text() + // Skip comments and empty lines. + if strings.HasPrefix(name, "//") || name == "" { + continue + } + excludes[name] = true + } + if err := scanner.Err(); err != nil { + return nil, err + } + + return excludes, nil } func main() { diff --git a/main_test.go b/main_test.go index 01fb5b1..99a0b85 100644 --- a/main_test.go +++ b/main_test.go @@ -4,6 +4,7 @@ import ( "bytes" "io" "os" + "reflect" "regexp" "strings" "testing" @@ -54,7 +55,7 @@ func TestMain(t *testing.T) { t.Errorf("Exit code is %d, expected %d", exitCode, exitUncheckedError) } - expectUnchecked := 15 + expectUnchecked := 29 if got := strings.Count(out, "UNCHECKED"); got != expectUnchecked { t.Errorf("Got %d UNCHECKED errors, expected %d in:\n%s", got, expectUnchecked, out) } @@ -75,7 +76,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{}, blank: false, asserts: false, @@ -84,7 +85,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-blank", "-asserts"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{}, blank: true, asserts: true, @@ -93,7 +94,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "foo", "bar"}, paths: []string{"foo", "bar"}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{}, blank: false, asserts: false, @@ -102,7 +103,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignore", "fmt:.*,encoding/binary:.*"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String(), "encoding/binary": dotStar.String()}, + ignore: map[string]string{"fmt": ".*", "encoding/binary": dotStar.String()}, tags: []string{}, blank: false, asserts: false, @@ -120,7 +121,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignore", "[rR]ead|[wW]rite"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String(), "": "[rR]ead|[wW]rite"}, + ignore: map[string]string{"": "[rR]ead|[wW]rite"}, tags: []string{}, blank: false, asserts: false, @@ -129,7 +130,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String(), "testing": dotStar.String()}, + ignore: map[string]string{"testing": dotStar.String()}, tags: []string{}, blank: false, asserts: false, @@ -138,7 +139,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing,foo"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String(), "testing": dotStar.String(), "foo": dotStar.String()}, + ignore: map[string]string{"testing": dotStar.String(), "foo": dotStar.String()}, tags: []string{}, blank: false, asserts: false, @@ -147,7 +148,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{"foo"}, blank: false, asserts: false, @@ -156,7 +157,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -165,7 +166,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: map[string]string{"fmt": dotStar.String()}, + ignore: map[string]string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -203,22 +204,55 @@ func TestParseFlags(t *testing.T) { argsStr := strings.Join(c.args, " ") if !slicesEqual(p, c.paths) { - t.Fatalf("%q: path got %q want %q", argsStr, p, c.paths) + t.Errorf("%q: path got %q want %q", argsStr, p, c.paths) } if ign := checker.Ignore; !ignoresEqual(ign, c.ignore) { - t.Fatalf("%q: ignore got %q want %q", argsStr, ign, c.ignore) + t.Errorf("%q: ignore got %q want %q", argsStr, ign, c.ignore) } if tags := checker.Tags; !slicesEqual(tags, c.tags) { - t.Fatalf("%q: tags got %v want %v", argsStr, tags, c.tags) + t.Errorf("%q: tags got %v want %v", argsStr, tags, c.tags) } if b := checker.Blank; b != c.blank { - t.Fatalf("%q: blank got %v want %v", argsStr, b, c.blank) + t.Errorf("%q: blank got %v want %v", argsStr, b, c.blank) } if a := checker.Asserts; a != c.asserts { - t.Fatalf("%q: asserts got %v want %v", argsStr, a, c.asserts) + t.Errorf("%q: asserts got %v want %v", argsStr, a, c.asserts) } if e != c.error { - t.Fatalf("%q: error got %q want %q", argsStr, e, c.error) + t.Errorf("%q: error got %q want %q", argsStr, e, c.error) } } } + +func TestReadExcludes(t *testing.T) { + expectedExcludes := map[string]bool{ + "hello()": true, + "world()": true, + } + t.Logf("expectedExcludes: %#v", expectedExcludes) + excludes, err := readExcludes("testdata/excludes.txt") + if err != nil { + t.Fatal(err) + } + t.Logf("excludes: %#v", excludes) + if !reflect.DeepEqual(expectedExcludes, excludes) { + t.Fatal("excludes did not match expectedExcludes") + } +} + +func TestReadEmptyExcludes(t *testing.T) { + excludes, err := readExcludes("testdata/empty_excludes.txt") + if err != nil { + t.Fatal(err) + } + if len(excludes) != 0 { + t.Fatalf("expected empty excludes, got %#v", excludes) + } +} + +func TestReadExcludesMissingFile(t *testing.T) { + _, err := readExcludes("testdata/missing_file") + if err == nil { + t.Fatal("expected non-nil err, got nil") + } +} diff --git a/testdata/empty_excludes.txt b/testdata/empty_excludes.txt new file mode 100644 index 0000000..e69de29 diff --git a/testdata/excludes.txt b/testdata/excludes.txt new file mode 100644 index 0000000..6a0b2dc --- /dev/null +++ b/testdata/excludes.txt @@ -0,0 +1,5 @@ +// comment +hello() + +//trickycomment() +world() diff --git a/testdata/main.go b/testdata/main.go index 6e4177d..48d2c3b 100644 --- a/testdata/main.go +++ b/testdata/main.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "crypto/sha256" "fmt" "io/ioutil" "math/rand" @@ -9,17 +10,17 @@ import ( ) func a() error { - fmt.Println("this function returns an error") // UNCHECKED + fmt.Println("this function returns an error") // EXCLUDED return nil } func b() (int, error) { - fmt.Println("this function returns an int and an error") // UNCHECKED + fmt.Println("this function returns an int and an error") // EXCLUDED return 0, nil } func c() int { - fmt.Println("this function returns an int") // UNCHECKED + fmt.Println("this function returns an int") // EXCLUDED return 7 } @@ -65,6 +66,14 @@ func customPointerErrorTuple() (int, *MyPointerError) { return 0, &e } +// Test custom excludes +type ErrorMakerInterface interface { + MakeNilError() error +} +type ErrorMakerInterfaceWrapper interface { + ErrorMakerInterface +} + func main() { // Single error return _ = a() // BLANK @@ -137,6 +146,10 @@ func main() { b2.Write(nil) rand.Read(nil) mrand.Read(nil) + sha256.New().Write([]byte{}) ioutil.ReadFile("main.go") // UNCHECKED + + var emiw ErrorMakerInterfaceWrapper + emiw.MakeNilError() } diff --git a/testdata/main2.go b/testdata/main2.go index 040ce67..19b81b6 100644 --- a/testdata/main2.go +++ b/testdata/main2.go @@ -6,9 +6,9 @@ import "fmt" type t struct{} func (x t) a() error { - fmt.Println("this method returns an error") // UNCHECKED + fmt.Println("this method returns an error") // EXCLUDED //line myfile.txt:100 - fmt.Println("this method also returns an error") // UNCHECKED + fmt.Println("this method also returns an error") // EXCLUDED return nil } diff --git a/testdata/main3.go b/testdata/main3.go new file mode 100644 index 0000000..851f571 --- /dev/null +++ b/testdata/main3.go @@ -0,0 +1,18 @@ +package main + +import ( + "bytes" + "fmt" + "os" +) + +func testfprintf() { + f, err := os.Create("/tmp/blah") + if err != nil { + panic(err) + } + buf := bytes.Buffer{} + fmt.Fprintln(f, "blah") // UNCHECKED + fmt.Fprintln(os.Stderr, "blah") + fmt.Fprintln(&buf, "blah") +} diff --git a/testdata/main_test.go b/testdata/main_test.go new file mode 100644 index 0000000..79e9659 --- /dev/null +++ b/testdata/main_test.go @@ -0,0 +1,91 @@ +package main + +import ( + "bytes" + "crypto/sha256" + "io/ioutil" + "math/rand" + mrand "math/rand" + + "testing" +) + +func TestFunc(tt *testing.T) { + // Single error return + _ = a() // BLANK + a() // UNCHECKED + + // Return another value and an error + _, _ = b() // BLANK + b() // UNCHECKED + + // Return a custom error type + _ = customError() // BLANK + customError() // UNCHECKED + + // Return a custom concrete error type + _ = customConcreteError() // BLANK + customConcreteError() // UNCHECKED + _, _ = customConcreteErrorTuple() // BLANK + customConcreteErrorTuple() // UNCHECKED + + // Return a custom pointer error type + _ = customPointerError() // BLANK + customPointerError() // UNCHECKED + _, _ = customPointerErrorTuple() // BLANK + customPointerErrorTuple() // UNCHECKED + + // Method with a single error return + x := t{} + _ = x.a() // BLANK + x.a() // UNCHECKED + + // Method call on a struct member + y := u{x} + _ = y.t.a() // BLANK + y.t.a() // UNCHECKED + + m1 := map[string]func() error{"a": a} + _ = m1["a"]() // BLANK + m1["a"]() // UNCHECKED + + // Additional cases for assigning errors to blank identifier + z, _ := b() // BLANK + _, w := a(), 5 // BLANK + + // Assign non error to blank identifier + _ = c() + + _ = z + w // Avoid complaints about unused variables + + // Type assertions + var i interface{} + s1 := i.(string) // ASSERT + s1 = i.(string) // ASSERT + s2, _ := i.(string) // ASSERT + s2, _ = i.(string) // ASSERT + s3, ok := i.(string) + s3, ok = i.(string) + switch s4 := i.(type) { + case string: + _ = s4 + } + _, _, _, _ = s1, s2, s3, ok + + // Goroutine + go a() // UNCHECKED + defer a() // UNCHECKED + + b1 := bytes.Buffer{} + b2 := &bytes.Buffer{} + b1.Write(nil) + b2.Write(nil) + rand.Read(nil) + mrand.Read(nil) + sha256.New().Write([]byte{}) + + ioutil.ReadFile("main.go") // UNCHECKED + + var emiw ErrorMakerInterfaceWrapper + emiw.MakeNilError() +}