core: New lint-checking mode for Validate

Here we establish a new mode for Context.Validate which enables some extra
checks which can produce warnings that don't necessarily need to be fixed
in order for a Terraform module to be accepted, but that which are
indicative of common mistakes that might lead to unintended behavior or
of non-idiomatic style.

The idea of this mode is that users could potentially enable it if they
are seeing some unexpected behavior, to potentially get some additional
hints as to what might be causing it. Since linting might sometimes
generate false negatives, this mode probably won't be suitable for use
as a pre-merge automated check.

For this first round, the lint checks are focused on the depends_on
arguments within "resource" and "data" blocks, catching situations where
entries in depends_on are redundant with other declarations or refer to
individual resource instances when Terraform only considers whole
resources.

This new mode is currently not exposed in the UI anywhere, so this is all
just dead code for the moment. Perhaps in future it will be exposed either
as a new mode for "terraform validate" or as a new command
"terraform lint", but we'll save that decision for later work.

internal/terraform/context_validate.go

@@ -13,13 +13,25 @@ import (
 // ValidateOpts are the various options that affect the details of how Terraform
 // will validate a configuration.
 type ValidateOpts struct {
-	// There are currently no special options for Validate. This struct is
-	// just here to allow for less-disruptive addition of new options later.
+	// LintChecks, if set to true, enables additional warnings that describe
+	// ways to possibly improve a Terraform configuration even though the
+	// current configuration is valid as written.
+	//
+	// The additional warnings produced in "lint" mode are more subjective and
+	// so module authors can evaluate each one and choose to evaluate warnings
+	// that don't apply in some particular situations. There might be additional
+	// lint warnings in later releases, thus making a previously-lint-free
+	// configuration potentially lint-y again, and so considering lint warnings
+	// should typically be a development task in its own right, rather than a
+	// blocker for completing other development tasks.
+	LintChecks bool
 }
 
 // DefaultValidateOpts is a reasonable default set of validate options to use
 // in common cases without any special needs.
-var DefaultValidateOpts = &ValidateOpts{}
+var DefaultValidateOpts = &ValidateOpts{
+	LintChecks: false,
+}
 
 // Validate performs semantic validation of a configuration, and returns
 // any warnings or errors.

internal/terraform/context_validate_lint_test.go

@@ -0,0 +1,131 @@
+package terraform
+
+import (
+	"testing"
+
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/configs/configschema"
+	"github.com/hashicorp/terraform/internal/providers"
+	"github.com/hashicorp/terraform/internal/tfdiags"
+	"github.com/zclconf/go-cty/cty"
+)
+
+// The tests in this file are for the extra warnings we produce when running
+// validation in linting mode. All of the tests in here should be calling
+// validate with LintChecks: true at some point, or else they'd be better
+// placed in context_validate_test.go instead.
+
+func TestContextValidateLint_redundantResourceDepends(t *testing.T) {
+	p := testProvider("test")
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_thing": {
+				Attributes: map[string]*configschema.Attribute{
+					"name": {
+						Type:     cty.String,
+						Optional: true,
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"single_block": {
+						Nesting: configschema.NestingSingle,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"id": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+					"map_block": {
+						Nesting: configschema.NestingMap,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"doodad": {
+									Type:     cty.String,
+									Optional: true,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+
+	m := testModule(t, "validate-lint-redundant-resource-depends")
+	c := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	gotDiags := c.Validate(m, &ValidateOpts{
+		LintChecks: true,
+	})
+	var wantDiags tfdiags.Diagnostics
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Redundant explicit dependency",
+		Detail:   "There is already an implied dependency for test_thing.a at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:23,10, so this declaration is redundant.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 34, Column: 5, Byte: 404},
+			End:      hcl.Pos{Line: 34, Column: 17, Byte: 416},
+		},
+	})
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Redundant explicit dependency",
+		Detail:   "There is already an explicit dependency for test_thing.b at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:35,5, so this declaration is redundant.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 36, Column: 5, Byte: 440},
+			End:      hcl.Pos{Line: 36, Column: 17, Byte: 452},
+		},
+	})
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Redundant explicit dependency",
+		Detail:   "There is already an implied dependency for test_thing.c at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:26,10, so this declaration is redundant.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 37, Column: 5, Byte: 458},
+			End:      hcl.Pos{Line: 37, Column: 17, Byte: 470},
+		},
+	})
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Redundant explicit dependency",
+		Detail:   "There is already an implied dependency for test_thing.d at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:30,14, so this declaration is redundant.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 38, Column: 5, Byte: 476},
+			End:      hcl.Pos{Line: 38, Column: 17, Byte: 488},
+		},
+	})
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Over-specified explicit dependency",
+		Detail:   "Terraform references are between resources as a whole, and don't consider individual resource instances. The [0] instance key in this address has no effect.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 39, Column: 5, Byte: 494},
+			End:      hcl.Pos{Line: 39, Column: 20, Byte: 509},
+		},
+	})
+	wantDiags = wantDiags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Redundant explicit dependency",
+		Detail:   "There is already an explicit dependency for test_thing.f at testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf:39,5, so this declaration is redundant.",
+		Subject: &hcl.Range{
+			Filename: "testdata/validate-lint-redundant-resource-depends/redundant-resource-depends.tf",
+			Start:    hcl.Pos{Line: 40, Column: 5, Byte: 515},
+			End:      hcl.Pos{Line: 40, Column: 17, Byte: 527},
+		},
+	})
+	assertDiagnosticsMatch(t, gotDiags, wantDiags)
+
+}

