Skip to content

Commit

Permalink
Merge pull request kisielk#189 from dtcaciuc/exclusions
Browse files Browse the repository at this point in the history 
Factor out exclusion flags into a separate struct
  • Loading branch information
@echlebek
echlebek authored Oct 23, 2020
2 parents 7c04f7d + bf59cbd commit 720fbcb
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 126 deletions.
173 changes: 84 additions & 89 deletions errcheck/errcheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"go/token"
"go/types"
"os"
"os/exec"
"regexp"
"runtime"
"sort"
Expand All @@ -28,9 +27,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",
Expand Down Expand Up @@ -123,48 +124,61 @@ 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
// checked against its regular expressions and if any of the expressions match the call
// is not checked.
Ignore map[string]*regexp.Regexp
// Exclusions define symbols and language elements that will be not checked
type Exclusions struct {

// Blank, if true, means assignments to the blank identifier are also considered to be
// ignored errors.
Blank bool
// Packages lists paths of excluded packages.
Packages []string

// Asserts causes ignored type assertion results to also be checked.
Asserts bool

// Tags are a list of build tags to use.
Tags []string
// 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

// Verbose causes extra information to be output to stdout.
Verbose bool
// Symbols lists patterns that exclude individual package symbols.
//
// For example:
//
// "fmt.Errorf" // function
// "fmt.Fprintf(os.Stderr)" // function with set argument value
// "(hash.Hash).Write" // method
//
Symbols []string

// WithoutTests disables checking of _test.go files.
WithoutTests bool
// TestFiles excludes _test.go files.
TestFiles bool

// WithoutGeneratedCode disables checking of files with generated code.
// It behaves according to the following regular expression:
// 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\\.$
WithoutGeneratedCode bool
//
GeneratedFiles bool

exclude map[string]bool
}
// BlankAssignments ignores assignments to blank identifier.
BlankAssignments bool

func NewChecker() *Checker {
return &Checker{exclude: map[string]bool{}}
// TypeAssertions ignores unchecked type assertions.
TypeAssertions 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
}
// Checker checks that you checked errors.
type Checker struct {
// Exclusions defines code packages, symbols, and other elements that will not be checked.
Exclusions Exclusions

// 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{}) {
Expand All @@ -181,16 +195,17 @@ 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...)
}

var generatedCodeRegexp = regexp.MustCompile("^// Code generated .* DO NOT EDIT\\.$")
var dotStar = regexp.MustCompile(".*")

func (c *Checker) shouldSkipFile(file *ast.File) bool {
if !c.WithoutGeneratedCode {
if !c.Exclusions.GeneratedFiles {
return false
}

Expand All @@ -205,30 +220,37 @@ 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.
// CheckPackage checks packages for errors.
func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError {
c.logf("Checking %s", pkg.Types.Path())

excludedSymbols := map[string]bool{}
for _, sym := range c.Exclusions.Symbols {
c.logf("Excluding %v", sym)
excludedSymbols[sym] = true
}

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 {
// 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
}

v := &visitor{
pkg: pkg,
ignore: c.Ignore,
blank: c.Blank,
asserts: c.Asserts,
ignore: ignore,
blank: !c.Exclusions.BlankAssignments,
asserts: !c.Exclusions.TypeAssertions,
lines: make(map[string][]string),
exclude: c.exclude,
exclude: excludedSymbols,
errors: []UncheckedError{},
}

Expand All @@ -241,21 +263,6 @@ func (c *Checker) CheckPackage(pkg *packages.Package) []UncheckedError {
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...)
Expand All @@ -272,8 +279,6 @@ func (c *Checker) CheckPaths(paths ...string) error {
}
close(work)

c.UpdateNonVendoredIgnore()

var wg sync.WaitGroup
u := &UncheckedErrors{}
for i := 0; i < runtime.NumCPU(); i++ {
Expand Down Expand Up @@ -474,34 +479,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
Expand Down
22 changes: 11 additions & 11 deletions errcheck/errcheck_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,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) {
Expand Down Expand Up @@ -272,8 +272,8 @@ require github.com/testlog v0.0.0
}

for i, currCase := range cases {
checker := NewChecker()
checker.Ignore = currCase.ignore
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,
Expand Down Expand Up @@ -368,8 +368,8 @@ require github.com/testlog v0.0.0
}

for i, currCase := range cases {
checker := NewChecker()
checker.WithoutGeneratedCode = currCase.withoutGeneratedCode
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,
Expand Down Expand Up @@ -404,13 +404,13 @@ func test(t *testing.T, f flags) {
asserts bool = f&CheckAsserts != 0
blank bool = f&CheckBlank != 0
)
checker := NewChecker()
checker.Asserts = asserts
checker.Blank = blank
checker.AddExcludes(DefaultExcludes)
checker.AddExcludes([]string{
var checker Checker
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),
})
)
err := checker.CheckPaths(testPackage)
uerr, ok := err.(*UncheckedErrors)
if !ok {
Expand Down
30 changes: 18 additions & 12 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,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 {
Expand All @@ -105,8 +103,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
}
Expand All @@ -127,10 +125,13 @@ 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")

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")

flags.BoolVar(&abspath, "abspath", false, "print absolute paths to files")
Expand All @@ -152,8 +153,11 @@ func parseFlags(checker *errcheck.Checker, args []string) ([]string, int) {
return nil, exitFatalError
}

checker.Exclusions.BlankAssignments = !checkBlanks
checker.Exclusions.TypeAssertions = !checkAsserts

if !excludeOnly {
checker.AddExcludes(errcheck.DefaultExcludes)
checker.Exclusions.Symbols = append(checker.Exclusions.Symbols, errcheck.DefaultExcludedSymbols...)
}

if excludeFile != "" {
Expand All @@ -162,21 +166,23 @@ 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
for _, pkg := range strings.Split(*ignorePkg, ",") {
if pkg != "" {
ignore[pkg] = dotStar
checker.Exclusions.Packages = append(checker.Exclusions.Packages, pkg)
}
}
checker.Ignore = ignore

checker.Exclusions.SymbolRegexpsByPackage = ignore

paths := flags.Args()
if len(paths) == 0 {
paths = []string{"."}
}

return paths, exitCodeOk
}

Expand Down
Loading

0 comments on commit 720fbcb

Please sign in to comment.