Repository: amplify-education/python-hcl2 (language: python)

Issue:
Incorrectly transforms attributes into lists
Hi,

I was excited to try this package but it seems to turn everything into a list. There is a test for this behaviour, which I think is wrong:

https://github.com/amplify-education/python-hcl2/blob/a4b29a76e34bbbd4bcac8d073f96392f451f79b3/test/helpers/terraform-config/vars.auto.tfvars#L1

https://github.com/amplify-education/python-hcl2/blob/a4b29a76e34bbbd4bcac8d073f96392f451f79b3/test/helpers/terraform-config-json/vars.auto.json#L2-L4

I think the JSON should be:

```json
"foo": "bar",
```

https://github.com/hashicorp/hcl2#information-model-and-syntax has examples with top-level strings.

I would like to get confirmation that this is a bug before I have a go at fixing it and trying to use this in my project. Thanks!


Discussion / hints:
This is actually intentional https://github.com/hashicorp/hcl2/blob/master/hcl/json/spec.md#blocks. It's possible to have duplicate block fields in HCL2. For example the `iam_policy_document` data source can have multiple `statement` fields. https://www.terraform.io/docs/providers/aws/d/iam_policy_document.html. 

This forces me to convert blocks into arrays because json dicts can't have duplicate fields. It isn't ideal though. I'm open to any alternative approaches. 
I'll need to read the spec and related specs a bit more but I think this falls under https://github.com/hashicorp/hcl2/blob/master/hcl/json/spec.md#attributes which, as far as I know, aren't meant to be arrays.
Would we expect that the `terraform-config-inspect variables.tf` creates the same output as `hcl2tojson variables.tf` ?
I also noticed that `hcl2tojson` adds arrays althoughs the hashicorp tool doesn't.
@aoskotsky-amplify it is correct only for blocks, but for values is not possible to have assignment of same variable in one block
```hcl
data "aws_iam_policy_document" "example" {
  statement {}
  statement {}
  statement {}
}
```
is valis but this code
```hcl
module  "own_module" {
 foo = "bar"
 foo = "bar2"
}
```
is not valid. Error raised:
```
Error: Attribute redefined

  on main.tf line 21, in module "own_module":
  21:   foo = "bar2"
```

I would say @monester is correct, as according to https://github.com/hashicorp/hcl/blob/hcl2/hclsyntax/spec.md#attribute-definitions:

"Each distinct attribute name may be defined no more than once within a single body."
This seems to happen all over the place. For example, I have an ec2 resource that uses the count field. The value is an expression (count = length(var.serverlist) ). 
But python-hcl2 turns this into a list (count = [ "length(var.serverlist)" ] ). Terraform chokes on this if you write this back out as json. This type of transformation is critical to my pipeline.

Fix patch:
diff --git a/hcl2/transformer.py b/hcl2/transformer.py
index b03aaee..74430f7 100644
--- a/hcl2/transformer.py
+++ b/hcl2/transformer.py
@@ -1,6 +1,7 @@
 """A Lark Transformer for transforming a Lark parse tree into a Python dict"""
 import re
 import sys
+from collections import namedtuple
 from typing import List, Dict, Any
 
 from lark import Transformer, Discard
@@ -8,6 +9,8 @@
 HEREDOC_PATTERN = re.compile(r'<<([a-zA-Z][a-zA-Z0-9._-]+)\n((.|\n)*?)\n\s*\1', re.S)
 HEREDOC_TRIM_PATTERN = re.compile(r'<<-([a-zA-Z][a-zA-Z0-9._-]+)\n((.|\n)*?)\n\s*\1', re.S)
 
+Attribute = namedtuple("Attribute", ("key", "value"))
+
 
 # pylint: disable=missing-docstring,unused-argument
 class DictTransformer(Transformer):
@@ -103,15 +106,12 @@ def block(self, args: List) -> Dict:
     def one_line_block(self, args: List) -> Dict:
         return self.block(args)
 
