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

hclsyntax: Detect and reject invalid nested splat result #724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Jan 3, 2025

Unfortunately two of the weird splat rules we included to mimic Terraform v0.11's bizarre splat behavior interact in a broken way when combined using nested splat expressions:

  1. Applying splat to a list or set value always returns a list value, rather than a tuple value, because that allows returning a more precise unknown value type when the source value is unknown.
  2. Applying a splat to a scalar value returns a tuple of either zero or one elements, depending on whether the scalar is null. This returns a tuple rather than a list again because that allows returning more precise information in some unknown value cases.

When both of these rules are combined using a nested splat -- rule 2 inside rule 1 -- the second rule causes the nested results to have varying types based on the scalar value rather than only on the list element type, which therefore violates the assumption made by rule 1 that because all elements of a list or set have the same type therefore all splat results on those elements must have the same type.

This previously caused a crash due to asking cty to construct an invalid list. We'll now handle this as a normal error diagnostic instead of a crash. The error message is not great -- it uses terminology that probably only an expert HCL user would understand fully -- but thankfully this is an edge case that apparently very few people have actually tried, since it took five years for anyone to trip over the bug and report it.

Ideally we'd make this situation work and generate a valid result of a different type, such as a list of lists rather than an invalid list of different tuple types. However, it's not clear that we can make that change retroactively without breaking existing code, and combining splat expressions in this way seems to be an edge case anyway -- it took many years before someone actually tried this combination and found the bug. Perhaps a future change will find a way to resolve this without an error, but for now we'll compromise on treating this as invalid since we know it wasn't working before anyway but at least this avoids crashing the host program completely.

This fixes #723 and would ultimately (after release/upgrade) fix both hashicorp/terraform#36284 and opentofu/opentofu#2327, although only addresses the fact that it was panicking, rather than making the reported situation actually work.

Unfortunately two of the weird splat rules we included to mimic Terraform
v0.11's bizarre splat behavior interact in a broken way when combined using
nested splat expressions:

1. Applying splat to a list or set value always returns a list value,
   rather than a tuple value, because that allows returning a more precise
   unknown value type when the source value is unknown.
2. Applying a splat to a scalar value returns a tuple of either zero or
   one elements, depending on whether the scalar is null. This returns a
   tuple rather than a list again because that allows returning more
   precise information in some unknown value cases.

When both of these rules are combined using a nested splat -- rule 2 inside
rule 1 -- the second rule causes the nested results to have varying types
based on the scalar _value_ rather than only on the list element type,
which therefore violates the assumption made by rule 1 that because all
elements of a list or set have the same type therefore all splat results
on those elements must have the same type.

This previously caused a crash due to asking cty to construct an invalid
list. We'll now handle this as a normal error diagnostic instead of a
crash.

Ideally we'd make this situation work and generate a valid result of a
different type, such as a list of lists rather than an invalid list of
different tuple types. However, it's not clear that we can make that change
retroactively without breaking existing code, and combining splat
expressions in this way seems to be an edge case anyway -- it took many
years before someone actually tried this combination and found the bug.
Perhaps a future change will find a way to resolve this without an error,
but for now we'll compromise on treating this as invalid since we know it
wasn't working before anyway but at least this avoids crashing the host
program completely.
@crw
Copy link
Contributor

crw commented Jan 7, 2025

Hi @apparentlymart, do you happen to know of an issue on the Terraform side that this would be related to? I'm looking to help get this review prioritized. Thanks!

@apparentlymart
Copy link
Contributor Author

Hi @crw,

I don't know of any existing report of this issue in the Terraform repository, but I recreated the problem with Terraform v1.10.3 and so I've opened hashicorp/terraform#36284 describing how to reproduce it there.

@crw
Copy link
Contributor

crw commented Jan 7, 2025

Thanks for confirming, I was under the impression this was due to some code divergence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic during nested splat evaluation with nulls
3 participants