From 4927a4105b9a563cd6c3e353ef90a413305f28bc Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Fri, 30 Nov 2018 18:23:09 -0800
Subject: [PATCH] configs/configupgrade: Replace calls to list() and map()

We now have native language features for declaring tuples and objects,
which are the idiomatic way to construct sequence and mapping values that
can then be converted to list, set, and map types as needed.
---
 .../funcs-replaced/input/funcs-replaced.tf    | 13 ++++
 .../funcs-replaced/want/funcs-replaced.tf     | 29 +++++++
 .../valid/funcs-replaced/want/versions.tf     |  3 +
 configs/configupgrade/upgrade_expr.go         | 78 ++++++++++++++++++-
 4 files changed, 119 insertions(+), 4 deletions(-)
 create mode 100644 configs/configupgrade/test-fixtures/valid/funcs-replaced/input/funcs-replaced.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/funcs-replaced/want/funcs-replaced.tf
 create mode 100644 configs/configupgrade/test-fixtures/valid/funcs-replaced/want/versions.tf

diff --git a/configs/configupgrade/test-fixtures/valid/funcs-replaced/input/funcs-replaced.tf b/configs/configupgrade/test-fixtures/valid/funcs-replaced/input/funcs-replaced.tf
new file mode 100644
index 0000000000..1f631a34cc
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/funcs-replaced/input/funcs-replaced.tf
@@ -0,0 +1,13 @@
+locals {
+  list        = "${list("a", "b", "c")}"
+  list_concat = "${concat(list("a", "b"), list("c"))}"
+  list_empty  = "${list()}"
+
+  map         = "${map("a", "b", "c", "d")}"
+  map_merge   = "${merge(map("a", "b"), map("c", "d"))}"
+  map_empty   = "${map()}"
+  map_invalid = "${map("a", "b", "c")}"
+
+  list_of_map = "${list(map("a", "b"))}"
+  map_of_list = "${map("a", list("b"))}"
+}
diff --git a/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/funcs-replaced.tf b/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/funcs-replaced.tf
new file mode 100644
index 0000000000..f6c3afaa51
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/funcs-replaced.tf
@@ -0,0 +1,29 @@
+locals {
+  list        = ["a", "b", "c"]
+  list_concat = concat(["a", "b"], ["c"])
+  list_empty  = []
+
+  map = {
+    "a" = "b"
+    "c" = "d"
+  }
+  map_merge = merge(
+    {
+      "a" = "b"
+    },
+    {
+      "c" = "d"
+    },
+  )
+  map_empty   = {}
+  map_invalid = map("a", "b", "c")
+
+  list_of_map = [
+    {
+      "a" = "b"
+    },
+  ]
+  map_of_list = {
+    "a" = ["b"]
+  }
+}
diff --git a/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/versions.tf b/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/versions.tf
new file mode 100644
index 0000000000..d9b6f790b9
--- /dev/null
+++ b/configs/configupgrade/test-fixtures/valid/funcs-replaced/want/versions.tf
@@ -0,0 +1,3 @@
+terraform {
+  required_version = ">= 0.12"
+}
diff --git a/configs/configupgrade/upgrade_expr.go b/configs/configupgrade/upgrade_expr.go
index a8d8eae31b..5452678717 100644
--- a/configs/configupgrade/upgrade_expr.go
+++ b/configs/configupgrade/upgrade_expr.go
@@ -172,16 +172,86 @@ func upgradeExpr(val interface{}, filename string, interp bool, an *analysis) ([
 		name := tv.Func
 		args := tv.Args
 
-		buf.WriteString(name)
-		buf.WriteByte('(')
+		argExprs := make([][]byte, len(args))
+		multiline := false
+		totalLen := 0
 		for i, arg := range args {
 			if i > 0 {
-				buf.WriteString(", ")
+				totalLen += 2
 			}
-
 			exprSrc, exprDiags := upgradeExpr(arg, filename, true, an)
 			diags = diags.Append(exprDiags)
+			argExprs[i] = exprSrc
+			if bytes.Contains(exprSrc, []byte{'\n'}) {
+				// If any of our arguments are multi-line then we'll also be multiline
+				multiline = true
+			}
+			totalLen += len(exprSrc)
+		}
+
+		if totalLen > 60 { // heuristic, since we don't know here how indented we are already
+			multiline = true
+		}
+
+		// Some functions are now better expressed as native language constructs.
+		// These cases will return early if they emit anything, or otherwise
+		// fall through to the default emitter.
+		switch name {
+		case "list":
+			// Should now use tuple constructor syntax
+			buf.WriteByte('[')
+			if multiline {
+				buf.WriteByte('\n')
+			}
+			for i, exprSrc := range argExprs {
+				buf.Write(exprSrc)
+				if multiline {
+					buf.WriteString(",\n")
+				} else {
+					if i < len(args)-1 {
+						buf.WriteString(", ")
+					}
+				}
+			}
+			buf.WriteByte(']')
+			return buf.Bytes(), diags
+		case "map":
+			// Should now use object constructor syntax, but we can only
+			// achieve that if the call is valid, which requires an even
+			// number of arguments.
+			if len(argExprs) == 0 {
+				buf.WriteString("{}")
+				return buf.Bytes(), diags
+			} else if len(argExprs)%2 == 0 {
+				buf.WriteString("{\n")
+				for i := 0; i < len(argExprs); i += 2 {
+					k := argExprs[i]
+					v := argExprs[i+1]
+
+					buf.Write(k)
+					buf.WriteString(" = ")
+					buf.Write(v)
+					buf.WriteByte('\n')
+				}
+				buf.WriteByte('}')
+				return buf.Bytes(), diags
+			}
+		}
+
+		buf.WriteString(name)
+		buf.WriteByte('(')
+		if multiline {
+			buf.WriteByte('\n')
+		}
+		for i, exprSrc := range argExprs {
 			buf.Write(exprSrc)
+			if multiline {
+				buf.WriteString(",\n")
+			} else {
+				if i < len(args)-1 {
+					buf.WriteString(", ")
+				}
+			}
 		}
 		buf.WriteByte(')')