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
Allow whitelist for the context parameter check #616
Changes from 1 commit
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 |
---|---|---|
@@ -1,22 +1,49 @@ | ||
package rule | ||
|
||
import ( | ||
"fmt" | ||
"go/ast" | ||
|
||
"github.com/mgechev/revive/lint" | ||
) | ||
|
||
// ContextAsArgumentRule lints given else constructs. | ||
type ContextAsArgumentRule struct{} | ||
type ContextAsArgumentRule struct { | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could memoize the rule's configuration to avoid reading it each time the rule is applied. |
||
if len(args) > 1 { | ||
euank marked this conversation as resolved.
Show resolved
Hide resolved
|
||
panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Expecting a single k,v map only, got %v args", len(args))) | ||
} | ||
if len(args) == 1 { | ||
argKV, ok := args[0].(map[string]interface{}) | ||
if !ok { | ||
panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Expecting a k,v map, got %T", args[0])) | ||
} | ||
for k, v := range argKV { | ||
switch k { | ||
case "allowTypesBefore": | ||
typesBefore, ok := v.([]string) | ||
if !ok { | ||
panic(fmt.Sprintf("Invalid argument to the context-as-argument.allowTypesBefore rule. Expecting a []string, got %T", v)) | ||
} | ||
allowTypesBefore = typesBefore | ||
default: | ||
panic(fmt.Sprintf("Invalid argument to the context-as-argument rule. Unrecognized key %s", k)) | ||
} | ||
} | ||
// validate there's no other unrecognized args | ||
} | ||
|
||
var failures []lint.Failure | ||
|
||
fileAst := file.AST | ||
walker := lintContextArguments{ | ||
file: file, | ||
fileAst: fileAst, | ||
allowTypesBefore: allowTypesBefore, | ||
file: file, | ||
fileAst: fileAst, | ||
onFailure: func(failure lint.Failure) { | ||
failures = append(failures, failure) | ||
}, | ||
|
@@ -33,20 +60,48 @@ func (r *ContextAsArgumentRule) Name() string { | |
} | ||
|
||
type lintContextArguments struct { | ||
file *lint.File | ||
fileAst *ast.File | ||
onFailure func(lint.Failure) | ||
file *lint.File | ||
fileAst *ast.File | ||
allowTypesBefore []string | ||
onFailure func(lint.Failure) | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why create and populate this |
||
allowTypesLookup[v] = struct{}{} | ||
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. Curious why not a boolean value? 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 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. |
||
} | ||
|
||
fnArgs := fn.Type.Params.List | ||
// trim off all types we've been configured to skip | ||
for len(fnArgs) > 0 { | ||
typeStr, ok := astExprTypeStr(fnArgs[0].Type) | ||
if !ok { | ||
// assume we're done. This can happen, for example, with a function type | ||
// argument (`func(x func()...)` which we choose not to represent. | ||
break | ||
} | ||
_, isAllowed := allowTypesLookup[typeStr] | ||
if isAllowed { | ||
// trim | ||
fnArgs = fnArgs[1:] | ||
} else { | ||
// first non-trimmed argument means we've trimmed all prefix args | ||
break | ||
} | ||
} | ||
|
||
if len(fnArgs) <= 1 { | ||
return w | ||
} | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
for _, arg := range fnArgs[1:] { | ||
argIsCtx := isPkgDot(arg.Type, "context", "Context") | ||
if argIsCtx && !previousArgIsCtx { | ||
w.onFailure(lint.Failure{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,14 @@ func isCgoExported(f *ast.FuncDecl) bool { | |
|
||
var allCapsRE = regexp.MustCompile(`^[A-Z0-9_]+$`) | ||
|
||
func identStr(expr ast.Expr) (string, bool) { | ||
id, ok := expr.(*ast.Ident) | ||
if !ok { | ||
return "", false | ||
} | ||
return id.Name, true | ||
} | ||
|
||
func isIdent(expr ast.Expr, ident string) bool { | ||
id, ok := expr.(*ast.Ident) | ||
return ok && id.Name == ident | ||
|
@@ -92,11 +100,42 @@ func validType(T types.Type) bool { | |
!strings.Contains(T.String(), "invalid type") // good but not foolproof | ||
} | ||
|
||
func astExprTypeStr(expr ast.Expr) (string, bool) { | ||
switch v := expr.(type) { | ||
case *ast.Ident: | ||
return identStr(v) | ||
case *ast.StarExpr: | ||
subID, ok := astExprTypeStr(v.X) | ||
if !ok { | ||
return "", false | ||
} | ||
return "*" + subID, true | ||
case *ast.SelectorExpr: | ||
pkg, ok := identStr(v.X) | ||
if !ok { | ||
return "", false | ||
} | ||
sel, ok := identStr(v.Sel) | ||
if !ok { | ||
return "", false | ||
} | ||
return pkg + "." + sel, true | ||
} | ||
return "", false | ||
} | ||
|
||
// isPkgDot checks if the expression is <pkg>.<name> | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function seems to be not used |
||
star, ok := expr.(*ast.StarExpr) | ||
return ok && isPkgDot(star.X, pkg, name) | ||
} | ||
|
||
func srcLine(src []byte, p token.Position) string { | ||
// Run to end of line in both directions if not at line start/end. | ||
lo, hi := p.Offset, p.Offset+1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/mgechev/revive/lint" | ||
"github.com/mgechev/revive/rule" | ||
) | ||
|
||
func TestContextAsArgument(t *testing.T) { | ||
testRule(t, "context-as-argument", &rule.ContextAsArgumentRule{}, &lint.RuleConfig{ | ||
Arguments: []interface{}{ | ||
map[string]interface{}{ | ||
"allowTypesBefore": []string{ | ||
"AllowedBeforeType", | ||
"AllowedBeforeStruct", | ||
"*AllowedBeforePtrStruct", | ||
"*testing.T", | ||
}, | ||
}, | ||
}, | ||
}) | ||
} |
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.
the README.md must be also updated to document this new configuration for the rule