-    def attribute(self, args: List) -> Dict:
+    def attribute(self, args: List) -> Attribute:
         key = str(args[0])
         if key.startswith('"') and key.endswith('"'):
             key = key[1:-1]
         value = self.to_string_dollar(args[1])
-
-        return {
-            key: value
-        }
+        return Attribute(key, value)
 
     def conditional(self, args: List) -> str:
         args = self.strip_new_line_tokens(args)
@@ -128,26 +128,42 @@ def binary_term(self, args: List) -> str:
         return " ".join([str(arg) for arg in args])
 
     def body(self, args: List) -> Dict[str, List]:
-        # A body can have multiple attributes with the same name
-        # For example multiple Statement attributes in a IAM resource body
-        # So This returns a dict of attribute names to lists
-        # The attribute values will always be lists even if they aren't repeated
-        # and only contain a single entry
+        # See https://github.com/hashicorp/hcl/blob/main/hclsyntax/spec.md#bodies
+        # ---
+        # A body is a collection of associated attributes and blocks.
+        #
+        # An attribute definition assigns a value to a particular attribute
+        # name within a body. Each distinct attribute name may be defined no
+        # more than once within a single body.
+        #
+        # A block creates a child body that is annotated with a block type and
+        # zero or more block labels. Blocks create a structural hierarchy which
+        # can be interpreted by the calling application.
+        # ---
+        #
+        # There can be more than one child body with the same block type and
+        # labels. This means that all blocks (even when there is only one)
+        # should be transformed into lists of blocks.
         args = self.strip_new_line_tokens(args)
+        attributes = set()
         result: Dict[str, Any] = {}
         for arg in args:
-            for key, value in arg.items():
-                key = str(key)
-                if key not in result:
-                    result[key] = [value]
-                else:
-                    if isinstance(result[key], list):
-                        if isinstance(value, list):
-                            result[key].extend(value)
-                        else:
-                            result[key].append(value)
+            if isinstance(arg, Attribute):
+                if arg.key in result:
+                    raise RuntimeError("{} already defined".format(arg.key))
+                result[arg.key] = arg.value
+                attributes.add(arg.key)
+            else:
+                # This is a block.
+                for key, value in arg.items():
+                    key = str(key)
+                    if key in result:
+                        if key in attributes:
+                            raise RuntimeError("{} already defined".format(key))
+                        result[key].append(value)
                     else:
-                        result[key] = [result[key], value]
+                        result[key] = [value]
+
         return result
 
     def start(self, args: List) -> Dict:


Test patch:
diff --git a/test/helpers/terraform-config-json/backend.json b/test/helpers/terraform-config-json/backend.json
index 7bb6df4..7c0bcd4 100644
--- a/test/helpers/terraform-config-json/backend.json
+++ b/test/helpers/terraform-config-json/backend.json
@@ -2,27 +2,19 @@
   "provider": [
     {
       "aws": {
-        "region": [
-          "${var.region}"
-        ]
+        "region": "${var.region}"
       }
     },
     {
       "aws": {
-        "region": [
-          "${var.backup_region}"
-        ],
-        "alias": [
-          "backup"
-        ]
+        "region": "${var.backup_region}",
+        "alias": "backup"
       }
     }
   ],
   "terraform": [
     {
-      "required_version": [
-        "0.12"
-      ]
+      "required_version": "0.12"
     },
     {
       "backend": [
@@ -32,21 +24,15 @@
       ],
       "required_providers": [
         {
-          "aws": [
-            {
-              "source": "hashicorp/aws"
-            }
-          ],
-          "null": [
-            {
-              "source": "hashicorp/null"
-            }
-          ],
-          "template": [
-            {
-              "source": "hashicorp/template"
-            }
-          ]
+          "aws": {
+            "source": "hashicorp/aws"
+          },
+          "null": {
+            "source": "hashicorp/null"
+          },
+          "template": {
+            "source": "hashicorp/template"
+          }
         }
       ]
     }
diff --git a/test/helpers/terraform-config-json/cloudwatch.json b/test/helpers/terraform-config-json/cloudwatch.json
index c8733dd..72a344a 100644
--- a/test/helpers/terraform-config-json/cloudwatch.json
+++ b/test/helpers/terraform-config-json/cloudwatch.json
@@ -3,36 +3,24 @@
     {
       "aws_cloudwatch_event_rule": {
         "aws_cloudwatch_event_rule": {
-          "name": [
-            "name"
-          ],
-          "event_pattern": [
-            "    {\n      \"foo\": \"bar\"\n    }"
-          ]
+          "name": "name",
+          "event_pattern": "    {\n      \"foo\": \"bar\"\n    }"
         }
       }
     },
     {
       "aws_cloudwatch_event_rule": {
         "aws_cloudwatch_event_rule2": {
-          "name": [
-            "name"
-          ],
-          "event_pattern": [
-            "{\n  \"foo\": \"bar\"\n}"
-          ]
+          "name": "name",
+          "event_pattern": "{\n  \"foo\": \"bar\"\n}"
         }
       }
     },
     {
       "aws_cloudwatch_event_rule": {
         "aws_cloudwatch_event_rule2": {
-          "name": [
-            "name"
-          ],
-          "event_pattern": [
-            "${jsonencode(var.cloudwatch_pattern_deploytool)}"
-          ]
+          "name": "name",
+          "event_pattern": "${jsonencode(var.cloudwatch_pattern_deploytool)}"
         }
       }
     }
diff --git a/test/helpers/terraform-config-json/data_sources.json b/test/helpers/terraform-config-json/data_sources.json
index d1356ce..f1f939d 100644
--- a/test/helpers/terraform-config-json/data_sources.json
+++ b/test/helpers/terraform-config-json/data_sources.json
@@ -3,12 +3,8 @@
     {
       "terraform_remote_state": {
         "map": {
-          "for_each": [
-            "${{for s3_bucket_key in data.aws_s3_bucket_objects.remote_state_objects.keys : regex(local.remote_state_regex,s3_bucket_key)[\"account_alias\"] => s3_bucket_key if length(regexall(local.remote_state_regex,s3_bucket_key)) > 0}}"
-          ],
-          "backend": [
-            "s3"
-          ]
+          "for_each": "${{for s3_bucket_key in data.aws_s3_bucket_objects.remote_state_objects.keys : regex(local.remote_state_regex,s3_bucket_key)[\"account_alias\"] => s3_bucket_key if length(regexall(local.remote_state_regex,s3_bucket_key)) > 0}}",
+          "backend": "s3"
         }
       }
     }
diff --git a/test/helpers/terraform-config-json/iam.json b/test/helpers/terraform-config-json/iam.json
index 599b3cd..a6b5339 100644
--- a/test/helpers/terraform-config-json/iam.json
+++ b/test/helpers/terraform-config-json/iam.json
@@ -5,29 +5,19 @@
         "policy": {
           "statement": [
             {
-              "effect": [
-                "Deny"
-              ],
+              "effect": "Deny",
               "principals": [
                 {
-                  "type": [
-                    "AWS"
-                  ],
+                  "type": "AWS",
                   "identifiers": [
-                    [
-                      "*"
-                    ]
+                    "*"
                   ]
                 }
               ],
               "actions": [
-                [
-                  "s3:PutObjectAcl"
-                ]
+                "s3:PutObjectAcl"
               ],
-              "resources": [
-                "${aws_s3_bucket.bucket.*.arn}"
-              ]
+              "resources": "${aws_s3_bucket.bucket.*.arn}"
             }
           ]
         }
@@ -39,13 +29,9 @@
           "statement": [
             {
               "actions": [
-                [
-                  "s3:GetObject"
-                ]
+                "s3:GetObject"
               ],
-              "resources": [
-                "${[for bucket_name in local.buckets_to_proxy : \"arn:aws:s3:::${bucket_name}/*\" if substr(bucket_name,0,1) == \"l\"]}"
-              ]
+              "resources": "${[for bucket_name in local.buckets_to_proxy : \"arn:aws:s3:::${bucket_name}/*\" if substr(bucket_name,0,1) == \"l\"]}"
             }
           ]
         }
