Logical binary operators don't short-circuit

[srinathrangaramanujam]

Terraform Version

0.12.20

Terraform Configuration Files

    diag_object_arr = flatten([for key, value in var.diag_object : [
        for cntr in range(length(value.resource_id)) : {
               bln_use_datasrc = (length(value.log) > 0) && (lower(value.log[0][0]) == "sdsds")
        } 
    ]])

Crash Output

Error: Invalid index

on ....\az-terraform-diagnostic-settings\module.tf line 12, in locals:
12: bln_use_datasrc = (length(value.log) > 0) && (lower(value.log[0][0]) == "alllogs")
|----------------
| value.log is empty list of tuple

The given key does not identify an element in this collection value.

Expected Behavior

If the variable log value in empty, the Length(log) > 0 shoudl return false and lower(value.log[0][0]) == "sdsds") should not have executed

Actual Behavior

eventhough the Length(log) > 0 is false, and short circuite is not happening and ower(value.log[0][0]) gets executed.

Steps to Reproduce

Additional Context

References

[dhoepelman]

This is doubly confusing as && and || is used for operator syntax, while in most languages && and || are the short-circuiting variants of & and |

Work-around is to use the conditional condition ? true_val : false_val expression, as that one is lazy since 0.12

edit: coalesce and coalescelist can also be used for working around this

[jeliker]

I have attempted a conditional expression for a work-around but it seems aggressively wordy when trying to handle the case "is not null and is not empty list"

image_ids = (var.image_ids == null ? var.default_ids : (length(var.image_ids) == 0 ? var.default_ids : var.image_ids))

I find this preferred syntax otherwise throws error "argument must not be null" when var.image_ids = null

image_ids = (var.image_ids == null || length(var.image_ids) == 0 ? var.default_ids : var.image_ids)

Is there a better way (prior to widespread adoption of variable validation)?

[dhoepelman]

@jeliker you can write those a bit more clear with coalesce and coalescelist

image_ids = coalescelist(coalesce(var.image_ids,[]), var.default_ids)

[sergei-ivanov]

Well, this came as a bit of a shock today. I've always assumed that logical operators in Terraform were short-circuiting, much like they are in Java or Go.

Although functions like compact, coalesce and coalescelist can deal with simpler cases, they do not work with maps, or when you have more complex conditions.

In cases like @jeliker's I had to replace this:

# var.rules is a map(object(...))
rules_provided = var.rules != null && length(var.rules) > 0

...with this:

rules_provided = length(var.rules != null ? var.rules : {}) > 0

So @jeliker , your expression can be simplified to:

image_ids = length(var.image_ids != null ? var.image_ids : []) == 0 ? var.default_ids : var.image_ids

...which is still somewhat ugly and less readable than a logical operator.

[jeliker]

Thank you for your comments @sergei-ivanov. Seems I have this pattern:

if ? then : (if ? then : else)

…and you have this pattern

(if ? then : else) ? then : else

...so we are forced to evaluate twice in any case. Hopefully this will resolve to more expected (in my opinion and evidently yours, perhaps others) short-circuit behavior soon

[sergei-ivanov]

@jeliker Yes, it is only about grouping.
Logically,
( x != null && length(x) > 0 )
is equivalent to
( length(x != null ? x : []) > 0 )
...so it's possible to replace one with the other, and keep right-hand side of the outer conditional intact.

I also wish that the problem is eventually fixed at source (in Terraform).

[apparentlymart]

Hi all!

It does seem reasonable to support a short-circuit behavior for the logical operators in the Terraform language. These operators are actually implemented upstream in the HCL library, which is why this was previously labelled as "dependencies".

Because implementing this represents a change in the behavior of HCL and HCL is used in other codebases apart from Terraform it will require some coordination to implement, although I think it should be safe to implement in a minor release because the change in behavior is a positive one: expressions that would previously have failed with an error will now succeed in some way, and no expressions that would previously have succeeded will start producing a new value or start returning an error.

[solarmosaic-kflorence]

Just ran into this myself doing foo = bar["a"] && bar["a"]["b"] ? "b" : "a" which resulted in an error if bar["a"] did not exist. Fixed with foo = bar["a"] ? (bar["a"]["b"] ? "b" : "a") : "a" which is much harder to read.

[aidan-mundy]

In lieu of HCL changes to the operators themselves, could we get lazy evaluating and and or functions? It is already possible to hack this functionality in with ternary operators, but it isn't viable for frequent reuse because custom functions aren't supported

[sergei-ivanov]

We already have alltrue() and anytrue() functions, which are, as far as I can tell, eagerly evaluated. I am not sure whether we need even more functions, I would personally prefer the operators to be changed to lazy evaluation.

[OJFord]

