Allow a whitelist for the context parameter check

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 #422 for a little more context on this.

Fixes #422

RULES_DESCRIPTIONS.md

@@ -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_:
+
+* `allowTypesBefore` : (string) comma-separated list of types that may be before 'context.Context'
+
+Example:
+
+```toml
+[rule.context-as-argument]
+  arguments = [{allowTypesBefore = "*testing.T,*github.com/user/repo/testing.Harness"}]
+```
 
 ## context-keys-type
 
@@ -252,7 +261,7 @@ _Configuration_: N/A
 _Description_: In GO it is idiomatic to minimize nesting statements, a typical example is to avoid if-then-else constructions. This rule spots constructions like
 ```go
 if cond {
-  // do something 
+  // do something
 } else {
   // do other thing
   return ...
@@ -262,7 +271,7 @@ that can be rewritten into more idiomatic:
 ```go
 if ! cond {
   // do other thing
-  return ... 
+  return ...
 }
 
 // do something
@@ -314,8 +323,8 @@ _Description_: Exported function and methods should have comments. This warns on
 
 More information [here](https://github.com/golang/go/wiki/CodeReviewComments#doc-comments)
 
-_Configuration_: ([]string) rule flags. 
-Please notice that without configuration, the default behavior of the rule is that of its `golint` counterpart. 
+_Configuration_: ([]string) rule flags.
+Please notice that without configuration, the default behavior of the rule is that of its `golint` counterpart.
 Available flags are:
 
 * _checkPrivateReceivers_ enables checking public methods of private types
@@ -425,8 +434,8 @@ Example:
 ```
 ## import-shadowing
 
-_Description_: In GO it is possible to declare identifiers (packages, structs, 
-interfaces, parameters, receivers, variables, constants...) that conflict with the 
+_Description_: In GO it is possible to declare identifiers (packages, structs,
+interfaces, parameters, receivers, variables, constants...) that conflict with the
 name of an imported package. This rule spots identifiers that shadow an import.
 
 _Configuration_: N/A
@@ -503,7 +512,7 @@ _Configuration_: N/A
 
 ## range-val-address
 
-_Description_: Range variables in a loop are reused at each iteration. This rule warns when assigning the address of the variable, passing the address to append() or using it in a map. 
+_Description_: Range variables in a loop are reused at each iteration. This rule warns when assigning the address of the variable, passing the address to append() or using it in a map.
 
 _Configuration_: N/A
 
@@ -521,7 +530,7 @@ Even if possible, redefining these built in names can lead to bugs very difficul
 _Configuration_: N/A
 
 ## string-of-int
-_Description_:  explicit type conversion `string(i)` where `i` has an integer type other than `rune` might behave not as expected by the developer (e.g. `string(42)` is not `"42"`). This rule spot that kind of suspicious conversions. 
+_Description_:  explicit type conversion `string(i)` where `i` has an integer type other than `rune` might behave not as expected by the developer (e.g. `string(42)` is not `"42"`). This rule spot that kind of suspicious conversions.
 
 _Configuration_: N/A
 
@@ -534,7 +543,7 @@ _Configuration_: Each argument is a slice containing 2-3 strings: a scope, a reg
 
 1. The first string defines a scope. This controls which string literals the regex will apply to, and is defined as a function argument. It must contain at least a function name (`core.WriteError`). Scopes may optionally contain a number specifying which argument in the function to check (`core.WriteError[1]`), as well as a struct field (`core.WriteError[1].Message`, only works for top level fields). Function arguments are counted starting at 0, so `[0]` would refer to the first argument, `[1]` would refer to the second, etc. If no argument number is provided, the first argument will be used (same as `[0]`).
 
-2. The second string is a regular expression (beginning and ending with a `/` character), which will be used to check the string literals in the scope. 
+2. The second string is a regular expression (beginning and ending with a `/` character), which will be used to check the string literals in the scope.
 
 3. The third string (optional) is a message containing the purpose for the regex, which will be used in lint errors.
 
@@ -649,8 +658,8 @@ _Configuration_: N/A
 
 ## useless-break
 
-_Description_: This rule warns on useless `break` statements in case clauses of switch and select statements. GO, unlike other programming languages like C, only executes statements of the selected case while ignoring the subsequent case clauses. 
-Therefore, inserting a `break` at the end of a case clause has no effect. 
+_Description_: This rule warns on useless `break` statements in case clauses of switch and select statements. GO, unlike other programming languages like C, only executes statements of the selected case while ignoring the subsequent case clauses.
+Therefore, inserting a `break` at the end of a case clause has no effect.
 
 Because `break` statements are rarely used in case clauses, when switch or select statements are inside a for-loop, the programmer might wrongly assume that a `break` in a case clause will take the control out of the loop.
 The rule emits a specific warning for such cases.

rule/context-as-argument.go

@@ -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
+	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)))
+	}
+	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 {
+		allowTypesLookup[v] = struct{}{}
+	}
+
+	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")
+	for _, arg := range fnArgs[1:] {
 		argIsCtx := isPkgDot(arg.Type, "context", "Context")
 		if argIsCtx && !previousArgIsCtx {
 			w.onFailure(lint.Failure{

rule/utils.go

@@ -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 {
+	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

test/context-as-argument_test.go

@@ -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",
+				},
+			},
+		},
+	})
+}

test/golint_test.go

@@ -31,7 +31,6 @@ var rules = []lint.Rule{
 	&rule.UnexportedReturnRule{},
 	&rule.TimeNamingRule{},
 	&rule.ContextKeysType{},
-	&rule.ContextAsArgumentRule{},
 }
 
 func TestAll(t *testing.T) {

testdata/golint/context-as-argument.go → testdata/context-as-argument.go

@@ -5,8 +5,18 @@ package foo
 
 import (
 	"context"
+	"testing"
 )
 
+// AllowedBeforeType is a type that is configured to be allowed before context.Context
+type AllowedBeforeType string
+
+// AllowedBeforeStruct is a type that is configured to be allowed before context.Context
+type AllowedBeforeStruct struct{}
+
+// AllowedBeforePtrStruct is a type that is configured to be allowed before context.Context
+type AllowedBeforePtrStruct struct{}
+
 // A proper context.Context location
 func x(ctx context.Context) { // ok
 }
@@ -15,6 +25,22 @@ func x(ctx context.Context) { // ok
 func x(ctx context.Context, s string) { // ok
 }
 
+// *testing.T is permitted in the linter config for the test
+func x(t *testing.T, ctx context.Context) { // ok
+}
+
+func x(_ AllowedBeforeType, _ AllowedBeforeType, ctx context.Context) { // ok
+}
+
+func x(_, _ AllowedBeforeType, ctx context.Context) { // ok
+}
+
+func x(_ *AllowedBeforePtrStruct, ctx context.Context) { // ok
+}
+
+func x(_ AllowedBeforePtrStruct, ctx context.Context) { // MATCH /context.Context should be the first parameter of a function/
+}
+
 // An invalid context.Context location
 func y(s string, ctx context.Context) { // MATCH /context.Context should be the first parameter of a function/
 }