hclwrite: parser inserts an unexpected TokenNewline before a TokenCParen

[minamijoyo]

If an expression ends with a TokenCParen, the hclwrite parser inserts an unexpected TokenNewline before a TokenCParen.

This issue was originally reported in minamijoyo/tfupdate#24,
but I found the root cause of this problem was in the upstream hcl library.

HCL Template

resource "foo_bar" "baz" {
  disabled = (true)
}

Expected behavior

Parse HCL with hclwrite.ParseConfig() , then Generate HCL with (*File).BuildTokens(nil) should be idempotent.

resource "foo_bar" "baz" {
  disabled = (true)
}

Actual behavior

the hclwrite parser inserts an unexpected TokenNewline before a TokenCParen.

resource "foo_bar" "baz" {
  disabled = (true
)}

The TokenNewline should be inserted after the TokenCParen not before it.
The output is broken. (invalid syntax)

Steps to reproduce

1. Save the following as go.mod and main.go

module tmp

go 1.14

require (
	github.com/davecgh/go-spew v1.1.1
	github.com/hashicorp/hcl/v2 v2.6.1-0.20200915195656-bf0a7fe4fe09
	github.com/zclconf/go-cty v1.2.0
)

package main

import (
	"fmt"

	"github.com/davecgh/go-spew/spew"
	"github.com/hashicorp/hcl/v2"
	"github.com/hashicorp/hcl/v2/hclwrite"
)

func main() {
	src := `
resource "foo_bar" "baz" {
  disabled = (true)
}
`
	f, _ := hclwrite.ParseConfig([]byte(src), "", hcl.Pos{Line: 1, Column: 1})
	got := f.BuildTokens(nil)
	fmt.Printf("got:\n%s\n", string(got.Bytes()))
	fmt.Printf("dump:\n%s\n", spew.Sdump(got))
}

2. go run main.go

$ go run main.go
got:

resource "foo_bar" "baz" {
  disabled = (true
)}

