From ff0dbd621553e54fab0a55de564bd1a8b023dd69 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 25 Sep 2020 17:16:26 -0700 Subject: [PATCH] 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 | 212 ++++++++++++++++++++++ command/fmt_test.go | 66 +++++++ command/testdata/fmt/general_in.tf | 41 +++++ command/testdata/fmt/general_out.tf | 41 +++++ command/testdata/fmt/variable_type_in.tf | 57 ++++++ command/testdata/fmt/variable_type_out.tf | 57 ++++++ 6 files changed, 474 insertions(+) create mode 100644 command/testdata/fmt/general_in.tf create mode 100644 command/testdata/fmt/general_out.tf create mode 100644 command/testdata/fmt/variable_type_in.tf create mode 100644 command/testdata/fmt/variable_type_out.tf diff --git a/command/fmt.go b/command/fmt.go index b31b025c7..13e84a218 100644 --- a/command/fmt.go +++ b/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] diff --git a/command/fmt_test.go b/command/fmt_test.go index 274784b10..999a33aff 100644 --- a/command/fmt_test.go +++ b/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) diff --git a/command/testdata/fmt/general_in.tf b/command/testdata/fmt/general_in.tf new file mode 100644 index 000000000..44432f973 --- /dev/null +++ b/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}" + } +} diff --git a/command/testdata/fmt/general_out.tf b/command/testdata/fmt/general_out.tf new file mode 100644 index 000000000..4b1bc489b --- /dev/null +++ b/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 + } +} diff --git a/command/testdata/fmt/variable_type_in.tf b/command/testdata/fmt/variable_type_in.tf new file mode 100644 index 000000000..3d6781202 --- /dev/null +++ b/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 + }) + ) +} diff --git a/command/testdata/fmt/variable_type_out.tf b/command/testdata/fmt/variable_type_out.tf new file mode 100644 index 000000000..f4f2df292 --- /dev/null +++ b/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 + }) + ) +}