diff --git a/test/helpers/terraform-config-json/route_table.json b/test/helpers/terraform-config-json/route_table.json
index 7c41788..b07ed40 100644
--- a/test/helpers/terraform-config-json/route_table.json
+++ b/test/helpers/terraform-config-json/route_table.json
@@ -3,36 +3,20 @@
     {
       "aws_route": {
         "tgw": {
-          "count": [
-            "${var.tgw_name == \"\" ? 0 : var.number_of_az}"
-          ],
-          "route_table_id": [
-            "${aws_route_table.rt[count.index].id}"
-          ],
-          "destination_cidr_block": [
-            "10.0.0.0/8"
-          ],
-          "transit_gateway_id": [
-            "${data.aws_ec2_transit_gateway.tgw[0].id}"
-          ]
+          "count": "${var.tgw_name == \"\" ? 0 : var.number_of_az}",
+          "route_table_id": "${aws_route_table.rt[count.index].id}",
+          "destination_cidr_block": "10.0.0.0/8",
+          "transit_gateway_id": "${data.aws_ec2_transit_gateway.tgw[0].id}"
         }
       }
     },
     {
       "aws_route": {
         "tgw-dot-index": {
-          "count": [
-            "${var.tgw_name == \"\" ? 0 : var.number_of_az}"
-          ],
-          "route_table_id": [
-            "${aws_route_table.rt[count.index].id}"
-          ],
-          "destination_cidr_block": [
-            "10.0.0.0/8"
-          ],
-          "transit_gateway_id": [
-            "${data.aws_ec2_transit_gateway.tgw[0].id}"
-          ]
+          "count": "${var.tgw_name == \"\" ? 0 : var.number_of_az}",
+          "route_table_id": "${aws_route_table.rt[count.index].id}",
+          "destination_cidr_block": "10.0.0.0/8",
+          "transit_gateway_id": "${data.aws_ec2_transit_gateway.tgw[0].id}"
         }
       }
     }
