terraform icon indicating copy to clipboard operation
terraform copied to clipboard

Unnecessary shape checking

Open skeggse opened this issue 3 years ago • 4 comments

Terraform Version

Terraform v1.3.5
on darwin_amd64

Terraform Configuration Files

I've tried narrowing this down more, but I'm not confident I understand what Terraform is doing here well enough to make further simplifications without losing the actual functionality I'm looking for.

output "unnecessary_restriction" {
  value = flatten([
    for n in [{ o = [1] }, { o = [2] }] :
    n.o == [1]
    ? [for k in [1, 2] : merge({ j = k, a = 2 }, n)]
    : [merge({ j = 3 }, n)]
  ])
}

output "restriction_workaround" {
  value = flatten([
    for n in [{ o = [1] }, { o = [2] }] :
    n.o == [1]
    ? [for k in [1, 2] : merge({ j = k, a = 2 }, n)]
    # Wrapping the value in a no-op ternary fixes the problem.
    : (true ? [merge({ j = 3 }, n)] : [])
  ])
}

Debug Output

https://gist.github.com/skeggse/b631420eb24969bc973f7adaa33fdd35

Expected Behavior

No error in either case: the objects are compatible, so they should just convert to a generic supertype.

Actual Behavior

Error in the unnecessary_restriction case.

Steps to Reproduce

  1. terraform init
  2. terraform validate (or terraform plan)

Additional Context

No response

References

  • #32301
  • Might be, in part, a vote for https://github.com/hashicorp/terraform/issues/29622#issuecomment-927585779?

skeggse avatar Nov 29 '22 04:11 skeggse

Hi @skeggse!

From what I can see in your expression, I think this is an example of an unhelpful error message but it is still correct for this to fail.

Although the error message focused on the tuple lengths, it seems that the real problem here is that not all of the elements in your tuples have the same object type -- that is, they have different attributes -- and so Terraform cannot convert the result to a list of that object type in order to make the two result arms have consistent types.

If you wrap tolist(...) calls around the tuple construction brackets [] I think you will see a more relevant error message explaining either that a conversion to list isn't possible at all or that the conditional expression arms are both lists but have different element types and are therefore incompatible.

To make this actually work I would suggest both adding the tolist calls to be explicit that you intend to use lists rather than tuples and make your object types all consistent by setting the same attribute names and types in each one.

For making this error message better, I think we should investigate whether the error message building logic can have some additional heuristics for noticing when a conversion to list was blocked by inconsistent types and then be explicit about what was inconsistent. Currently it seems to just stop as soon as it notices that the tuple types are incompatible, without trying to investigate why a conversion to a common list type wasn't possible.

apparentlymart avatar Nov 29 '22 16:11 apparentlymart

Hi @apparentlymart!

I think this is an example of an unhelpful error message

For making this error message better, I think we should investigate whether the error message building logic can have some additional heuristics for noticing when a conversion to list was blocked by inconsistent types and then be explicit about what was inconsistent. Currently it seems to just stop as soon as it notices that the tuple types are incompatible, without trying to investigate why a conversion to a common list type wasn't possible.

I had filed https://github.com/hashicorp/terraform/issues/32301 for the error message part; this ticket was intended more to discuss the type system side of things.

Although the error message focused on the tuple lengths, it seems that the real problem here is that not all of the elements in your tuples have the same object type -- that is, they have different attributes -- and so Terraform cannot convert the result to a list of that object type in order to make the two result arms have consistent types.

I'm not sure I see how that's the problem; given that the workaround functions, Terraform clearly does not enforce this requirement, and it's not clear to me what that restriction is trying to achieve, anyway. Why have this constraint when the runtime doesn't care? "Terraform cannot convert the result to a list of that object type" seems not correct to me, since it clearly can in the workaround example.

As an aside, having different types for tuples and lists feels arbitrary and unhelpful; I'm sure there's a problem it solves, but having no idea whether a given value is a tuple or a list just by looking at it makes reasoning about the types difficult.

skeggse avatar Nov 29 '22 20:11 skeggse

Let's dig in a bit and see how Terraform is evaluating these types.

To start, let's see what type the working expression has. To see this I changed the output value into a local value because that allowed me to use the type pseudo-function in terraform console to get the exact type information:

locals {
  workaround = flatten([
    for n in [{ o = [1] }, { o = [2] }] :
    n.o == [1]
    ? [for k in [1, 2] : merge({ j = k, a = 2 }, n)]
    # Wrapping the value in a no-op ternary fixes the problem.
    : (true ? [merge({ j = 3 }, n)] : [])
  ])
}
> type(local.workaround)
tuple([
    object({
        j: number,
        o: tuple([
            number,
        ]),
    }),
    object({
        j: number,
        o: tuple([
            number,
        ]),
    }),
    object({
        j: number,
        o: tuple([
            number,
        ]),
    }),
])

This seems to be a tuple of three elements that are all of the same object type, and so it's possible to convert this into a list, although conversion into a list apparently isn't necessary here:

> type(tolist(local.workaround))
list(
    object({
        j: number,
        o: tuple([
            number,
        ]),
    }),
)

Next I tried simplifying this expression in two ways that remove the outermost conditional expression so we can see what each of the arms of the conditional are producing before being unified to determine the conditional expression result type:

