configs/configupgrade: Remove redundant list brackets

In early versions of Terraform where the interpolation language didn't
have any real list support, list brackets around a single string was the
signal to split the string on a special uuid separator to produce a list
just in time for processing, giving expressions like this:

    foo = ["${test_instance.foo.*.id}"]

Logically this is weird because it looks like it should produce a list
of lists of strings. When we added real list support in Terraform 0.7 we
retained support for this behavior by trimming off extra levels of list
during evaluation, and inadvertently continued relying on this notation
for correct type checking.

During the Terraform 0.10 line we fixed the type checker bugs (a few
remaining issues notwithstanding) so that it was finally possible to
use the more intuitive form:

    foo = "${test_instance.foo.*.id}"

...but we continued trimming off extra levels of list for backward
compatibility.

Terraform 0.12 finally removes that compatibility shim, causing redundant
list brackets to be interpreted as a list of lists.

This upgrade rule attempts to identify situations that are relying on the
old compatibility behavior and trim off the redundant extra brackets. It's
not possible to do this fully-generally using only static analysis, but
we can gather enough information through or partial type inference
mechanism here to deal with the most common situations automatically and
produce a TF-UPGRADE-TODO comment for more complex scenarios where the
user intent isn't decidable with only static analysis.

In particular, this handles by far the most common situation of wrapping
list brackets around a splat expression like the first example above.
After this and the other upgrade rules are applied, the first example
above will become:

    foo = test_instance.foo.*.id

configs/configupgrade/test-fixtures/valid/redundant-list/input/redundant-list.tf

@@ -0,0 +1,52 @@
+variable "listy" {
+  type = "list"
+}
+
+resource "test_instance" "other" {
+  count = 2
+}
+
+resource "test_instance" "bad1" {
+  security_groups = ["${test_instance.other.*.id}"]
+}
+
+resource "test_instance" "bad2" {
+  security_groups = ["${var.listy}"]
+}
+
+resource "test_instance" "bad3" {
+  security_groups = ["${module.foo.outputs_always_dynamic}"]
+}
+
+resource "test_instance" "bad4" {
+  security_groups = ["${list("a", "b", "c")}"]
+}
+
+# The rest of these should keep the same amount of list-ness
+
+resource "test_instance" "ok1" {
+  security_groups = []
+}
+
+resource "test_instance" "ok2" {
+  security_groups = ["notalist"]
+}
+
+resource "test_instance" "ok3" {
+  security_groups = ["${path.module}"]
+}
+
+resource "test_instance" "ok4" {
+  security_groups = [["foo"], ["bar"]]
+}
+
+resource "test_instance" "ok5" {
+  security_groups = "${test_instance.other.*.id}"
+}
+
+resource "test_instance" "ok6" {
+  security_groups = [
+    "${test_instance.other1.*.id}",
+    "${test_instance.other2.*.id}",
+  ]
+}

configs/configupgrade/test-fixtures/valid/redundant-list/want/redundant-list.tf

@@ -0,0 +1,60 @@
+variable "listy" {
+  type = list(string)
+}
+
+resource "test_instance" "other" {
+  count = 2
+}
+
+resource "test_instance" "bad1" {
+  security_groups = test_instance.other.*.id
+}
+
+resource "test_instance" "bad2" {
+  security_groups = var.listy
+}
+
+resource "test_instance" "bad3" {
+  # TF-UPGRADE-TODO: In Terraform v0.10 and earlier, it was sometimes necessary to
+  # force an interpolation expression to be interpreted as a list by wrapping it
+  # in an extra set of list brackets. That form was supported for compatibilty in
+  # v0.11, but is no longer supported in Terraform v0.12.
+  #
+  # If the expression in the following list itself returns a list, remove the
+  # brackets to avoid interpretation as a list of lists. If the expression
+  # returns a single list item then leave it as-is and remove this TODO comment.
+  security_groups = [module.foo.outputs_always_dynamic]
+}
+
+resource "test_instance" "bad4" {
+  security_groups = ["a", "b", "c"]
+}
+
+# The rest of these should keep the same amount of list-ness
+
+resource "test_instance" "ok1" {
+  security_groups = []
+}
+
+resource "test_instance" "ok2" {
+  security_groups = ["notalist"]
+}
+
+resource "test_instance" "ok3" {
+  security_groups = [path.module]
+}
+
+resource "test_instance" "ok4" {
+  security_groups = [["foo"], ["bar"]]
+}
+
+resource "test_instance" "ok5" {
+  security_groups = test_instance.other.*.id
+}
+
+resource "test_instance" "ok6" {
+  security_groups = [
+    test_instance.other1.*.id,
+    test_instance.other2.*.id,
+  ]
+}

configs/configupgrade/test-fixtures/valid/redundant-list/want/versions.tf

@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}

configs/configupgrade/upgrade_body.go

@@ -153,7 +153,7 @@ func dependsOnAttributeRule(filename string, an *analysis) bodyItemRule {
 	}
 }
 
