New Check: R019: check for (*schema.ResourceData).HasChanges() rece…

…iver method usage with many arguments (#217)

Reference: #212

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 FEATURES
 
+* **New Check:** `R019`: check for `(*schema.ResourceData).HasChanges()` receiver method usage with many arguments
 * **New Check:** `V009`: check for `validation.StringMatch()` calls with empty message argument
 * **New Check:** `V010`: check for `validation.StringDoesNotMatch()` calls with empty message argument
 

README.md

@@ -126,6 +126,7 @@ Standard lint checks are enabled by default in the `tfproviderlint` tool. Opt-in
 | [R016](passes/R016) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `resource.PrefixedUniqueId()` value | AST |
 | [R017](passes/R017) | check for `(*schema.ResourceData).SetId()` receiver method usage with unstable `time.Now()` value | AST |
 | [R018](passes/R018) | check for `time.Sleep()` function usage | AST |
+| [R019](passes/R019) | check for `(*schema.ResourceData).HasChanges()` receiver method usage with many arguments | AST |
 
 ### Standard Schema Checks
 

passes/R019/R019.go

@@ -0,0 +1,62 @@
+package R019
+
+import (
+	"flag"
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"github.com/bflad/tfproviderlint/passes/helper/schema/resourcedatahaschangescallexpr"
+)
+
+const Doc = `check for (*schema.ResourceData).HasChanges() calls with many arguments
+
+The R019 analyzer reports when there are a large number of arguments being
+passed to (*schema.ResourceData).HasChanges(), which it may be preferable to
+use (*schema.ResourceData).HasChangesExcept() instead.
+
+Optional parameters:
+  -threshold=5 Number of arguments for reporting
+`
+
+const analyzerName = "R019"
+
+var (
+	threshold int
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name:  analyzerName,
+	Doc:   Doc,
+	Flags: parseFlags(),
+	Requires: []*analysis.Analyzer{
+		commentignore.Analyzer,
+		resourcedatahaschangescallexpr.Analyzer,
+	},
+	Run: run,
+}
+
+func parseFlags() flag.FlagSet {
+	var flags = flag.NewFlagSet(analyzerName, flag.ExitOnError)
+	flags.IntVar(&threshold, "threshold", 5, "Number of arguments for reporting")
+	return *flags
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	callExprs := pass.ResultOf[resourcedatahaschangescallexpr.Analyzer].([]*ast.CallExpr)
+	for _, callExpr := range callExprs {
+		if ignorer.ShouldIgnore(analyzerName, callExpr) {
+			continue
+		}
+
+		if len(callExpr.Args) < threshold {
+			continue
+		}
+
+		pass.Reportf(callExpr.Pos(), "%s: d.HasChanges() has many arguments, consider d.HasChangesExcept()", analyzerName)
+	}
+
+	return nil, nil
+}

passes/R019/R019_test.go

@@ -0,0 +1,26 @@
+package R019_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/bflad/tfproviderlint/passes/R019"
+)
+
+func TestR019(t *testing.T) {
+	testdata := analysistest.TestData()
+	analyzer := R019.Analyzer
+	analysistest.Run(t, testdata, analyzer, "a")
+}
+
+func TestR019Threshold(t *testing.T) {
+	testdata := analysistest.TestData()
+	analyzer := R019.Analyzer
+
+	if err := analyzer.Flags.Set("threshold", "3"); err != nil {
+		t.Fatalf("error setting threshold flag: %s", err)
+	}
+
+	analysistest.Run(t, testdata, analyzer, "threshold")
+}

passes/R019/README.md