locals {
  workaround_just_true = flatten([
    for n in [{ o = [1] }, { o = [2] }] :
    [for k in [1, 2] : merge({ j = k, a = 2 }, n)]
    if n.o == [1]
  ])
  workaround_just_false = flatten([
    for n in [{ o = [1] }, { o = [2] }] :
    (true ? [merge({ j = 3 }, n)] : [])
    if n.o != [1]
  ])
}
> type(local.workaround_just_true)
tuple([
    object({
        a: number,
        j: number,
        o: tuple([
            number,
        ]),
    }),
    object({
        a: number,
        j: number,
        o: tuple([
            number,
        ]),
    }),
])
> type(local.workaround_just_false)
tuple([
    object({
        j: number,
        o: tuple([
            number,
        ]),
    }),
])

It's interesting that these two now clearly have different nested object types: the first one has an additional attribute a that was somehow lost in the form with the conditional, which then allowed the type unification to succeed.

So now we can see each of those conditional result operand values directly, with their different nested object types:

locals {
  workaround_just_true_literal = [
    {
      "a" = 2
      "j" = 1
      "o" = [
        1,
      ]
    },
    {
      "a" = 2
      "j" = 2
      "o" = [
        1,
      ]
    },
  ]
  workaround_just_false_literal = [
    {
      "j" = 3
      "o" = [
        2,
      ]
    },
  ]
}
> true ? local.workaround_just_true : local.workaround_just_false
╷
│ Error: Inconsistent conditional result types
│ 
│   on <console-input> line 1:
│   (source code not available)
│ 
│ The true and false result expressions must have consistent types. The 'true' tuple has length 2, but the 'false' tuple has length 1.
╵

Uh-oh! We're back to the original reported error again. Apparently peeling the expression apart in this way has somehow cancelled the effect of the workaround.

It seems like something about your workaround expression is causing Terraform to silently discard the a attributes in the true arm and therefore allow type unification to succeed. Conversion between object types (as opposed to object type unification) does allow discarding unexpected attributes, so it seems like something about that expression is causing an object type conversion that ultimately allows the overall expression to succeed but to lose information in the process.

apparentlymart avatar Nov 30 '22 00:11 apparentlymart

Regarding why list types and tuple types are distinct in Terraform:

The Terraform language type system is a static type system with some pragmatic escape hatches to make it more convenient to work with dynamic data from functions like jsonencode. The various compound data types form a matrix like this:

Lookup key type Single element type, any element count Distinct element types, fixed element count
Integer index List Tuple
String key Map Object
None Set (not supported)

The analogy most familiar to other languages is the "string key" row, where e.g. in Go we have both struct types (analogous to object types) and map types (analogous to map types). Go's type system doesn't have anything analogous to "tuple" but it does have slices which are analogous to Terraform's list types.

The Terraform language terminology is closest to TypeScript's, which does literally call its "fixed number of distinct element types" type kind Tuple. TypeScript calls its list type kind array, though.

Terraform has this distinction for largely the same reason that TypeScript does: it's a way to ascribe static types to data coming from dynamically-typed languages while constraining the input data as little as possible. In TypeScript's case it's doing its best to model JavaScript's type system, while in Terraform's case it was JSON (the jsondecode function in particular) that originally imposed the requirement for tuple types, so as to be able to describe the result of decoding a JSON array.

(That's also why there isn't a type corresponding to "set" in the third column: JSON has no concept of sets and so there was no clear need to model a hetrogenous set, whereas the concept of a homogenous set arises a lot in real-world APIs even though they need to be serialized as some sort of sequence in order to traverse a wire protocol.)


The fact that Terraform doesn't have any syntax for directly constructing a list value -- tolist([ ... ]) is the closest thing -- is an unfortunate historical design wart that we can thank Terraform v0.11 and earlier for. Early Terraform only supported lists of strings and so it had no need to explicitly model element types, and therefore no need to capture any type information beyond "string", "list", or "map". Therefore brackets [ ] were enough to denote "list" and braces { } were enough to denote "map".

While designing the Terraform v0.12 language (the basis of the modern Terraform language), we made a bet that most of the time it wouldn't really matter that much if people mix up list and tuple types (or map and object types), because we can often guess from context which the user intended. That bet has broadly paid off in practice because providers give us information about what they are expecting, but of course there are more complex situations such as the deeply-nested conditional / comprehension expressions you showed here where there aren't enough hints to rely on and so Terraform gives up and requires more information. This is particularly tricky for output values because they don't have static type constraints of their own, and so Terraform needs to infer the intended type entirely from the value itself; that's one of the problems #31728 was aimed at.

In your case here it arguably didn't give up soon enough: it guessed that you were trying to return a tuple type and so tried to decide one but couldn't find a solution. It then reported an error about the tuple types in particular, which was confusing because you didn't intend to produce tuple types in the first place. Ideally it should've noticed that it isn't clear whether you intended a list or a tuple and returned an error message about that instead, explaining that neither tuple or list are a viable result type here.

I typically advise to add more type information using the type conversion functions in these complex cases because it can eliminate most or all of the guesswork and therefore allow Terraform to return a more precise and relevant error message. If we had the freedom to break the language more in the v0.12 release then I would've proposed to have fewer automatic guesses and require more explicit type information (including better syntax for specifying it), but there was only so much breakage the community could bear across the v0.11-to-v0.12 transition and so, as is the case for many languages that have experienced gradual evolution, we have some historical design warts that we need to live with and make the best of.

apparentlymart avatar Nov 30 '22 01:11 apparentlymart