New Checks for common unstable d.SetId() values and introduce standard library package helpers (#193)

* Introduce standard library helpers and pass for fmt.Sprintf() calls

Functionality to replace current Terraform AWS Provider handling and generally will be useful in the future.

* passes: New Checks for common unstable d.SetId() values

Reference: #191
Reference: #192

Author: bflad

CHANGELOG.md

@@ -1,3 +1,18 @@
+# v0.17.0
+
+FEATURES
+
+* **New Check:** `R015`: check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.UniqueId()` value
+* **New Check:** `R016`: check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.PrefixedUniqueId()` value
+* **New Check:** `R017`: check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `time.Now()` value
+
+ENHANCEMENTS
+
+* helper/analysisutils: Add `StdlibFunctionCallExprAnalyzer` with associated runner
+* helper/astutils: Support standard library handling equivalents (no vendoring) for package functions
+* passes/helper/schema: Pass for collecting `(*schema.ResourceData).SetId()` calls
+* passes/stdlib: Pass for collecting `fmt.Sprintf()` calls
+
 # v0.16.0
 
 BREAKING CHANGES

README.md

@@ -109,6 +109,9 @@ Standard lint checks are enabled by default in the `tfproviderlint` tool. Opt-in
 | [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 |
+| [R015](passes/R015/README.md) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.UniqueId()` value | AST |
+| [R016](passes/R016/README.md) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.PrefixedUniqueId()` value | AST |
+| [R017](passes/R017/README.md) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `time.Now()` value | AST |
 
 ### Standard Schema Checks
 

helper/analysisutils/stdlib_analyzers.go

@@ -0,0 +1,23 @@
+package analysisutils
+
+import (
+	"fmt"
+	"go/ast"
+	"reflect"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+)
+
+// StdlibFunctionCallExprAnalyzer returns an Analyzer for standard library function *ast.CallExpr
+func StdlibFunctionCallExprAnalyzer(analyzerName string, packagePath string, functionName string) *analysis.Analyzer {
+	return &analysis.Analyzer{
+		Name: analyzerName,
+		Doc:  fmt.Sprintf("find %s.%s() calls for later passes", packagePath, functionName),
+		Requires: []*analysis.Analyzer{
+			inspect.Analyzer,
+		},
+		Run:        StdlibFunctionCallExprRunner(packagePath, functionName),
+		ResultType: reflect.TypeOf([]*ast.CallExpr{}),
+	}
+}

helper/analysisutils/stdlib_runners.go

@@ -0,0 +1,33 @@
+package analysisutils
+
+import (
+	"go/ast"
+
+	"github.com/bflad/tfproviderlint/helper/astutils"
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+// StdlibFunctionCallExprRunner returns an Analyzer runner for function *ast.CallExpr
+func StdlibFunctionCallExprRunner(packagePath string, functionName string) func(*analysis.Pass) (interface{}, error) {
+	return func(pass *analysis.Pass) (interface{}, error) {
+		inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+		nodeFilter := []ast.Node{
+			(*ast.CallExpr)(nil),
+		}
+		var result []*ast.CallExpr
+
+		inspect.Preorder(nodeFilter, func(n ast.Node) {
+			callExpr := n.(*ast.CallExpr)
+
+			if !astutils.IsStdlibPackageFunc(callExpr.Fun, pass.TypesInfo, packagePath, functionName) {
+				return
+			}
+
+			result = append(result, callExpr)
+		})
+
+		return result, nil
+	}
+}

helper/astutils/package.go

@@ -185,6 +185,94 @@ func IsPackageType(t types.Type, packageSuffix string, typeName string) bool {
 	return false
 }
 
+// IsStdlibPackageReceiverMethod returns true if the package suffix (for vendoring), receiver name, and method name match
+//
+// This function checks an explicit import path without vendoring. To allow
+// vendored paths, use IsPackageReceiverMethod instead.
+func IsStdlibPackageReceiverMethod(e ast.Expr, info *types.Info, packagePath string, receiverName, methodName string) bool {
+	switch e := e.(type) {
+	case *ast.SelectorExpr:
+		if e.Sel.Name != methodName {
+			return false
+		}
+
+		return IsStdlibPackageType(info.TypeOf(e.X), packagePath, receiverName)
+	}
+
+	return false
+}
+
+// IsStdlibPackageFunc returns true if the function package suffix (for vendoring) and name matches
+//
+// This function checks an explicit import path without vendoring. To allow
+// vendored paths, use IsPackageFunc instead.
+func IsStdlibPackageFunc(e ast.Expr, info *types.Info, packagePath string, funcName string) bool {
+	switch e := e.(type) {
+	case *ast.SelectorExpr:
+		if e.Sel.Name != funcName {
+			return false
+		}
+
+		switch x := e.X.(type) {
+		case *ast.Ident:
+			return info.ObjectOf(x).(*types.PkgName).Imported().Path() == packagePath
+		}
+	case *ast.StarExpr:
+		return IsStdlibPackageFunc(e.X, info, packagePath, funcName)
+	}
+
+	return false
+}
+
+// IsStdlibPackageFunctionFieldListType returns true if the function parameter package suffix (for vendoring) and name matches
+//
+// This function checks an explicit import path without vendoring. To allow
+// vendored paths, use IsPackageFunctionFieldListType instead.
+func IsStdlibPackageFunctionFieldListType(e ast.Expr, info *types.Info, packagePath 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 info.ObjectOf(x).(*types.PkgName).Imported().Path() == packagePath
+		}
+	case *ast.StarExpr:
+		return IsStdlibPackageFunctionFieldListType(e.X, info, packagePath, typeName)
+	}
+
+	return false
+}
+
+// IsStdlibPackageNamedType returns if the type name matches and is from the package suffix
+//
+// This function checks an explicit import path without vendoring. To allow
+// vendored paths, use IsPackageNamedType instead.
+func IsStdlibPackageNamedType(t *types.Named, packagePath string, typeName string) bool {
+	if t.Obj().Name() != typeName {
+		return false
+	}
+
+	return t.Obj().Pkg().Path() == packagePath
+}
+
+// IsStdlibPackageType returns true if the type name can be matched and is from the package suffix
+//
+// This function checks an explicit import path without vendoring. To allow
+// vendored paths, use IsPackageType instead.
+func IsStdlibPackageType(t types.Type, packagePath string, typeName string) bool {
+	switch t := t.(type) {
+	case *types.Named:
+		return IsStdlibPackageNamedType(t, packagePath, typeName)
+	case *types.Pointer:
+		return IsStdlibPackageType(t.Elem(), packagePath, typeName)
+	}
+
+	return false
+}
+
 func isModulePackagePath(module string, packageSuffix string, path string) bool {
 	// Only check end of path due to vendoring
 	r := regexp.MustCompile(fmt.Sprintf("%s(/v[1-9][0-9]*)?/%s$", module, packageSuffix))

helper/terraformtype/helper/resource/funcs.go

@@ -5,6 +5,7 @@ const (
 	FuncNameComposeTestCheckFunc          = `ComposeTestCheckFunc`
 	FuncNameNonRetryableError             = `NonRetryableError`
 	FuncNameParallelTest                  = `ParallelTest`
+	FuncNamePrefixedUniqueId              = `PrefixedUniqueId`
 	FuncNameRetryableError                = `RetryableError`
 	FuncNameTest                          = `Test`
 	FuncNameTestCheckNoResourceAttr       = `TestCheckNoResourceAttr`
@@ -14,4 +15,5 @@ const (
 	FuncNameTestCheckResourceAttrPtr      = `TestCheckResourceAttrPtr`
 	FuncNameTestCheckResourceAttrSet      = `TestCheckResourceAttrSet`
 	FuncNameTestMatchResourceAttr         = `TestMatchResourceAttr`
+	FuncNameUniqueId                      = `UniqueId`
 )

passes/R015/R015.go

@@ -0,0 +1,58 @@
+package R015
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/resource"
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"github.com/bflad/tfproviderlint/passes/helper/schema/resourcedatasetidcallexpr"
+)
+
+const Doc = `check for (*schema.ResourceData).SetId() usage with unstable resource.UniqueId() value
+
+Schema attributes should be stable across Terraform runs.`
+
+const analyzerName = "R015"
+
+var Analyzer = &analysis.Analyzer{
+	Name: analyzerName,
+	Doc:  Doc,
+	Requires: []*analysis.Analyzer{
+		commentignore.Analyzer,
+		resourcedatasetidcallexpr.Analyzer,
+	},
+	Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	callExprs := pass.ResultOf[resourcedatasetidcallexpr.Analyzer].([]*ast.CallExpr)
+	for _, callExpr := range callExprs {
+		if ignorer.ShouldIgnore(analyzerName, callExpr) {
+			continue
+		}
+
+		if len(callExpr.Args) < 1 {
+			continue
+		}
+
+		ast.Inspect(callExpr.Args[0], func(n ast.Node) bool {
+			callExpr, ok := n.(*ast.CallExpr)
+
+			if !ok {
+				return true
+			}
+
+			if resource.IsFunc(callExpr.Fun, pass.TypesInfo, resource.FuncNameUniqueId) {
+				pass.Reportf(callExpr.Pos(), "%s: schema attributes should be stable across Terraform runs", analyzerName)
+				return false
+			}
+
+			return true
+		})
+	}
+
+	return nil, nil
+}

passes/R015/R015_test.go

@@ -0,0 +1,14 @@
+package R015_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/bflad/tfproviderlint/passes/R015"
+)
+
+func TestR015(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, R015.Analyzer, "a")
+}

