core: Specialized errors for incorrect indexes in resource reference

In prior versions of Terraform we permitted inconsistent use of indexes
in resource references, but in as of 0.12 the index usage must correlate
properly with whether "count" is set on the resource.

Since users are likely to have existing configurations with incorrect
usage, here we introduce some specialized error messages for situations
where we can detect such issues statically. This seems to cover all of the
common patterns we've seen in practice.

Some usage patterns will fall back on a less-helpful dynamic error here,
but no configurations coming from 0.11 can end up that way because 0.11
did not permit forms such as aws_instance.no_count[count.index].bar that
this validation would not be able to "see".

Our configuration upgrade tool also contains a fix for this already, but
it takes a more conservative approach of adding the index [1] rather than
[count.index] because it can't be sure (without human help) if correlation
of indices is what was intended.

builtin/providers/test/data_source_test.go

@@ -192,7 +192,7 @@ data "test_data_source" "y" {
 }
 
 // referencing test_data_source.one.output_map["a"] should produce an error when
-// there's a count, but instead we end up with an unknown value after apply.
+// there's a count.
 func TestDataSource_indexedCountOfOne(t *testing.T) {
 	resource.UnitTest(t, resource.TestCase{
 		Providers: testAccProviders,
@@ -212,7 +212,7 @@ data "test_data_source" "two" {
 	}
 }
 				`),
-				ExpectError: regexp.MustCompile("value does not have any attributes"),
+				ExpectError: regexp.MustCompile("Because data.test_data_source.one has \"count\" set, its attributes must be accessed on specific instances"),
 			},
 		},
 	})

terraform/evaluate_valid.go

@@ -71,11 +71,20 @@ func (d *evaluationStateData) staticValidateReference(ref *addrs.Reference, self
 
 	switch addr := ref.Subject.(type) {
 
-	// For static validation we validate both resource and resource instance references the same way, disregarding the index
+	// For static validation we validate both resource and resource instance references the same way.
+	// We mostly disregard the index, though we do some simple validation of
+	// its _presence_ in staticValidateSingleResourceReference and
+	// staticValidateMultiResourceReference respectively.
 	case addrs.Resource:
-		return d.staticValidateResourceReference(modCfg, addr, ref.Remaining, ref.SourceRange)
+		var diags tfdiags.Diagnostics
+		diags = diags.Append(d.staticValidateSingleResourceReference(modCfg, addr, ref.Remaining, ref.SourceRange))
+		diags = diags.Append(d.staticValidateResourceReference(modCfg, addr, ref.Remaining, ref.SourceRange))
+		return diags
 	case addrs.ResourceInstance:
-		return d.staticValidateResourceReference(modCfg, addr.ContainingResource(), ref.Remaining, ref.SourceRange)
+		var diags tfdiags.Diagnostics
+		diags = diags.Append(d.staticValidateMultiResourceReference(modCfg, addr, ref.Remaining, ref.SourceRange))
+		diags = diags.Append(d.staticValidateResourceReference(modCfg, addr.ContainingResource(), ref.Remaining, ref.SourceRange))
+		return diags
 
 	// We also handle all module call references the same way, disregarding index.
 	case addrs.ModuleCall:
@@ -106,6 +115,78 @@ func (d *evaluationStateData) staticValidateReference(ref *addrs.Reference, self
 	}
 }
 
+func (d *evaluationStateData) staticValidateSingleResourceReference(modCfg *configs.Config, addr addrs.Resource, remain hcl.Traversal, rng tfdiags.SourceRange) tfdiags.Diagnostics {
+	// If we have at least one step in "remain" and this resource has
+	// "count" set then we know for sure this in invalid because we have
+	// something like:
+	//     aws_instance.foo.bar
+	// ...when we really need
+	//     aws_instance.foo[count.index].bar
+
+	// It is _not_ safe to do this check when remain is empty, because that
+	// would also match aws_instance.foo[count.index].bar due to `count.index`
+	// not being statically-resolvable as part of a reference, and match
+	// direct references to the whole aws_instance.foo tuple.
+	if len(remain) == 0 {
+		return nil
+	}
+
+	var diags tfdiags.Diagnostics
+
+	cfg := modCfg.Module.ResourceByAddr(addr)
+	if cfg == nil {
+		// We'll just bail out here and catch this in our subsequent call to
+		// staticValidateResourceReference, then.
+		return diags
+	}
+
+	if cfg.Count != nil {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  `Missing resource instance key`,
+			Detail:   fmt.Sprintf("Because %s has \"count\" set, its attributes must be accessed on specific instances.\n\nFor example, to correlate with indices of a referring resource, use:\n    %s[count.index]", addr, addr),
+			Subject:  rng.ToHCL().Ptr(),
+		})
+	}
+	if cfg.ForEach != nil {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  `Missing resource instance key`,
+			Detail:   fmt.Sprintf("Because %s has \"for_each\" set, its attributes must be accessed on specific instances.\n\nFor example, to correlate with indices of a referring resource, use:\n    %s[each.key]", addr, addr),
+			Subject:  rng.ToHCL().Ptr(),
+		})
+	}
+
+	return diags
+}
+
+func (d *evaluationStateData) staticValidateMultiResourceReference(modCfg *configs.Config, addr addrs.ResourceInstance, remain hcl.Traversal, rng tfdiags.SourceRange) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+
+	cfg := modCfg.Module.ResourceByAddr(addr.ContainingResource())
+	if cfg == nil {
+		// We'll just bail out here and catch this in our subsequent call to
+		// staticValidateResourceReference, then.
+		return diags
+	}
+
+	if addr.Key == addrs.NoKey {
+		// This is a different path into staticValidateSingleResourceReference
+		return d.staticValidateSingleResourceReference(modCfg, addr.ContainingResource(), remain, rng)
+	} else {
+		if cfg.Count == nil && cfg.ForEach == nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  `Unexpected resource instance key`,
+				Detail:   fmt.Sprintf(`Because %s does not have "count" or "for_each" set, references to it must not include an index key. Remove the bracketed index to refer to the single instance of this resource.`, addr.ContainingResource()),
+				Subject:  rng.ToHCL().Ptr(),
+			})
+		}
+	}
+
+	return diags
+}
+
 func (d *evaluationStateData) staticValidateResourceReference(modCfg *configs.Config, addr addrs.Resource, remain hcl.Traversal, rng tfdiags.SourceRange) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
@@ -125,7 +206,7 @@ func (d *evaluationStateData) staticValidateResourceReference(modCfg *configs.Co
 		diags = diags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,
 			Summary:  `Reference to undeclared resource`,
-			Detail:   fmt.Sprintf(`A %s resource %q %q has not been declared in %s`, modeAdjective, addr.Type, addr.Name, moduleConfigDisplayAddr(modCfg.Path)),
+			Detail:   fmt.Sprintf(`A %s resource %q %q has not been declared in %s.`, modeAdjective, addr.Type, addr.Name, moduleConfigDisplayAddr(modCfg.Path)),
 			Subject:  rng.ToHCL().Ptr(),
 		})
 		return diags

terraform/evaluate_valid_test.go

@@ -0,0 +1,102 @@
+package terraform
+
+import (
+	"testing"
+
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/hashicorp/hcl2/hcl/hclsyntax"
+
+	"github.com/hashicorp/terraform/configs/configschema"
+	"github.com/hashicorp/terraform/lang"
+)
+
+func TestStaticValidateReferences(t *testing.T) {
+	tests := []struct {
+		Ref     string
+		WantErr string
+	}{
+		{
+			"aws_instance.no_count",
+			``,
+		},
+		{
+			"aws_instance.count",
+			``,
+		},
+		{
+			"aws_instance.count[0]",
+			``,
+		},
+		{
+			"aws_instance.nonexist",
+			`Reference to undeclared resource: A managed resource "aws_instance" "nonexist" has not been declared in the root module.`,
+		},
+		{
+			"aws_instance.no_count[0]",
+			`Unexpected resource instance key: Because aws_instance.no_count does not have "count" or "for_each" set, references to it must not include an index key. Remove the bracketed index to refer to the single instance of this resource.`,
+		},
+		{
+			"aws_instance.count.foo",
+			// In this case we return two errors that are somewhat redundant with
+			// one another, but we'll accept that because they both report the
+			// problem from different perspectives and so give the user more
+			// opportunity to understand what's going on here.
+			`2 problems:
+
+- Missing resource instance key: Because aws_instance.count has "count" set, its attributes must be accessed on specific instances.
+
+For example, to correlate with indices of a referring resource, use:
+    aws_instance.count[count.index]
+- Unsupported attribute: This object has no argument, nested block, or exported attribute named "foo".`,
+		},
+	}
+
+	cfg := testModule(t, "static-validate-refs")
+	evaluator := &Evaluator{
+		Config: cfg,
+		Schemas: &Schemas{
+			Providers: map[string]*ProviderSchema{
+				"aws": {
+					ResourceTypes: map[string]*configschema.Block{
+						"aws_instance": {},
+					},
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Ref, func(t *testing.T) {
+			traversal, hclDiags := hclsyntax.ParseTraversalAbs([]byte(test.Ref), "", hcl.Pos{Line: 1, Column: 1})
+			if hclDiags.HasErrors() {
+				t.Fatal(hclDiags.Error())
+			}
+
+			refs, diags := lang.References([]hcl.Traversal{traversal})
+			if diags.HasErrors() {
+				t.Fatal(diags.Err())
+			}
+
+			data := &evaluationStateData{
+				Evaluator: evaluator,
+			}
+
+			diags = data.StaticValidateReferences(refs, nil)
+			if diags.HasErrors() {
+				if test.WantErr == "" {
+					t.Fatalf("Unexpected diagnostics: %s", diags.Err())
+				}
+
+				gotErr := diags.Err().Error()
+				if gotErr != test.WantErr {
+					t.Fatalf("Wrong diagnostics\ngot:  %s\nwant: %s", gotErr, test.WantErr)
+				}
+				return
+			}
+
+			if test.WantErr != "" {
+				t.Fatalf("Expected diagnostics, but got none\nwant: %s", test.WantErr)
+			}
+		})
+	}
+}

terraform/test-fixtures/static-validate-refs/static-validate-refs.tf

@@ -0,0 +1,6 @@
+resource "aws_instance" "no_count" {
+}
+
+resource "aws_instance" "count" {
+  count = 1
+}