diff --git a/.travis.yml b/.travis.yml index 22a4c73..5c9f68a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,9 +3,6 @@ sudo: false matrix: include: - - go: "1.6" - - go: "1.7" - - go: "1.8" - go: "1.9" - go: "1.10" - go: "tip" diff --git a/README.md b/README.md index 59b2642..55bbc41 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ errcheck is a program for checking for unchecked errors in go programs. 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.9 or newer and depends on the package go/packages from the golang.org/x/tools repository. ## Use @@ -98,12 +98,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..e693836 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,3 @@ -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 -) +require golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..e7e69c9 --- /dev/null +++ b/go.sum @@ -0,0 +1,4 @@ +golang.org/x/tools v0.0.0-20180803180156-3c07937fe18c h1:Z6Xcg33osoOLCiz/EFPHedK6zJ9nl1KK4WH/LlykMxs= +golang.org/x/tools v0.0.0-20180803180156-3c07937fe18c/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563 h1:NIou6eNFigscvKJmsbyez16S2cIS6idossORlFtSt2E= +golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= diff --git a/internal/errcheck/errcheck.go b/internal/errcheck/errcheck.go index d0c0e21..a33500f 100644 --- a/internal/errcheck/errcheck.go +++ b/internal/errcheck/errcheck.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "go/ast" - "go/build" "go/token" "go/types" "os" @@ -17,9 +16,7 @@ import ( "strings" "sync" - "go/parser" - - "golang.org/x/tools/go/loader" + "golang.org/x/tools/go/packages" ) var errorType *types.Interface @@ -175,28 +172,18 @@ 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, - } - - if c.WithoutGeneratedCode { - loadcfg.ParserMode = parser.ParseComments - } +// loadPackages is used for testing. +var loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { + return packages.Load(cfg, paths...) +} - rest, err := loadcfg.FromArgs(paths, !c.WithoutTests) - if err != nil { - return nil, fmt.Errorf("could not parse arguments: %s", err) - } - if len(rest) > 0 { - return nil, fmt.Errorf("unhandled extra arguments: %v", rest) +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, " "))}, } - - return loadcfg.Load() + return loadPackages(cfg, paths...) } var generatedCodeRegexp = regexp.MustCompile("^// Code generated .* DO NOT EDIT\\.$") @@ -219,27 +206,28 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { // 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. + for _, pkg := range pkgs { + if len(pkg.Errors) > 0 { + return fmt.Errorf("errors while loading package %s: %v", pkg.ID, pkg.Errors) + } } var wg sync.WaitGroup u := &UncheckedErrors{} - for _, pkgInfo := range program.InitialPackages() { - if pkgInfo.Pkg.Path() == "unsafe" { // not a real package - continue - } - + for _, pkg := range pkgs { wg.Add(1) - go func(pkgInfo *loader.PackageInfo) { + go func(pkg *packages.Package) { defer wg.Done() - c.logf("Checking %s", pkgInfo.Pkg.Path()) + c.logf("Checking %s", pkg.Types.Path()) v := &visitor{ - prog: program, - pkg: pkgInfo, + pkg: pkg, ignore: c.Ignore, blank: c.Blank, asserts: c.Asserts, @@ -248,19 +236,28 @@ func (c *Checker) CheckPackages(paths ...string) error { errors: []UncheckedError{}, } - for _, astFile := range v.pkg.Files { + for _, astFile := range v.pkg.Syntax { if c.shouldSkipFile(astFile) { continue } ast.Walk(v, astFile) } u.Append(v.errors...) - }(pkgInfo) + }(pkg) } 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 @@ -268,8 +265,7 @@ func (c *Checker) CheckPackages(paths ...string) error { // visitor implements the errcheck algorithm type visitor struct { - prog *loader.Program - pkg *loader.PackageInfo + pkg *packages.Package ignore map[string]*regexp.Regexp blank bool asserts bool @@ -294,7 +290,7 @@ func (v *visitor) selectorAndFunc(call *ast.CallExpr) (*ast.SelectorExpr, *types 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 nil, nil, false @@ -350,7 +346,7 @@ func (v *visitor) namesForExcludeCheck(call *ast.CallExpr) []string { // 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.Selections[sel] + selection, ok := v.pkg.TypesInfo.Selections[sel] if !ok { return []string{name} } @@ -377,14 +373,14 @@ func (v *visitor) namesForExcludeCheck(call *ast.CallExpr) []string { 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.ObjectOf(sel.Sel); obj != nil { + 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.Info.TypeOf(expr) + t := v.pkg.TypesInfo.TypeOf(expr) if t == nil { return "" } @@ -435,7 +431,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) @@ -469,7 +465,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)} @@ -511,7 +507,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" } } @@ -519,7 +515,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) diff --git a/internal/errcheck/errcheck_test.go b/internal/errcheck/errcheck_test.go index dfcd8ae..2c4e9ee 100644 --- a/internal/errcheck/errcheck_test.go +++ b/internal/errcheck/errcheck_test.go @@ -2,9 +2,6 @@ package errcheck import ( "fmt" - "go/build" - "go/parser" - "go/token" "io/ioutil" "os" "path" @@ -12,6 +9,8 @@ import ( "runtime" "strings" "testing" + + "golang.org/x/tools/go/packages" ) const testPackage = "github.com/kisielk/errcheck/testdata" @@ -40,28 +39,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") + panic("failed to import 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 + 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 + } } } } @@ -122,11 +121,17 @@ package custom ` ) - testBuildTagsDir, err := ioutil.TempDir(".", "testbuildtags") + tmpGopath, err := ioutil.TempDir("", "testbuildtags") if err != nil { t.Fatalf("unable to create testbuildtags directory: %v", err) } - defer os.RemoveAll(testBuildTagsDir) + 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, "custom1.go"), []byte(testBuildCustom1Tag), 0644); err != nil { t.Fatalf("Failed to write testbuildtags custom1: %v", err) @@ -162,7 +167,14 @@ package custom for i, currCase := range cases { checker := NewChecker() checker.Tags = currCase.tags - err := checker.CheckPackages(path.Join("github.com/kisielk/errcheck/internal/errcheck", testBuildTagsDir)) + + 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 { @@ -209,12 +221,18 @@ func TestIgnore(t *testing.T) { 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, "main.go"), []byte(testVendorMain), 0755); err != nil { t.Fatalf("Failed to write testvendor main: %v", err) @@ -238,7 +256,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 @@ -252,7 +270,13 @@ 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) + 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 { @@ -263,7 +287,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 } @@ -296,12 +320,18 @@ func TestWithoutGeneratedCode(t *testing.T) { 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, "main.go"), []byte(testVendorMain), 0755); err != nil { t.Fatalf("Failed to write testvendor main: %v", err) @@ -322,7 +352,7 @@ func TestWithoutGeneratedCode(t *testing.T) { withoutGeneratedCode: false, numExpectedErrs: 1, }, - // ignoring vendored import works + // ignoring generated code works { withoutGeneratedCode: true, numExpectedErrs: 0, @@ -332,7 +362,13 @@ func TestWithoutGeneratedCode(t *testing.T) { for i, currCase := range cases { checker := NewChecker() checker.WithoutGeneratedCode = currCase.withoutGeneratedCode - 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) + 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 { @@ -343,7 +379,7 @@ func TestWithoutGeneratedCode(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 } @@ -367,7 +403,7 @@ func test(t *testing.T, f flags) { 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 a4f612a..7481c9e 100644 --- a/main.go +++ b/main.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/kisielk/errcheck/internal/errcheck" - "github.com/kisielk/gotool" ) const ( @@ -133,7 +132,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.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") @@ -183,8 +182,11 @@ 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 } func main() { diff --git a/main_test.go b/main_test.go index dfb1a68..9f22933 100644 --- a/main_test.go +++ b/main_test.go @@ -54,7 +54,7 @@ func TestMain(t *testing.T) { t.Errorf("Exit code is %d, expected %d", exitCode, exitUncheckedError) } - expectUnchecked := 16 + expectUnchecked := 29 if got := strings.Count(out, "UNCHECKED"); got != expectUnchecked { t.Errorf("Got %d UNCHECKED errors, expected %d in:\n%s", got, expectUnchecked, out) } 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() +}