dump:
(hclwrite.Tokens) (len=19 cap=24) {
 (*hclwrite.Token)(0xc0001c6600)({
  Type: (hclsyntax.TokenType) TokenNewline,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  0a                                                |.|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6628)({
  Type: (hclsyntax.TokenType) TokenIdent,
  Bytes: ([]uint8) (len=8 cap=8) {
   00000000  72 65 73 6f 75 72 63 65                           |resource|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6650)({
  Type: (hclsyntax.TokenType) TokenOQuote,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  22                                                |"|
  },
  SpacesBefore: (int) 1
 }),
 (*hclwrite.Token)(0xc0001c6678)({
  Type: (hclsyntax.TokenType) TokenQuotedLit,
  Bytes: ([]uint8) (len=7 cap=7) {
   00000000  66 6f 6f 5f 62 61 72                              |foo_bar|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c66a0)({
  Type: (hclsyntax.TokenType) TokenCQuote,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  22                                                |"|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c66c8)({
  Type: (hclsyntax.TokenType) TokenOQuote,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  22                                                |"|
  },
  SpacesBefore: (int) 1
 }),
 (*hclwrite.Token)(0xc0001c66f0)({
  Type: (hclsyntax.TokenType) TokenQuotedLit,
  Bytes: ([]uint8) (len=3 cap=3) {
   00000000  62 61 7a                                          |baz|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6718)({
  Type: (hclsyntax.TokenType) TokenCQuote,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  22                                                |"|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6740)({
  Type: (hclsyntax.TokenType) TokenOBrace,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  7b                                                |{|
  },
  SpacesBefore: (int) 1
 }),
 (*hclwrite.Token)(0xc0001c6768)({
  Type: (hclsyntax.TokenType) TokenNewline,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  0a                                                |.|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6790)({
  Type: (hclsyntax.TokenType) TokenIdent,
  Bytes: ([]uint8) (len=8 cap=8) {
   00000000  64 69 73 61 62 6c 65 64                           |disabled|
  },
  SpacesBefore: (int) 2
 }),
 (*hclwrite.Token)(0xc0001c67b8)({
  Type: (hclsyntax.TokenType) TokenEqual,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  3d                                                |=|
  },
  SpacesBefore: (int) 1
 }),
 (*hclwrite.Token)(0xc0001c67e0)({
  Type: (hclsyntax.TokenType) TokenOParen,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  28                                                |(|
  },
  SpacesBefore: (int) 1
 }),
 (*hclwrite.Token)(0xc0001c6808)({
  Type: (hclsyntax.TokenType) TokenIdent,
  Bytes: ([]uint8) (len=4 cap=4) {
   00000000  74 72 75 65                                       |true|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6858)({
  Type: (hclsyntax.TokenType) TokenNewline,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  0a                                                |.|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6830)({
  Type: (hclsyntax.TokenType) TokenCParen,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  29                                                |)|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c6880)({
  Type: (hclsyntax.TokenType) TokenCBrace,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  7d                                                |}|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c68a8)({
  Type: (hclsyntax.TokenType) TokenNewline,
  Bytes: ([]uint8) (len=1 cap=1) {
   00000000  0a                                                |.|
  },
  SpacesBefore: (int) 0
 }),
 (*hclwrite.Token)(0xc0001c68d0)({
  Type: (hclsyntax.TokenType) TokenEOF,
  Bytes: ([]uint8) {
  },
  SpacesBefore: (int) 0
 })
}

References

minamijoyo/tfupdate#24

[minamijoyo]

If I swap the following lines, it seems to fix the issue and pass all existing tests.

hcl/hclwrite/parser.go

Lines 281 to 284 in bf0a7fe

	children.AppendUnstructuredTokens(newline.Tokens())
	// Collect any stragglers, though there shouldn't be any
	children.AppendUnstructuredTokens(from.Tokens())

However, I'm not sure if we can simply swap those because the comment says:

// Collect any stragglers, though there shouldn't be any

from.Tokens() returns a TokenCParen in this case.
Is it always true that an expression ends with a TokenNewline?
If so, can we insert a TokenNewline after from.Tokens() ?

[shlomimatichin]

created pr #408 , forked in the mean time, seems to be working for us

[apparentlymart]

Thanks for reporting this, @minamijoyo!

I've not looked closely at the implementation here but based on experience with some similar bugs I expect this problem has as a root cause that the underlying hclsyntax parser isn't reporting its source locations accurately enough. Specifically, I think the main parser is "optimizing away" the wrapping parentheses and just reporting the true inside, and so the source range of the resulting expression only covers the true token and not the ( and ) surrounding it.

When we fixed some similar problems to this in the past we changed the hclsyntax parser to return accurate source locations that actually cover the whole expression. I think that would be the best solution here too, though I'm not sure if it will be an easy fix because IIRC the parser currently handles grouping parentheses only as precedence guidance and not as a real AST node. 🤔

[shlomimatichin]

@apparentlymart if it helps, i've traced through the code multiple times, specifically on the example here below (the relevant line is "replicas". hclwrite does not seem to optimize anything, we just see the new line token and the closing parenthesis token swapped in the resulting token list, which transposing those two lines (#408) seems to resolve.

resource "kubernetes_deployment" "audit_server" {
  metadata {
    name = "terraform-example"
  }
  spec {
    template {
      metadata {
        labels = {
          app = "as"
        }
      }
      spec {
        container {
          image = "helloworld"
          name  = "as"
        }
      }
    }
    replicas = true ? 0 : (false ? 3 : 4)
    strategy {
      type = "Recreate"
    }
  }
}

resource "google_pubsub_topic" "audit_topic" {
  name = "yuvu"
  labels = local.terratag_added_main
}

[minamijoyo]

@apparentlymart Thank you for the hints.

I'm reading hclsyntax parser code and checking an expr variable here:

hcl/hclsyntax/parser.go

Line 918 in bf0a7fe

expr, diags := p.ParseExpression()

Actually the expression contains only true, and it's SrcRange doesn't include outside parentheses:

(*hclsyntax.LiteralValueExpr)(0xc00004afc0)({
 Val: (cty.Value) {
  ty: (cty.Type) {
   typeImpl: (cty.primitiveType) {
    typeImplSigil: (cty.typeImplSigil) {
    },
    Kind: (cty.primitiveTypeKind) 66
   }
  },
  v: (bool) true
 },
 SrcRange: (hcl.Range) :3,15-19
})

If my understanding is correct, we need to fix SrcRange for the expr. However, it seems that there is no way to update it with the current Expression interface. How can we fix it? Do you have any thoughts about this?

[aaronsteers]

Looks like this bug may be expressed in the latest terraform 0.14.0 release candidate - which takes a more opinionated approach to terraform fmt - I think (?) backed by this hclwrite module.

Cross-posting my related ticket for 0.14.0 here for reference: hashicorp/terraform#27040

---

UPDATE: on reading more closely the above thread, it looks like @shlomimatichin has a possible resolution path in PR: #408

Unclear if that fix is confirmed or still wip.

[minamijoyo]

I confirmed that this issue was fixed by #426 in v2.8.0.

Thanks to @apparentlymart @aaronsteers @shlomimatichin 🎉