@@ -0,0 +1,28 @@
+# R019
+
+The R019 analyzer reports when there are a large number of arguments being passed to [`(*schema.ResourceData).HasChanges()`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema#ResourceData.HasChanges), which it may be preferable to use [`(*schema.ResourceData).HasChangesExcept()`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema#ResourceData.HasChangesExcept) instead.
+
+## Optional Arguments
+
+- `-threshold=5` Number of arguments before reporting
+
+## Flagged Code
+
+```go
+d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+```
+
+## Passing Code
+
+```go
+d.HasChangesExcept("metadata_attr")
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:R019` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:R019
+d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+```

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	s "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func falias() {
+	var d s.ResourceData
+
+	// Comment ignored
+
+	//lintignore:R019
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019
+
+	// Failing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)"
+
+	// Passing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4")
+}

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	s "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func falias_v2() {
+	var d s.ResourceData
+
+	// Comment ignored
+
+	//lintignore:R019
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019
+
+	// Failing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)"
+
+	// Passing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4")
+}

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func f() {
+	var d schema.ResourceData
+
+	// Comment ignored
+
+	//lintignore:R019
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019
+
+	// Failing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)"
+
+	// Passing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4")
+}

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func f_v2() {
+	var d schema.ResourceData
+
+	// Comment ignored
+
+	//lintignore:R019
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") //lintignore:R019
+
+	// Failing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)"
+
+	// Passing
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4")
+}

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

@@ -0,0 +1,11 @@
+package a
+
+import (
+	"a/schema"
+)
+
+func foutside() {
+	var d schema.ResourceData
+
+	d.HasChanges("attr1", "attr2", "attr3", "attr4", "attr5")
+}

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

@@ -0,0 +1,7 @@
+package schema
+
+type ResourceData struct{}
+
+func (d *ResourceData) HasChanges(keys ...string) bool {
+	return false
+}

passes/R019/testdata/src/a/vendor

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

passes/R019/testdata/src/threshold/main.go

@@ -0,0 +1,17 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func f() {
+	var d schema.ResourceData
+
+	// Failing with lower threshold
+
+	d.HasChanges("attr1", "attr2", "attr3") // want "d.HasChanges\\(\\) has many arguments, consider d.HasChangesExcept\\(\\)"
+
+	// Passing with lower threshold
+
+	d.HasChanges("attr1", "attr2")
+}

passes/R019/testdata/src/threshold/vendor

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

passes/checks.go

@@ -28,6 +28,7 @@ import (
 	"github.com/bflad/tfproviderlint/passes/R016"
 	"github.com/bflad/tfproviderlint/passes/R017"
 	"github.com/bflad/tfproviderlint/passes/R018"
+	"github.com/bflad/tfproviderlint/passes/R019"
 	"github.com/bflad/tfproviderlint/passes/S001"
 	"github.com/bflad/tfproviderlint/passes/S002"
 	"github.com/bflad/tfproviderlint/passes/S003"
@@ -109,6 +110,7 @@ var AllChecks = []*analysis.Analyzer{
 	R016.Analyzer,
 	R017.Analyzer,
 	R018.Analyzer,
+	R019.Analyzer,
 	S001.Analyzer,
 	S002.Analyzer,
 	S003.Analyzer,

passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr.go

@@ -0,0 +1,14 @@
+package resourcedatahaschangecallexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
+)
+
+var Analyzer = analysisutils.ReceiverMethodCallExprAnalyzer(
+	"resourcedatahaschangecallexpr",
+	schema.IsReceiverMethod,
+	schema.PackagePath,
+	schema.TypeNameResourceData,
+	"HasChange",
+)

passes/helper/schema/resourcedatahaschangecallexpr/resourcedatahaschangecallexpr_test.go

@@ -0,0 +1,15 @@
+package resourcedatahaschangecallexpr
+
+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)
+	}
+}

passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr.go

@@ -0,0 +1,14 @@
+package resourcedatahaschangescallexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
+)
+
+var Analyzer = analysisutils.ReceiverMethodCallExprAnalyzer(
+	"resourcedatahaschangescallexpr",
+	schema.IsReceiverMethod,
+	schema.PackagePath,
+	schema.TypeNameResourceData,
+	"HasChanges",
+)

passes/helper/schema/resourcedatahaschangescallexpr/resourcedatahaschangescallexpr_test.go

@@ -0,0 +1,15 @@
+package resourcedatahaschangescallexpr
+
+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)
+	}
+}