passes/R015/README.md

@@ -0,0 +1,24 @@
+# R015
+
+The R015 analyzer reports [`(*schema.ResourceData).SetId()`](https://godoc.org/github.com/hashicorp/terraform-plugin-sdk/helper/schema#ResourceData.Set) usage with unstable `resource.UniqueId()` value. Schema attributes should be stable across Terraform runs.
+
+## Flagged Code
+
+```go
+d.SetId(resource.UniqueId())
+```
+
+## Passing Code
+
+```go
+d.SetId("stablestring")
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:R015` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:R015
+d.SetId(resource.UniqueId())
+```

passes/R015/testdata/src/a/alias.go

@@ -0,0 +1,14 @@
+package a
+
+import (
+	r "github.com/hashicorp/terraform-plugin-sdk/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func falias() {
+	var d schema.ResourceData
+
+	d.SetId(r.UniqueId()) // want "schema attributes should be stable across Terraform runs"
+
+	d.SetId("test")
+}

passes/R015/testdata/src/a/alias_v2.go

@@ -0,0 +1,14 @@
+package a
+
+import (
+	r "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func falias_v2() {
+	var d schema.ResourceData
+
+	d.SetId(r.UniqueId()) // want "schema attributes should be stable across Terraform runs"
+
+	d.SetId("test")
+}

passes/R015/testdata/src/a/comment_ignore.go

@@ -0,0 +1,15 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func fcommentignore() {
+	var d schema.ResourceData
+
+	d.SetId(resource.UniqueId()) //lintignore:R015
+
+	//lintignore:R015
+	d.SetId(resource.UniqueId())
+}

passes/R015/testdata/src/a/comment_ignore_v2.go

@@ -0,0 +1,15 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func fcommentignore_v2() {
+	var d schema.ResourceData
+
+	d.SetId(resource.UniqueId()) //lintignore:R015
+
+	//lintignore:R015
+	d.SetId(resource.UniqueId())
+}

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func f() {
+	var d schema.ResourceData
+
+	d.SetId(resource.UniqueId()) // want "schema attributes should be stable across Terraform runs"
+
+	d.SetId("test")
+
+	fResourceFunc(&d, nil)
+}
+
+func fResourceFunc(d *schema.ResourceData, meta interface{}) error {
+	d.SetId(resource.UniqueId()) // want "schema attributes should be stable across Terraform runs"
+
+	d.SetId("test")
+
+	return nil
+}