Stop requiring multi-vars (splats) to be in array brackets

Prior to Terraform 0.7, lists in Terraform were just a shallow abstraction
on top of strings with a magic delimiter between items. Wrapping a single
string in brackets in the configuration was Terraform's prompt that it
needed to split the string on that delimiter during interpolation.

In 0.7, when first-class lists were added, this convention was preserved
by flattening lists-of-lists by one level when they were encountered in
configuration. However, there was an oversight in that change where it
did not correctly handle the case where the inner list was unknown.

In #14135 we removed some code that was flattening partially-unknown lists
into fully-unknown (untyped) values. This inadvertently exposed the missed
case from the previous paragraph, causing issues for list-wrapped splat
expressions with unknown members. While this worked fine for resources,
due to some fixup done inside helper/schema, this did not work for other
interpolation contexts such as module blocks.

Various attempts to fix this up and restore the flattening behavior
selectively were unsuccessful, due to a proliferation of assumptions all
over the core code that would be too risky to change just to fix this bug.

This change, then, takes the different approach of removing the
requirement that splats be presented inside list brackets. This
requirement didn't make much sense anymore anyway, since no other
list-returning expression had this constraint and so the rest of Terraform
was already successfully dealing with both cases.

This leaves us with two different scenarios:

- For resource arguments, existing normalization code in helper/schema
  does its own flattening that preserves compatibility with the common
  practice of using bracketed splats. This change proves this with a test
  within the "test" provider that exercises the whole Terraform core and
  helper/schema stack that assigns bracketed splats to list and set
  attributes.

- For arguments in other blocks, such as in module callsites, the
  interpolator's own flattening behavior applies to known lists,
  preserving compatibility with configurations from before
  partially-computed splats were possible, but those wishing to use
  partially-computed splats are required to drop the surrounding brackets.
  This is less concerning because this scenario was introduced only in
  0.9.5, so the scope for breakage is limited to those who adopted this
  new feature quickly after upgrading.

As of this commit, the recommendation is to stop using brackets around
splats but the old form continues to be supported for backward
compatibility. In a future _major_ version of Terraform we will probably
phase out this legacy form to improve consistency, but for now both
forms are acceptable at the expense of some (pre-existing) weird behavior
when _actual_ lists-of-lists are used.

This addresses #14521 by officially adopting the suggested workaround of
dropping the brackets around the splat. However, it doesn't yet allow
passing of a partially-unknown list between modules: that still violates
assumptions in Terraform's core, so for the moment partially-unknown lists
work only within a _single_ interpolation expression, and cannot be
passed around between expressions. Until more holistic work is done to
improve Terraform's type handling, passing a partially-unknown splat
through to a module will result in a fully-unknown list emerging on
the other side, just as was the case before #14135; this change just
addresses the fact that this was failing with an error in 0.9.5.

builtin/providers/test/resource.go

@@ -46,6 +46,11 @@ func testResource() *schema.Resource {
 				Computed: true,
 				ForceNew: true,
 			},
+			"computed_from_required": {
+				Type:     schema.TypeString,
+				Computed: true,
+				ForceNew: true,
+			},
 			"computed_read_only_force_new": {
 				Type:     schema.TypeString,
 				Computed: true,
@@ -129,6 +134,8 @@ func testResourceCreate(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("Missing attribute 'required_map', but it's required!")
 	}
 
+	d.Set("computed_from_required", d.Get("required"))
+
 	return testResourceRead(d, meta)
 }
 

builtin/providers/test/splat_flatten_test.go

@@ -0,0 +1,79 @@
+package test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
+)
+
+// This is actually a test of some core functionality in conjunction with
+// helper/schema, rather than of the test provider itself.
+//
+// Here we're just verifying that unknown splats get flattened when assigned
+// to list and set attributes. A variety of other situations are tested in
+// an apply context test in the core package, but for this part we lean on
+// helper/schema and thus need to exercise it at a higher level.
+
+func TestSplatFlatten(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: `
+resource "test_resource" "source" {
+	required = "foo ${count.index}"
+	required_map = {
+	    key = "value"
+	}
+	count = 3
+}
+
+resource "test_resource" "splatted" {
+	# This legacy form of splatting into a list is still supported for
+	# backward-compatibility but no longer suggested.
+	set = ["${test_resource.source.*.computed_from_required}"]
+	list = ["${test_resource.source.*.computed_from_required}"]
+
+	required = "yep"
+	required_map = {
+	    key = "value"
+	}
+}
+				`,
+				Check: func(s *terraform.State) error {
+					gotAttrs := s.RootModule().Resources["test_resource.splatted"].Primary.Attributes
+					t.Logf("attrs %#v", gotAttrs)
+					wantAttrs := map[string]string{
+						"list.#": "3",
+						"list.0": "foo 0",
+						"list.1": "foo 1",
+						"list.2": "foo 2",
+
+						// This depends on the default set hash implementation.
+						// If that changes, these keys will need to be updated.
+						"set.#":          "3",
+						"set.1136855734": "foo 0",
+						"set.885275168":  "foo 1",
+						"set.2915920794": "foo 2",
+					}
+					errored := false
+					for k, want := range wantAttrs {
+						got := gotAttrs[k]
+						if got != want {
+							t.Errorf("Wrong %s value %q; want %q", k, got, want)
+							errored = true
+						}
+					}
+					if errored {
+						return errors.New("incorrect attribute values")
+					}
+					return nil
+				},
+			},
+		},
+	})
+
+}