-func attributeRule(filename string, wantTy cty.Type, an *analysis, upgradeExpr func(val interface{}) ([]byte, tfdiags.Diagnostics)) bodyItemRule {
+func attributeRule(filename string, wantTy cty.Type, an *analysis, exprRule func(val interface{}) ([]byte, tfdiags.Diagnostics)) bodyItemRule {
 	return func(buf *bytes.Buffer, blockAddr string, item *hcl1ast.ObjectItem) tfdiags.Diagnostics {
 		var diags tfdiags.Diagnostics
 
@@ -174,7 +174,77 @@ func attributeRule(filename string, wantTy cty.Type, an *analysis, upgradeExpr f
 			return diags
 		}
 
-		valSrc, valDiags := upgradeExpr(item.Val)
+		val := item.Val
+
+		if typeIsSettableFromTupleCons(wantTy) && !typeIsSettableFromTupleCons(wantTy.ElementType()) {
+			// In Terraform circa 0.10 it was required to wrap any expression
+			// that produces a list in HCL list brackets to allow type analysis
+			// to complete successfully, even though logically that ought to
+			// have produced a list of lists.
+			//
+			// In Terraform 0.12 this construct _does_ produce a list of lists, so
+			// we need to update expressions that look like older usage. We can't
+			// do this exactly with static analysis, but we can make a best-effort
+			// and produce a warning if type inference is impossible for a
+			// particular expression. This should give good results for common
+			// simple examples, like splat expressions.
+			//
+			// There are four possible cases here:
+			// - The value isn't an HCL1 list expression at all, or is one that
+			//   contains more than one item, in which case this special case
+			//   does not apply.
+			// - The inner expression after upgrading can be proven to return
+			//   a sequence type, in which case we must definitely remove
+			//   the wrapping brackets.
+			// - The inner expression after upgrading can be proven to return
+			//   a non-sequence type, in which case we fall through and treat
+			//   the whole item like a normal expression.
+			// - Static type analysis is impossible (it returns cty.DynamicPseudoType),
+			//   in which case we will make no changes but emit a warning and
+			//   a TODO comment for the user to decide whether a change needs
+			//   to be made in practice.
+			if list, ok := val.(*hcl1ast.ListType); ok {
+				if len(list.List) == 1 {
+					maybeAlsoList := list.List[0]
+					if exprSrc, diags := upgradeExpr(maybeAlsoList, filename, true, an); !diags.HasErrors() {
+						// Ideally we would set "self" here but we don't have
+						// enough context to set it and in practice not setting
+						// it only affects expressions inside provisioner and
+						// connection blocks, and the list-wrapping thing isn't
+						// common there.
+						gotTy := an.InferExpressionType(exprSrc, nil)
+						if typeIsSettableFromTupleCons(gotTy) {
+							// Since this expression was already inside HCL list brackets,
+							// the ultimate result would be a list of lists and so we
+							// need to unwrap it by taking just the portion within
+							// the brackets here.
+							val = maybeAlsoList
+						}
+						if gotTy == cty.DynamicPseudoType {
+							// User must decide.
+							diags = diags.Append(&hcl2.Diagnostic{
+								Severity: hcl2.DiagError,
+								Summary:  "Possible legacy dynamic list usage",
+								Detail:   "This list may be using the legacy redundant-list expression style from Terraform v0.10 and earlier. If the expression within these brackets returns a list itself, remove these brackets.",
+								Subject:  hcl1PosRange(filename, list.Lbrack).Ptr(),
+							})
+							buf.WriteString(
+								"# TF-UPGRADE-TODO: In Terraform v0.10 and earlier, it was sometimes necessary to\n" +
+									"# force an interpolation expression to be interpreted as a list by wrapping it\n" +
+									"# in an extra set of list brackets. That form was supported for compatibilty in\n" +
+									"# v0.11, but is no longer supported in Terraform v0.12.\n" +
+									"#\n" +
+									"# If the expression in the following list itself returns a list, remove the\n" +
+									"# brackets to avoid interpretation as a list of lists. If the expression\n" +
+									"# returns a single list item then leave it as-is and remove this TODO comment.\n",
+							)
+						}
+					}
+				}
+			}
+		}
+
+		valSrc, valDiags := exprRule(val)
 		diags = diags.Append(valDiags)
 		printAttribute(buf, item.Keys[0].Token.Value().(string), valSrc, item.LineComment)
 

configs/configupgrade/upgrade_expr.go

@@ -9,6 +9,7 @@ import (
 
 	hcl2 "github.com/hashicorp/hcl2/hcl"
 	hcl2syntax "github.com/hashicorp/hcl2/hcl/hclsyntax"
+	"github.com/zclconf/go-cty/cty"
 
 	hcl1ast "github.com/hashicorp/hcl/hcl/ast"
 	hcl1printer "github.com/hashicorp/hcl/hcl/printer"
@@ -546,3 +547,7 @@ func upgradeTerraformRemoteStateTraversalParts(parts []string, an *analysis) []s
 	copy(newParts[attrIdx+1:], parts[attrIdx:])
 	return newParts
 }
+
+func typeIsSettableFromTupleCons(ty cty.Type) bool {
+	return ty.IsListType() || ty.IsTupleType() || ty.IsSetType()
+}

configs/configupgrade/upgrade_test.go

@@ -190,10 +190,11 @@ var testProviders = map[string]providers.Factory{
 			ResourceTypes: map[string]*configschema.Block{
 				"test_instance": {
 					Attributes: map[string]*configschema.Attribute{
-						"id":    {Type: cty.String, Computed: true},
-						"type":  {Type: cty.String, Optional: true},
-						"image": {Type: cty.String, Optional: true},
-						"tags":  {Type: cty.Map(cty.String), Optional: true},
+						"id":              {Type: cty.String, Computed: true},
+						"type":            {Type: cty.String, Optional: true},
+						"image":           {Type: cty.String, Optional: true},
+						"tags":            {Type: cty.Map(cty.String), Optional: true},
+						"security_groups": {Type: cty.List(cty.String), Optional: true},
 					},
 					BlockTypes: map[string]*configschema.NestedBlock{
 						"network": {