From 233bc0237f96bb7ec8479ef5fe581c53acb621e3 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Fri, 4 Feb 2022 06:48:06 +0200 Subject: [PATCH 1/6] dev: refactor `regexp` usage --- wrapcheck/wrapcheck.go | 61 +++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/wrapcheck/wrapcheck.go b/wrapcheck/wrapcheck.go index 2b8be02..7ee439b 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. @@ -72,6 +71,9 @@ type WrapcheckConfig struct { // ignorePackageGlobs: // - encoding/* IgnorePackageGlobs []string `mapstructure:"ignorePackageGlobs" yaml:"ignorePackageGlobs"` + + // ignoreSigRegexpCompiled use to hold compiled results of the IgnoreSigRegexps + ignoreSigRegexpCompiled []*regexp.Regexp } func NewDefaultConfig() WrapcheckConfig { @@ -91,7 +93,24 @@ func NewAnalyzer(cfg WrapcheckConfig) *analysis.Analyzer { } func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { + var Err error + + for i := range cfg.IgnoreSigRegexps { + re, err := regexp.Compile(cfg.IgnoreSigRegexps[i]) + if err != nil { + Err = fmt.Errorf("unable to parse regexp: %v at %s\n", err, cfg.IgnoreSigRegexps[i]) + log.Printf(Err.Error()) + break + } + cfg.ignoreSigRegexpCompiled = append(cfg.ignoreSigRegexpCompiled, re) + } + return func(pass *analysis.Pass) (interface{}, error) { + // handling load errors. + 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,6 +131,7 @@ 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) return true @@ -146,9 +166,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 { @@ -216,9 +234,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(cfg.ignoreSigRegexpCompiled, fnSig) { return } @@ -331,14 +350,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 } From 9befe484bfc837b0d5795b84fc64aa3e3e13f942 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Fri, 4 Feb 2022 22:26:53 +0200 Subject: [PATCH 2/6] fix: separating `ignoreSigRegexp` usage --- wrapcheck/wrapcheck.go | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/wrapcheck/wrapcheck.go b/wrapcheck/wrapcheck.go index 7ee439b..c775fa1 100644 --- a/wrapcheck/wrapcheck.go +++ b/wrapcheck/wrapcheck.go @@ -71,9 +71,6 @@ type WrapcheckConfig struct { // ignorePackageGlobs: // - encoding/* IgnorePackageGlobs []string `mapstructure:"ignorePackageGlobs" yaml:"ignorePackageGlobs"` - - // ignoreSigRegexpCompiled use to hold compiled results of the IgnoreSigRegexps - ignoreSigRegexpCompiled []*regexp.Regexp } func NewDefaultConfig() WrapcheckConfig { @@ -93,22 +90,24 @@ func NewAnalyzer(cfg WrapcheckConfig) *analysis.Analyzer { } func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { - var Err error + var ( + regexpErr error + ignoreSigRegexp []*regexp.Regexp + ) for i := range cfg.IgnoreSigRegexps { re, err := regexp.Compile(cfg.IgnoreSigRegexps[i]) if err != nil { - Err = fmt.Errorf("unable to parse regexp: %v at %s\n", err, cfg.IgnoreSigRegexps[i]) - log.Printf(Err.Error()) + regexpErr = fmt.Errorf("unable to parse regexp: %v at %s\n", err, cfg.IgnoreSigRegexps[i]) + log.Printf(regexpErr.Error()) break } - cfg.ignoreSigRegexpCompiled = append(cfg.ignoreSigRegexpCompiled, re) + ignoreSigRegexp = append(ignoreSigRegexp, re) } return func(pass *analysis.Pass) (interface{}, error) { - // handling load errors. - if Err != nil { - return nil, Err + if regexpErr != nil { + return nil, regexpErr } for _, file := range pass.Files { @@ -133,7 +132,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { // 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 } @@ -151,7 +150,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 } } @@ -213,7 +212,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 @@ -226,7 +225,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 @@ -237,7 +236,7 @@ func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos if contains(cfg.IgnoreSigs, fnSig) { return - } else if containsMatch(cfg.ignoreSigRegexpCompiled, fnSig) { + } else if containsMatch(regexps, fnSig) { return } From fcc6caf476f8806b6a80780fd34ae3004332dd88 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Sat, 5 Feb 2022 14:24:06 +0200 Subject: [PATCH 3/6] test: add missing testcase --- wrapcheck/wrapcheck_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/wrapcheck/wrapcheck_test.go b/wrapcheck/wrapcheck_test.go index 4760b69..edc38ff 100644 --- a/wrapcheck/wrapcheck_test.go +++ b/wrapcheck/wrapcheck_test.go @@ -26,6 +26,10 @@ func TestAnalyzer(t *testing.T) { t.Fatalf("cannot run on non-directory: %s", f.Name()) } + if f.Name() == "config_ignoreSigRegexps_fail" { + t.Skipf("skipping %s, as it expect to fail in this test", f.Name()) + } + dirPath, err := filepath.Abs(path.Join("./testdata", f.Name())) assert.NoError(t, err) @@ -40,11 +44,26 @@ 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 TestFail(t *testing.T) { + // A config file exists, use it + 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 passing ... + + assert.Nil(t, results) + assert.EqualError(t, err, + "unable to parse regexp: error parsing regexp: missing closing ]: `[a-zA-Z0-9_-` at json\\.[a-zA-Z0-9_-\n") +} From 0c95bd6d552bc1c9cc6ba78862232ba2af2eeaec Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Mon, 14 Feb 2022 20:17:48 +0200 Subject: [PATCH 4/6] fix: added missing `.wrapcheck.yaml` --- wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 wrapcheck/testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml 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_- From cd5dcfed8275209845eb5b8e1cf67d5e71c426ee Mon Sep 17 00:00:00 2001 From: Tom Arrell Date: Thu, 17 Feb 2022 14:44:50 +0000 Subject: [PATCH 5/6] Slight refactor, file for test analysistest skipping --- .../analysistest_skip | 0 wrapcheck/wrapcheck.go | 39 +++++++++++-------- wrapcheck/wrapcheck_test.go | 22 ++++++----- 3 files changed, 36 insertions(+), 25 deletions(-) create mode 100644 wrapcheck/testdata/config_ignoreSigRegexps_fail/analysistest_skip 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 c775fa1..d39014d 100644 --- a/wrapcheck/wrapcheck.go +++ b/wrapcheck/wrapcheck.go @@ -90,24 +90,12 @@ func NewAnalyzer(cfg WrapcheckConfig) *analysis.Analyzer { } func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { - var ( - regexpErr error - ignoreSigRegexp []*regexp.Regexp - ) - - for i := range cfg.IgnoreSigRegexps { - re, err := regexp.Compile(cfg.IgnoreSigRegexps[i]) - if err != nil { - regexpErr = fmt.Errorf("unable to parse regexp: %v at %s\n", err, cfg.IgnoreSigRegexps[i]) - log.Printf(regexpErr.Error()) - break - } - ignoreSigRegexp = append(ignoreSigRegexp, re) - } + // Precompile the regexps, report the error + ignoreSigRegexp, err := compileRegexps(cfg.IgnoreSigRegexps) return func(pass *analysis.Pass) (interface{}, error) { - if regexpErr != nil { - return nil, regexpErr + if err != nil { + return nil, err } for _, file := range pass.Files { @@ -263,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) @@ -377,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 parse 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 edc38ff..001483d 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") @@ -26,13 +30,15 @@ func TestAnalyzer(t *testing.T) { t.Fatalf("cannot run on non-directory: %s", f.Name()) } - if f.Name() == "config_ignoreSigRegexps_fail" { - t.Skipf("skipping %s, as it expect to fail in this test", f.Name()) - } - 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 @@ -45,7 +51,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()) } @@ -53,7 +58,7 @@ func TestAnalyzer(t *testing.T) { } } -func TestFail(t *testing.T) { +func TestRegexpCompileFail(t *testing.T) { // A config file exists, use it configFile, err := os.ReadFile("./testdata/config_ignoreSigRegexps_fail/.wrapcheck.yaml") assert.NoError(t, err) @@ -61,9 +66,8 @@ func TestFail(t *testing.T) { var config WrapcheckConfig assert.NoError(t, yaml.Unmarshal(configFile, &config)) a := NewAnalyzer(config) - results, err := a.Run(nil) // doesn't matter what we passing ... + results, err := a.Run(nil) // Doesn't matter what we pass assert.Nil(t, results) - assert.EqualError(t, err, - "unable to parse regexp: error parsing regexp: missing closing ]: `[a-zA-Z0-9_-` at json\\.[a-zA-Z0-9_-\n") + assert.Contains(t, err.Error(), "unable to parse regexp json\\.[a-zA-Z0-9_-") } From a3a613808924d5f2e4409be443b7d0306c0aa906 Mon Sep 17 00:00:00 2001 From: Tom Arrell Date: Thu, 17 Feb 2022 15:01:08 +0000 Subject: [PATCH 6/6] Comment refactor --- wrapcheck/wrapcheck.go | 2 +- wrapcheck/wrapcheck_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/wrapcheck/wrapcheck.go b/wrapcheck/wrapcheck.go index d39014d..8a63105 100644 --- a/wrapcheck/wrapcheck.go +++ b/wrapcheck/wrapcheck.go @@ -376,7 +376,7 @@ func compileRegexps(regexps []string) ([]*regexp.Regexp, error) { for _, reg := range regexps { re, err := regexp.Compile(reg) if err != nil { - return nil, fmt.Errorf("unable to parse regexp %s: %v\n", reg, err) + return nil, fmt.Errorf("unable to compile regexp %s: %v\n", reg, err) } compiledRegexps = append(compiledRegexps, re) diff --git a/wrapcheck/wrapcheck_test.go b/wrapcheck/wrapcheck_test.go index 001483d..1f76747 100644 --- a/wrapcheck/wrapcheck_test.go +++ b/wrapcheck/wrapcheck_test.go @@ -59,15 +59,15 @@ func TestAnalyzer(t *testing.T) { } func TestRegexpCompileFail(t *testing.T) { - // A config file exists, use it 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 + results, err := a.Run(nil) // Doesn't matter what we pass assert.Nil(t, results) - assert.Contains(t, err.Error(), "unable to parse regexp json\\.[a-zA-Z0-9_-") + assert.Contains(t, err.Error(), "unable to compile regexp json\\.[a-zA-Z0-9_-") }