New Checks: XS003: Check Schema for potential panic/diff

README.md

@@ -221,6 +221,7 @@ Extra lint checks are not included in the `tfproviderlint` tool and must be acce
 |---|---|---|
 | [XS001](xpasses/XS001) | check for `map[string]*Schema` that `Description` is configured | AST |
 | [XS002](xpasses/XS002) | check for `map[string]*Schema` that keys are in alphabetical order | AST |
+| [XS003](xpasses/XS003) | check for `Schema` that might introduce crash or diff as it allows to be an empty block | AST |
 
 ## Development and Testing
 

helper/astutils/basiclit.go

@@ -42,7 +42,7 @@ func ExprIntValue(e ast.Expr) *int {
 }
 
 // ExprStringValue fetches a string value from the Expr
-// If the Expr is not BasicLit, returns an empty string.
+// If the Expr is not BasicLit, returns nil.
 func ExprStringValue(e ast.Expr) *string {
 	switch v := e.(type) {
 	case *ast.BasicLit:

helper/terraformtype/helper/schema/type_resource.go

@@ -35,23 +35,23 @@ const (
 	TypeNameResource = `Resource`
 )
 
-// resourceType is an internal representation of the SDK helper/schema.Resource type
+// ResourceType is an internal representation of the SDK helper/schema.Resource type
 //
 // This is used to prevent importing the real type since the project supports
 // multiple versions of the Terraform Plugin SDK, while allowing passes to
 // access the data in a familiar manner.
-type resourceType struct {
+type ResourceType struct {
 	Description  string
 	MigrateState func(int, interface{}, interface{}) (interface{}, error)
-	Schema       map[string]*schemaType
+	Schema       map[string]*SchemaType
 	Timeouts     resourceTimeoutType
 }
 
 // ResourceInfo represents all gathered Resource data for easier access
 type ResourceInfo struct {
 	AstCompositeLit *ast.CompositeLit
 	Fields          map[string]*ast.KeyValueExpr
-	Resource        *resourceType
+	Resource        *ResourceType
 	TypesInfo       *types.Info
 }
 
@@ -60,7 +60,7 @@ func NewResourceInfo(cl *ast.CompositeLit, info *types.Info) *ResourceInfo {
 	result := &ResourceInfo{
 		AstCompositeLit: cl,
 		Fields:          astutils.CompositeLitFields(cl),
-		Resource:        &resourceType{},
+		Resource:        &ResourceType{},
 		TypesInfo:       info,
 	}
 
@@ -110,7 +110,7 @@ func NewResourceInfo(cl *ast.CompositeLit, info *types.Info) *ResourceInfo {
 	}
 
 	if kvExpr := result.Fields[ResourceFieldSchema]; kvExpr != nil && astutils.ExprValue(kvExpr.Value) != nil {
-		result.Resource.Schema = map[string]*schemaType{}
+		result.Resource.Schema = map[string]*SchemaType{}
 		if smap, ok := kvExpr.Value.(*ast.CompositeLit); ok {
 			for _, expr := range smap.Elts {
 				switch elt := expr.(type) {
@@ -135,6 +135,8 @@ func NewResourceInfo(cl *ast.CompositeLit, info *types.Info) *ResourceInfo {
 					switch valueExpr := elt.Value.(type) {
 					case *ast.CompositeLit:
 						result.Resource.Schema[key] = NewSchemaInfo(valueExpr, info).Schema
+					default:
+						result.Resource.Schema[key] = nil
 					}
 				}
 			}

helper/terraformtype/helper/schema/type_schema.go

@@ -48,12 +48,12 @@ const (
 	TypeNameValueType = `ValueType`
 )
 
-// schemaType is an internal representation of the SDK helper/schema.Schema type
+// SchemaType is an internal representation of the SDK helper/schema.Schema type
 //
 // This is used to prevent importing the real type since the project supports
 // multiple versions of the Terraform Plugin SDK, while allowing passes to
 // access the data in a familiar manner.
-type schemaType struct {
+type SchemaType struct {
 	AtLeastOneOf     []string
 	Computed         bool
 	ConflictsWith    []string
@@ -100,7 +100,7 @@ const (
 type SchemaInfo struct {
 	AstCompositeLit *ast.CompositeLit
 	Fields          map[string]*ast.KeyValueExpr
-	Schema          *schemaType
+	Schema          *SchemaType
 	SchemaValueType string
 	TypesInfo       *types.Info
 }
@@ -110,7 +110,7 @@ func NewSchemaInfo(cl *ast.CompositeLit, info *types.Info) *SchemaInfo {
 	result := &SchemaInfo{
 		AstCompositeLit: cl,
 		Fields:          astutils.CompositeLitFields(cl),
-		Schema:          &schemaType{},
+		Schema:          &SchemaType{},
 		SchemaValueType: typeSchemaType(cl, info),
 		TypesInfo:       info,
 	}

xpasses/XS003/README.md

@@ -0,0 +1,73 @@
+# XS003
+
+The XS003 analyzer reports cases of schemas where a non-computed nested block property of `TypeList` that only contains optional-only and non-default child properties, which has no `AtLeastOneOf`/`ExactlyOneOf` constraint.
+
+Note that if the schema is not a composite literal, this rule will be skipped so as to make this rule "complete" (less estimation).
+
+## Flagged Code
+
+```go
+map[string]*schema.Schema{
+  "a": {
+    Type: schema.TypeList,
+    Optional: true,
+    Elem: &schema.Resource{
+      Schema: map[string]*schema.Schema{
+        "foo": {
+          Optional: true,
+        },
+        "bar": {
+          Optional: true,
+        },
+      },
+    },
+  },
+}
+```
+
+## Passing Code
+
+```go
+map[string]*schema.Schema{
+  "a": {
+    Type: schema.TypeList,
+    Optional: true,
+    Elem: &schema.Resource{
+      Schema: map[string]*schema.Schema{
+        "foo": {
+          Optional: true,
+          AtLeastOneOf: []string{"foo", "bar"},
+        },
+        "bar": {
+          Optional: true,
+		  AtLeastOneOf: []string{"foo", "bar"},
+},
+      },
+    },
+  },
+}
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:XS003` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:XS003
+map[string]*schema.Schema{
+  "a": {
+    Type: schema.TypeList,
+    Optional: true,
+    Elem: &schema.Resource{
+      Schema: map[string]*schema.Schema{
+        "foo": {
+          Optional: true,
+        },
+        "bar": {
+          Optional: true,
+        },
+      },
+    },
+  },
+}
+```

xpasses/XS003/XS003.go

@@ -0,0 +1,77 @@
+package XS003
+
+import (
+	"go/ast"
+
+	"github.com/bflad/tfproviderlint/helper/terraformtype/helper/schema"
+	"github.com/bflad/tfproviderlint/passes/commentignore"
+	"github.com/bflad/tfproviderlint/passes/helper/schema/schemainfo"
+	"golang.org/x/tools/go/analysis"
+)
+
+const Doc = `check for Schema that might introduce crash or diff as it allows to be an empty block
+
+The XS003 analyzer reports cases of schemas where a non-computed nested block property of
+"TypeList" contains optional-only and non-default child properties, which has no "AtLeastOneOf"/"ExactlyOneOf" constraint.`
+
+const analyzerName = "XS003"
+
+var Analyzer = &analysis.Analyzer{
+	Name: analyzerName,
+	Doc:  Doc,
+	Requires: []*analysis.Analyzer{
+		schemainfo.Analyzer,
+		commentignore.Analyzer,
+	},
+	Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+	schemaInfos := pass.ResultOf[schemainfo.Analyzer].([]*schema.SchemaInfo)
+
+schemaLoop:
+	for _, schemaInfo := range schemaInfos {
+		if ignorer.ShouldIgnore(analyzerName, schemaInfo.AstCompositeLit) {
+			continue
+		}
+
+		// Ignore if this is not a non-computed nested block of TypeList.
+		if (!schemaInfo.Schema.Optional && !schemaInfo.Schema.Required) ||
+			schemaInfo.SchemaValueType != schema.SchemaValueTypeList || schemaInfo.Schema.Elem == nil {
+			continue
+		}
+
+		elem := schemaInfo.Schema.Elem
+		resource, ok := elem.(*schema.ResourceType)
+		// Ignore if the Elem doesn't exist or is not a composite literal
+		if !ok || resource == nil {
+			continue
+		}
+
+		// Ignore if the nested schema map is not a composite literal
+		if len(resource.Schema) == 0 {
+			continue
+		}
+
+		// Ignore if the child properties of this nested block has at least one required property, or Default/DefaultFunc,
+		// or one of "AtLeastOnOf"/"ExactlyOneOf" constraint.
+		for _, prop := range resource.Schema {
+			if prop == nil {
+				continue schemaLoop
+			}
+			if prop.Required || prop.Default != nil || prop.DefaultFunc != nil || prop.AtLeastOneOf != nil || prop.ExactlyOneOf != nil {
+				continue schemaLoop
+			}
+		}
+
+		switch t := schemaInfo.AstCompositeLit.Type.(type) {
+		default:
+			pass.Reportf(schemaInfo.AstCompositeLit.Lbrace, "%s: schema might introduce crash or diff as it allows to be an empty block", analyzerName)
+		case *ast.SelectorExpr:
+			pass.Reportf(t.Sel.Pos(), "%s: schema might introduce crash or diff as it allows to be an empty block", analyzerName)
+		}
+	}
+
+	return nil, nil
+}

xpasses/XS003/XS003_test.go

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

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

@@ -0,0 +1,23 @@
+package a
+
+import (
+	s "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func falias() {
+	_ = s.Resource{
+		Schema: map[string]*s.Schema{
+			"a": { // want "XS003: schema might introduce crash or diff as it allows to be an empty block"
+				Optional: true,
+				Type:     s.TypeList,
+				Elem: &s.Resource{
+					Schema: map[string]*s.Schema{
+						"foo": {
+							Optional: true,
+						},
+					},
+				},
+			},
+		},
+	}
+}

xpasses/XS003/testdata/src/a/alias_v2.go

@@ -0,0 +1,23 @@
+package a
+
+import (
+	s "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func falias_v2() {
+	_ = s.Resource{
+		Schema: map[string]*s.Schema{
+			"a": { // want "XS003: schema might introduce crash or diff as it allows to be an empty block"
+				Optional: true,
+				Type:     s.TypeList,
+				Elem: &s.Resource{
+					Schema: map[string]*s.Schema{
+						"foo": {
+							Optional: true,
+						},
+					},
+				},
+			},
+		},
+	}
+}

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

@@ -0,0 +1,24 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func fcommentignore() {
+	//lintignore:XS003
+	_ = schema.Resource{
+		Schema: map[string]*schema.Schema{
+			"a": {
+				Optional: true,
+				Type:     schema.TypeList,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"foo": {
+							Optional: true,
+						},
+					},
+				},
+			},
+		},
+	}
+}

xpasses/XS003/testdata/src/a/comment_ignore_v2.go

@@ -0,0 +1,24 @@
+package a
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func fcommentignore_v2() {
+	//lintignore:XS003
+	_ = schema.Resource{
+		Schema: map[string]*schema.Schema{
+			"a": {
+				Optional: true,
+				Type:     schema.TypeList,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"foo": {
+							Optional: true,
+						},
+					},
+				},
+			},
+		},
+	}
+}