command/fmt: Restore some opinionated behaviors

In Terraform 0.11 and earlier, the "terraform fmt" command was very
opinionated in the interests of consistency. While that remains its goal,
for pragmatic reasons Terraform 0.12 significantly reduced the number
of formatting behaviors in the fmt command. We've held off on introducing
0.12-and-later-flavored cleanups out of concern it would make it harder
to maintain modules that are cross-compatible with both Terraform 0.11
and 0.12, but with this aimed to land in 0.14 -- two major releases
later -- our new goal is to help those who find older Terraform language
examples learn about the more modern idiom.

More rules may follow later, now that the implementation is set up to
allow modifications to tokens as well as modifications to whitespace, but
for this initial pass the command will now apply the following formatting
conventions:

 - 0.11-style quoted variable type constraints will be replaced with their
   0.12 syntax equivalents. For example, "string" becomes just string.
   (This change quiets a deprecation warning.)
 - Collection type constraints that don't specify an element type will
   be rewritten to specify the "any" element type explicitly, so
   list becomes list(any).
 - Arguments whose expressions consist of a quoted string template with
   only a single interpolation sequence inside will be "unwrapped" to be
   the naked expression instead, which is functionally equivalent.
   (This change quiets a deprecation warning.)
 - Block labels are given in quotes.

Two of the rules above are coming from a secondary motivation of
continuing down the deprecation path for two existing warnings, so authors
can have two active deprecation warnings quieted automatically by
"terraform fmt", without the need to run any third-party tools.

All of these rules match with current documented idiom as shown in the
Terraform documentation, so anyone who follows the documented style should
see no changes as a result of this. Those who have adopted other local
style will see their configuration files rewritten to the standard
Terraform style, but it should not make any changes that affect the
functionality of the configuration.

There are some further similar rewriting rules that could be added in
future, such as removing 0.11-style quotes around various keyword or
static reference arguments, but this initial pass focused only on some
rules that have been proven out in the third-party tool
terraform-clean-syntax, from which much of this commit is a direct port.

For now this doesn't attempt to re-introduce any rules about vertical
whitespace, even though the 0.11 "terraform fmt" would previously apply
such changes. We'll be more cautious about those because the results of
those rules in Terraform 0.11 were often sub-optimal and so we'd prefer
to re-introduce those with some care to the implications for those who
may be using vertical formatting differences for some semantic purpose,
like grouping together related arguments.

command/fmt.go

@@ -277,9 +277,221 @@ func (c *FmtCommand) formatSourceCode(src []byte, filename string) []byte {
 		return src
 	}
 
+	c.formatBody(f.Body(), nil)
+
 	return f.Bytes()
 }
 
