New Check: R014: check for CreateFunc, DeleteFunc, ReadFunc, and Upda…

…teFunc parameter naming (#102)

Reference: #12
Reference: #41

Includes small breaking change to `NewSchemaValidationFuncInfo` parameters to simplify usage.

CHANGELOG.md

@@ -1,5 +1,9 @@
 # v0.10.0
 
+BREAKING CHANGES
+
+* helper/terraformtype/helper/schema: `NewSchemaValidateFuncInfo` now accepts a single `ast.Node` instead of separate `*ast.FuncDecl` and `*ast.FuncLit`
+
 FEATURES
 
 * **New Check:** `AT008`: check for acceptance test function declaration `*testing.T` parameter naming
@@ -10,6 +14,7 @@ FEATURES
 * **New Check:** `R011`: check for `Resource` that configure `MigrateState`
 * **New Check:** `R012`: check for data source `Resource` that configure `CustomizeDiff`
 * **New Check:** `R013`: check for `map[string]*Resource` that resource names contain at least one underscore
+* **New Check:** `R014`: check for `CreateFunc`, `DeleteFunc`, `ReadFunc`, and `UpdateFunc` parameter naming
 * **New Check:** `S034`: check for `Schema` that configure `PromoteSingle`
 
 # v0.9.0

README.md

@@ -100,6 +100,7 @@ Standard lint checks are enabled by default in the `tfproviderlint` tool. Opt-in
 | [R011](passes/R011/README.md) | check for `Resource` that configure `MigrateState` | AST |
 | [R012](passes/R012/README.md) | check for data source `Resource` that configure `CustomizeDiff` | AST |
 | [R013](passes/R013/README.md) | check for `map[string]*Resource` that resource names contain at least one underscore | AST |
+| [R014](passes/R014/README.md) | check for `CreateFunc`, `DeleteFunc`, `ReadFunc`, and `UpdateFunc` parameter naming | AST |
 
 ### Standard Schema Checks
 

helper/astutils/fieldlist.go

@@ -0,0 +1,75 @@
+package astutils
+
+import (
+	"go/ast"
+	"go/types"
+)
+
+// FieldListName returns field name at field position and name position if found
+func FieldListName(fieldList *ast.FieldList, fieldPosition int, namePosition int) *string {
+	names := FieldListNames(fieldList, fieldPosition)
+
+	if names == nil || len(names) <= namePosition {
+		return nil
+	}
+
+	name := names[namePosition]
+
+	if name == nil {
+		return nil
+	}
+
+	return &name.Name
+}
+
+// FieldListNames returns field names at field position if found
+func FieldListNames(fieldList *ast.FieldList, position int) []*ast.Ident {
+	if fieldList == nil {
+		return nil
+	}
+
+	if len(fieldList.List) <= position {
+		return nil
+	}
+
+	field := fieldList.List[position]
+
+	if field == nil {
+		return nil
+	}
+
+	return field.Names
+}
+
+// FieldListType returns type at field position if found
+func FieldListType(fieldList *ast.FieldList, position int) *ast.Expr {
+	if fieldList == nil {
+		return nil
+	}
+
+	if len(fieldList.List) <= position {
+		return nil
+	}
+
+	field := fieldList.List[position]
+
+	if field == nil {
+		return nil
+	}
+
+	return &field.Type
+}
+
+// IsFieldListType returns true if the field at position is present and matches expected ast.Expr
+func IsFieldListType(fieldList *ast.FieldList, position int, exprFunc func(ast.Expr) bool) bool {
+	t := FieldListType(fieldList, position)
+
+	return t != nil && exprFunc(*t)
+}
+
+// IsFieldListTypePackageType returns true if the field at position is present and matches expected package type
+func IsFieldListTypePackageType(fieldList *ast.FieldList, position int, info *types.Info, packageSuffix string, typeName string) bool {
+	t := FieldListType(fieldList, position)
+
+	return t != nil && IsPackageFunctionFieldListType(*t, info, packageSuffix, typeName)
+}

helper/astutils/functype.go

@@ -0,0 +1,16 @@
+package astutils
+
+import (
+	"go/ast"
+)
+
+func FuncTypeFromNode(node ast.Node) *ast.FuncType {
+	switch node := node.(type) {
+	case *ast.FuncDecl:
+		return node.Type
+	case *ast.FuncLit:
+		return node.Type
+	}
+
+	return nil
+}

helper/astutils/package.go

@@ -37,6 +37,25 @@ func IsPackageFunc(e ast.Expr, info *types.Info, packageSuffix string, funcName
 	return false
 }
 
+// IsPackageFunctionFieldListType returns true if the function parameter package suffix (for vendoring) and name matches
+func IsPackageFunctionFieldListType(e ast.Expr, info *types.Info, packageSuffix string, typeName string) bool {
+	switch e := e.(type) {
+	case *ast.SelectorExpr:
+		if e.Sel.Name != typeName {
+			return false
+		}
+
+		switch x := e.X.(type) {
+		case *ast.Ident:
+			return strings.HasSuffix(info.ObjectOf(x).(*types.PkgName).Imported().Path(), packageSuffix)
+		}
+	case *ast.StarExpr:
+		return IsPackageFunctionFieldListType(e.X, info, packageSuffix, typeName)
+	}
+
+	return false
+}
+
 // IsPackageNamedType returns if the type name matches and is from the package suffix
 func IsPackageNamedType(t *types.Named, packageSuffix string, typeName string) bool {
 	if t.Obj().Name() != typeName {

helper/terraformtype/helper/schema/type_crudfunc.go

@@ -0,0 +1,45 @@
+package schema
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+)
+
+// CRUDFuncInfo represents all gathered CreateFunc, ReadFunc, UpdateFunc, and DeleteFunc data for easier access
+// Since Create, Delete, Read, and Update functions all have the same function
+// signature, we cannot differentiate them in AST (except by potentially by
+// function declaration naming heuristics later on).
+type CRUDFuncInfo struct {
+	AstFuncDecl *ast.FuncDecl
+	AstFuncLit  *ast.FuncLit
+	Body        *ast.BlockStmt
+	Node        ast.Node
+	Pos         token.Pos
+	Type        *ast.FuncType
+	TypesInfo   *types.Info
+}
+
+// NewCRUDFuncInfo instantiates a CRUDFuncInfo
+func NewCRUDFuncInfo(node ast.Node, info *types.Info) *CRUDFuncInfo {
+	result := &CRUDFuncInfo{
+		TypesInfo: info,
+	}
+
+	switch node := node.(type) {
+	case *ast.FuncDecl:
+		result.AstFuncDecl = node
+		result.Body = node.Body
+		result.Node = node
+		result.Pos = node.Pos()
+		result.Type = node.Type
+	case *ast.FuncLit:
+		result.AstFuncLit = node
+		result.Body = node.Body
+		result.Node = node
+		result.Pos = node.Pos()
+		result.Type = node.Type
+	}
+
+	return result
+}

helper/terraformtype/helper/schema/type_schemavalidatefunc.go

@@ -18,23 +18,24 @@ type SchemaValidateFuncInfo struct {
 }
 
 // NewSchemaValidateFuncInfo instantiates a SchemaValidateFuncInfo
-func NewSchemaValidateFuncInfo(funcDecl *ast.FuncDecl, funcLit *ast.FuncLit, info *types.Info) *SchemaValidateFuncInfo {
+func NewSchemaValidateFuncInfo(node ast.Node, info *types.Info) *SchemaValidateFuncInfo {
 	result := &SchemaValidateFuncInfo{
-		AstFuncDecl: funcDecl,
-		AstFuncLit:  funcLit,
-		TypesInfo:   info,
+		TypesInfo: info,
 	}
 
-	if funcDecl != nil {
-		result.Body = funcDecl.Body
-		result.Node = funcDecl
-		result.Pos = funcDecl.Pos()
-		result.Type = funcDecl.Type
-	} else if funcLit != nil {
-		result.Body = funcLit.Body
-		result.Node = funcLit
-		result.Pos = funcLit.Pos()
-		result.Type = funcLit.Type
+	switch node := node.(type) {
+	case *ast.FuncDecl:
+		result.AstFuncDecl = node
+		result.Body = node.Body
+		result.Node = node
+		result.Pos = node.Pos()
+		result.Type = node.Type
+	case *ast.FuncLit:
+		result.AstFuncLit = node
+		result.Body = node.Body
+		result.Node = node
+		result.Pos = node.Pos()
+		result.Type = node.Type
 	}
 
 	return result

passes/R014/R014.go

@@ -0,0 +1,51 @@
+package R014
+
+import (
+	"github.com/bflad/tfproviderlint/helper/astutils"
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"github.com/bflad/tfproviderlint/passes/helper/schema/crudfuncinfo"
+	"golang.org/x/tools/go/analysis"
+)
+
+const Doc = `check for CreateFunc, DeleteFunc, ReadFunc, and UpdateFunc parameter naming
+
+The R014 analyzer reports when CreateFunc, DeleteFunc, ReadFunc, and UpdateFunc
+declarations do not use d as the name for the *schema.ResourceData parameter
+or meta as the name for the interface{} parameter. This parameter naming is the
+standard convention for resources.`
+
+const analyzerName = "R014"
+
+var Analyzer = &analysis.Analyzer{
+	Name: analyzerName,
+	Doc:  Doc,
+	Requires: []*analysis.Analyzer{
+		commentignore.Analyzer,
+		crudfuncinfo.Analyzer,
+	},
+	Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	crudFuncs := pass.ResultOf[crudfuncinfo.Analyzer].([]*schema.CRUDFuncInfo)
+
+	for _, crudFunc := range crudFuncs {
+		if ignorer.ShouldIgnore(analyzerName, crudFunc.Node) {
+			continue
+		}
+
+		params := crudFunc.Type.Params
+
+		if name := astutils.FieldListName(params, 0, 0); name != nil && *name != "_" && *name != "d" {
+			pass.Reportf(params.List[0].Pos(), "%s: *schema.ResourceData parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named d", analyzerName)
+		}
+
+		if name := astutils.FieldListName(params, 1, 0); name != nil && *name != "_" && *name != "meta" {
+			pass.Reportf(params.List[1].Pos(), "%s: interface{} parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named meta", analyzerName)
+		}
+	}
+
+	return nil, nil
+}

passes/R014/R014_test.go

@@ -0,0 +1,12 @@
+package R014
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestR014(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, Analyzer, "a")
+}

passes/R014/README.md

@@ -0,0 +1,32 @@
+# R014
+
+The R014 analyzer reports when `CreateFunc`, `DeleteFunc`, `ReadFunc`, and `UpdateFunc` declarations do not use `d` as the name for the `*schema.ResourceData` parameter or `meta` as the name for the `interface{}` parameter. This parameter naming is the standard convention for resources.
+
+## Flagged Code
+
+```go
+func resourceExampleThingCreate(invalid *schema.ResourceData, meta interface{}) error { /* ... */ }
+
+func resourceExampleThingRead(d *schema.ResourceData, invalid interface{}) error { /* ... */ }
+
+func resourceExampleThingDelete(invalid *schema.ResourceData, invalid interface{}) error { /* ... */ }
+```
+
+## Passing Code
+
+```go
+func resourceExampleThingCreate(d *schema.ResourceData, meta interface{}) error { /* ... */ }
+
+func resourceExampleThingRead(d *schema.ResourceData, meta interface{}) error { /* ... */ }
+
+func resourceExampleThingDelete(d *schema.ResourceData, meta interface{}) error { /* ... */ }
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:R014` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:R014
+func resourceExampleThingCreate(invalid *schema.ResourceData, meta interface{}) error { /* ... */ }
+```

passes/R014/testdata/src/a/main.go

@@ -0,0 +1,53 @@
+package a
+
+import "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+
+// Comment ignored
+
+//lintignore:R014
+func commentIgnoreFirstParameter(invalid *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+//lintignore:R014
+func commentIgnoreSecondParameter(d *schema.ResourceData, invalid interface{}) error {
+	return nil
+}
+
+// Failing
+
+func failingAnonymousFirstParameter() {
+	_ = func(invalid *schema.ResourceData, meta interface{}) error { // want "\\*schema.ResourceData parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named d"
+		return nil
+	}
+}
+
+func failingAnonymousSecondParameter() {
+	_ = func(d *schema.ResourceData, invalid interface{}) error { // want "interface\\{\\} parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named meta"
+		return nil
+	}
+}
+
+func failingFirstParameter(invalid *schema.ResourceData, meta interface{}) error { // want "\\*schema.ResourceData parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named d"
+	return nil
+}
+
+func failingSecondParameter(d *schema.ResourceData, invalid interface{}) error { // want "interface\\{\\} parameter of CreateFunc, ReadFunc, UpdateFunc, or DeleteFunc should be named meta"
+	return nil
+}
+
+// Passing
+
+func passingAnonymous() {
+	_ = func(d *schema.ResourceData, meta interface{}) error {
+		return nil
+	}
+}
+
+func passing(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+func passingOther(diff *schema.ResourceDiff, meta interface{}) error {
+	return nil
+}

passes/R014/testdata/src/a/vendor

@@ -0,0 +1 @@
+../../../../../vendor