Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lang/funcs: "one" function #27454

Merged
merged 1 commit into from
Apr 12, 2021
Merged

lang/funcs: "one" function #27454

merged 1 commit into from
Apr 12, 2021

Conversation

apparentlymart
Copy link
Contributor

In the Terraform language we typically use lists of zero or one values in some sense interchangably with single values that might be null, because various Terraform language constructs are designed to work with collections rather than with nullable values.

In Terraform v0.12 we made the splat operator [*] have a "special power" of concisely converting from a possibly-null single value into a zero-or-one list as a way to make that common operation more concise.

In a sense this one function is the opposite operation to that variant of splat: it goes from a zero-or-one collection (list, set, or tuple) to a possibly-null single value.

This is a concise alternative to the following clunky conditional expression, with the additional benefit that the following expression is also not viable for set values, and it also properly handles the case where there's unexpectedly more than one value:

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

Instead, we can write:

one(var.foo)

As with the splat operator, this is a tricky tradeoff because it could be argued that it's not something that'd be immediately intuitive to someone unfamiliar with Terraform. However, I think that's justified given how often zero-or-one collections arise in typical Terraform configurations. Unlike the splat operator, it should at least be easier to search for its name and find its documentation the first time you see it in a configuration.

My expectation that this will become a common pattern is also my justification for giving it a short, concise name. Arguably it could be better named something like oneornull, but that's a pretty clunky name and I'm not convinced it really adds any clarity for someone who isn't already familiar with it.

@codecov
Copy link

codecov bot commented Jan 9, 2021

Codecov Report

Merging #27454 (f541185) into main (33e5d11) will decrease coverage by 0.67%.
The diff coverage is 95.34%.

❗ Current head f541185 differs from pull request most recent head dc608f9. Consider uploading reports for the commit dc608f9 to get more accurate results

Impacted Files Coverage Δ
lang/funcs/collection.go 82.79% <95.23%> (+2.20%) ⬆️
lang/functions.go 96.29% <100.00%> (-0.03%) ⬇️
configs/provider_meta.go 0.00% <0.00%> (-83.34%) ⬇️
command/console_interactive.go 0.00% <0.00%> (-48.15%) ⬇️
configs/configschema/implied_type.go 37.50% <0.00%> (-43.59%) ⬇️
addrs/output_value.go 0.00% <0.00%> (-42.11%) ⬇️
command/format/diagnostic.go 64.39% <0.00%> (-15.77%) ⬇️
states/state_deepcopy.go 63.95% <0.00%> (-15.36%) ⬇️
command/state_mv.go 40.95% <0.00%> (-14.23%) ⬇️
terraform/eval_count.go 68.42% <0.00%> (-14.04%) ⬇️
... and 188 more

@pselle
Copy link
Contributor

pselle commented Jan 11, 2021

What's the motivation behind one() rather than head() or first(), if there is a motivation?

@apparentlymart
Copy link
Contributor Author

head wasn't one of the options I considered. first I did consider but decided against because that seemed to disagree with the behavior of failing if there is more than one item in the collection (which is necessary for this function to be applicable to sets, where there's no sense of "first element"). Now considering head too I think the same "objection" would apply there too; assuming you were thinking about this as in the head/tail terminology for cons lists, I think I'd likewise expect the function to succeed with more than one element.

One other name I considered was only, as in "the only item in ..." but subjectively I didn't find that to read as nicely when written out in an expression without other context:

output "example" {
  value = only(aws_instance.example[*].private_ip)
}

The fact that this function doubles as an assertion that there should never be more than one item in the collection -- along with a general desire to keep it short/concise for common use -- is what ultimately led me to one as the proposed function name.

@mildwonkey
Copy link
Contributor

🤔 If we're looking to bikeshed the name of this function, I might suggest single, but your choice (one) makes enough sense to me.

I feel ambivalent about the function but in the end agree that this is a common pattern worth simplifying for users.

@ktham
Copy link

ktham commented Feb 2, 2021

I'd prefer the clarity of having the "or" clause in oneornull, especially if it's going to return a null rather than throw an error when the assertion of one fails. Just my two cents. 🙏

@ktham
Copy link

ktham commented Feb 2, 2021

On a related note, there are some instances where I do:

dynamic "foobar" {
  for_each = var.some_optional_string == "" ? [] : [var.some_optional_string]
  content {
    foo = each.value
  } 
}

Would be great if these type of expressions can be simplified too. If there was an option type that can be a drop-in replacement for a set type, where the only difference is that an option would have only two states, the empty state, and the size==1 state. And perhaps the syntax to create an option is simplify using a function invocation option("foobar") # option(string) or option(32) # option(number), or option() # empty option.

Anyways, just some thoughts, not sure if this might be considered 🙂

@apparentlymart
Copy link
Contributor Author

Thanks for that feedback, @ktham.

I think the treatment of null effectively makes the "option type" behavior be inherent in all values in the Terraform language, and there's already a special behavior in the [*] operator building on that interpretation:

dynamic "foobar" {
  for_each = var.some_optional_string[*]
  content {
    foo = each.value
  } 
}

The above will work only if the "empty state" for your some_optional_string is null rather than "", but otherwise what I've shown above already works in the language today. (That's the "special power" of [*] I was referring to in the original writeup here.)

