New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/ignore interface regex #25
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
ignoreInterfaceRegexps: | ||
- (errorer|error) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,25 @@ | ||||||
package main | ||||||
|
||||||
import ( | ||||||
"encoding/json" | ||||||
"strings" | ||||||
) | ||||||
|
||||||
type errorer interface { | ||||||
Decode(v interface{}) error | ||||||
} | ||||||
|
||||||
func main() { | ||||||
d := json.NewDecoder(strings.NewReader("hello world")) | ||||||
do(d) | ||||||
} | ||||||
|
||||||
func do(fn errorer) error { | ||||||
var str string | ||||||
err := fn.Decode(&str) | ||||||
if err != nil { | ||||||
return err // errorer interface ignored | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a minor suggestion
Suggested change
|
||||||
} | ||||||
|
||||||
return nil | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,11 +51,11 @@ type WrapcheckConfig struct { | |
// unwrapped. | ||
// | ||
// For example, an ignoreSigRegexp of `[]string{"\.New.*Err\("}`` will ignore errors | ||
// returned from any signture whose method name starts with "New" and ends with "Err" | ||
// returned from any signature whose method name starts with "New" and ends with "Err" | ||
// due to the signature matching the regular expression `\.New.*Err\(`. | ||
// | ||
// Note that this is similar to the ignoreSigs configuration, but provides | ||
// slightly more flexibility in defining rules by which signtures will be | ||
// slightly more flexibility in defining rules by which signatures will be | ||
// ignored. | ||
IgnoreSigRegexps []string `mapstructure:"ignoreSigRegexps" yaml:"ignoreSigRegexps"` | ||
|
||
|
@@ -71,13 +71,23 @@ type WrapcheckConfig struct { | |
// ignorePackageGlobs: | ||
// - encoding/* | ||
IgnorePackageGlobs []string `mapstructure:"ignorePackageGlobs" yaml:"ignorePackageGlobs"` | ||
|
||
// IgnoreInterfaceRegexps defines a list of regular expressions which, if matched | ||
// to a underlying interface name, will ignore unwrapped errors returned from a | ||
// function whose call is defined on the given interface. | ||
// | ||
// For example, an ignoreInterfaceRegexps of `[]string{"Transac(tor|tion)"}`` will ignore errors | ||
// returned from any function whose call is defined on a interface named 'Transactor' | ||
// or 'Transaction' due to the name matching the regular expression `Transac(tor|tion)`. | ||
IgnoreInterfaceRegexps []string `mapstructure:"ignoreInterfaceRegexps" yaml:"ignoreInterfaceRegexps"` | ||
} | ||
|
||
func NewDefaultConfig() WrapcheckConfig { | ||
return WrapcheckConfig{ | ||
IgnoreSigs: DefaultIgnoreSigs, | ||
IgnoreSigRegexps: []string{}, | ||
IgnorePackageGlobs: []string{}, | ||
IgnoreSigs: DefaultIgnoreSigs, | ||
IgnoreSigRegexps: []string{}, | ||
IgnorePackageGlobs: []string{}, | ||
IgnoreInterfaceRegexps: []string{}, | ||
} | ||
} | ||
|
||
|
@@ -91,11 +101,15 @@ 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) | ||
ignoreSigRegexp, err1 := compileRegexps(cfg.IgnoreSigRegexps) | ||
ignoreInterfaceRegexps, err2 := compileRegexps(cfg.IgnoreInterfaceRegexps) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking, I've actually made some similar changes, still local for now, in order to precompile the globs. Could we maybe update this to something along the lines of: var (
ignoreSigRegexp []*regexp.Regexp
ignorePackageGlobs []glob.Glob
err error
)
ignoreSigRegexp, err = compileRegexps(cfg.IgnoreSigRegexps)
if err != nil {
ignorePackageGlobs, err = compileGlobs(cfg.IgnorePackageGlobs)
} I will likely refactor this anyway in an upcoming PR, but for now I'd prefer not to have multiple error checks in the returned func below. |
||
|
||
return func(pass *analysis.Pass) (interface{}, error) { | ||
if err != nil { | ||
return nil, err | ||
if err1 != nil { | ||
return nil, err1 | ||
} | ||
if err2 != nil { | ||
return nil, err2 | ||
} | ||
|
||
for _, file := range pass.Files { | ||
|
@@ -120,7 +134,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, ignoreSigRegexp) | ||
reportUnwrapped(pass, retFn, retFn.Pos(), cfg, ignoreSigRegexp, ignoreInterfaceRegexps) | ||
return true | ||
} | ||
|
||
|
@@ -138,7 +152,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { | |
return true | ||
} | ||
if isError(v.Type()) { | ||
reportUnwrapped(pass, retFn, expr.Pos(), cfg, ignoreSigRegexp) | ||
reportUnwrapped(pass, retFn, expr.Pos(), cfg, ignoreSigRegexp, ignoreInterfaceRegexps) | ||
return true | ||
} | ||
} | ||
|
@@ -200,7 +214,7 @@ func run(cfg WrapcheckConfig) func(*analysis.Pass) (interface{}, error) { | |
return true | ||
} | ||
|
||
reportUnwrapped(pass, call, ident.NamePos, cfg, ignoreSigRegexp) | ||
reportUnwrapped(pass, call, ident.NamePos, cfg, ignoreSigRegexp, ignoreInterfaceRegexps) | ||
} | ||
|
||
return true | ||
|
@@ -213,7 +227,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, regexps []*regexp.Regexp) { | ||
func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos, cfg WrapcheckConfig, regexpsSig []*regexp.Regexp, regexpsInter []*regexp.Regexp) { | ||
sel, ok := call.Fun.(*ast.SelectorExpr) | ||
if !ok { | ||
return | ||
|
@@ -224,15 +238,18 @@ func reportUnwrapped(pass *analysis.Pass, call *ast.CallExpr, tokenPos token.Pos | |
|
||
if contains(cfg.IgnoreSigs, fnSig) { | ||
return | ||
} else if containsMatch(regexps, fnSig) { | ||
} else if containsMatch(regexpsSig, fnSig) { | ||
return | ||
} | ||
|
||
// Check if the underlying type of the "x" in x.y.z is an interface, as | ||
// errors returned from interface types should be wrapped. | ||
if isInterface(pass, sel) { | ||
pass.Reportf(tokenPos, "error returned from interface method should be wrapped: sig: %s", fnSig) | ||
return | ||
if containsMatch(regexpsInter, types.TypeString(pass.TypesInfo.TypeOf(sel.X), func(p *types.Package) string { return p.Name() })) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we maybe pull this out of a single line? It's a little dense to read as it is and could be simpler. |
||
} else { | ||
pass.Reportf(tokenPos, "error returned from interface method should be wrapped: sig: %s", fnSig) | ||
return | ||
} | ||
} | ||
|
||
// Check whether the function being called comes from another package, | ||
|
@@ -251,7 +268,7 @@ func isInterface(pass *analysis.Pass, sel *ast.SelectorExpr) bool { | |
return ok | ||
} | ||
|
||
// isFromotherPkg returns whether the function is defined in the pacakge | ||
// isFromotherPkg returns whether the function is defined in the package | ||
// 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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make this a little more succinct. I liked the description on the config struct more, maybe we could use that here.