config/config.go

@@ -705,17 +705,6 @@ func (c *Config) Validate() error {
 		}
 	}
 
-	// Check that all variables are in the proper context
-	for source, rc := range c.rawConfigs() {
-		walker := &interpolationWalker{
-			ContextF: c.validateVarContextFn(source, &errs),
-		}
-		if err := reflectwalk.Walk(rc.Raw, walker); err != nil {
-			errs = append(errs, fmt.Errorf(
-				"%s: error reading config: %s", source, err))
-		}
-	}
-
 	// Validate the self variable
 	for source, rc := range c.rawConfigs() {
 		// Ignore provisioners. This is a pretty brittle way to do this,
@@ -787,57 +776,6 @@ func (c *Config) rawConfigs() map[string]*RawConfig {
 	return result
 }
 
-func (c *Config) validateVarContextFn(
-	source string, errs *[]error) interpolationWalkerContextFunc {
-	return func(loc reflectwalk.Location, node ast.Node) {
-		// If we're in a slice element, then its fine, since you can do
-		// anything in there.
-		if loc == reflectwalk.SliceElem {
-			return
-		}
-
-		// Otherwise, let's check if there is a splat resource variable
-		// at the top level in here. We do this by doing a transform that
-		// replaces everything with a noop node unless its a variable
-		// access or concat. This should turn the AST into a flat tree
-		// of Concat(Noop, ...). If there are any variables left that are
-		// multi-access, then its still broken.
-		node = node.Accept(func(n ast.Node) ast.Node {
-			// If it is a concat or variable access, we allow it.
-			switch n.(type) {
-			case *ast.Output:
-				return n
-			case *ast.VariableAccess:
-				return n
-			}
-
-			// Otherwise, noop
-			return &noopNode{}
-		})
-
-		vars, err := DetectVariables(node)
-		if err != nil {
-			// Ignore it since this will be caught during parse. This
-			// actually probably should never happen by the time this
-			// is called, but its okay.
-			return
-		}
-
-		for _, v := range vars {
-			rv, ok := v.(*ResourceVariable)
-			if !ok {
-				return
-			}
-
-			if rv.Multi && rv.Index == -1 {
-				*errs = append(*errs, fmt.Errorf(
-					"%s: use of the splat ('*') operator must be wrapped in a list declaration",
-					source))
-			}
-		}
-	}
-}
-
 func (c *Config) validateDependsOn(
 	n string,
 	v []string,

config/config_test.go

@@ -593,20 +593,6 @@ func TestConfigValidate_varMultiExactNonSlice(t *testing.T) {
 	}
 }
 
-func TestConfigValidate_varMultiNonSlice(t *testing.T) {
-	c := testConfig(t, "validate-var-multi-non-slice")
-	if err := c.Validate(); err == nil {
-		t.Fatal("should not be valid")
-	}
-}
-
-func TestConfigValidate_varMultiNonSliceProvisioner(t *testing.T) {
-	c := testConfig(t, "validate-var-multi-non-slice-provisioner")
-	if err := c.Validate(); err == nil {
-		t.Fatal("should not be valid")
-	}
-}
-
 func TestConfigValidate_varMultiFunctionCall(t *testing.T) {
 	c := testConfig(t, "validate-var-multi-func")
 	if err := c.Validate(); err != nil {

terraform/context_apply_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/hashicorp/terraform/config/module"
 )
 
@@ -3278,6 +3279,154 @@ func TestContext2Apply_multiVar(t *testing.T) {
 	}
 }
 
+// This is a holistic test of multi-var (aka "splat variable") handling
+// across several different Terraform subsystems. This is here because
+// historically there were quirky differences in handling across different
+// parts of Terraform and so here we want to assert the expected behavior and
+// ensure that it remains consistent in future.
+func TestContext2Apply_multiVarComprehensive(t *testing.T) {
+	m := testModule(t, "apply-multi-var-comprehensive")
+	p := testProvider("test")
+
+	configs := map[string]*ResourceConfig{}
+
+	p.ApplyFn = testApplyFn
+
+	p.DiffFn = func(info *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) {
+		configs[info.HumanId()] = c
+
+		// Return a minimal diff to make sure this resource gets included in
+		// the apply graph and thus the final state, but otherwise we're just
+		// gathering data for assertions.
+		return &InstanceDiff{
+			Attributes: map[string]*ResourceAttrDiff{
+				"id": &ResourceAttrDiff{
+					NewComputed: true,
+				},
+				"name": &ResourceAttrDiff{
+					New: info.HumanId(),
+				},
+			},
+		}, nil
+	}
+
+	// First, apply with a count of 3
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"test": testProviderFuncFixed(p),
+		},
+		Variables: map[string]interface{}{
+			"count": "3",
+		},
+	})
+
+	if _, err := ctx.Plan(); err != nil {
+		t.Fatalf("error during plan: %s", err)
+	}
+
+	checkConfig := func(name string, want map[string]interface{}) {
+		got := configs[name].Config
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf(
+				"wrong config for %s\ngot:  %s\nwant: %s",
+				name, spew.Sdump(got), spew.Sdump(want),
+			)
+		}
+	}
+
+	checkConfig("test_thing.multi_count_var.0", map[string]interface{}{
+		"source_id":   unknownValue(),
+		"source_name": "test_thing.source.0",
+	})
+	checkConfig("test_thing.multi_count_var.2", map[string]interface{}{
+		"source_id":   unknownValue(),
+		"source_name": "test_thing.source.2",
+	})
+	checkConfig("test_thing.multi_count_derived.0", map[string]interface{}{
+		"source_id":   unknownValue(),
+		"source_name": "test_thing.source.0",
+	})
+	checkConfig("test_thing.multi_count_derived.2", map[string]interface{}{
+		"source_id":   unknownValue(),
+		"source_name": "test_thing.source.2",
+	})
+	checkConfig("test_thing.whole_splat", map[string]interface{}{
+		"source_ids": unknownValue(),
+		"source_names": []interface{}{
+			"test_thing.source.0",
+			"test_thing.source.1",
+			"test_thing.source.2",
+		},
+		"source_ids_from_func": unknownValue(),
+		"source_names_from_func": []interface{}{
+			"test_thing.source.0",
+			"test_thing.source.1",
+			"test_thing.source.2",
+		},
+
+		// This one ends up being a list with a single unknown value at this
+		// layer, but is fixed up inside helper/schema. There is a test for
+		// this inside the "test" provider, since core tests can't exercise
+		// helper/schema functionality.
+		"source_ids_wrapped": []interface{}{unknownValue()},
+
+		"source_names_wrapped": []interface{}{
+			"test_thing.source.0",
+			"test_thing.source.1",
+			"test_thing.source.2",
+		},
+		"first_source_id":   unknownValue(),
+		"first_source_name": "test_thing.source.0",
+	})
+	checkConfig("module.child.test_thing.whole_splat", map[string]interface{}{
+		"source_ids": unknownValue(),
+		"source_names": []interface{}{
+			"test_thing.source.0",
+			"test_thing.source.1",
+			"test_thing.source.2",
+		},
+
+		// This one ends up being a list with a single unknown value at this
+		// layer, but is fixed up inside helper/schema. There is a test for
+		// this inside the "test" provider, since core tests can't exercise
+		// helper/schema functionality.
+		"source_ids_wrapped": []interface{}{unknownValue()},
+
+		"source_names_wrapped": []interface{}{
+			"test_thing.source.0",
+			"test_thing.source.1",
+			"test_thing.source.2",
+		},
+	})
+
+	state, err := ctx.Apply()
+	if err != nil {
+		t.Fatalf("error during apply: %s", err)
+	}
+
+	{
+		want := map[string]interface{}{
+			"source_ids": []interface{}{"foo", "foo", "foo"},
+			"source_names": []interface{}{
+				"test_thing.source.0",
+				"test_thing.source.1",
+				"test_thing.source.2",
+			},
+		}
+		got := map[string]interface{}{}
+		for k, s := range state.RootModule().Outputs {
+			got[k] = s.Value
+		}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf(
+				"wrong outputs\ngot:  %s\nwant: %s",
+				spew.Sdump(got), spew.Sdump(want),
+			)
+		}
+	}
+}
+
 // Test that multi-var (splat) access is ordered by count, not by
 // value.
 func TestContext2Apply_multiVarOrder(t *testing.T) {