From ea6ea2fa7078e87939fce9f78c47a64c94de3036 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Sat, 1 Aug 2020 22:27:55 -0700 Subject: [PATCH 01/19] Move errcheck package out of internal Signed-off-by: Eric Chlebek --- {internal/errcheck => errcheck}/embedded_walker.go | 0 {internal/errcheck => errcheck}/embedded_walker_test.go | 0 {internal/errcheck => errcheck}/errcheck.go | 0 {internal/errcheck => errcheck}/errcheck_test.go | 0 main.go | 2 +- main_test.go | 2 +- 6 files changed, 2 insertions(+), 2 deletions(-) rename {internal/errcheck => errcheck}/embedded_walker.go (100%) rename {internal/errcheck => errcheck}/embedded_walker_test.go (100%) rename {internal/errcheck => errcheck}/errcheck.go (100%) rename {internal/errcheck => errcheck}/errcheck_test.go (100%) diff --git a/internal/errcheck/embedded_walker.go b/errcheck/embedded_walker.go similarity index 100% rename from internal/errcheck/embedded_walker.go rename to errcheck/embedded_walker.go diff --git a/internal/errcheck/embedded_walker_test.go b/errcheck/embedded_walker_test.go similarity index 100% rename from internal/errcheck/embedded_walker_test.go rename to errcheck/embedded_walker_test.go diff --git a/internal/errcheck/errcheck.go b/errcheck/errcheck.go similarity index 100% rename from internal/errcheck/errcheck.go rename to errcheck/errcheck.go diff --git a/internal/errcheck/errcheck_test.go b/errcheck/errcheck_test.go similarity index 100% rename from internal/errcheck/errcheck_test.go rename to errcheck/errcheck_test.go diff --git a/main.go b/main.go index 7fee911..8f215ef 100644 --- a/main.go +++ b/main.go @@ -11,7 +11,7 @@ import ( "regexp" "strings" - "github.com/kisielk/errcheck/internal/errcheck" + "github.com/kisielk/errcheck/errcheck" ) const ( diff --git a/main_test.go b/main_test.go index 2613e1f..6cc256e 100644 --- a/main_test.go +++ b/main_test.go @@ -9,7 +9,7 @@ import ( "strings" "testing" - "github.com/kisielk/errcheck/internal/errcheck" + "github.com/kisielk/errcheck/errcheck" ) func TestMain(t *testing.T) { From b03026b98dac970e9cffc6ee12a273f26c389026 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Mon, 3 Aug 2020 21:45:23 -0700 Subject: [PATCH 02/19] Fix up the docs a bit Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index ddf9b11..710aee2 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -1,6 +1,4 @@ // Package errcheck is the library used to implement the errcheck command-line tool. -// -// Note: The API of this package has not been finalized and may change at any point. package errcheck import ( @@ -87,6 +85,7 @@ type UncheckedErrors struct { Errors []UncheckedError } +// Append appends errors to e. It is goroutine-safe. func (e *UncheckedErrors) Append(errors ...UncheckedError) { e.mu.Lock() defer e.mu.Unlock() @@ -124,28 +123,33 @@ func (e byName) Less(i, j int) bool { return ei.Line < ej.Line } +// Checker checks that you checked errors. type Checker struct { - // ignore is a map of package names to regular expressions. Identifiers from a package are + // Ignore is a map of package names to regular expressions. Identifiers from a package are // checked against its regular expressions and if any of the expressions match the call // is not checked. Ignore map[string]*regexp.Regexp - // If blank is true then assignments to the blank identifier are also considered to be + // Blank, if true, means assignments to the blank identifier are also considered to be // ignored errors. Blank bool - // If asserts is true then ignored type assertion results are also checked + // Asserts causes ignored type assertion results to also be checked. Asserts bool - // build tags + // Tags are a list of build tags to use. Tags []string + // Verbose causes extra information to be output to stdout. Verbose bool - // If true, checking of _test.go files is disabled + // WithoutTests disables checking of _test.go files. WithoutTests bool - // If true, checking of files with generated code is disabled + // WithoutGeneratedCode disables checking of files with generated code. + // It behaves according to the following regular expression: + // + // ^// Code generated .* DO NOT EDIT\\.$ WithoutGeneratedCode bool exclude map[string]bool @@ -155,6 +159,7 @@ func NewChecker() *Checker { return &Checker{exclude: map[string]bool{}} } +// AddExcludes adds expressions to exclude from checking. func (c *Checker) AddExcludes(excludes []string) { for _, k := range excludes { c.logf("Excluding %v", k) From 5665fc95cdf968554fa76dc647d3fa31627c79ac Mon Sep 17 00:00:00 2001 From: Sergey Vilgelm Date: Tue, 18 Aug 2020 16:34:08 -0500 Subject: [PATCH 03/19] CheckPackage function --- errcheck/errcheck.go | 105 ++++++++++++++++++++++---------------- errcheck/errcheck_test.go | 8 +-- main.go | 2 +- 3 files changed, 67 insertions(+), 48 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 710aee2..8f6ea07 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -205,8 +205,59 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { return false } -// CheckPackages checks packages for errors. -func (c *Checker) CheckPackages(paths ...string) error { +var ( + goModStatus bool + goModOnce sync.Once +) + +func isGoMod() bool { + goModOnce.Do(func() { + gomod, err := exec.Command("go", "env", "GOMOD").Output() + goModStatus = (err == nil) && strings.TrimSpace(string(gomod)) != "" + }) + return goModStatus +} + +// CheckPaths checks packages for errors. +func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { + c.logf("Checking %s", pkg.Types.Path()) + + v := &visitor{ + pkg: pkg, + ignore: c.Ignore, + blank: c.Blank, + asserts: c.Asserts, + lines: make(map[string][]string), + exclude: c.exclude, + errors: []UncheckedError{}, + } + + for _, astFile := range v.pkg.Syntax { + if c.shouldSkipFile(astFile) { + continue + } + ast.Walk(v, astFile) + } + return v.errors +} + +// UpdateNonVendoredIgnore updates the Ignores to use non vendored paths +func (c *Checker) UpdateNonVendoredIgnore() { + if isGoMod() { + 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 + } + } + c.Ignore = ignore + } +} + +// CheckPaths checks packages for errors. +func (c *Checker) CheckPaths(paths ...string) error { pkgs, err := c.load(paths...) if err != nil { return err @@ -221,19 +272,7 @@ func (c *Checker) CheckPackages(paths ...string) error { } 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 - } - } - } + c.UpdateNonVendoredIgnore() var wg sync.WaitGroup u := &UncheckedErrors{} @@ -243,26 +282,7 @@ func (c *Checker) CheckPackages(paths ...string) error { go func() { defer wg.Done() 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.Syntax { - if c.shouldSkipFile(astFile) { - continue - } - ast.Walk(v, astFile) - } - u.Append(v.errors...) + u.Append(c.CheckPackage(pkg)...) } }() } @@ -286,13 +306,12 @@ func (c *Checker) CheckPackages(paths ...string) error { // visitor implements the errcheck algorithm type visitor struct { - pkg *packages.Package - ignore map[string]*regexp.Regexp - blank bool - asserts bool - lines map[string][]string - exclude map[string]bool - go111module bool + pkg *packages.Package + ignore map[string]*regexp.Regexp + blank bool + asserts bool + lines map[string][]string + exclude map[string]bool errors []UncheckedError } @@ -461,7 +480,7 @@ 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 !v.go111module { + if !isGoMod() { if nonVendoredPkg, ok := nonVendoredPkgPath(pkg.Path()); ok { if re, ok := v.ignore[nonVendoredPkg]; ok { return re.MatchString(id.Name) diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index f3343ad..2a825c1 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -176,7 +176,7 @@ package custom pkgs, err := packages.Load(cfg, paths...) return pkgs, err } - err := checker.CheckPackages("github.com/testbuildtags") + err := checker.CheckPaths("github.com/testbuildtags") if currCase.numExpectedErrs == 0 { if err != nil { @@ -282,7 +282,7 @@ require github.com/testlog v0.0.0 pkgs, err := packages.Load(cfg, paths...) return pkgs, err } - err := checker.CheckPackages("github.com/testvendor") + err := checker.CheckPaths("github.com/testvendor") if currCase.numExpectedErrs == 0 { if err != nil { @@ -378,7 +378,7 @@ require github.com/testlog v0.0.0 pkgs, err := packages.Load(cfg, paths...) return pkgs, err } - err := checker.CheckPackages(path.Join("github.com/testvendor")) + err := checker.CheckPaths(path.Join("github.com/testvendor")) if currCase.numExpectedErrs == 0 { if err != nil { @@ -411,7 +411,7 @@ func test(t *testing.T, f flags) { checker.AddExcludes([]string{ fmt.Sprintf("(%s.ErrorMakerInterface).MakeNilError", testPackage), }) - err := checker.CheckPackages(testPackage) + err := checker.CheckPaths(testPackage) uerr, ok := err.(*UncheckedErrors) if !ok { t.Fatalf("wrong error type returned: %v", err) diff --git a/main.go b/main.go index cf44aa6..2bcce3c 100644 --- a/main.go +++ b/main.go @@ -111,7 +111,7 @@ func mainCmd(args []string) int { return err } - if err := checker.CheckPackages(paths...); err != nil { + if err := checker.CheckPaths(paths...); err != nil { if e, ok := err.(*errcheck.UncheckedErrors); ok { reportUncheckedErrors(e, checker.Verbose) return exitUncheckedError From b4ad18c9bd2e439519306b36bbb8254cb6537328 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Tue, 18 Aug 2020 19:37:37 -0700 Subject: [PATCH 04/19] Start factoring out exclusion flags into separate struct. --- errcheck/errcheck.go | 35 ++++++++++++++++++++++++----------- errcheck/errcheck_test.go | 2 +- main.go | 5 +++-- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 710aee2..3f48031 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -123,6 +123,26 @@ func (e byName) Less(i, j int) bool { return ei.Line < ej.Line } +// Exclusions define symbols and language elements that will be not checked +type Exclusions struct { + // Packages []string + // Symbols []string + + // TestFiles excludes _test.go files. + TestFiles bool + + // GeneratedFiles excludes generated source files. + // + // Source file is assumed to be generated if its contents + // match the following regular expression: + // + // ^// Code generated .* DO NOT EDIT\\.$ + GeneratedFiles bool + + // BlankAssignments bool + // TypeAssertions bool +} + // Checker checks that you checked errors. type Checker struct { // Ignore is a map of package names to regular expressions. Identifiers from a package are @@ -137,21 +157,14 @@ type Checker struct { // Asserts causes ignored type assertion results to also be checked. Asserts bool + Exclusions Exclusions + // Tags are a list of build tags to use. Tags []string // Verbose causes extra information to be output to stdout. Verbose bool - // WithoutTests disables checking of _test.go files. - WithoutTests bool - - // WithoutGeneratedCode disables checking of files with generated code. - // It behaves according to the following regular expression: - // - // ^// Code generated .* DO NOT EDIT\\.$ - WithoutGeneratedCode bool - exclude map[string]bool } @@ -181,7 +194,7 @@ var loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Pack func (c *Checker) load(paths ...string) ([]*packages.Package, error) { cfg := &packages.Config{ Mode: packages.LoadAllSyntax, - Tests: !c.WithoutTests, + Tests: !c.Exclusions.TestFiles, BuildFlags: []string{fmtTags(c.Tags)}, } return loadPackages(cfg, paths...) @@ -190,7 +203,7 @@ func (c *Checker) load(paths ...string) ([]*packages.Package, error) { var generatedCodeRegexp = regexp.MustCompile("^// Code generated .* DO NOT EDIT\\.$") func (c *Checker) shouldSkipFile(file *ast.File) bool { - if !c.WithoutGeneratedCode { + if !c.Exclusions.GeneratedFiles { return false } diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index f3343ad..6c3a09a 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -369,7 +369,7 @@ require github.com/testlog v0.0.0 for i, currCase := range cases { checker := NewChecker() - checker.WithoutGeneratedCode = currCase.withoutGeneratedCode + checker.Exclusions.GeneratedFiles = currCase.withoutGeneratedCode loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { cfg.Env = append(os.Environ(), "GOPATH="+tmpGopath, diff --git a/main.go b/main.go index cf44aa6..887a36e 100644 --- a/main.go +++ b/main.go @@ -127,10 +127,11 @@ func mainCmd(args []string) int { func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { flags := flag.NewFlagSet(args[0], flag.ContinueOnError) + 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.Exclusions.TestFiles, "ignoretests", false, "if true, checking of _test.go files is disabled") + flags.BoolVar(&checker.Exclusions.GeneratedFiles, "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") From f1024b1b8de4ab1a20b281faa9d3c5018171e5a9 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Tue, 18 Aug 2020 20:00:56 -0700 Subject: [PATCH 05/19] Move Checker.exclude to Checker.Exclusions.Symbols --- errcheck/errcheck.go | 40 +++++++++++++++++++++++---------------- errcheck/errcheck_test.go | 6 +++--- main.go | 4 ++-- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 3f48031..087201f 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -28,9 +28,11 @@ func init() { var ( // ErrNoGoFiles is returned when CheckPackage is run on a package with no Go source files ErrNoGoFiles = errors.New("package contains no go source files") - // DefaultExcludes is a list of symbols that are excluded from checks by default. - // Note that they still need to be explicitly added to checker with AddExcludes(). - DefaultExcludes = []string{ + + // DefaultExcludedSymbols is a list of symbol names that are usually excluded from checks by default. + // + // Note, that they still need to be explicitly copied to Checker.Exclusions.Symbols + DefaultExcludedSymbols = []string{ // bytes "(*bytes.Buffer).Write", "(*bytes.Buffer).WriteByte", @@ -126,7 +128,16 @@ func (e byName) Less(i, j int) bool { // Exclusions define symbols and language elements that will be not checked type Exclusions struct { // Packages []string - // Symbols []string + + // Symbols lists regular expression patterns that exclude package symbols. + // + // For example: + // + // "fmt.Errorf" // function + // "fmt.Fprintf(os.Stderr)" // function with set argument value + // "(hash.Hash).Write" // method + // + Symbols []string // TestFiles excludes _test.go files. TestFiles bool @@ -137,6 +148,7 @@ type Exclusions struct { // match the following regular expression: // // ^// Code generated .* DO NOT EDIT\\.$ + // GeneratedFiles bool // BlankAssignments bool @@ -164,20 +176,10 @@ type Checker struct { // Verbose causes extra information to be output to stdout. Verbose bool - - exclude map[string]bool } func NewChecker() *Checker { - return &Checker{exclude: map[string]bool{}} -} - -// AddExcludes adds expressions to exclude from checking. -func (c *Checker) AddExcludes(excludes []string) { - for _, k := range excludes { - c.logf("Excluding %v", k) - c.exclude[k] = true - } + return &Checker{} } func (c *Checker) logf(msg string, args ...interface{}) { @@ -248,6 +250,12 @@ func (c *Checker) CheckPackages(paths ...string) error { } } + excludedSymbols := map[string]bool{} + for _, sym := range c.Exclusions.Symbols { + c.logf("Excluding %v", sym) + excludedSymbols[sym] = true + } + var wg sync.WaitGroup u := &UncheckedErrors{} for i := 0; i < runtime.NumCPU(); i++ { @@ -264,7 +272,7 @@ func (c *Checker) CheckPackages(paths ...string) error { blank: c.Blank, asserts: c.Asserts, lines: make(map[string][]string), - exclude: c.exclude, + exclude: excludedSymbols, go111module: go111module, errors: []UncheckedError{}, } diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 6c3a09a..2b19d50 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -407,10 +407,10 @@ func test(t *testing.T, f flags) { checker := NewChecker() checker.Asserts = asserts checker.Blank = blank - checker.AddExcludes(DefaultExcludes) - checker.AddExcludes([]string{ + checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, DefaultExcludedSymbols...) + checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, fmt.Sprintf("(%s.ErrorMakerInterface).MakeNilError", testPackage), - }) + ) err := checker.CheckPackages(testPackage) uerr, ok := err.(*UncheckedErrors) if !ok { diff --git a/main.go b/main.go index 887a36e..447ba63 100644 --- a/main.go +++ b/main.go @@ -154,7 +154,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { } if !excludeOnly { - checker.AddExcludes(errcheck.DefaultExcludes) + checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, errcheck.DefaultExcludedSymbols...) } if excludeFile != "" { @@ -163,7 +163,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { fmt.Fprintf(os.Stderr, "Could not read exclude file: %v\n", err) return nil, exitFatalError } - checker.AddExcludes(excludes) + checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, excludes...) } checker.Tags = tags From b694f8ea01a8da1f3f38edc62407a48889bbe738 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Tue, 18 Aug 2020 20:20:47 -0700 Subject: [PATCH 06/19] Move Checker.Blank and Checker.Asserts to Exclusions --- errcheck/errcheck.go | 18 +++++++----------- errcheck/errcheck_test.go | 4 ++-- main.go | 9 +++++++-- main_test.go | 17 +++++++++-------- 4 files changed, 25 insertions(+), 23 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 087201f..8565477 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -151,8 +151,11 @@ type Exclusions struct { // GeneratedFiles bool - // BlankAssignments bool - // TypeAssertions bool + // BlankAssignments ignores assignments to blank identifier. + BlankAssignments bool + + // TypeAssertions ignores unchecked type assertions. + TypeAssertions bool } // Checker checks that you checked errors. @@ -162,13 +165,6 @@ type Checker struct { // is not checked. Ignore map[string]*regexp.Regexp - // Blank, if true, means assignments to the blank identifier are also considered to be - // ignored errors. - Blank bool - - // Asserts causes ignored type assertion results to also be checked. - Asserts bool - Exclusions Exclusions // Tags are a list of build tags to use. @@ -269,8 +265,8 @@ func (c *Checker) CheckPackages(paths ...string) error { v := &visitor{ pkg: pkg, ignore: ignore, - blank: c.Blank, - asserts: c.Asserts, + blank: !c.Exclusions.BlankAssignments, + asserts: !c.Exclusions.TypeAssertions, lines: make(map[string][]string), exclude: excludedSymbols, go111module: go111module, diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 2b19d50..6f0666e 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -405,8 +405,8 @@ func test(t *testing.T, f flags) { blank bool = f&CheckBlank != 0 ) checker := NewChecker() - checker.Asserts = asserts - checker.Blank = blank + checker.Exclusions.TypeAssertions = !asserts + checker.Exclusions.BlankAssignments = !blank checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, DefaultExcludedSymbols...) checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, fmt.Sprintf("(%s.ErrorMakerInterface).MakeNilError", testPackage), diff --git a/main.go b/main.go index 447ba63..7985a57 100644 --- a/main.go +++ b/main.go @@ -128,8 +128,10 @@ func mainCmd(args []string) int { func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { flags := flag.NewFlagSet(args[0], flag.ContinueOnError) - 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") + var checkAsserts, checkBlanks bool + + flags.BoolVar(&checkBlanks, "blank", false, "if true, check for errors assigned to blank identifier") + flags.BoolVar(&checkAsserts, "asserts", false, "if true, check for ignored type assertion results") flags.BoolVar(&checker.Exclusions.TestFiles, "ignoretests", false, "if true, checking of _test.go files is disabled") flags.BoolVar(&checker.Exclusions.GeneratedFiles, "ignoregenerated", false, "if true, checking of files with generated code is disabled") flags.BoolVar(&checker.Verbose, "verbose", false, "produce more verbose logging") @@ -153,6 +155,9 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { return nil, exitFatalError } + checker.Exclusions.BlankAssignments = !checkBlanks + checker.Exclusions.TypeAssertions = !checkAsserts + if !excludeOnly { checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, errcheck.DefaultExcludedSymbols...) } diff --git a/main_test.go b/main_test.go index 1dbb960..65439bf 100644 --- a/main_test.go +++ b/main_test.go @@ -62,10 +62,11 @@ func TestMain(t *testing.T) { } type parseTestCase struct { - args []string - paths []string - ignore map[string]string - tags []string + args []string + paths []string + ignore map[string]string + tags []string + // here, blank and asserts are the inverse of TypeAssertions and BlankAssignments blank bool asserts bool error int @@ -221,11 +222,11 @@ func TestParseFlags(t *testing.T) { if tags := checker.Tags; !slicesEqual(tags, c.tags) { t.Errorf("%q: tags got %v want %v", argsStr, tags, c.tags) } - if b := checker.Blank; b != c.blank { - t.Errorf("%q: blank got %v want %v", argsStr, b, c.blank) + if b := checker.Exclusions.BlankAssignments; b != !c.blank { + t.Errorf("%q: BlankAssignments got %v want %v", argsStr, b, !c.blank) } - if a := checker.Asserts; a != c.asserts { - t.Errorf("%q: asserts got %v want %v", argsStr, a, c.asserts) + if a := checker.Exclusions.TypeAssertions; a != !c.asserts { + t.Errorf("%q: TypeAssertions got %v want %v", argsStr, a, !c.asserts) } if e != c.error { t.Errorf("%q: error got %q want %q", argsStr, e, c.error) From 5c4c91482021d113662018286fbd796a02121803 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Tue, 18 Aug 2020 20:39:56 -0700 Subject: [PATCH 07/19] Move Checker.Ignore to Exclusions.Packages, remove NewChecker --- errcheck/errcheck.go | 31 +++++++++------------- errcheck/errcheck_test.go | 21 +++++++-------- main.go | 55 +++++++-------------------------------- main_test.go | 53 ++++++++++++------------------------- 4 files changed, 48 insertions(+), 112 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 8565477..15da254 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -127,7 +127,8 @@ func (e byName) Less(i, j int) bool { // Exclusions define symbols and language elements that will be not checked type Exclusions struct { - // Packages []string + // Packages lists regular expression patterns that exclude whole packages. + Packages []string // Symbols lists regular expression patterns that exclude package symbols. // @@ -160,11 +161,7 @@ type Exclusions struct { // Checker checks that you checked errors. type Checker struct { - // Ignore is a map of package names to regular expressions. Identifiers from a package are - // checked against its regular expressions and if any of the expressions match the call - // is not checked. - Ignore map[string]*regexp.Regexp - + // Exclusions defines code packages, symbols, and other elements that will not be checked. Exclusions Exclusions // Tags are a list of build tags to use. @@ -174,10 +171,6 @@ type Checker struct { Verbose bool } -func NewChecker() *Checker { - return &Checker{} -} - func (c *Checker) logf(msg string, args ...interface{}) { if c.Verbose { fmt.Fprintf(os.Stderr, msg+"\n", args...) @@ -199,6 +192,7 @@ func (c *Checker) load(paths ...string) ([]*packages.Package, error) { } var generatedCodeRegexp = regexp.MustCompile("^// Code generated .* DO NOT EDIT\\.$") +var dotStar = regexp.MustCompile(".*") func (c *Checker) shouldSkipFile(file *ast.File) bool { if !c.Exclusions.GeneratedFiles { @@ -234,15 +228,14 @@ func (c *Checker) CheckPackages(paths ...string) error { 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 - } + + ignore := map[string]*regexp.Regexp{} + + for _, pkg := range c.Exclusions.Packages { + if nonVendoredPkg, ok := nonVendoredPkgPath(pkg); go111module && ok { + ignore[nonVendoredPkg] = dotStar + } else { + ignore[pkg] = dotStar } } diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 6f0666e..3b6348c 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "os" "path" - "regexp" "testing" "golang.org/x/tools/go/packages" @@ -166,7 +165,7 @@ package custom } for i, currCase := range cases { - checker := NewChecker() + var checker Checker checker.Tags = currCase.tags loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { @@ -249,7 +248,7 @@ require github.com/testlog v0.0.0 } cases := []struct { - ignore map[string]*regexp.Regexp + ignore []string numExpectedErrs int }{ // basic case has one error @@ -259,21 +258,21 @@ require github.com/testlog v0.0.0 }, // ignoring vendored import works { - ignore: map[string]*regexp.Regexp{ - path.Join("github.com/testvendor/vendor/github.com/testlog"): regexp.MustCompile("Info"), + ignore: []string{ + "github.com/testvendor/vendor/github.com/testlog", }, }, // non-vendored path ignores vendored import { - ignore: map[string]*regexp.Regexp{ - "github.com/testlog": regexp.MustCompile("Info"), + ignore: []string{ + "github.com/testlog", }, }, } for i, currCase := range cases { - checker := NewChecker() - checker.Ignore = currCase.ignore + var checker Checker + checker.Exclusions.Packages = currCase.ignore loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { cfg.Env = append(os.Environ(), "GOPATH="+tmpGopath, @@ -368,7 +367,7 @@ require github.com/testlog v0.0.0 } for i, currCase := range cases { - checker := NewChecker() + var checker Checker checker.Exclusions.GeneratedFiles = currCase.withoutGeneratedCode loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { cfg.Env = append(os.Environ(), @@ -404,7 +403,7 @@ func test(t *testing.T, f flags) { asserts bool = f&CheckAsserts != 0 blank bool = f&CheckBlank != 0 ) - checker := NewChecker() + var checker Checker checker.Exclusions.TypeAssertions = !asserts checker.Exclusions.BlankAssignments = !blank checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, DefaultExcludedSymbols...) diff --git a/main.go b/main.go index 7985a57..6f11ade 100644 --- a/main.go +++ b/main.go @@ -8,7 +8,6 @@ import ( "io/ioutil" "os" "path/filepath" - "regexp" "strings" "github.com/kisielk/errcheck/errcheck" @@ -22,43 +21,6 @@ const ( var abspath bool -type ignoreFlag map[string]*regexp.Regexp - -func (f ignoreFlag) String() string { - pairs := make([]string, 0, len(f)) - for pkg, re := range f { - prefix := "" - if pkg != "" { - prefix = pkg + ":" - } - pairs = append(pairs, prefix+re.String()) - } - return fmt.Sprintf("%q", strings.Join(pairs, ",")) -} - -func (f ignoreFlag) Set(s string) error { - if s == "" { - return nil - } - for _, pair := range strings.Split(s, ",") { - colonIndex := strings.Index(pair, ":") - var pkg, re string - if colonIndex == -1 { - pkg = "" - re = pair - } else { - pkg = pair[:colonIndex] - re = pair[colonIndex+1:] - } - regex, err := regexp.Compile(re) - if err != nil { - return err - } - f[pkg] = regex - } - return nil -} - type tagsFlag []string func (f *tagsFlag) String() string { @@ -80,8 +42,6 @@ func (f *tagsFlag) Set(s string) error { return nil } -var dotStar = regexp.MustCompile(".*") - func reportUncheckedErrors(e *errcheck.UncheckedErrors, verbose bool) { wd, err := os.Getwd() if err != nil { @@ -105,8 +65,8 @@ func reportUncheckedErrors(e *errcheck.UncheckedErrors, verbose bool) { } func mainCmd(args []string) int { - checker := errcheck.NewChecker() - paths, err := parseFlags(checker, args) + var checker errcheck.Checker + paths, err := parseFlags(&checker, args) if err != exitCodeOk { return err } @@ -141,9 +101,12 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { tags := tagsFlag{} flags.Var(&tags, "tags", "comma or 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{}) - flags.Var(ignore, "ignore", "[deprecated] comma-separated list of pairs of the form pkg:regex\n"+ + ignore := flags.String("ignore", "", "[deprecated] comma-separated list of pairs of the form pkg:regex\n"+ " the regex is used to ignore names within pkg.") + if *ignore != "" { + fmt.Fprintf(os.Stderr, "error: -ignore flag is deprecated, please use -ignorepkg instead") + return nil, exitFatalError + } var excludeFile string flags.StringVar(&excludeFile, "exclude", "", "Path to a file containing a list of functions to exclude from checking") @@ -174,15 +137,15 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { checker.Tags = tags for _, pkg := range strings.Split(*ignorePkg, ",") { if pkg != "" { - ignore[pkg] = dotStar + checker.Exclusions.Packages = append(checker.Exclusions.Packages, pkg) } } - checker.Ignore = ignore paths := flags.Args() if len(paths) == 0 { paths = []string{"."} } + return paths, exitCodeOk } diff --git a/main_test.go b/main_test.go index 65439bf..0ded443 100644 --- a/main_test.go +++ b/main_test.go @@ -5,7 +5,6 @@ import ( "io" "os" "reflect" - "regexp" "strings" "testing" @@ -64,7 +63,7 @@ func TestMain(t *testing.T) { type parseTestCase struct { args []string paths []string - ignore map[string]string + ignore []string tags []string // here, blank and asserts are the inverse of TypeAssertions and BlankAssignments blank bool @@ -77,7 +76,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{}, blank: false, asserts: false, @@ -86,7 +85,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-blank", "-asserts"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{}, blank: true, asserts: true, @@ -95,34 +94,16 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "foo", "bar"}, paths: []string{"foo", "bar"}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{}, blank: false, asserts: false, error: exitCodeOk, }, parseTestCase{ - args: []string{"errcheck", "-ignore", "fmt:.*,encoding/binary:.*"}, + args: []string{"errcheck", "-ignorepkg", "fmt,encoding/binary"}, paths: []string{"."}, - ignore: map[string]string{"fmt": ".*", "encoding/binary": dotStar.String()}, - tags: []string{}, - blank: false, - asserts: false, - error: exitCodeOk, - }, - parseTestCase{ - args: []string{"errcheck", "-ignore", "fmt:[FS]?[Pp]rint*"}, - paths: []string{"."}, - ignore: map[string]string{"fmt": "[FS]?[Pp]rint*"}, - tags: []string{}, - blank: false, - asserts: false, - error: exitCodeOk, - }, - parseTestCase{ - args: []string{"errcheck", "-ignore", "[rR]ead|[wW]rite"}, - paths: []string{"."}, - ignore: map[string]string{"": "[rR]ead|[wW]rite"}, + ignore: []string{"fmt", "encoding/binary"}, tags: []string{}, blank: false, asserts: false, @@ -131,7 +112,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing"}, paths: []string{"."}, - ignore: map[string]string{"testing": dotStar.String()}, + ignore: []string{"testing"}, tags: []string{}, blank: false, asserts: false, @@ -140,7 +121,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing,foo"}, paths: []string{"."}, - ignore: map[string]string{"testing": dotStar.String(), "foo": dotStar.String()}, + ignore: []string{"testing", "foo"}, tags: []string{}, blank: false, asserts: false, @@ -149,7 +130,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{"foo"}, blank: false, asserts: false, @@ -158,7 +139,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -167,7 +148,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo,bar,!baz"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -176,7 +157,7 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: map[string]string{}, + ignore: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -196,12 +177,12 @@ func TestParseFlags(t *testing.T) { return true } - ignoresEqual := func(a map[string]*regexp.Regexp, b map[string]string) bool { + ignoresEqual := func(a []string, b []string) bool { if len(a) != len(b) { return false } for k, v := range a { - if v.String() != b[k] { + if v != b[k] { return false } } @@ -209,14 +190,14 @@ func TestParseFlags(t *testing.T) { } for _, c := range cases { - checker := errcheck.NewChecker() - p, e := parseFlags(checker, c.args) + var checker errcheck.Checker + p, e := parseFlags(&checker, c.args) argsStr := strings.Join(c.args, " ") if !slicesEqual(p, c.paths) { t.Errorf("%q: path got %q want %q", argsStr, p, c.paths) } - if ign := checker.Ignore; !ignoresEqual(ign, c.ignore) { + if ign := checker.Exclusions.Packages; !ignoresEqual(ign, c.ignore) { t.Errorf("%q: ignore got %q want %q", argsStr, ign, c.ignore) } if tags := checker.Tags; !slicesEqual(tags, c.tags) { From f394fab58bbee8e285c1ea32607b5b6335785eaf Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Tue, 18 Aug 2020 21:46:00 -0700 Subject: [PATCH 08/19] Clarify -ignore deprecation message --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 6f11ade..bc2a256 100644 --- a/main.go +++ b/main.go @@ -104,7 +104,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { ignore := flags.String("ignore", "", "[deprecated] comma-separated list of pairs of the form pkg:regex\n"+ " the regex is used to ignore names within pkg.") if *ignore != "" { - fmt.Fprintf(os.Stderr, "error: -ignore flag is deprecated, please use -ignorepkg instead") + fmt.Fprintf(os.Stderr, "error: -ignore flag is deprecated, please use -exclude and/or -ignorepkg instead") return nil, exitFatalError } From 181d1bd11f7375d3b7433e4f8ee29a32c80bbb5f Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Fri, 9 Oct 2020 22:38:49 -0700 Subject: [PATCH 09/19] Simplify vendored package resolution. Independent of whether go mod is used or not, simply strip off vendor directory prefix from both imports and excludes if it was found. --- errcheck/errcheck.go | 60 ++++++++++---------------------------------- 1 file changed, 13 insertions(+), 47 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 2782976..53a6780 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -9,7 +9,6 @@ import ( "go/token" "go/types" "os" - "os/exec" "regexp" "runtime" "sort" @@ -210,19 +209,6 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { return false } -var ( - goModStatus bool - goModOnce sync.Once -) - -func isGoMod() bool { - goModOnce.Do(func() { - gomod, err := exec.Command("go", "env", "GOMOD").Output() - goModStatus = (err == nil) && strings.TrimSpace(string(gomod)) != "" - }) - return goModStatus -} - // CheckPaths checks packages for errors. func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { c.logf("Checking %s", pkg.Types.Path()) @@ -233,9 +219,14 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { excludedSymbols[sym] = true } + ignore := map[string]*regexp.Regexp{} + for _, pkg := range c.Exclusions.Packages { + ignore[nonVendoredPkgPath(pkg)] = dotStar + } + v := &visitor{ pkg: pkg, - ignore: c.getNonVendoredIgnores(), + ignore: ignore, blank: !c.Exclusions.BlankAssignments, asserts: !c.Exclusions.TypeAssertions, lines: make(map[string][]string), @@ -252,21 +243,6 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { return v.errors } -// getNonVendoredIgnores gets the ignore expressions for non-vendored packages. -// They are returned as a map from package path names to '.*' regular -// expressions. -func (c *Checker) getNonVendoredIgnores() map[string]*regexp.Regexp { - ignore := map[string]*regexp.Regexp{} - for _, pkg := range c.Exclusions.Packages { - if nonVendoredPkg, ok := nonVendoredPkgPath(pkg); isGoMod() && ok { - ignore[nonVendoredPkg] = dotStar - } else { - ignore[pkg] = dotStar - } - } - return ignore -} - // CheckPaths checks packages for errors. func (c *Checker) CheckPaths(paths ...string) error { pkgs, err := c.load(paths...) @@ -483,34 +459,24 @@ func (v *visitor) ignoreCall(call *ast.CallExpr) bool { if obj := v.pkg.TypesInfo.Uses[id]; obj != nil { if pkg := obj.Pkg(); pkg != nil { - if re, ok := v.ignore[pkg.Path()]; ok { + if re, ok := v.ignore[nonVendoredPkgPath(pkg.Path())]; ok { return re.MatchString(id.Name) } - - // if current package being considered is vendored, check to see if it should be ignored based - // on the unvendored path. - if !isGoMod() { - if nonVendoredPkg, ok := nonVendoredPkgPath(pkg.Path()); ok { - if re, ok := v.ignore[nonVendoredPkg]; ok { - return re.MatchString(id.Name) - } - } - } } } return false } -// nonVendoredPkgPath returns the unvendored version of the provided package path (or returns the provided path if it -// does not represent a vendored path). The second return value is true if the provided package was vendored, false -// otherwise. -func nonVendoredPkgPath(pkgPath string) (string, bool) { +// nonVendoredPkgPath returns the unvendored version of the provided package +// path (or returns the provided path if it does not represent a vendored +// path). +func nonVendoredPkgPath(pkgPath string) string { lastVendorIndex := strings.LastIndex(pkgPath, "/vendor/") if lastVendorIndex == -1 { - return pkgPath, false + return pkgPath } - return pkgPath[lastVendorIndex+len("/vendor/"):], true + return pkgPath[lastVendorIndex+len("/vendor/"):] } // errorsByArg returns a slice s such that From bd31643f78521d06e1d53f3442a01c7e60588d53 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Sat, 10 Oct 2020 00:16:10 -0700 Subject: [PATCH 10/19] Restore legacy ignore functionality; fix docstrings --- errcheck/errcheck.go | 24 ++++++++++-- errcheck/errcheck_test.go | 13 ++++--- main.go | 47 ++++++++++++++++++++--- main_test.go | 80 ++++++++++++++++++++++++++++----------- 4 files changed, 128 insertions(+), 36 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 53a6780..00c59db 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -126,10 +126,21 @@ func (e byName) Less(i, j int) bool { // Exclusions define symbols and language elements that will be not checked type Exclusions struct { - // Packages lists regular expression patterns that exclude whole packages. + + // Packages lists paths of excluded packages. Packages []string - // Symbols lists regular expression patterns that exclude package symbols. + // SymbolRegexpsByPackage maps individual package paths to regular + // expressions that match symbols to be excluded. + // + // Packages whose paths appear both here and in Packages list will + // be excluded entirely. + // + // This is a legacy input that will be deprecated in errcheck version 2 and + // should not be used. + SymbolRegexpsByPackage map[string]*regexp.Regexp + + // Symbols lists patterns that exclude individual package symbols. // // For example: // @@ -209,7 +220,7 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { return false } -// CheckPaths checks packages for errors. +// CheckPackage checks packages for errors. func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { c.logf("Checking %s", pkg.Types.Path()) @@ -220,6 +231,13 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { } ignore := map[string]*regexp.Regexp{} + // Apply SymbolRegexpsByPackage first so that if the same path appears in + // Packages, a more narrow regexp will be superceded by dotStar below. + if regexps := c.Exclusions.SymbolRegexpsByPackage; regexps != nil { + for pkg, re := range regexps { + ignore[nonVendoredPkgPath(pkg)] = re + } + } for _, pkg := range c.Exclusions.Packages { ignore[nonVendoredPkgPath(pkg)] = dotStar } diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index dba2666..0bac49d 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "path" + "regexp" "testing" "golang.org/x/tools/go/packages" @@ -248,7 +249,7 @@ require github.com/testlog v0.0.0 } cases := []struct { - ignore []string + ignore map[string]*regexp.Regexp numExpectedErrs int }{ // basic case has one error @@ -258,21 +259,21 @@ require github.com/testlog v0.0.0 }, // ignoring vendored import works { - ignore: []string{ - "github.com/testvendor/vendor/github.com/testlog", + ignore: map[string]*regexp.Regexp{ + path.Join("github.com/testvendor/vendor/github.com/testlog"): regexp.MustCompile("Info"), }, }, // non-vendored path ignores vendored import { - ignore: []string{ - "github.com/testlog", + ignore: map[string]*regexp.Regexp{ + "github.com/testlog": regexp.MustCompile("Info"), }, }, } for i, currCase := range cases { var checker Checker - checker.Exclusions.Packages = currCase.ignore + checker.Exclusions.SymbolRegexpsByPackage = currCase.ignore loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Package, error) { cfg.Env = append(os.Environ(), "GOPATH="+tmpGopath, diff --git a/main.go b/main.go index 8847779..c627ede 100644 --- a/main.go +++ b/main.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "path/filepath" + "regexp" "strings" "github.com/kisielk/errcheck/errcheck" @@ -21,6 +22,43 @@ const ( var abspath bool +type ignoreFlag map[string]*regexp.Regexp + +func (f ignoreFlag) String() string { + pairs := make([]string, 0, len(f)) + for pkg, re := range f { + prefix := "" + if pkg != "" { + prefix = pkg + ":" + } + pairs = append(pairs, prefix+re.String()) + } + return fmt.Sprintf("%q", strings.Join(pairs, ",")) +} + +func (f ignoreFlag) Set(s string) error { + if s == "" { + return nil + } + for _, pair := range strings.Split(s, ",") { + colonIndex := strings.Index(pair, ":") + var pkg, re string + if colonIndex == -1 { + pkg = "" + re = pair + } else { + pkg = pair[:colonIndex] + re = pair[colonIndex+1:] + } + regex, err := regexp.Compile(re) + if err != nil { + return err + } + f[pkg] = regex + } + return nil +} + type tagsFlag []string func (f *tagsFlag) String() string { @@ -101,12 +139,9 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { tags := tagsFlag{} flags.Var(&tags, "tags", "comma or space-separated list of build tags to include") ignorePkg := flags.String("ignorepkg", "", "comma-separated list of package paths to ignore") - ignore := flags.String("ignore", "", "[deprecated] comma-separated list of pairs of the form pkg:regex\n"+ + 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.") - if *ignore != "" { - fmt.Fprintf(os.Stderr, "error: -ignore flag is deprecated, please use -exclude and/or -ignorepkg instead") - return nil, exitFatalError - } var excludeFile string flags.StringVar(&excludeFile, "exclude", "", "Path to a file containing a list of functions to exclude from checking") @@ -141,6 +176,8 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { } } + checker.Exclusions.SymbolRegexpsByPackage = ignore + paths := flags.Args() if len(paths) == 0 { paths = []string{"."} diff --git a/main_test.go b/main_test.go index 0ded443..c03a964 100644 --- a/main_test.go +++ b/main_test.go @@ -5,12 +5,15 @@ import ( "io" "os" "reflect" + "regexp" "strings" "testing" "github.com/kisielk/errcheck/errcheck" ) +var dotStar = regexp.MustCompile(".*") + func TestMain(t *testing.T) { saveStderr := os.Stderr saveStdout := os.Stdout @@ -61,13 +64,13 @@ func TestMain(t *testing.T) { } type parseTestCase struct { - args []string - paths []string - ignore []string - tags []string - // here, blank and asserts are the inverse of TypeAssertions and BlankAssignments - blank bool - asserts bool + args []string + paths []string + ignore map[string]string // Exclusions.SymbolRegexpsByPackage + pkgs []string // == Exclusions.Packages + tags []string // Tags + blank bool // == !BlankAssignments + asserts bool // == !TypeAssertions error int } @@ -76,7 +79,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{}, blank: false, asserts: false, @@ -85,7 +89,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-blank", "-asserts"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{}, blank: true, asserts: true, @@ -94,16 +99,38 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "foo", "bar"}, paths: []string{"foo", "bar"}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{}, blank: false, asserts: false, error: exitCodeOk, }, parseTestCase{ - args: []string{"errcheck", "-ignorepkg", "fmt,encoding/binary"}, + args: []string{"errcheck", "-ignore", "fmt:.*,encoding/binary:.*"}, paths: []string{"."}, - ignore: []string{"fmt", "encoding/binary"}, + ignore: map[string]string{"fmt": ".*", "encoding/binary": dotStar.String()}, + pkgs: []string{}, + tags: []string{}, + blank: false, + asserts: false, + error: exitCodeOk, + }, + parseTestCase{ + args: []string{"errcheck", "-ignore", "fmt:[FS]?[Pp]rint*"}, + paths: []string{"."}, + ignore: map[string]string{"fmt": "[FS]?[Pp]rint*"}, + pkgs: []string{}, + tags: []string{}, + blank: false, + asserts: false, + error: exitCodeOk, + }, + parseTestCase{ + args: []string{"errcheck", "-ignore", "[rR]ead|[wW]rite"}, + paths: []string{"."}, + ignore: map[string]string{"": "[rR]ead|[wW]rite"}, + pkgs: []string{}, tags: []string{}, blank: false, asserts: false, @@ -112,7 +139,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing"}, paths: []string{"."}, - ignore: []string{"testing"}, + ignore: map[string]string{}, + pkgs: []string{"testing"}, tags: []string{}, blank: false, asserts: false, @@ -121,7 +149,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-ignorepkg", "testing,foo"}, paths: []string{"."}, - ignore: []string{"testing", "foo"}, + ignore: map[string]string{}, + pkgs: []string{"testing", "foo"}, tags: []string{}, blank: false, asserts: false, @@ -130,7 +159,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{"foo"}, blank: false, asserts: false, @@ -139,7 +169,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -148,7 +179,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo,bar,!baz"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -157,7 +189,8 @@ func TestParseFlags(t *testing.T) { parseTestCase{ args: []string{"errcheck", "-tags", "foo bar !baz"}, paths: []string{"."}, - ignore: []string{}, + ignore: map[string]string{}, + pkgs: []string{}, tags: []string{"foo", "bar", "!baz"}, blank: false, asserts: false, @@ -177,12 +210,12 @@ func TestParseFlags(t *testing.T) { return true } - ignoresEqual := func(a []string, b []string) bool { - if len(a) != len(b) { + ignoresEqual := func(a map[string]*regexp.Regexp, b map[string]string) bool { + if (a == nil && b != nil) || (a != nil && b == nil) || (len(a) != len(b)) { return false } for k, v := range a { - if v != b[k] { + if v.String() != b[k] { return false } } @@ -197,9 +230,12 @@ func TestParseFlags(t *testing.T) { if !slicesEqual(p, c.paths) { t.Errorf("%q: path got %q want %q", argsStr, p, c.paths) } - if ign := checker.Exclusions.Packages; !ignoresEqual(ign, c.ignore) { + if ign := checker.Exclusions.SymbolRegexpsByPackage; !ignoresEqual(ign, c.ignore) { t.Errorf("%q: ignore got %q want %q", argsStr, ign, c.ignore) } + if pkgs := checker.Exclusions.Packages; !slicesEqual(pkgs, c.pkgs) { + t.Errorf("%q: packages got %v want %v", argsStr, pkgs, c.pkgs) + } if tags := checker.Tags; !slicesEqual(tags, c.tags) { t.Errorf("%q: tags got %v want %v", argsStr, tags, c.tags) } From bf59cbd33b9b83ff42c57f1f9af44ab1c4b7a7f8 Mon Sep 17 00:00:00 2001 From: Dimitri Tcaciuc Date: Sat, 10 Oct 2020 00:23:26 -0700 Subject: [PATCH 11/19] Clean up comments; add future improvement TODO --- errcheck/errcheck.go | 2 ++ main_test.go | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 00c59db..50b8d12 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -235,10 +235,12 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { // Packages, a more narrow regexp will be superceded by dotStar below. if regexps := c.Exclusions.SymbolRegexpsByPackage; regexps != nil { for pkg, re := range regexps { + // TODO warn if previous entry overwritten? ignore[nonVendoredPkgPath(pkg)] = re } } for _, pkg := range c.Exclusions.Packages { + // TODO warn if previous entry overwritten? ignore[nonVendoredPkgPath(pkg)] = dotStar } diff --git a/main_test.go b/main_test.go index c03a964..60bf7d2 100644 --- a/main_test.go +++ b/main_test.go @@ -67,10 +67,10 @@ type parseTestCase struct { args []string paths []string ignore map[string]string // Exclusions.SymbolRegexpsByPackage - pkgs []string // == Exclusions.Packages + pkgs []string // Exclusions.Packages tags []string // Tags - blank bool // == !BlankAssignments - asserts bool // == !TypeAssertions + blank bool // !BlankAssignments + asserts bool // !TypeAssertions error int } From 677f1d67bcaed71f53df7703781718ebd0851e21 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Sun, 25 Oct 2020 14:06:18 -0700 Subject: [PATCH 12/19] Remove CheckPaths This commit removes the CheckPaths method of Checker. It also adds a method called LoadPackages, and exports more of the logic that errcheck uses to do aggregation. The UncheckedErrors type has been renamed to Result, and the CheckPackage function now returns a *Result instead of []UncheckedErrors. Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 137 +++++++++++----------------- errcheck/errcheck_test.go | 182 +++++++++++++++++++++----------------- main.go | 63 +++++++++++-- 3 files changed, 211 insertions(+), 171 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 50b8d12..33c404f 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -10,7 +10,6 @@ import ( "go/types" "os" "regexp" - "runtime" "sort" "strings" "sync" @@ -75,10 +74,10 @@ type UncheckedError struct { FuncName string } -// UncheckedErrors is returned from the CheckPackage function if the package contains +// Result is returned from the CheckPackage function if the package contains // any unchecked errors. // Errors should be appended using the Append method, which is safe to use concurrently. -type UncheckedErrors struct { +type Result struct { mu sync.Mutex // Errors is a list of all the unchecked errors in the package. @@ -86,24 +85,7 @@ type UncheckedErrors struct { Errors []UncheckedError } -// Append appends errors to e. It is goroutine-safe. -func (e *UncheckedErrors) Append(errors ...UncheckedError) { - e.mu.Lock() - defer e.mu.Unlock() - e.Errors = append(e.Errors, errors...) -} - -func (e *UncheckedErrors) Error() string { - return fmt.Sprintf("%d unchecked errors", len(e.Errors)) -} - -// Len is the number of elements in the collection. -func (e *UncheckedErrors) Len() int { return len(e.Errors) } - -// Swap swaps the elements with indexes i and j. -func (e *UncheckedErrors) Swap(i, j int) { e.Errors[i], e.Errors[j] = e.Errors[j], e.Errors[i] } - -type byName struct{ *UncheckedErrors } +type byName struct{ *Result } // Less reports whether the element with index i should sort before the element with index j. func (e byName) Less(i, j int) bool { @@ -124,6 +106,48 @@ func (e byName) Less(i, j int) bool { return ei.Line < ej.Line } +// Append appends errors to e. It is goroutine-safe, unlike the other methods +// on Result. Append does not do any duplicate checking. +func (r *Result) Append(other *Result) { + r.mu.Lock() + defer r.mu.Unlock() + r.Errors = append(r.Errors, other.Errors...) +} + +// Returns the unique errors that have been accumulated. Duplicates may occur +// when a file containing an unchecked error belongs to > 1 package. +// +// This function works in place, but returns the result anyways. +func (r *Result) Unique() *Result { + sort.Sort(byName{r}) + uniq := r.Errors[:0] // compact in-place + for i, err := range r.Errors { + if i == 0 || err != r.Errors[i-1] { + uniq = append(uniq, err) + } + } + r.Errors = uniq + return r +} + +func (r *Result) Error() string { + return fmt.Sprintf("%d unchecked errors", r.Len()) +} + +// Len is the number of elements in the collection. +func (r *Result) Len() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.Errors) +} + +// Swap swaps the elements with indexes i and j. +func (r *Result) Swap(i, j int) { + r.mu.Lock() + defer r.mu.Unlock() + r.Errors[i], r.Errors[j] = r.Errors[j], r.Errors[i] +} + // Exclusions define symbols and language elements that will be not checked type Exclusions struct { @@ -176,15 +200,6 @@ type Checker struct { // Tags are a list of build tags to use. Tags []string - - // Verbose causes extra information to be output to stdout. - Verbose bool -} - -func (c *Checker) logf(msg string, args ...interface{}) { - if c.Verbose { - fmt.Fprintf(os.Stderr, msg+"\n", args...) - } } // loadPackages is used for testing. @@ -192,7 +207,9 @@ var loadPackages = func(cfg *packages.Config, paths ...string) ([]*packages.Pack return packages.Load(cfg, paths...) } -func (c *Checker) load(paths ...string) ([]*packages.Package, error) { +// LoadPackages loads all the packages in all the paths provided. It uses the +// exclusions and build tags provided to by the user when loading the packages. +func (c *Checker) LoadPackages(paths ...string) ([]*packages.Package, error) { cfg := &packages.Config{ Mode: packages.LoadAllSyntax, Tests: !c.Exclusions.TestFiles, @@ -220,13 +237,13 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { return false } -// CheckPackage checks packages for errors. -func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { - c.logf("Checking %s", pkg.Types.Path()) - +// CheckPackage checks packages for errors that have not been checked. +// +// It will exclude specific errors from analysis if the user has configured +// exclusions. +func (c *Checker) CheckPackage(pkg *packages.Package) *Result { excludedSymbols := map[string]bool{} for _, sym := range c.Exclusions.Symbols { - c.logf("Excluding %v", sym) excludedSymbols[sym] = true } @@ -260,53 +277,7 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError { } ast.Walk(v, astFile) } - return v.errors -} - -// CheckPaths checks packages for errors. -func (c *Checker) CheckPaths(paths ...string) error { - pkgs, err := c.load(paths...) - if err != nil { - 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) - - var wg sync.WaitGroup - u := &UncheckedErrors{} - for i := 0; i < runtime.NumCPU(); i++ { - wg.Add(1) - - go func() { - defer wg.Done() - for pkg := range work { - u.Append(c.CheckPackage(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 + return &Result{Errors: v.errors} } // visitor implements the errcheck algorithm diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 0bac49d..63e3959 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -6,6 +6,7 @@ import ( "os" "path" "regexp" + "strings" "testing" "golang.org/x/tools/go/packages" @@ -165,35 +166,40 @@ package custom }, } - for i, currCase := range cases { - var checker Checker - checker.Tags = currCase.tags + for _, test := range cases { + testName := strings.Join(test.tags, ",") + t.Run(testName, func(t *testing.T) { + var checker Checker + checker.Tags = test.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.CheckPaths("github.com/testbuildtags") - - if currCase.numExpectedErrs == 0 { + 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 + } + packages, err := checker.LoadPackages("github.com/testbuildtags") if err != nil { - t.Errorf("Case %d: expected no errors, but got: %v", i, err) + t.Fatal(err) } - continue - } - uerr, ok := err.(*UncheckedErrors) - if !ok { - t.Errorf("Case %d: wrong error type returned: %v", i, err) - continue - } + uerr := &Result{} + for _, pkg := range packages { + uerr.Append(checker.CheckPackage(pkg)) + } + uerr = uerr.Unique() + if test.numExpectedErrs == 0 { + if uerr.Len() != 0 { + t.Errorf("expected no errors, but got: %v", uerr) + } + return + } - if currCase.numExpectedErrs != len(uerr.Errors) { - t.Errorf("Case %d:\nExpected: %d errors\nActual: %d errors", i, currCase.numExpectedErrs, len(uerr.Errors)) - } + if test.numExpectedErrs != uerr.Len() { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + } + }) } } @@ -271,35 +277,39 @@ require github.com/testlog v0.0.0 }, } - for i, currCase := range cases { - var checker Checker - checker.Exclusions.SymbolRegexpsByPackage = currCase.ignore - 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.CheckPaths("github.com/testvendor") - - if currCase.numExpectedErrs == 0 { + for i, test := range cases { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + var checker Checker + checker.Exclusions.SymbolRegexpsByPackage = test.ignore + 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 + } + packages, err := checker.LoadPackages("github.com/testvendor") if err != nil { - t.Errorf("Case %d: expected no errors, but got: %v", i, err) + t.Fatal(err) } - continue - } + uerr := &Result{} + for _, pkg := range packages { + uerr.Append(checker.CheckPackage(pkg)) + } + uerr = uerr.Unique() - uerr, ok := err.(*UncheckedErrors) - if !ok { - t.Errorf("Case %d: wrong error type returned: %v", i, err) - continue - } + if test.numExpectedErrs == 0 { + if uerr.Len() != 0 { + t.Errorf("expected no errors, but got: %v", uerr) + } + return + } - if currCase.numExpectedErrs != len(uerr.Errors) { - t.Errorf("Case %d:\nExpected: %d errors\nActual: %d errors", i, currCase.numExpectedErrs, len(uerr.Errors)) - } + if test.numExpectedErrs != uerr.Len() { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + } + }) } } @@ -367,35 +377,39 @@ require github.com/testlog v0.0.0 }, } - for i, currCase := range cases { - var checker Checker - checker.Exclusions.GeneratedFiles = 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.CheckPaths(path.Join("github.com/testvendor")) - - if currCase.numExpectedErrs == 0 { + for i, test := range cases { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + var checker Checker + checker.Exclusions.GeneratedFiles = test.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 + } + packages, err := checker.LoadPackages("github.com/testvendor") if err != nil { - t.Errorf("Case %d: expected no errors, but got: %v", i, err) + t.Fatal(err) } - continue - } + uerr := &Result{} + for _, pkg := range packages { + uerr.Append(checker.CheckPackage(pkg)) + } + uerr = uerr.Unique() - uerr, ok := err.(*UncheckedErrors) - if !ok { - t.Errorf("Case %d: wrong error type returned: %v", i, err) - continue - } + if test.numExpectedErrs == 0 { + if uerr.Len() != 0 { + t.Errorf("expected no errors, but got: %v", uerr) + } + return + } - if currCase.numExpectedErrs != len(uerr.Errors) { - t.Errorf("Case %d:\nExpected: %d errors\nActual: %d errors", i, currCase.numExpectedErrs, len(uerr.Errors)) - } + if test.numExpectedErrs != uerr.Len() { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + } + }) } } @@ -411,12 +425,11 @@ func test(t *testing.T, f flags) { checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, fmt.Sprintf("(%s.ErrorMakerInterface).MakeNilError", testPackage), ) - err := checker.CheckPaths(testPackage) - uerr, ok := err.(*UncheckedErrors) - if !ok { - t.Fatalf("wrong error type returned: %v", err) + packages, err := checker.LoadPackages(testPackage) + if err != nil { + t.Fatal(err) } - + uerr := &Result{} numErrors := len(uncheckedMarkers) if blank { numErrors += len(blankMarkers) @@ -425,8 +438,15 @@ func test(t *testing.T, f flags) { numErrors += len(assertMarkers) } - if len(uerr.Errors) != numErrors { - t.Errorf("got %d errors, want %d", len(uerr.Errors), numErrors) + for _, pkg := range packages { + err := checker.CheckPackage(pkg) + uerr.Append(err) + } + + uerr = uerr.Unique() + + if uerr.Len() != numErrors { + t.Errorf("got %d errors, want %d", uerr.Len(), numErrors) unchecked_loop: for k := range uncheckedMarkers { for _, e := range uerr.Errors { diff --git a/main.go b/main.go index c627ede..f9cd8d8 100644 --- a/main.go +++ b/main.go @@ -9,9 +9,12 @@ import ( "os" "path/filepath" "regexp" + "runtime" "strings" + "sync" "github.com/kisielk/errcheck/errcheck" + "golang.org/x/tools/go/packages" ) const ( @@ -20,10 +23,14 @@ const ( exitFatalError ) -var abspath bool - type ignoreFlag map[string]*regexp.Regexp +// global flags +var ( + abspath bool + verbose bool +) + func (f ignoreFlag) String() string { pairs := make([]string, 0, len(f)) for pkg, re := range f { @@ -80,7 +87,7 @@ func (f *tagsFlag) Set(s string) error { return nil } -func reportUncheckedErrors(e *errcheck.UncheckedErrors, verbose bool) { +func reportResult(e *errcheck.Result) { wd, err := os.Getwd() if err != nil { wd = "" @@ -102,6 +109,12 @@ func reportUncheckedErrors(e *errcheck.UncheckedErrors, verbose bool) { } } +func logf(msg string, args ...interface{}) { + if verbose { + fmt.Fprintf(os.Stderr, msg+"\n", args...) + } +} + func mainCmd(args []string) int { var checker errcheck.Checker paths, err := parseFlags(&checker, args) @@ -109,9 +122,9 @@ func mainCmd(args []string) int { return err } - if err := checker.CheckPaths(paths...); err != nil { - if e, ok := err.(*errcheck.UncheckedErrors); ok { - reportUncheckedErrors(e, checker.Verbose) + if err := checkPaths(&checker, paths...); err != nil { + if e, ok := err.(*errcheck.Result); ok { + reportResult(e) return exitUncheckedError } else if err == errcheck.ErrNoGoFiles { fmt.Fprintln(os.Stderr, err) @@ -123,6 +136,42 @@ func mainCmd(args []string) int { return exitCodeOk } +func checkPaths(c *errcheck.Checker, paths ...string) error { + pkgs, err := c.LoadPackages(paths...) + if err != nil { + 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) + + var wg sync.WaitGroup + u := &errcheck.Result{} + for i := 0; i < runtime.NumCPU(); i++ { + wg.Add(1) + + go func() { + defer wg.Done() + for pkg := range work { + logf("checking %s", pkg.Types.Path()) + u.Append(c.CheckPackage(pkg)) + } + }() + } + + wg.Wait() + if u.Len() > 0 { + return u.Unique() + } + return nil +} + func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { flags := flag.NewFlagSet(args[0], flag.ContinueOnError) @@ -132,7 +181,7 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { flags.BoolVar(&checkAsserts, "asserts", false, "if true, check for ignored type assertion results") flags.BoolVar(&checker.Exclusions.TestFiles, "ignoretests", false, "if true, checking of _test.go files is disabled") flags.BoolVar(&checker.Exclusions.GeneratedFiles, "ignoregenerated", false, "if true, checking of files with generated code is disabled") - flags.BoolVar(&checker.Verbose, "verbose", false, "produce more verbose logging") + flags.BoolVar(&verbose, "verbose", false, "produce more verbose logging") flags.BoolVar(&abspath, "abspath", false, "print absolute paths to files") From 8d25d8e4db2a685176d2a80d87c755245fb68692 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Sun, 25 Oct 2020 14:26:08 -0700 Subject: [PATCH 13/19] Refactor main.checkPaths a bit Signed-off-by: Eric Chlebek --- main.go | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/main.go b/main.go index f9cd8d8..fba4cb7 100644 --- a/main.go +++ b/main.go @@ -117,42 +117,44 @@ func logf(msg string, args ...interface{}) { func mainCmd(args []string) int { var checker errcheck.Checker - paths, err := parseFlags(&checker, args) - if err != exitCodeOk { - return err + paths, rc := parseFlags(&checker, args) + if rc != exitCodeOk { + return rc } - if err := checkPaths(&checker, paths...); err != nil { - if e, ok := err.(*errcheck.Result); ok { - reportResult(e) - return exitUncheckedError - } else if err == errcheck.ErrNoGoFiles { + result, err := checkPaths(&checker, paths...) + if err != nil { + if err == errcheck.ErrNoGoFiles { fmt.Fprintln(os.Stderr, err) return exitCodeOk } fmt.Fprintf(os.Stderr, "error: failed to check packages: %s\n", err) return exitFatalError } + if result.Len() > 0 { + reportResult(result) + return exitUncheckedError + } return exitCodeOk } -func checkPaths(c *errcheck.Checker, paths ...string) error { +func checkPaths(c *errcheck.Checker, paths ...string) (*errcheck.Result, error) { pkgs, err := c.LoadPackages(paths...) if err != nil { - return err + return nil, 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) + return nil, fmt.Errorf("errors while loading package %s: %v", pkg.ID, pkg.Errors) } work <- pkg } close(work) var wg sync.WaitGroup - u := &errcheck.Result{} + result := &errcheck.Result{} for i := 0; i < runtime.NumCPU(); i++ { wg.Add(1) @@ -160,16 +162,13 @@ func checkPaths(c *errcheck.Checker, paths ...string) error { defer wg.Done() for pkg := range work { logf("checking %s", pkg.Types.Path()) - u.Append(c.CheckPackage(pkg)) + result.Append(c.CheckPackage(pkg)) } }() } wg.Wait() - if u.Len() > 0 { - return u.Unique() - } - return nil + return result.Unique(), nil } func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) { From beb76a6146e40b139b69aaced5cc56080a5abc1d Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Sun, 25 Oct 2020 14:31:50 -0700 Subject: [PATCH 14/19] Remove mutex, Errors->UncheckedErrors Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 37 ++++++++++++++----------------------- errcheck/errcheck_test.go | 8 ++++---- main.go | 2 +- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 33c404f..253b6cb 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -12,7 +12,6 @@ import ( "regexp" "sort" "strings" - "sync" "golang.org/x/tools/go/packages" ) @@ -76,20 +75,19 @@ type UncheckedError struct { // Result is returned from the CheckPackage function if the package contains // any unchecked errors. -// Errors should be appended using the Append method, which is safe to use concurrently. +// +// Aggregation can be done using the Append method. type Result struct { - mu sync.Mutex - - // Errors is a list of all the unchecked errors in the package. + // UncheckedErrors is a list of all the unchecked errors in the package. // Printing an error reports its position within the file and the contents of the line. - Errors []UncheckedError + UncheckedErrors []UncheckedError } type byName struct{ *Result } // Less reports whether the element with index i should sort before the element with index j. func (e byName) Less(i, j int) bool { - ei, ej := e.Errors[i], e.Errors[j] + ei, ej := e.UncheckedErrors[i], e.UncheckedErrors[j] pi, pj := ei.Pos, ej.Pos @@ -106,12 +104,9 @@ func (e byName) Less(i, j int) bool { return ei.Line < ej.Line } -// Append appends errors to e. It is goroutine-safe, unlike the other methods -// on Result. Append does not do any duplicate checking. +// Append appends errors to e. Append does not do any duplicate checking. func (r *Result) Append(other *Result) { - r.mu.Lock() - defer r.mu.Unlock() - r.Errors = append(r.Errors, other.Errors...) + r.UncheckedErrors = append(r.UncheckedErrors, other.UncheckedErrors...) } // Returns the unique errors that have been accumulated. Duplicates may occur @@ -120,13 +115,13 @@ func (r *Result) Append(other *Result) { // This function works in place, but returns the result anyways. func (r *Result) Unique() *Result { sort.Sort(byName{r}) - uniq := r.Errors[:0] // compact in-place - for i, err := range r.Errors { - if i == 0 || err != r.Errors[i-1] { + uniq := r.UncheckedErrors[:0] // compact in-place + for i, err := range r.UncheckedErrors { + if i == 0 || err != r.UncheckedErrors[i-1] { uniq = append(uniq, err) } } - r.Errors = uniq + r.UncheckedErrors = uniq return r } @@ -136,16 +131,12 @@ func (r *Result) Error() string { // Len is the number of elements in the collection. func (r *Result) Len() int { - r.mu.Lock() - defer r.mu.Unlock() - return len(r.Errors) + return len(r.UncheckedErrors) } // Swap swaps the elements with indexes i and j. func (r *Result) Swap(i, j int) { - r.mu.Lock() - defer r.mu.Unlock() - r.Errors[i], r.Errors[j] = r.Errors[j], r.Errors[i] + r.UncheckedErrors[i], r.UncheckedErrors[j] = r.UncheckedErrors[j], r.UncheckedErrors[i] } // Exclusions define symbols and language elements that will be not checked @@ -277,7 +268,7 @@ func (c *Checker) CheckPackage(pkg *packages.Package) *Result { } ast.Walk(v, astFile) } - return &Result{Errors: v.errors} + return &Result{UncheckedErrors: v.errors} } // visitor implements the errcheck algorithm diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 63e3959..7814992 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -449,7 +449,7 @@ func test(t *testing.T, f flags) { t.Errorf("got %d errors, want %d", uerr.Len(), numErrors) unchecked_loop: for k := range uncheckedMarkers { - for _, e := range uerr.Errors { + for _, e := range uerr.UncheckedErrors { if newMarker(e) == k { continue unchecked_loop } @@ -459,7 +459,7 @@ func test(t *testing.T, f flags) { if blank { blank_loop: for k := range blankMarkers { - for _, e := range uerr.Errors { + for _, e := range uerr.UncheckedErrors { if newMarker(e) == k { continue blank_loop } @@ -470,7 +470,7 @@ func test(t *testing.T, f flags) { if asserts { assert_loop: for k := range assertMarkers { - for _, e := range uerr.Errors { + for _, e := range uerr.UncheckedErrors { if newMarker(e) == k { continue assert_loop } @@ -480,7 +480,7 @@ func test(t *testing.T, f flags) { } } - for i, err := range uerr.Errors { + for i, err := range uerr.UncheckedErrors { m := marker{err.Pos.Filename, err.Pos.Line} if !uncheckedMarkers[m] && !blankMarkers[m] && !assertMarkers[m] { t.Errorf("%d: unexpected error: %v", i, err) diff --git a/main.go b/main.go index fba4cb7..5edc685 100644 --- a/main.go +++ b/main.go @@ -92,7 +92,7 @@ func reportResult(e *errcheck.Result) { if err != nil { wd = "" } - for _, uncheckedError := range e.Errors { + for _, uncheckedError := range e.UncheckedErrors { pos := uncheckedError.Pos.String() if !abspath { newPos, err := filepath.Rel(wd, pos) From 53abe154d5747a93199a03518d358277e110d1e8 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Sun, 25 Oct 2020 14:34:40 -0700 Subject: [PATCH 15/19] Implement locking in main Signed-off-by: Eric Chlebek --- main.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index 5edc685..de123c2 100644 --- a/main.go +++ b/main.go @@ -155,6 +155,7 @@ func checkPaths(c *errcheck.Checker, paths ...string) (*errcheck.Result, error) var wg sync.WaitGroup result := &errcheck.Result{} + mu := &sync.Mutex{} for i := 0; i < runtime.NumCPU(); i++ { wg.Add(1) @@ -162,7 +163,10 @@ func checkPaths(c *errcheck.Checker, paths ...string) (*errcheck.Result, error) defer wg.Done() for pkg := range work { logf("checking %s", pkg.Types.Path()) - result.Append(c.CheckPackage(pkg)) + r := c.CheckPackage(pkg) + mu.Lock() + result.Append(r) + mu.Unlock() } }() } From cfa64d0ebe81d984adef810da348e02d740c5a42 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Mon, 9 Nov 2020 23:47:08 -0800 Subject: [PATCH 16/19] Remove sort.Interface from Result As suggested by dtcaciuc Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 24 +++++++++++------------- errcheck/errcheck_test.go | 22 +++++++++++----------- main.go | 2 +- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 253b6cb..d9e1149 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -86,8 +86,8 @@ type Result struct { type byName struct{ *Result } // Less reports whether the element with index i should sort before the element with index j. -func (e byName) Less(i, j int) bool { - ei, ej := e.UncheckedErrors[i], e.UncheckedErrors[j] +func (b byName) Less(i, j int) bool { + ei, ej := b.UncheckedErrors[i], b.UncheckedErrors[j] pi, pj := ei.Pos, ej.Pos @@ -104,6 +104,14 @@ func (e byName) Less(i, j int) bool { return ei.Line < ej.Line } +func (b byName) Swap(i, j int) { + b.UncheckedErrors[i], b.UncheckedErrors[j] = b.UncheckedErrors[j], b.UncheckedErrors[i] +} + +func (b byName) Len() int { + return len(b.UncheckedErrors) +} + // Append appends errors to e. Append does not do any duplicate checking. func (r *Result) Append(other *Result) { r.UncheckedErrors = append(r.UncheckedErrors, other.UncheckedErrors...) @@ -126,17 +134,7 @@ func (r *Result) Unique() *Result { } func (r *Result) Error() string { - return fmt.Sprintf("%d unchecked errors", r.Len()) -} - -// Len is the number of elements in the collection. -func (r *Result) Len() int { - return len(r.UncheckedErrors) -} - -// Swap swaps the elements with indexes i and j. -func (r *Result) Swap(i, j int) { - r.UncheckedErrors[i], r.UncheckedErrors[j] = r.UncheckedErrors[j], r.UncheckedErrors[i] + return fmt.Sprintf("%d unchecked errors", len(r.UncheckedErrors)) } // Exclusions define symbols and language elements that will be not checked diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index 7814992..bc9422d 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -190,14 +190,14 @@ package custom } uerr = uerr.Unique() if test.numExpectedErrs == 0 { - if uerr.Len() != 0 { + if len(uerr.UncheckedErrors) != 0 { t.Errorf("expected no errors, but got: %v", uerr) } return } - if test.numExpectedErrs != uerr.Len() { - t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + if test.numExpectedErrs != len(uerr.UncheckedErrors) { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, len(uerr.UncheckedErrors)) } }) } @@ -300,14 +300,14 @@ require github.com/testlog v0.0.0 uerr = uerr.Unique() if test.numExpectedErrs == 0 { - if uerr.Len() != 0 { + if len(uerr.UncheckedErrors) != 0 { t.Errorf("expected no errors, but got: %v", uerr) } return } - if test.numExpectedErrs != uerr.Len() { - t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + if test.numExpectedErrs != len(uerr.UncheckedErrors) { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, len(uerr.UncheckedErrors)) } }) } @@ -400,14 +400,14 @@ require github.com/testlog v0.0.0 uerr = uerr.Unique() if test.numExpectedErrs == 0 { - if uerr.Len() != 0 { + if len(uerr.UncheckedErrors) != 0 { t.Errorf("expected no errors, but got: %v", uerr) } return } - if test.numExpectedErrs != uerr.Len() { - t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, uerr.Len()) + if test.numExpectedErrs != len(uerr.UncheckedErrors) { + t.Errorf("expected: %d errors\nactual: %d errors", test.numExpectedErrs, len(uerr.UncheckedErrors)) } }) } @@ -445,8 +445,8 @@ func test(t *testing.T, f flags) { uerr = uerr.Unique() - if uerr.Len() != numErrors { - t.Errorf("got %d errors, want %d", uerr.Len(), numErrors) + if len(uerr.UncheckedErrors) != numErrors { + t.Errorf("got %d errors, want %d", len(uerr.UncheckedErrors), numErrors) unchecked_loop: for k := range uncheckedMarkers { for _, e := range uerr.UncheckedErrors { diff --git a/main.go b/main.go index de123c2..f29b183 100644 --- a/main.go +++ b/main.go @@ -131,7 +131,7 @@ func mainCmd(args []string) int { fmt.Fprintf(os.Stderr, "error: failed to check packages: %s\n", err) return exitFatalError } - if result.Len() > 0 { + if len(result.UncheckedErrors) > 0 { reportResult(result) return exitUncheckedError } From c4c6bdd8ac8eb202228c4404130f95ca85ee2c46 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Tue, 10 Nov 2020 00:01:08 -0800 Subject: [PATCH 17/19] Don't mutate Result on Unique() Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index d9e1149..154c68a 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -83,11 +83,11 @@ type Result struct { UncheckedErrors []UncheckedError } -type byName struct{ *Result } +type byName []UncheckedError // Less reports whether the element with index i should sort before the element with index j. func (b byName) Less(i, j int) bool { - ei, ej := b.UncheckedErrors[i], b.UncheckedErrors[j] + ei, ej := b[i], b[j] pi, pj := ei.Pos, ej.Pos @@ -105,11 +105,11 @@ func (b byName) Less(i, j int) bool { } func (b byName) Swap(i, j int) { - b.UncheckedErrors[i], b.UncheckedErrors[j] = b.UncheckedErrors[j], b.UncheckedErrors[i] + b[i], b[j] = b[j], b[i] } func (b byName) Len() int { - return len(b.UncheckedErrors) + return len(b) } // Append appends errors to e. Append does not do any duplicate checking. @@ -120,17 +120,18 @@ func (r *Result) Append(other *Result) { // Returns the unique errors that have been accumulated. Duplicates may occur // when a file containing an unchecked error belongs to > 1 package. // -// This function works in place, but returns the result anyways. +// The method receiver remains unmodified after the call to Unique. func (r *Result) Unique() *Result { - sort.Sort(byName{r}) - uniq := r.UncheckedErrors[:0] // compact in-place - for i, err := range r.UncheckedErrors { - if i == 0 || err != r.UncheckedErrors[i-1] { + result := make([]UncheckedError, len(r.UncheckedErrors)) + copy(result, r.UncheckedErrors) + sort.Sort((byName)(result)) + uniq := result[:0] // compact in-place + for i, err := range result { + if i == 0 || err != result[i-1] { uniq = append(uniq, err) } } - r.UncheckedErrors = uniq - return r + return &Result{UncheckedErrors: uniq} } func (r *Result) Error() string { From 6afc191a3074803d5c675cf901706d8f7aad285a Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Wed, 9 Dec 2020 21:39:43 -0800 Subject: [PATCH 18/19] Clarify Result docs Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 154c68a..010c849 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -73,10 +73,11 @@ type UncheckedError struct { FuncName string } -// Result is returned from the CheckPackage function if the package contains -// any unchecked errors. +// Result is returned from the CheckPackage function, and holds all the errors +// that were found to be unchecked in a package. // -// Aggregation can be done using the Append method. +// Aggregation can be done using the Append method for users that want to +// combine results from multiple packages. type Result struct { // UncheckedErrors is a list of all the unchecked errors in the package. // Printing an error reports its position within the file and the contents of the line. From 97c82e37fad1fbb99917469123753ce5ae8a07c3 Mon Sep 17 00:00:00 2001 From: Eric Chlebek Date: Wed, 9 Dec 2020 22:21:58 -0800 Subject: [PATCH 19/19] Make API less pointery. Get rid of error interface. Signed-off-by: Eric Chlebek --- errcheck/errcheck.go | 14 +++++--------- errcheck/errcheck_test.go | 8 ++++---- main.go | 8 ++++---- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/errcheck/errcheck.go b/errcheck/errcheck.go index 010c849..74bfaf0 100644 --- a/errcheck/errcheck.go +++ b/errcheck/errcheck.go @@ -114,7 +114,7 @@ func (b byName) Len() int { } // Append appends errors to e. Append does not do any duplicate checking. -func (r *Result) Append(other *Result) { +func (r *Result) Append(other Result) { r.UncheckedErrors = append(r.UncheckedErrors, other.UncheckedErrors...) } @@ -122,7 +122,7 @@ func (r *Result) Append(other *Result) { // when a file containing an unchecked error belongs to > 1 package. // // The method receiver remains unmodified after the call to Unique. -func (r *Result) Unique() *Result { +func (r Result) Unique() Result { result := make([]UncheckedError, len(r.UncheckedErrors)) copy(result, r.UncheckedErrors) sort.Sort((byName)(result)) @@ -132,11 +132,7 @@ func (r *Result) Unique() *Result { uniq = append(uniq, err) } } - return &Result{UncheckedErrors: uniq} -} - -func (r *Result) Error() string { - return fmt.Sprintf("%d unchecked errors", len(r.UncheckedErrors)) + return Result{UncheckedErrors: uniq} } // Exclusions define symbols and language elements that will be not checked @@ -232,7 +228,7 @@ func (c *Checker) shouldSkipFile(file *ast.File) bool { // // It will exclude specific errors from analysis if the user has configured // exclusions. -func (c *Checker) CheckPackage(pkg *packages.Package) *Result { +func (c *Checker) CheckPackage(pkg *packages.Package) Result { excludedSymbols := map[string]bool{} for _, sym := range c.Exclusions.Symbols { excludedSymbols[sym] = true @@ -268,7 +264,7 @@ func (c *Checker) CheckPackage(pkg *packages.Package) *Result { } ast.Walk(v, astFile) } - return &Result{UncheckedErrors: v.errors} + return Result{UncheckedErrors: v.errors} } // visitor implements the errcheck algorithm diff --git a/errcheck/errcheck_test.go b/errcheck/errcheck_test.go index bc9422d..472c023 100644 --- a/errcheck/errcheck_test.go +++ b/errcheck/errcheck_test.go @@ -188,7 +188,7 @@ package custom for _, pkg := range packages { uerr.Append(checker.CheckPackage(pkg)) } - uerr = uerr.Unique() + *uerr = uerr.Unique() if test.numExpectedErrs == 0 { if len(uerr.UncheckedErrors) != 0 { t.Errorf("expected no errors, but got: %v", uerr) @@ -297,7 +297,7 @@ require github.com/testlog v0.0.0 for _, pkg := range packages { uerr.Append(checker.CheckPackage(pkg)) } - uerr = uerr.Unique() + *uerr = uerr.Unique() if test.numExpectedErrs == 0 { if len(uerr.UncheckedErrors) != 0 { @@ -393,7 +393,7 @@ require github.com/testlog v0.0.0 if err != nil { t.Fatal(err) } - uerr := &Result{} + uerr := Result{} for _, pkg := range packages { uerr.Append(checker.CheckPackage(pkg)) } @@ -429,7 +429,7 @@ func test(t *testing.T, f flags) { if err != nil { t.Fatal(err) } - uerr := &Result{} + uerr := Result{} numErrors := len(uncheckedMarkers) if blank { numErrors += len(blankMarkers) diff --git a/main.go b/main.go index f29b183..6369927 100644 --- a/main.go +++ b/main.go @@ -87,7 +87,7 @@ func (f *tagsFlag) Set(s string) error { return nil } -func reportResult(e *errcheck.Result) { +func reportResult(e errcheck.Result) { wd, err := os.Getwd() if err != nil { wd = "" @@ -138,16 +138,16 @@ func mainCmd(args []string) int { return exitCodeOk } -func checkPaths(c *errcheck.Checker, paths ...string) (*errcheck.Result, error) { +func checkPaths(c *errcheck.Checker, paths ...string) (errcheck.Result, error) { pkgs, err := c.LoadPackages(paths...) if err != nil { - return nil, err + return errcheck.Result{}, 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 nil, fmt.Errorf("errors while loading package %s: %v", pkg.ID, pkg.Errors) + return errcheck.Result{}, fmt.Errorf("errors while loading package %s: %v", pkg.ID, pkg.Errors) } work <- pkg }