Allow whitelist for the context parameter check #616
Conversation
euank
force-pushed
the
allow-testing-t
branch
from
63a3a5c
to
7aae412
Compare
This allows users to configure a set of types that may appear before `context.Context`. Notably, I think this rule is useful for allowing the `*testing.T` type to come before `context.Context`, though there may be other uses (such as putting a tracer before it, etc). See mgechev#605 for a little more context on this. Fixes mgechev#605
euank
force-pushed
the
allow-testing-t
branch
from
7aae412
to
85e43d0
Compare
rule/context-as-argument.go
Outdated
| } | ||
|
|
||
| func (w lintContextArguments) Visit(n ast.Node) ast.Visitor { | ||
| fn, ok := n.(*ast.FuncDecl) | ||
| if !ok || len(fn.Type.Params.List) <= 1 { | ||
| return w | ||
| } | ||
| allowTypesLookup := make(map[string]struct{}, len(w.allowTypesBefore)) | ||
| for _, v := range w.allowTypesBefore { | ||
| allowTypesLookup[v] = struct{}{} |
There was a problem hiding this comment.
Curious why not a boolean value?
There was a problem hiding this comment.
I find struct{} is more clear for sets. If it's a string -> bool map, I have to wonder if both true/false in the bool have meaning, or only true (i.e. if it's a set or a map). If the value's a struct{}, I immediately know it's a set of strings.
There was a problem hiding this comment.
Thank you for the feature! Left a few comments. Will let @chavacava have a look as well.
We can unindent if we make the above check more specific
rule/context-as-argument.go
Outdated
| func (r *ContextAsArgumentRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { | ||
| func (r *ContextAsArgumentRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure { | ||
| var allowTypesBefore []string | ||
| if len(args) != 1 { |
There was a problem hiding this comment.
This makes the rule argument mandatory thus it will break all current configurations.
The argument must be optional.
rule/context-as-argument.go
Outdated
|
|
||
| // Apply applies the rule to given file. | ||
| func (r *ContextAsArgumentRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { | ||
| func (r *ContextAsArgumentRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure { | ||
| var allowTypesBefore []string |
There was a problem hiding this comment.
We could memoize the rule's configuration to avoid reading it each time the rule is applied.
Take a look to https://github.com/mgechev/revive/blob/master/rule/function-length.go or the PR #595
rule/context-as-argument.go
Outdated
| if len(args) != 1 { | ||
| panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Expecting a single k,v map only, got %v args", len(args))) | ||
| } | ||
| argKV, ok := args[0].(map[string]interface{}) |
There was a problem hiding this comment.
All the configuration processing (parsing, type checking, ...) could be refactored into a function to easy the reading of the Apply method
rule/utils.go
Outdated
| func isPkgDot(expr ast.Expr, pkg, name string) bool { | ||
| sel, ok := expr.(*ast.SelectorExpr) | ||
| return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) | ||
| } | ||
|
|
||
| // isPtrPkgDot checks if the expression is *<pkg>.<name>. | ||
| func isPtrPkgDot(expr ast.Expr, pkg, name string) bool { |
There was a problem hiding this comment.
This function seems to be not used
rule/context-as-argument.go
Outdated
| // A context.Context should be the first parameter of a function. | ||
| // Flag any that show up after the first. | ||
| previousArgIsCtx := isPkgDot(fn.Type.Params.List[0].Type, "context", "Context") | ||
| for _, arg := range fn.Type.Params.List[1:] { | ||
| previousArgIsCtx := isPkgDot(fnArgs[0].Type, "context", "Context") |
There was a problem hiding this comment.
We might simplify the check with something like:
ctxIsAllowed := true
for _, arg := range fnArgs {
argIsCtx := isPkgDot(arg.Type, "context", "Context")
if argIsCtx && !ctxIsAllowed {
w.onFailure( ... )
}
// check if the type is in the allowTypesLookup map or is context.Context
// if is not the case, make ctxIsAllowed = false
}This lets us avoid doing all the parameter list trimming stuff
rule/context-as-argument.go
Outdated
| } | ||
|
|
||
| func (w lintContextArguments) Visit(n ast.Node) ast.Visitor { | ||
| fn, ok := n.(*ast.FuncDecl) | ||
| if !ok || len(fn.Type.Params.List) <= 1 { | ||
| return w | ||
| } | ||
| allowTypesLookup := make(map[string]struct{}, len(w.allowTypesBefore)) | ||
| for _, v := range w.allowTypesBefore { |
There was a problem hiding this comment.
Why create and populate this allowTypesLookup at each visited node? (warn: the rule visits a huge number of AST nodes)
It seems that we can create and populate this lookup just once (at the creation of the lintContextArguments struct.
| @@ -186,7 +186,16 @@ _Configuration_: N/A | |||
|
|
|||
| _Description_: By [convention](https://github.com/golang/go/wiki/CodeReviewComments#contexts), `context.Context` should be the first parameter of a function. This rule spots function declarations that do not follow the convention. | |||
|
|
|||
| _Configuration_: N/A | |||
| _Configuration_: | |||
There was a problem hiding this comment.
the README.md must be also updated to document this new configuration for the rule
This allows users to configure a set of types that may appear before
context.Context.Notably, I think this rule is useful for allowing the
*testing.Ttypeto come before
context.Context, though there may be other uses (suchas putting a tracer before it, etc).
See #605 for a little more context on this.
Fixes #605