internal/terraform/graph_builder_validate.go

@@ -11,6 +11,10 @@ import (
 // PlanGraphBuilder given will be modified so it shouldn't be used for anything
 // else after calling this function.
 func validateGraphBuilder(p *PlanGraphBuilder, opts *ValidateOpts) GraphBuilder {
+	if opts == nil {
+		opts = DefaultValidateOpts
+	}
+
 	// We're going to customize the concrete functions
 	p.CustomConcrete = true
 
@@ -24,6 +28,7 @@ func validateGraphBuilder(p *PlanGraphBuilder, opts *ValidateOpts) GraphBuilder
 	p.ConcreteResource = func(a *NodeAbstractResource) dag.Vertex {
 		return &NodeValidatableResource{
 			NodeAbstractResource: a,
+			LintChecks:           opts.LintChecks,
 		}
 	}
 

internal/terraform/linter.go

@@ -0,0 +1,91 @@
+package terraform
+
+import (
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/terraform/internal/configs/configschema"
+	"github.com/hashicorp/terraform/internal/tfdiags"
+)
+
+// lintExpressionGeneric runs a generic set of linting rules on the given
+// expression, which we assume is just a normal expression that'll be evaluated
+// for its value in a lang.Scope, but with no specific assumptions about
+// where in the configuration it is placed.
+//
+// It's safe to pass a nil expression to this function, in which case this
+// function will never produce any lint warnings.
+func lintExpressionGeneric(expr hcl.Expression) tfdiags.Diagnostics {
+	if expr == nil {
+		return nil
+	}
+
+	// No individual expression rules yet
+	return nil
+}
+
+// lintExpressionsInBodyGeneric runs lintExpressionGeneric against each of
+// the expressions detected inside the given body, which it detects using
+// the given schema.
+//
+// If the body doesn't fully conform to the schema then
+// lintExpressionsInBodyGeneric will skip over some or all of the body contents
+// in order to still produce a partial result where possible.
+//
+// It's safe to pass a nil body or schema to this function, in which case this
+// function will never produce any lint warnings.
+func lintExpressionsInBodyGeneric(body hcl.Body, schema *configschema.Block) tfdiags.Diagnostics {
+	if body == nil || schema == nil {
+		return nil
+	}
+
+	return visitExpressionsInBody(body, schema, lintExpressionGeneric)
+}
+
+// visitExpressionsInBody is a helper for writting linter-like logic which
+// applies to all expressions declared in a particular HCL body, which should
+// conform to the given schema.
+//
+// If the body doesn't fully conform to the schema then visitExpressionsInBody
+// will skip over some or all of the body contents in order to still produce
+// a partial result. Therefore this expression isn't suitable for implementing
+// blocking validation, because it only makes a best effort to find expressions.
+func visitExpressionsInBody(body hcl.Body, schema *configschema.Block, cb func(hcl.Expression) tfdiags.Diagnostics) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+
+	var hclSchema hcl.BodySchema
+	for name := range schema.Attributes {
+		hclSchema.Attributes = append(hclSchema.Attributes, hcl.AttributeSchema{
+			Name: name,
+			// We intentionally don't set "Required" here because we still
+			// want to scan all given expressions, even if the author omitted
+			// some arguments.
+		})
+	}
+	for typeName, blockS := range schema.BlockTypes {
+		var labelNames []string
+		if blockS.Nesting == configschema.NestingMap {
+			labelNames = []string{"key"}
+		}
+
+		hclSchema.Blocks = append(hclSchema.Blocks, hcl.BlockHeaderSchema{
+			Type:       typeName,
+			LabelNames: labelNames,
+		})
+	}
+
+	content, _, _ := body.PartialContent(&hclSchema)
+
+	for _, attr := range content.Attributes {
+		diags = diags.Append(cb(attr.Expr))
+	}
+
+	for _, block := range content.Blocks {
+		typeName := block.Type
+		blockS, ok := schema.BlockTypes[typeName]
+		if !ok {
+			continue // Weird, but we'll tolerate it to be robust
+		}
+		diags = diags.Append(visitExpressionsInBody(block.Body, &blockS.Block, cb))
+	}
+
+	return diags
+}