zclconf / go-cty

A type system for dynamic values in Go applications
MIT License
348 stars 71 forks source link

third param for `lookup` function is not optional #123

Closed johakoch closed 6 months ago

johakoch commented 2 years ago

The documentation for the lookup() stdlib function states:

There are two required arguments, map and key, plus an optional default, which is a value to return if no key is found in map.

However, if I call lookup({a = 1, b = 2}, "a") without a third argument, I get the error

Not enough function arguments; Function "lookup" expects 3 argument(s). Missing value for "default".

The function specification lists exactly three Params and no VarParam.

apparentlymart commented 2 years ago

Thanks for reporting this, @johakoch.

Indeed, the documentation here is incorrect. I think what happened here is that this function got here by someone at HashiCorp proposing to upstream Terraform's lookup function, which does indeed have an optional final argument for backward-compatibility with how older versions of Terraform (v0.6 and earlier, I think) dealt with map indexing exclusively using functions, whereas modern Terraform has an explicit indexing operator.

As part of reviewing that proposal I requested various changes to the functions to eliminate Terraform-specific quirks given that cty itself has no requirement to be backward-compatible with old versions of Terraform that predate it. I expect I called for lookup to have a mandatory third argument as part of that feedback, because the no-default case is already represented by the function index.

I think the best fix here is to update the documentation to accurately describe the behavior, and perhaps to suggest using index for situations where a default value isn't needed.

If you want the behavior of Terraform's lookup function for your application then you'll need to take the implementation from Terraform itself. I don't think that implementation is available as a library anywhere, but you could copy it directly from the Terraform codebase as long as your project has a compatible open source license.

(Though since your project seems to use HCL, you probably don't actually need the Terraform backward-compatibility behavior: HCL's {a = 1, b = 2}["a"] means the same thing that lookup({a = 1, b = 2}, "a") would achieve in Terraform, and is also the same as cty's index function which HCL's indexing operator is implemented in terms of.)

johakoch commented 2 years ago

Correct, I don't need the third param to be optional. So I'd be happy for the documentation to be corrected :-)

johakoch commented 6 months ago

Please see PR https://github.com/zclconf/go-cty/pull/178

johakoch commented 6 months ago

Then we can close this issue.