From 4fa91e325486039f4dc748fce9517fe1814d1ef4 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 17 Feb 2020 22:12:09 -0500 Subject: [PATCH] New Check: R014: check for CreateFunc, DeleteFunc, ReadFunc, and UpdateFunc parameter naming (#102) Reference: https://github.com/bflad/tfproviderlint/issues/12 Reference: https://github.com/bflad/tfproviderlint/issues/41 Includes small breaking change to `NewSchemaValidationFuncInfo` parameters to simplify usage. --- CHANGELOG.md | 5 ++ README.md | 1 + helper/astutils/fieldlist.go | 75 +++++++++++++++++++ helper/astutils/functype.go | 16 ++++ helper/astutils/package.go | 19 +++++ .../helper/schema/type_crudfunc.go | 45 +++++++++++ .../helper/schema/type_schemavalidatefunc.go | 29 +++---- passes/R014/R014.go | 51 +++++++++++++ passes/R014/R014_test.go | 12 +++ passes/R014/README.md | 32 ++++++++ passes/R014/testdata/src/a/main.go | 53 +++++++++++++ passes/R014/testdata/src/a/vendor | 1 + passes/checks.go | 2 + .../schema/crudfuncinfo/crudfuncinfo.go | 55 ++++++++++++++ .../schema/crudfuncinfo/crudfuncinfo_test.go | 15 ++++ .../schemavalidatefuncinfo.go | 33 ++------ 16 files changed, 404 insertions(+), 40 deletions(-) create mode 100644 helper/astutils/fieldlist.go create mode 100644 helper/astutils/functype.go create mode 100644 helper/terraformtype/helper/schema/type_crudfunc.go create mode 100644 passes/R014/R014.go create mode 100644 passes/R014/R014_test.go create mode 100644 passes/R014/README.md create mode 100644 passes/R014/testdata/src/a/main.go create mode 120000 passes/R014/testdata/src/a/vendor create mode 100644 passes/helper/schema/crudfuncinfo/crudfuncinfo.go create mode 100644 passes/helper/schema/crudfuncinfo/crudfuncinfo_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 315d2e40..05ea26c3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index e2cc3c33..a192c854 100644 --- a/README.md +++ b/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 diff --git a/helper/astutils/fieldlist.go b/helper/astutils/fieldlist.go new file mode 100644 index 00000000..81e19bab --- /dev/null +++ b/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) +} diff --git a/helper/astutils/functype.go b/helper/astutils/functype.go new file mode 100644 index 00000000..4272dba9 --- /dev/null +++ b/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 +} diff --git a/helper/astutils/package.go b/helper/astutils/package.go index f1a6af35..e983b9d6 100644 --- a/helper/astutils/package.go +++ b/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 { diff --git a/helper/terraformtype/helper/schema/type_crudfunc.go b/helper/terraformtype/helper/schema/type_crudfunc.go new file mode 100644 index 00000000..3488697c --- /dev/null +++ b/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 +} diff --git a/helper/terraformtype/helper/schema/type_schemavalidatefunc.go b/helper/terraformtype/helper/schema/type_schemavalidatefunc.go index 9219c1cf..119fbf81 100644 --- a/helper/terraformtype/helper/schema/type_schemavalidatefunc.go +++ b/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 diff --git a/passes/R014/R014.go b/passes/R014/R014.go new file mode 100644 index 00000000..20a5ce51 --- /dev/null +++ b/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 +} diff --git a/passes/R014/R014_test.go b/passes/R014/R014_test.go new file mode 100644 index 00000000..fac1610f --- /dev/null +++ b/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") +} diff --git a/passes/R014/README.md b/passes/R014/README.md new file mode 100644 index 00000000..3d38e7b2 --- /dev/null +++ b/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 { /* ... */ } +``` diff --git a/passes/R014/testdata/src/a/main.go b/passes/R014/testdata/src/a/main.go new file mode 100644 index 00000000..7f729214 --- /dev/null +++ b/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 +} diff --git a/passes/R014/testdata/src/a/vendor b/passes/R014/testdata/src/a/vendor new file mode 120000 index 00000000..776800d2 --- /dev/null +++ b/passes/R014/testdata/src/a/vendor @@ -0,0 +1 @@ +../../../../../vendor \ No newline at end of file diff --git a/passes/checks.go b/passes/checks.go index 314a84aa..76e26ee4 100644 --- a/passes/checks.go +++ b/passes/checks.go @@ -22,6 +22,7 @@ import ( "github.com/bflad/tfproviderlint/passes/R011" "github.com/bflad/tfproviderlint/passes/R012" "github.com/bflad/tfproviderlint/passes/R013" + "github.com/bflad/tfproviderlint/passes/R014" "github.com/bflad/tfproviderlint/passes/S001" "github.com/bflad/tfproviderlint/passes/S002" "github.com/bflad/tfproviderlint/passes/S003" @@ -92,6 +93,7 @@ var AllChecks = []*analysis.Analyzer{ R011.Analyzer, R012.Analyzer, R013.Analyzer, + R014.Analyzer, S001.Analyzer, S002.Analyzer, S003.Analyzer, diff --git a/passes/helper/schema/crudfuncinfo/crudfuncinfo.go b/passes/helper/schema/crudfuncinfo/crudfuncinfo.go new file mode 100644 index 00000000..d176266b --- /dev/null +++ b/passes/helper/schema/crudfuncinfo/crudfuncinfo.go @@ -0,0 +1,55 @@ +package crudfuncinfo + +import ( + "go/ast" + "reflect" + + "github.com/bflad/tfproviderlint/helper/astutils" + "github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "crudfuncinfo", + Doc: "find github.com/hashicorp/terraform-plugin-sdk/helper/schema CreateFunc, ReadFunc, UpdateFunc, and DeleteFunc declarations for later passes", + Requires: []*analysis.Analyzer{ + inspect.Analyzer, + }, + Run: run, + ResultType: reflect.TypeOf([]*schema.CRUDFuncInfo{}), +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + (*ast.FuncLit)(nil), + } + var result []*schema.CRUDFuncInfo + + inspect.Preorder(nodeFilter, func(n ast.Node) { + funcType := astutils.FuncTypeFromNode(n) + + if funcType == nil { + return + } + + if !astutils.IsFieldListTypePackageType(funcType.Params, 0, pass.TypesInfo, schema.PackagePath, schema.TypeNameResourceData) { + return + } + + if !astutils.IsFieldListType(funcType.Params, 1, astutils.IsFunctionParameterTypeInterface) { + return + } + + if !astutils.IsFieldListType(funcType.Results, 0, astutils.IsFunctionParameterTypeError) { + return + } + + result = append(result, schema.NewCRUDFuncInfo(n, pass.TypesInfo)) + }) + + return result, nil +} diff --git a/passes/helper/schema/crudfuncinfo/crudfuncinfo_test.go b/passes/helper/schema/crudfuncinfo/crudfuncinfo_test.go new file mode 100644 index 00000000..88b18f2e --- /dev/null +++ b/passes/helper/schema/crudfuncinfo/crudfuncinfo_test.go @@ -0,0 +1,15 @@ +package crudfuncinfo + +import ( + "testing" + + "golang.org/x/tools/go/analysis" +) + +func TestValidateAnalyzer(t *testing.T) { + err := analysis.Validate([]*analysis.Analyzer{Analyzer}) + + if err != nil { + t.Fatal(err) + } +} diff --git a/passes/helper/schema/schemavalidatefuncinfo/schemavalidatefuncinfo.go b/passes/helper/schema/schemavalidatefuncinfo/schemavalidatefuncinfo.go index b46dd49f..92c577e1 100644 --- a/passes/helper/schema/schemavalidatefuncinfo/schemavalidatefuncinfo.go +++ b/passes/helper/schema/schemavalidatefuncinfo/schemavalidatefuncinfo.go @@ -30,48 +30,29 @@ func run(pass *analysis.Pass) (interface{}, error) { var result []*schema.SchemaValidateFuncInfo inspect.Preorder(nodeFilter, func(n ast.Node) { - funcDecl, funcDeclOk := n.(*ast.FuncDecl) - funcLit, funcLitOk := n.(*ast.FuncLit) + funcType := astutils.FuncTypeFromNode(n) - var funcType *ast.FuncType - - if funcDeclOk && funcDecl != nil { - funcType = funcDecl.Type - } else if funcLitOk && funcLit != nil { - funcType = funcLit.Type - } else { - return - } - - params := funcType.Params - - if params == nil || len(params.List) != 2 { + if funcType == nil { return } - if !astutils.IsFunctionParameterTypeInterface(params.List[0].Type) { + if !astutils.IsFieldListType(funcType.Params, 0, astutils.IsFunctionParameterTypeInterface) { return } - if !astutils.IsFunctionParameterTypeString(params.List[1].Type) { - return - } - - results := funcType.Results - - if results == nil || len(results.List) != 2 { + if !astutils.IsFieldListType(funcType.Params, 1, astutils.IsFunctionParameterTypeString) { return } - if !astutils.IsFunctionParameterTypeArrayString(results.List[0].Type) { + if !astutils.IsFieldListType(funcType.Results, 0, astutils.IsFunctionParameterTypeArrayString) { return } - if !astutils.IsFunctionParameterTypeArrayError(results.List[1].Type) { + if !astutils.IsFieldListType(funcType.Results, 1, astutils.IsFunctionParameterTypeArrayError) { return } - result = append(result, schema.NewSchemaValidateFuncInfo(funcDecl, funcLit, pass.TypesInfo)) + result = append(result, schema.NewSchemaValidateFuncInfo(n, pass.TypesInfo)) }) return result, nil