From fab0c410c3be618283b6a45b27344b34e6309a38 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Thu, 17 Feb 2022 17:07:39 +0200 Subject: [PATCH] refactor: earlier compilation of `ignoreSigRegexp` (#24) * dev: refactor `regexp` usage * fix: separating `ignoreSigRegexp` usage * test: add missing testcase * fix: added missing `.wrapcheck.yaml` * Slight refactor, file for test analysistest skipping * Comment refactor Co-authored-by: Tom Arrell --- .../.wrapcheck.yaml | 2 + .../analysistest_skip | 0 wrapcheck/wrapcheck.go | 75 ++++++++++++------- wrapcheck/wrapcheck_test.go | 25 ++++++- 4 files changed, 73 insertions(+), 29 deletions(-) create mode 100644 wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml create mode 100644 wrapcheck/testdata/config_ignoreSigRegexps_fail/analysistest_skip diff --git a/wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml b/wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml new file mode 100644 index 0000000..1321c10 --- /dev/null +++ b/wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml @@ -0,0 +1,2 @@ +ignoreSigRegexps: +- json\.[a-zA-Z0-9_- diff --git a/wrapcheck/testdata/config_ignoreSigRegexps_fail/analysistest_skip b/wrapcheck/testdata/config_ignoreSigRegexps_fail/analysistest_skip new file mode 100644 index 0000000..e69de29 diff --git a/wrapcheck/wrapcheck.go b/wrapcheck/wrapcheck.go index 2b8be02..8a63105 100644 --- a/wrapcheck/wrapcheck.go +++ b/wrapcheck/wrapcheck.go @@ -1,6 +1,7 @@ package wrapcheck import ( + "fmt" "go/ast" "go/token" "go/types" @@ -13,18 +14,16 @@ import ( "golang.org/x/tools/go/analysis" ) -var ( - DefaultIgnoreSigs = []string{ - ".Errorf(", - "errors.New(", - "errors.Unwrap(", - ".Wrap(", - ".Wrapf(", - ".WithMessage(", - ".WithMessagef(", - ".WithStack(", - } -) +var DefaultIgnoreSigs = []string{ + ".Errorf(", + "errors.New(", + "errors.Unwrap(", + ".Wrap(", + ".Wrapf(", + ".WithMessage(", + ".WithMessagef(", + ".WithStack(", +} // WrapcheckConfig is the set of configuration values which configure the // behaviour of the linter. @@ -91,7 +90,14 @@ func NewAnalyzer(cfg WrapcheckConfig) *analysis.Analyzer { } func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { + // Precompile the regexps, report the error + ignoreSigRegexp, err := compileRegexps(cfg.IgnoreSigRegexps) + return func(pass *analysis.Pass) (interface{}, error) { + if err != nil { + return nil, err + } + for _, file := range pass.Files { ast.Inspect(file, func(n ast.Node) bool { ret, ok := n.(*ast.ReturnStmt) @@ -112,8 +118,9 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { // If the return type of the function is a single error. This will not // match an error within multiple return values, for that, the below // tuple check is required. + if isError(pass.TypesInfo.TypeOf(expr)) { - reportUnwrapped(pass, retFn, retFn.Pos(), cfg) + reportUnwrapped(pass, retFn, retFn.Pos(), cfg, ignoreSigRegexp) return true } @@ -131,7 +138,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { return true } if isError(v.Type()) { - reportUnwrapped(pass, retFn, expr.Pos(), cfg) + reportUnwrapped(pass, retFn, expr.Pos(), cfg, ignoreSigRegexp) return true } } @@ -146,9 +153,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { return true } - var ( - call *ast.CallExpr - ) + var call *ast.CallExpr // Attempt to find the most recent short assign if shortAss := prevErrAssign(pass, file, ident); shortAss != nil { @@ -195,7 +200,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { return true } - reportUnwrapped(pass, call, ident.NamePos, cfg) + reportUnwrapped(pass, call, ident.NamePos, cfg, ignoreSigRegexp) } return true @@ -208,7 +213,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { // Report unwrapped takes a call expression and an identifier and reports // if the call is unwrapped. -func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos, cfg WrapcheckConfig) { +func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos, cfg WrapcheckConfig, regexps []*regexp.Regexp) { sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return @@ -216,9 +221,10 @@ func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos // Check for ignored signatures fnSig := pass.TypesInfo.ObjectOf(sel.Sel).String() + if contains(cfg.IgnoreSigs, fnSig) { return - } else if containsMatch(cfg.IgnoreSigRegexps, fnSig) { + } else if containsMatch(regexps, fnSig) { return } @@ -245,6 +251,9 @@ func isInterface(pass *analysis.Pass, sel *ast.SelectorExpr) bool { return ok } +// isFromotherPkg returns whether the function is defined in the pacakge +// currently under analysis or is considered external. It will ignore packages +// defined in config.IgnorePackageGlobs. func isFromOtherPkg(pass *analysis.Pass, sel *ast.SelectorExpr, config WrapcheckConfig) bool { // The package of the function that we are calling which returns the error fn := pass.TypesInfo.ObjectOf(sel.Sel) @@ -331,14 +340,8 @@ func contains(slice []string, el string) bool { return false } -func containsMatch(slice []string, el string) bool { - for _, s := range slice { - re, err := regexp.Compile(s) - if err != nil { - log.Printf("unable to parse regexp: %s\n", s) - os.Exit(1) - } - +func containsMatch(regexps []*regexp.Regexp, el string) bool { + for _, re := range regexps { if re.MatchString(el) { return true } @@ -365,3 +368,19 @@ func isUnresolved(file *ast.File, ident *ast.Ident) bool { return false } + +// compileRegexps compiles a set of regular expressions returning them for use, +// or the first encountered error due to an invalid expression. +func compileRegexps(regexps []string) ([]*regexp.Regexp, error) { + var compiledRegexps []*regexp.Regexp + for _, reg := range regexps { + re, err := regexp.Compile(reg) + if err != nil { + return nil, fmt.Errorf("unable to compile regexp %s: %v\n", reg, err) + } + + compiledRegexps = append(compiledRegexps, re) + } + + return compiledRegexps, nil +} diff --git a/wrapcheck/wrapcheck_test.go b/wrapcheck/wrapcheck_test.go index 4760b69..1f76747 100644 --- a/wrapcheck/wrapcheck_test.go +++ b/wrapcheck/wrapcheck_test.go @@ -12,6 +12,10 @@ import ( "gopkg.in/yaml.v3" ) +// A file present in the directory named "analysis_skip" will cause the primary +// analysis tests to skip this directory due to needing explicit tests. +const skipfile = "analysistest_skip" + func TestAnalyzer(t *testing.T) { // Load the dirs under ./testdata p, err := filepath.Abs("./testdata") @@ -29,6 +33,12 @@ func TestAnalyzer(t *testing.T) { dirPath, err := filepath.Abs(path.Join("./testdata", f.Name())) assert.NoError(t, err) + // Check if the test is marked for skipping analysistest + if _, err := os.Stat(path.Join(dirPath, skipfile)); err == nil { + t.Logf("skipping test: %s\n", t.Name()) + t.Skip() + } + configPath := path.Join(dirPath, ".wrapcheck.yaml") if _, err := os.Stat(configPath); os.IsNotExist(err) { // There is no config @@ -40,7 +50,6 @@ func TestAnalyzer(t *testing.T) { var config WrapcheckConfig assert.NoError(t, yaml.Unmarshal(configFile, &config)) - analysistest.Run(t, dirPath, NewAnalyzer(config)) } else { assert.FailNow(t, err.Error()) @@ -48,3 +57,17 @@ func TestAnalyzer(t *testing.T) { }) } } + +func TestRegexpCompileFail(t *testing.T) { + configFile, err := os.ReadFile("./testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml") + assert.NoError(t, err) + + var config WrapcheckConfig + assert.NoError(t, yaml.Unmarshal(configFile, &config)) + + a := NewAnalyzer(config) + + results, err := a.Run(nil) // Doesn't matter what we pass + assert.Nil(t, results) + assert.Contains(t, err.Error(), "unable to compile regexp json\\.[a-zA-Z0-9_-") +}