diff --git a/test/helpers/terraform-config-json/s3.json b/test/helpers/terraform-config-json/s3.json
index 5b4982d..6fd2687 100644
--- a/test/helpers/terraform-config-json/s3.json
+++ b/test/helpers/terraform-config-json/s3.json
@@ -3,43 +3,27 @@
     {
       "aws_s3_bucket": {
         "name": {
-          "bucket": [
-            "name"
-          ],
-          "acl": [
-            "log-delivery-write"
-          ],
+          "bucket": "name",
+          "acl": "log-delivery-write",
           "lifecycle_rule": [
             {
-              "id": [
-                "to_glacier"
-              ],
-              "prefix": [
-                ""
-              ],
-              "enabled": [
-                true
-              ],
+              "id": "to_glacier",
+              "prefix": "",
+              "enabled": true,
               "expiration": [
                 {
-                  "days": [
-                    365
-                  ]
+                  "days": 365
                 }
               ],
-              "transition": [
-                {
-                  "days": 30,
-                  "storage_class": "GLACIER"
-                }
-              ]
+              "transition": {
+                "days": 30,
+                "storage_class": "GLACIER"
+              }
             }
           ],
           "versioning": [
             {
-              "enabled": [
-                true
-              ]
+              "enabled": true
             }
           ]
         }
@@ -49,18 +33,10 @@
   "module": [
     {
       "bucket_name": {
-        "source": [
-          "s3_bucket_name"
-        ],
-        "name": [
-          "audit"
-        ],
-        "account": [
-          "${var.account}"
-        ],
-        "region": [
-          "${var.region}"
-        ]
+        "source": "s3_bucket_name",
+        "name": "audit",
+        "account": "${var.account}",
+        "region": "${var.region}"
       }
     }
   ]
diff --git a/test/helpers/terraform-config-json/variables.json b/test/helpers/terraform-config-json/variables.json
index 56f5874..13afddb 100644
--- a/test/helpers/terraform-config-json/variables.json
+++ b/test/helpers/terraform-config-json/variables.json
@@ -8,44 +8,34 @@
     },
     {
       "azs": {
-        "default": [
-          {
-            "us-west-1": "us-west-1c,us-west-1b",
-            "us-west-2": "us-west-2c,us-west-2b,us-west-2a",
-            "us-east-1": "us-east-1c,us-east-1b,us-east-1a",
-            "eu-central-1": "eu-central-1a,eu-central-1b,eu-central-1c",
-            "sa-east-1": "sa-east-1a,sa-east-1c",
-            "ap-northeast-1": "ap-northeast-1a,ap-northeast-1c,ap-northeast-1d",
-            "ap-southeast-1": "ap-southeast-1a,ap-southeast-1b,ap-southeast-1c",
-            "ap-southeast-2": "ap-southeast-2a,ap-southeast-2b,ap-southeast-2c"
-          }
-        ]
+        "default": {
+          "us-west-1": "us-west-1c,us-west-1b",
+          "us-west-2": "us-west-2c,us-west-2b,us-west-2a",
+          "us-east-1": "us-east-1c,us-east-1b,us-east-1a",
+          "eu-central-1": "eu-central-1a,eu-central-1b,eu-central-1c",
+          "sa-east-1": "sa-east-1a,sa-east-1c",
+          "ap-northeast-1": "ap-northeast-1a,ap-northeast-1c,ap-northeast-1d",
+          "ap-southeast-1": "ap-southeast-1a,ap-southeast-1b,ap-southeast-1c",
+          "ap-southeast-2": "ap-southeast-2a,ap-southeast-2b,ap-southeast-2c"
+        }
       }
     },
     {
       "options": {
-        "default": [{}]
+        "default": {}
       }
     }
   ],
   "locals": [
     {
-      "foo": [
-        "${var.account}_bar"
-      ],
-      "bar": [
-        {
-          "baz": 1
-        }
-      ]
+      "foo": "${var.account}_bar",
+      "bar": {
+        "baz": 1
+      }
     },
     {
-      "route53_forwarding_rule_shares": [
-        "${{for forwarding_rule_key in keys(var.route53_resolver_forwarding_rule_shares) : \"${forwarding_rule_key}\" => {'aws_account_ids': '${[for account_name in var.route53_resolver_forwarding_rule_shares[forwarding_rule_key].aws_account_names : module.remote_state_subaccounts.map[account_name].outputs[\"aws_account_id\"]]}'}}}"
-      ],
-      "has_valid_forwarding_rules_template_inputs": [
-        "${length(keys(var.forwarding_rules_template.copy_resolver_rules)) > 0 && length(var.forwarding_rules_template.replace_with_target_ips) > 0 && length(var.forwarding_rules_template.exclude_cidrs) > 0}"
-      ]
+      "route53_forwarding_rule_shares": "${{for forwarding_rule_key in keys(var.route53_resolver_forwarding_rule_shares) : \"${forwarding_rule_key}\" => {'aws_account_ids': '${[for account_name in var.route53_resolver_forwarding_rule_shares[forwarding_rule_key].aws_account_names : module.remote_state_subaccounts.map[account_name].outputs[\"aws_account_id\"]]}'}}}",
+      "has_valid_forwarding_rules_template_inputs": "${length(keys(var.forwarding_rules_template.copy_resolver_rules)) > 0 && length(var.forwarding_rules_template.replace_with_target_ips) > 0 && length(var.forwarding_rules_template.exclude_cidrs) > 0}"
     }
   ]
-}
+}
\ No newline at end of file
diff --git a/test/helpers/terraform-config-json/vars.auto.json b/test/helpers/terraform-config-json/vars.auto.json
index 60e5235..fee0e28 100644
--- a/test/helpers/terraform-config-json/vars.auto.json
+++ b/test/helpers/terraform-config-json/vars.auto.json
@@ -1,11 +1,7 @@
 {
-  "foo": [
-    "bar"
-  ],
+  "foo": "bar",
   "arr": [
-    [
-      "foo",
-      "bar"
-    ]
+    "foo",
+    "bar"
   ]
 }
\ No newline at end of file