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 <tom.arrell@gmail.com>

Author: butuzov

wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml

@@ -0,0 +1,2 @@
+ignoreSigRegexps:
+- json\.[a-zA-Z0-9_-

wrapcheck/testdata/config_ignoreSigRegexps_fail/analysistest_skip

@@ -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,17 +213,18 @@ 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
 	}
 
 	// 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
+}

wrapcheck/wrapcheck.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,11 +50,24 @@ 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())
 			}
 		})
 	}
 }
+
+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_-")
+}