coalesce is mentioned a couple of times as being a work-around, but trying to use it in the first place is actually how I ran into this. I assume it works in some cases, but it doesn't when an accessed attribute (which should be short-circuited) does not exist:

> coalesce("hunter2", {a=1}["b"])

╷
│ Error: Invalid index
│
│   on <console-input> line 1:
│   (source code not available)
│
│ The given key does not identify an element in this collection value.
╵

The ternary operator does however, (as long as you know the type I suppose) thanks for that suggestion.

> "hunter2" != "" ? "hunter2" : {a=1}["b"]
"hunter2"

[tculp]

Just ran into this with length(var.overrides) > count.index && lookup(var.overrides[count.index]), "name", null) != null. I was able to workaround by changing it to length(var.overrides) > count.index && lookup(try(var.overrides[count.index], {})), "name", null) != null

[markmsmith]

Is there a ticket tracking the upstream work for this in the HCL library? I couldn't see one when I searched:
https://github.com/hashicorp/hcl/issues?q=is%3Aissue+is%3Aopen+lazy+

[doanduyhai]

It is beyond understanding that such a fundamental language construct is not properly implemented ....

So instead of writing condition1 && condition2 && condition3 which is quite straightforward, one should use the ternary expression that leads to bloated code: condition1 ? condition2 ? condition3 : false: false

An example of validation code that we are producing right now

  precondition {
    condition     = local.dnat != null ? try(local.dnat.rules, null) != null ? alltrue([for rule in local.dnat.rules: (try(rule.translated,null) != null ? try(rule.translated.value, null) != null ? rule.translated.value != "" : false: true) ]) : true: true
    error_message = "'local.dnat.rules[*].translated.value' should not be null or blank"
  }

It sure works fine but this code is not maintainable in the long term

[artm]

We've come up with a workaround for another edgecase.

The logic:

• when credential ID is supplied a data resource is created and used to fetch username / password from the API.
• when credential ID isn't supplied a default username / password are used

The implementation:

variable "credential_id" {
  type    = string
  default = null
}

data "lastpass_secret" "credential" {
  for_each = var.credential_id != null ? { enabled = true } : {}
  id       = var.credential_id
}

locals {
  default_credential = {
    username = "provisioner",
    password = null # rely on ssh-agent for auth
  }
  credential = merge(
    { enabled = local.default_credential },
    data.lastpass_secret.credential
  )["enabled"]
}

The for_each with a ternary producing an object is how we handle optional resources in general, so it's idiomatic for our codebase. Merging such optional resource onto a default indexed by "enabled" allows to have local.credential refer to either the data resource or a default.

I'm positing this because our initial attempt was to use ternary operator return either data resource or default object but it wouldn't work when data resource wasn't created because ternary short-circuits "too late" if at all.

---

now that I think about it it's just a variant of coalesce workaround adjusted to our idiom of using .enabled for optional data sources / resources.

[bwinter]

@apparentlymart - this issue has been around for a while now. Any idea what the current status of this feature is?

Thanks!

[crw]

I believe this was examined in hashicorp/hcl#538, although not completed.

[microbioticajon]

Just bumped into this too. This is how we resolved it for anyone interested (this example is more complicated due to the optional nature of the offending variable):

 variable "logging" {
   type = object(
     {
       target_bucket  = string,
       target_prefix  = optional(string), 
       target_prefix_append_bucket_name = optional(bool) ## perhaps this should not be optional, but it highlights how this issue can become messy...
     }
   )
   default  = null
}
# This is where the error was being thrown...
locals {
  bucket_logging_appended_bucket_name  = (var.logging != null && coalesce(try(var.logging.target_prefix_append_bucket_name, false), false)) ? "my-bucket-name-to-append/" : ""
}

The try captures the error and returns false to play nicely with the boolean operator that should be being short-circuited . The coalesce is needed because the variable could be null which is not actually an error and does not play nicely with the boolean operator.

[mloskot]

Since this is a now documented behaviour as per he merge of this

• Document logical operators not short-circuiting #30631

then, shouldn't this have now been closed?

[OJFord]

@mloskot No, because it is labelled 'enhancement', not 'bug' or 'missing docs' or something. (By a maintainer, with #24128 (comment) on intention, and draft PR hashicorp/hcl#538.)

[mloskot]

@OJFord

it is labelled 'enhancement'

Yes, but the label pre-dates the #30631, so it could be the case the label no longer applies

draft PR hashicorp/hcl#538

I have missed the draft. Thanks.

[NewbMiao]

Is there any plan to improve this with short-circuit? or we need to bear it in mind that HCL does not support short-circuit(saw the draft PR hashicorp/hcl#538 was closed, seems there is no other PR for this)