HCL2 spec ambiguity around binary and unary operations.

The current spec appears to suggest that chaining arithmetic operations is not possible without nesting each pair in parenthesis:

Operation = unaryOp | binaryOp;
unaryOp = ("-" | "!") ExprTerm;
binaryOp = ExprTerm binaryOperator ExprTerm;
binaryOperator = compareOperator | arithmeticOperator | logicOperator;
compareOperator = "==" | "!=" | "<" | ">" | "<=" | ">=";
arithmeticOperator = "+" | "-" | "*" | "/" | "%";
logicOperator = "&&" | "||" | "!";

ExprTerm = (
    LiteralValue |
    CollectionValue |
    TemplateExpr |
    VariableExpr |
    FunctionCall |
    ForExpr |
    ExprTerm Index |
    ExprTerm GetAttr |
    ExprTerm Splat |
    "(" Expression ")"
);

This seems to imply that an expression such as x + y / z is not a valid Operation, but (x + y) / z is.

Running Terraform 1.2.9, it appears that the former is valid:

locals {
  x = 1
  y = 2
  z = 3
  abc = local.x + local.y / local.z
}

resource "null_resource" "result" {
  triggers = {
    id = uuid()
  }

  provisioner "local-exec" {
    interpreter = ["/usr/bin/env", "sh", "-c"]
    command = "echo '${local.abc}'"
  }
}

(.venv) ➜  hcl-test terraform version
Terraform v1.2.9
on linux_amd64
+ provider registry.terraform.io/hashicorp/null v3.1.1
(.venv) ➜  hcl-test terraform apply -auto-approve
null_resource.result: Refreshing state... [id=4532689630753613577]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # null_resource.result must be replaced
-/+ resource "null_resource" "result" {
      ~ id       = "4532689630753613577" -> (known after apply)
      ~ triggers = {
          - "id" = "57b28aab-2d89-2c7b-5f86-95393921ce85"
        } -> (known after apply) # forces replacement
    }

Plan: 1 to add, 0 to change, 1 to destroy.
null_resource.result: Destroying... [id=4532689630753613577]
null_resource.result: Destruction complete after 0s
null_resource.result: Creating...
null_resource.result: Provisioning with 'local-exec'...
null_resource.result (local-exec): Executing: ["/usr/bin/env" "sh" "-c" "echo '1.6666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666668'"]
null_resource.result (local-exec): 1.6666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666668
null_resource.result: Creation complete after 0s [id=5999965405986185361]

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

If this is not correct (and I am not just misunderstanding something), I think a grammar that more accurately represents the behaviour for the implementation. This would address the following issues:

Operator issue (this ticket)
Existing grammar does not communicate operator precedence very well, instead a table is provided and the grammar has to be rewritten as per that table.
Conditional (ternary) operator does not communicate associativity very well, and currently would result in an infinite loop on the first term, which makes it unsuitable for grammar generators such as ANTLR without modification.
550

Expression = orOp , "?" , Expression , ":" , Expression
           | orOp
           ;

orOp       = andOp , "||" , orOp
           | andOp
           ;

andOp      = eqOp , "&&" , andOp
           | eqOp
           ;

eqOp       = compOp , "==" , eqOp
           | compOp , "!=" , eqOp
           | compOp
           ;

compOp     = addOp , "<"  , compOp
           | addOp , "<=" , compOp
           | addOp , ">"  , compOp
           | addOp , ">=" , compOp
           | addOp
           ;

addOp      = mulOp , "+" , addOp
           | mulOp , "-" , addOp
           | mulOp
           ;

mulOp      = unaryOp , "*" , mulOp
           | unaryOp , "/" , mulOp
           | unaryOp , "%" , mulOp
           | unaryOp
           ;

unaryOp    = "-" , unaryOp
           | "!" , unaryOp
           | ExprTerm
           ;

While this is more verbose, I think it provides a few additional benefits:

Grammar is more deterministic, making it easier to port to parser generators
Conditional is now handled as part of operations, meaning the conditional and operation production can now be removed, making it a little less confusing to handle.
Operator precedence is now clear from the grammar rather than needing an additional table.
Operators can be chained (e.g. x + y / z) clearly.
Ternary conditional operator no longer produces an infinite loop on the first term, and communicates left associativity.

Hope this doesn't feel too pedantic and that I am not stepping on anyones toes or anything with this suggestion, I am currently attempting to write an HCL2 parser from this spec for Java 17, but these issues are a few that I have encountered while working on the project.

hashicorp / hcl

HCL2 spec ambiguity around binary and unary operations. #553

550