+func (c *FmtCommand) formatBody(body *hclwrite.Body, inBlocks []string) {
+	attrs := body.Attributes()
+	for name, attr := range attrs {
+		if len(inBlocks) == 1 && inBlocks[0] == "variable" && name == "type" {
+			cleanedExprTokens := c.formatTypeExpr(attr.Expr().BuildTokens(nil))
+			body.SetAttributeRaw(name, cleanedExprTokens)
+			continue
+		}
+		cleanedExprTokens := c.formatValueExpr(attr.Expr().BuildTokens(nil))
+		body.SetAttributeRaw(name, cleanedExprTokens)
+	}
+
+	blocks := body.Blocks()
+	for _, block := range blocks {
+		// Normalize the label formatting, removing any weird stuff like
+		// interleaved inline comments and using the idiomatic quoted
+		// label syntax.
+		block.SetLabels(block.Labels())
+
+		inBlocks := append(inBlocks, block.Type())
+		c.formatBody(block.Body(), inBlocks)
+	}
+}
+
+func (c *FmtCommand) formatValueExpr(tokens hclwrite.Tokens) hclwrite.Tokens {
+	if len(tokens) < 5 {
+		// Can't possibly be a "${ ... }" sequence without at least enough
+		// tokens for the delimiters and one token inside them.
+		return tokens
+	}
+	oQuote := tokens[0]
+	oBrace := tokens[1]
+	cBrace := tokens[len(tokens)-2]
+	cQuote := tokens[len(tokens)-1]
+	if oQuote.Type != hclsyntax.TokenOQuote || oBrace.Type != hclsyntax.TokenTemplateInterp || cBrace.Type != hclsyntax.TokenTemplateSeqEnd || cQuote.Type != hclsyntax.TokenCQuote {
+		// Not an interpolation sequence at all, then.
+		return tokens
+	}
+
+	inside := tokens[2 : len(tokens)-2]
+
+	// We're only interested in sequences that are provable to be single
+	// interpolation sequences, which we'll determine by hunting inside
+	// the interior tokens for any other interpolation sequences. This is
+	// likely to produce false negatives sometimes, but that's better than
+	// false positives and we're mainly interested in catching the easy cases
+	// here.
+	quotes := 0
+	for _, token := range inside {
+		if token.Type == hclsyntax.TokenOQuote {
+			quotes++
+			continue
+		}
+		if token.Type == hclsyntax.TokenCQuote {
+			quotes--
+			continue
+		}
+		if quotes > 0 {
+			// Interpolation sequences inside nested quotes are okay, because
+			// they are part of a nested expression.
+			// "${foo("${bar}")}"
+			continue
+		}
+		if token.Type == hclsyntax.TokenTemplateInterp || token.Type == hclsyntax.TokenTemplateSeqEnd {
+			// We've found another template delimiter within our interior
+			// tokens, which suggests that we've found something like this:
+			// "${foo}${bar}"
+			// That isn't unwrappable, so we'll leave the whole expression alone.
+			return tokens
+		}
+		if token.Type == hclsyntax.TokenQuotedLit {
+			// If there's any literal characters in the outermost
+			// quoted sequence then it is not unwrappable.
+			return tokens
+		}
+	}
+
+	// If we got down here without an early return then this looks like
+	// an unwrappable sequence, but we'll trim any leading and trailing
+	// newlines that might result in an invalid result if we were to
+	// naively trim something like this:
+	// "${
+	//    foo
+	// }"
+	return c.trimNewlines(inside)
+}
+
+func (c *FmtCommand) formatTypeExpr(tokens hclwrite.Tokens) hclwrite.Tokens {
+	switch len(tokens) {
+	case 1:
+		kwTok := tokens[0]
+		if kwTok.Type != hclsyntax.TokenIdent {
+			// Not a single type keyword, then.
+			return tokens
+		}
+
+		// Collection types without an explicit element type mean
+		// the element type is "any", so we'll normalize that.
+		switch string(kwTok.Bytes) {
+		case "list", "map", "set":
+			return hclwrite.Tokens{
+				kwTok,
+				{
+					Type:  hclsyntax.TokenOParen,
+					Bytes: []byte("("),
+				},
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("any"),
+				},
+				{
+					Type:  hclsyntax.TokenCParen,
+					Bytes: []byte(")"),
+				},
+			}
+		default:
+			return tokens
+		}
+
+	case 3:
+		// A pre-0.12 legacy quoted string type, like "string".
+		oQuote := tokens[0]
+		strTok := tokens[1]
+		cQuote := tokens[2]
+		if oQuote.Type != hclsyntax.TokenOQuote || strTok.Type != hclsyntax.TokenQuotedLit || cQuote.Type != hclsyntax.TokenCQuote {
+			// Not a quoted string sequence, then.
+			return tokens
+		}
+
+		// Because this quoted syntax is from Terraform 0.11 and
+		// earlier, which didn't have the idea of "any" as an,
+		// element type, we use string as the default element
+		// type. That will avoid oddities if somehow the configuration
+		// was relying on numeric values being auto-converted to
+		// string, as 0.11 would do. This mimicks what terraform
+		// 0.12upgrade used to do, because we'd found real-world
+		// modules that were depending on the auto-stringing.)
+		switch string(strTok.Bytes) {
+		case "string":
+			return hclwrite.Tokens{
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("string"),
+				},
+			}
+		case "list":
+			return hclwrite.Tokens{
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("list"),
+				},
+				{
+					Type:  hclsyntax.TokenOParen,
+					Bytes: []byte("("),
+				},
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("string"),
+				},
+				{
+					Type:  hclsyntax.TokenCParen,
+					Bytes: []byte(")"),
+				},
+			}
+		case "map":
+			return hclwrite.Tokens{
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("map"),
+				},
+				{
+					Type:  hclsyntax.TokenOParen,
+					Bytes: []byte("("),
+				},
+				{
+					Type:  hclsyntax.TokenIdent,
+					Bytes: []byte("string"),
+				},
+				{
+					Type:  hclsyntax.TokenCParen,
+					Bytes: []byte(")"),
+				},
+			}
+		default:
+			// Something else we're not expecting, then.
+			return tokens
+		}
+	default:
+		return tokens
+	}
+}
+
+func (c *FmtCommand) trimNewlines(tokens hclwrite.Tokens) hclwrite.Tokens {
+	if len(tokens) == 0 {
+		return nil
+	}
+	var start, end int
+	for start = 0; start < len(tokens); start++ {
+		if tokens[start].Type != hclsyntax.TokenNewline {
+			break
+		}
+	}
+	for end = len(tokens); end > 0; end-- {
+		if tokens[end-1].Type != hclsyntax.TokenNewline {
+			break
+		}
+	}
+	return tokens[start:end]
+}
+
 func (c *FmtCommand) Help() string {
 	helpText := `
 Usage: terraform fmt [options] [DIR]

command/fmt_test.go

@@ -9,9 +9,75 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/mitchellh/cli"
 )
 
+func TestFmt(t *testing.T) {
+	const inSuffix = "_in.tf"
+	const outSuffix = "_out.tf"
+	const gotSuffix = "_got.tf"
+	entries, err := ioutil.ReadDir("testdata/fmt")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tmpDir, err := ioutil.TempDir("", "terraform-fmt-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	for _, info := range entries {
+		if info.IsDir() {
+			continue
+		}
+		filename := info.Name()
+		if !strings.HasSuffix(filename, inSuffix) {
+			continue
+		}
+		testName := filename[:len(filename)-len(inSuffix)]
+		t.Run(testName, func(t *testing.T) {
+			inFile := filepath.Join("testdata", "fmt", testName+inSuffix)
+			wantFile := filepath.Join("testdata", "fmt", testName+outSuffix)
+			gotFile := filepath.Join(tmpDir, testName+gotSuffix)
+			input, err := ioutil.ReadFile(inFile)
+			if err != nil {
+				t.Fatal(err)
+			}
+			want, err := ioutil.ReadFile(wantFile)
+			if err != nil {
+				t.Fatal(err)
+			}
+			err = ioutil.WriteFile(gotFile, input, 0700)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			ui := cli.NewMockUi()
+			c := &FmtCommand{
+				Meta: Meta{
+					testingOverrides: metaOverridesForProvider(testProvider()),
+					Ui:               ui,
+				},
+			}
+			args := []string{gotFile}
+			if code := c.Run(args); code != 0 {
+				t.Fatalf("fmt command was unsuccessful:\n%s", ui.ErrorWriter.String())
+			}
+
+			got, err := ioutil.ReadFile(gotFile)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if diff := cmp.Diff(string(want), string(got)); diff != "" {
+				t.Errorf("wrong result\n%s", diff)
+			}
+		})
+	}
+}
+
 func TestFmt_nonexist(t *testing.T) {
 	tempDir := fmtFixtureWriteDir(t)
 

command/testdata/fmt/general_in.tf

@@ -0,0 +1,41 @@
+# This test case is intended to cover many of the main formatting
+# rules of "terraform fmt" at once. It's fine to add new stuff in
+# here, but you can also add other _in.tf/_out.tf pairs in the
+# same directory if you want to test something complicated that,
+# for example, requires specific nested context.
+#
+# The input file of this test intentionally has strange whitespace
+# alignment, because the goal is to see the fmt command fix it.
+# If you're applying batch formatting to all .tf files in the
+# repository (or similar), be sure to skip this one to avoid
+# invalidating the test.
+
+terraform {
+required_providers {
+foo = { version = "1.0.0" }
+barbaz = {
+            version = "2.0.0"
+}
+}
+}
+
+variable instance_type {
+
+}
+
+resource foo_instance foo {
+  instance_type = "${var.instance_type}"
+}
+
+resource foo_instance "bar" {
+    instance_type = "${var.instance_type}-2"
+}
+
+resource "foo_instance" /* ... */ "baz" {
+  instance_type = "${var.instance_type}${var.instance_type}"
+
+  beep boop {}
+  beep blep {
+    thingy = "${var.instance_type}"
+  }
+}

command/testdata/fmt/general_out.tf

@@ -0,0 +1,41 @@
+# This test case is intended to cover many of the main formatting
+# rules of "terraform fmt" at once. It's fine to add new stuff in
+# here, but you can also add other _in.tf/_out.tf pairs in the
+# same directory if you want to test something complicated that,
+# for example, requires specific nested context.
+#
+# The input file of this test intentionally has strange whitespace
+# alignment, because the goal is to see the fmt command fix it.
+# If you're applying batch formatting to all .tf files in the
+# repository (or similar), be sure to skip this one to avoid
+# invalidating the test.
+
+terraform {
+  required_providers {
+    foo = { version = "1.0.0" }
+    barbaz = {
+      version = "2.0.0"
+    }
+  }
+}
+
+variable "instance_type" {
+
+}
+
+resource "foo_instance" "foo" {
+  instance_type = var.instance_type
+}
+
+resource "foo_instance" "bar" {
+  instance_type = "${var.instance_type}-2"
+}
+
+resource "foo_instance" "baz" {
+  instance_type = "${var.instance_type}${var.instance_type}"
+
+  beep "boop" {}
+  beep "blep" {
+    thingy = var.instance_type
+  }
+}

command/testdata/fmt/variable_type_in.tf

@@ -0,0 +1,57 @@
+variable "a" {
+  type = string
+}
+
+variable "b" {
+  type = list
+}
+
+variable "c" {
+  type = map
+}
+
+variable "d" {
+  type = set
+}
+
+variable "e" {
+  type = "string"
+}
+
+variable "f" {
+  type = "list"
+}
+
+variable "g" {
+  type = "map"
+}
+
+variable "h" {
+  type = object({})
+}
+
+variable "i" {
+  type = object({
+    foo = string
+  })
+}
+
+variable "j" {
+  type = tuple([])
+}
+
+variable "k" {
+  type = tuple([number])
+}
+
+variable "l" {
+  type = list(string)
+}
+
+variable "m" {
+  type = list(
+    object({
+      foo = bool
+    })
+  )
+}

command/testdata/fmt/variable_type_out.tf

@@ -0,0 +1,57 @@
+variable "a" {
+  type = string
+}
+
+variable "b" {
+  type = list(any)
+}
+
+variable "c" {
+  type = map(any)
+}
+
+variable "d" {
+  type = set(any)
+}
+
+variable "e" {
+  type = string
+}
+
+variable "f" {
+  type = list(string)
+}
+
+variable "g" {
+  type = map(string)
+}
+
+variable "h" {
+  type = object({})
+}
+
+variable "i" {
+  type = object({
+    foo = string
+  })
+}
+
+variable "j" {
+  type = tuple([])
+}
+
+variable "k" {
+  type = tuple([number])
+}
+
+variable "l" {
+  type = list(string)
+}
+
+variable "m" {
+  type = list(
+    object({
+      foo = bool
+    })
+  )
+}