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

Reference: #191
Reference: #192

CHANGELOG.md

@@ -1,9 +1,16 @@
 # 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

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/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
+}

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

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

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

@@ -0,0 +1,12 @@
+package a
+
+import (
+	"a/resource"
+	"a/schema"
+)
+
+func foutside() {
+	var d schema.ResourceData
+
+	d.SetId(resource.UniqueId())
+}

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

@@ -0,0 +1,5 @@
+package resource
+
+func UniqueId() string {
+	return ""
+}

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

@@ -0,0 +1,7 @@
+package schema
+
+type ResourceData struct{}
+
+func (d *ResourceData) SetId(value interface{}) error {
+	return nil
+}

passes/R015/testdata/src/a/vendor

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

passes/R016/R016.go

@@ -0,0 +1,58 @@
+package R016
+
+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.PrefixedUniqueId() value
+
+Schema attributes should be stable across Terraform runs.`
+
+const analyzerName = "R016"
+
+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.FuncNamePrefixedUniqueId) {
+				pass.Reportf(callExpr.Pos(), "%s: schema attributes should be stable across Terraform runs", analyzerName)
+				return false
+			}
+
+			return true
+		})
+	}
+
+	return nil, nil
+}

passes/R016/R016_test.go

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

passes/R016/README.md

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

passes/R016/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.PrefixedUniqueId("test")) // want "schema attributes should be stable across Terraform runs"
+
+	d.SetId("test")
+}