hashicorp / hcl2

Former temporary home for experimental new version of HCL
https://github.com/hashicorp/hcl
Mozilla Public License 2.0
373 stars 66 forks source link

maps values string parsing conflict #55

Closed bernadinm closed 5 years ago

bernadinm commented 5 years ago

When testing out terraform-0.12.0 for forwards compatibility, I wanted to see if our scripts need to be changed. We've started changing some things until we've encountered this below which seems like underscores needs to be processed and looks like it may be a minor bug.

$ terraform init
...
Error: Missing key/value separator

  on .terraform/modules/dcos-bootstrap-instance.dcos-bootstrap-instance.dcos-tested-oses/dcos-terraform-terraform-aws-tested-oses-94a4f4c/variables.tf line 35:
  33:   default = {
  34:     # Centos 7.2
  35:     centos_7.2_ap-south-1     = "ami-95cda6fa"

Expected an equals sign ("=") to mark the beginning of the attribute value.
$ cat variables.tf
...
# AWS recommends all HVM vs PV. HVM Below.
variable "aws_ami" {
  description = "AMI that will be used for the instances instead of Mesosphere provided AMIs"
  type        = "map"

  default = {
    # Centos 7.2
    centos_7.2_ap-south-1     = "ami-95cda6fa"
    centos_7.2_eu-west-2      = "ami-bb373ddf"
    centos_7.2_eu-west-1      = "ami-7abd0209"
    centos_7.2_ap-northeast-2 = "ami-c74789a9"
    centos_7.2_ap-northeast-1 = "ami-eec1c380"
    centos_7.2_sa-east-1      = "ami-26b93b4a"
    centos_7.2_ca-central-1   = "ami-af62d0cb"
    centos_7.2_ap-southeast-1 = "ami-f068a193"
    centos_7.2_ap-southeast-2 = "ami-fedafc9d"
    centos_7.2_eu-central-1   = "ami-9bf712f4"
    centos_7.2_us-east-1      = "ami-6d1c2007"
    centos_7.2_us-east-2      = "ami-6a2d760f"
    centos_7.2_us-west-1      = "ami-af4333cf"
    centos_7.2_us-west-2      = "ami-d2c924b2"
    }
}
apparentlymart commented 5 years ago

Hi @bernadinm,

I think actually the problem there is that period is not a valid character in an identifier, and unquoted map keys are now required to be identifiers. Wrapping those strings in quotes should get the desired effect. However, that error message could certainly be more actionable by noticing that the next character is a period and stating more specifically that periods are not allowed there and to put the value in quotes, since indeed HCL1 was more forgiving about that.

apparentlymart commented 5 years ago

Having looked more closely at this, I see that it's a more subtle problem than I first thought.

The string centos_7.2_ap-south-1 tokenizes as Ident("centos_7") Dot Number(2) Ident("_ap-south-1"), because an identifier can't start with a digit. This means the parser interprets it as an alternative syntax for centos_7[2] and then terminates expression parsing, returning to the object constructor parser with Ident("_ap-south-1") as the next token, which then fails because an = is expected there.

This means we can't get quite as precise a helpful error message as I originally hoped, but I'm going to add a "Did you mean...?" suggestion to the error message in the situation where an identifier appears right after the attribute key expression to at least give a hint as to what fix is required here.

Some good news side from that is that for Terraform 0.12 in particular it will include a tool to help with making these upgrades and it will be able to make this particular fix automatically, wrapping your keys here in quotes once it detects that they are not valid HCL2 identifiers.

apparentlymart commented 5 years ago

In 7ace05a3bed93021f87190cbb1f2e58bde8d593c I have added two new error messages, one of which addresses the situation you encountered here. As noted in my previous comment, we can't recognize this pattern unambiguously and so I instead phrased the error message as a hint as to what the problem might be:

Error: Missing key/value separator

  (source code reference)

Expected an equals sign ("=") to mark the beginning of the attribute
value. If you intended to given an attribute name containing periods
or spaces, write the name in quotes to create a string literal.

This combined with the Terraform-specific configuration upgrade tool is, I think, the best we can do here without introducing confusing ambiguities into the language now that object/map keys are allowed to be arbitrary expressions rather than just literal strings. This is one of the situations where a slight incompatibility is an unavoidable consequence of supporting first-class expressions where HCL 1 did not.