New Extra Check: XR004: check for ResourceData.Set() calls that shoul…

…d implement error checking with complex values (#82)

Reference: #50

CHANGELOG.md

@@ -11,6 +11,7 @@ FEATURES
 * **New Extra Check:** `XR001`: check for usage of `ResourceData.GetOkExists()` calls
 * **New Extra Check:** `XR002`: check for `Resource` that should implement `Importer`
 * **New Extra Check:** `XR003`: check for `Resource` that should implement `Timeouts`
+* **New Extra Check:** `XR004`: check for `ResourceData.Set()` calls that should implement error checking with complex values
 
 # v0.7.0
 

README.md

@@ -138,6 +138,7 @@ Extra lint checks are not included in the `tfproviderlint` tool and must be acce
 | [XR001](xpasses/XR001/README.md) | check for usage of `ResourceData.GetOkExists()` calls | AST |
 | [XR002](xpasses/XR002/README.md) | check for `Resource` that should implement `Importer` | AST |
 | [XR003](xpasses/XR003/README.md) | check for `Resource` that should implement `Timeouts` | AST |
+| [XR004](xpasses/XR004/README.md) | check for `ResourceData.Set()` calls that should implement error checking with complex values | AST |
 
 ### Extra Schema Checks
 

xpasses/XR004/README.md

@@ -0,0 +1,38 @@
+# XR004
+
+The XR004 analyzer reports [`Set()`](https://godoc.org/github.com/hashicorp/terraform-plugin-sdk/helper/schema#ResourceData.Set) calls that receive a complex value type, but do not perform error checking. This error checking is to prevent issues where the code is not able to properly set the Terraform state for drift detection. Addition details are available in the [Extending Terraform documentation](https://www.terraform.io/docs/extend/best-practices/detecting-drift.html#error-checking-aggregate-types).
+
+## Flagged Code
+
+```go
+d.Set("example", []interface{}{})
+
+d.Set("example", map[string]interface{}{})
+
+d.Set("example", schema.NewSet(/* ... */))
+```
+
+## Passing Code
+
+```go
+if err := d.Set("example", []interface{}{}); err != nil {
+    return fmt.Errorf("error setting example: %s", err)
+}
+
+if err := d.Set("example", map[string]interface{}{}); err != nil {
+    return fmt.Errorf("error setting example: %s", err)
+}
+
+if err := d.Set("example", schema.NewSet(/* ... */)); err != nil {
+    return fmt.Errorf("error setting example: %s", err)
+}
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:XR004` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:XR004
+d.Set("example", []interface{}{})
+```

xpasses/XR004/XR004.go

@@ -0,0 +1,108 @@
+// Package XR004 defines an Analyzer that checks for
+// ResourceData.Set() calls missing error checking with
+// complex types
+package XR004
+
+import (
+	"go/ast"
+	"go/types"
+
+	"github.com/bflad/tfproviderlint/helper/terraformtype"
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const Doc = `check for ResourceData.Set() calls missing error checking with complex values
+
+The XR004 analyzer reports Set() calls that receive a complex value type, but
+does not perform error checking. This error checking is to prevent issues where
+the code is not able to properly set the Terraform state for drift detection.
+
+Reference: https://www.terraform.io/docs/extend/best-practices/detecting-drift.html#error-checking-aggregate-types`
+
+const analyzerName = "XR004"
+
+var Analyzer = &analysis.Analyzer{
+	Name: analyzerName,
+	Doc:  Doc,
+	Requires: []*analysis.Analyzer{
+		inspect.Analyzer,
+		commentignore.Analyzer,
+	},
+	Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{
+		(*ast.ExprStmt)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		exprStmt := n.(*ast.ExprStmt)
+
+		callExpr, ok := exprStmt.X.(*ast.CallExpr)
+
+		if !ok {
+			return
+		}
+
+		if !terraformtype.IsHelperSchemaReceiverMethod(callExpr.Fun, pass.TypesInfo, terraformtype.TypeNameResourceData, "Set") {
+			return
+		}
+
+		if ignorer.ShouldIgnore(analyzerName, callExpr) {
+			return
+		}
+
+		if len(callExpr.Args) < 2 {
+			return
+		}
+
+		if isBasicType(pass.TypesInfo.TypeOf(callExpr.Args[1]).Underlying()) {
+			return
+		}
+
+		pass.Reportf(callExpr.Pos(), "%s: ResourceData.Set() should perform error checking with complex values", analyzerName)
+	})
+
+	return nil, nil
+}
+
+func isBasicType(t types.Type) bool {
+	switch t := t.(type) {
+	case *types.Basic:
+		return isAllowedBasicType(t)
+	case *types.Pointer:
+		return isBasicType(t.Elem())
+	}
+
+	return false
+}
+
+var allowedBasicKindTypes = []types.BasicKind{
+	types.Bool,
+	types.Float32,
+	types.Float64,
+	types.Int,
+	types.Int8,
+	types.Int16,
+	types.Int32,
+	types.Int64,
+	types.String,
+	types.UntypedNil,
+}
+
+func isAllowedBasicType(b *types.Basic) bool {
+	for _, allowedBasicKindType := range allowedBasicKindTypes {
+		if b.Kind() == allowedBasicKindType {
+			return true
+		}
+	}
+
+	return false
+}

xpasses/XR004/XR004_test.go

@@ -0,0 +1,13 @@
+package XR004_test
+
+import (
+	"testing"
+
+	"github.com/bflad/tfproviderlint/xpasses/XR004"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestXR004(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, XR004.Analyzer, "a")
+}

xpasses/XR004/testdata/src/a/alias.go

@@ -0,0 +1,95 @@
+package a
+
+import (
+	"time"
+
+	s "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func falias() {
+	var d s.ResourceData
+	var t time.Time
+
+	d.Set("test", t)                               // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", []time.Time{t})                  // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", map[string]time.Time{"test": t}) // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+
+	d.Set("test", nil)
+
+	var b bool
+
+	d.Set("test", b)
+	d.Set("test", &b)
+
+	var f32 float32
+
+	d.Set("test", f32)
+	d.Set("test", &f32)
+
+	var f64 float64
+
+	d.Set("test", f64)
+	d.Set("test", &f64)
+
+	var i int
+
+	d.Set("test", i)
+	d.Set("test", &i)
+
+	var i8 int8
+
+	d.Set("test", i8)
+	d.Set("test", &i8)
+
+	var i16 int16
+
+	d.Set("test", i16)
+	d.Set("test", &i16)
+
+	var i32 int32
+
+	d.Set("test", i32)
+	d.Set("test", &i32)
+
+	var i64 int64
+
+	d.Set("test", i64)
+	d.Set("test", &i64)
+
+	var s string
+
+	d.Set("test", s)
+	d.Set("test", &s)
+
+	d.Set("test", []interface{}{})  // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", []string{})       // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", sliceInterface()) // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+
+	d.Set("test", map[string]interface{}{}) // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", map[string]string{})      // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+	d.Set("test", mapInterface())           // want "ResourceData.Set\\(\\) should perform error checking with complex values"
+
+	if err := d.Set("test", []interface{}{}); err != nil {
+		return
+	}
+
+	if err := d.Set("test", []string{}); err != nil {
+		return
+	}
+
+	if err := d.Set("test", sliceInterface()); err != nil {
+		return
+	}
+
+	if err := d.Set("test", map[string]interface{}{}); err != nil {
+		return
+	}
+
+	if err := d.Set("test", map[string]string{}); err != nil {
+		return
+	}
+
+	if err := d.Set("test", mapInterface()); err != nil {
+		return
+	}
+}

xpasses/XR004/testdata/src/a/comment_ignore.go

@@ -0,0 +1,31 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func fcommentignore() {
+	var d schema.ResourceData
+
+	d.Set("test", []interface{}{})  //lintignore:XR004
+	d.Set("test", []string{})       //lintignore:XR004
+	d.Set("test", sliceInterface()) //lintignore:XR004
+
+	d.Set("test", map[string]interface{}{}) //lintignore:XR004
+	d.Set("test", map[string]string{})      //lintignore:XR004
+	d.Set("test", mapInterface())           //lintignore:XR004
+
+	//lintignore:XR004
+	d.Set("test", []interface{}{})
+	//lintignore:XR004
+	d.Set("test", []string{})
+	//lintignore:XR004
+	d.Set("test", sliceInterface())
+
+	//lintignore:XR004
+	d.Set("test", map[string]interface{}{})
+	//lintignore:XR004
+	d.Set("test", map[string]string{})
+	//lintignore:XR004
+	d.Set("test", mapInterface())
+}