If we were designing the language anew today then making nullability explicit is something I would consider, but I think it's too late in the life of the Terraform language for such a fundamental type system change now. Instead, I think we're stuck with the compromise of improving the ergonomics of the existing "everything is nullable" model, which this one function is aiming to be a part of.

@ktham
Copy link

ktham commented Feb 3, 2021

Ah! Oh I see, I hadn't caught that when initially reading your issue description 🤦 , my bad!

That is neat, I didn't realize that the [*] operator can be used on a null value to return an empty list.

Perhaps this behavior regarding the splat operator when applied on a null value resulting in an empty list could be clarified a bit on this page https://www.terraform.io/docs/language/expressions/splat.html.

It currently reads:

[...] if a splat expression is applied to a value that is not a list or tuple
then the value is automatically wrapped in a single-element list before processing.

For example, var.single_object[*].id is equivalent to [var.single_object][*].id, 
or effectively [var.single_object.id]. 

Maybe it can instead read

[...] if a splat expression is applied to a value that is not a list or tuple
then the value is automatically wrapped in a single-element list before processing.
/For a null value, it would be turned into an empty list./

For example, var.single_object[*].id is equivalent to [var.single_object][*].id, 
or effectively [var.single_object.id]. 

(EDIT: Ah, nevermind, I saw that you added this tidbit in your doc page for the one function 👍 )

If we were designing the language anew today then making nullability explicit is something I would consider, but I think it's too late in the life of the Terraform language for such a fundamental type system change now.

Got it, makes sense 👍

@apparentlymart
Copy link
Contributor Author

apparentlymart commented Apr 12, 2021

Oh, whoops! We had a bit of debate about whether to move forward with this function and what it ought to be called and so I ended up forgetting to return to it after some other work. I only remembered it today when I was helping someone elsewhere who had encountered a situation like this function is intended to deal with.

After the discussion here and some discussion internally I tried out a few different function names to see which ones seemed to (subjectively) "work best". The most compelling ones were zeroorone and oneornull, both of which aiming to reflect the zero-length case more explicitly.

Unfortunately we have a long-standing convention that function names are just strings of letters/digits with no word separators, and so that often hampers what would otherwise be nice and descriptive names. That seems to be the case with these options too, where too many of these words start and end with vowels, making the result feel like a soup of letters that it's unclear how to parse: "zeroo rone"? "on eor null"? 🤯

With that said, despite spending some time preferring zeroorone I've ended up returning back to preferring one again. While it's true that it's not fully descriptive, that's a typical tradeoff with naming things in code, and given the prevalence of the count = cond ? 1 : 0 pattern I'm anticipating that the converse value = one(aws_instance.example[*].id) will also become common enough to be idiomatic, and thus folks will learn what it means in due course as they grow their Terraform knowledge. Common search engines have also tended to do well with queries like terraform foo function to turn up the function documentation pages relatively easily for someone who sees it in existing code and wants to learn more about what it means.

This was originally intended to land for v0.15, and although it's now missed the window for v0.15-rc1/rc2 the addition of a function is orthogonal enough to everything else that it's relatively low risk to add in here. Since it was already reviewed and approved as one, and since that's the name I ended up talking myself back into anyway, I'm going to go ahead and merge this as one after some mechanical fiddling to fit within the new website hierarchy, which has changed since I originally opened this PR.

In the Terraform language we typically use lists of zero or one values in
some sense interchangably with single values that might be null, because
various Terraform language constructs are designed to work with
collections rather than with nullable values.

In Terraform v0.12 we made the splat operator [*] have a "special power"
of concisely converting from a possibly-null single value into a
zero-or-one list as a way to make that common operation more concise.

In a sense this "one" function is the opposite operation to that special
power: it goes from a zero-or-one collection (list, set, or tuple) to a
possibly-null single value.

This is a concise alternative to the following clunky conditional
expression, with the additional benefit that the following expression is
also not viable for set values, and it also properly handles the case
where there's unexpectedly more than one value:

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

Instead, we can write:

    one(var.foo)

As with the splat operator, this is a tricky tradeoff because it could be
argued that it's not something that'd be immediately intuitive to someone
unfamiliar with Terraform. However, I think that's justified given how
often zero-or-one collections arise in typical Terraform configurations.
Unlike the splat operator, it should at least be easier to search for its
name and find its documentation the first time you see it in a
configuration.

My expectation that this will become a common pattern is also my
justification for giving it a short, concise name. Arguably it could be
better named something like "oneornull", but that's a pretty clunky name
and I'm not convinced it really adds any clarity for someone who isn't
already familiar with it.
@apparentlymart apparentlymart added the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 12, 2021
@apparentlymart apparentlymart merged commit 140c613 into main Apr 12, 2021
@apparentlymart apparentlymart deleted the one-func branch April 12, 2021 22:32
@aaronsteers
Copy link

@apparentlymart - I stumbled on this for the first time today in the release notes for 0.15.0 - exciting work! This simple update, I think, goes a long way to simplify a very common pattern terraform, and makes things very easy read at the same time!

Congrats on the release, and thanks!!

@ghost
Copy link

ghost commented May 13, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged config enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants