Skip to content

ext/typeexpr: Avoid refinements on dynamic values#625

Merged
apparentlymart merged 1 commit intohashicorp:mainfrom
wata727:avoid_refinements_on_dynamic_values
Aug 30, 2023
Merged

ext/typeexpr: Avoid refinements on dynamic values#625
apparentlymart merged 1 commit intohashicorp:mainfrom
wata727:avoid_refinements_on_dynamic_values

Conversation

@wata727
Copy link
Contributor

@wata727 wata727 commented Aug 30, 2023

See also #590
See also terraform-linters/tflint#1831

Refinements introduced in #590 has a restriction that it cannot refine unknown values of unknown types (cty.DynamicVal). Refining dynamic values causes a panic.
https://github.com/zclconf/go-cty/blob/v1.13.3/cty/unknown_refinement.go#L46-L47

The typeexpr extension may refine optional attribute as non-null, but dynamic values are not taken into account here, causing a panic.

This PR fixes the panic by avoiding refinement when the value type is cty.DynamicPseudoType. This is the same approach in the splat operator, so it's probably reasonable to do the same here.

hcl/hclsyntax/expression.go

Lines 1735 to 1737 in a9f8d65

if ty != cty.DynamicPseudoType {
ret = ret.RefineNotNull()
}

As a side note, I reviewed #590 again, and this is probably the only place we should consider dynamic values. Perhaps the hcldec also requires a determination before calling RefineWith, but I'm not familiar enough with the hcldec package, so I haven't included it here.

return wrappedVal.RefineWith(s.Refine), diags

@apparentlymart
Copy link
Contributor

Thanks for finding and fixing this!

Having encountered variants of this bug a few times now I'm considering (wearing my other hat as cty maintainer) changing cty to relax this rule and just silently ignore attempts to refine cty.DynamicVal, rather than panicking, but I don't really like having incorrect usage be silently ignored so I'm considering that as a last resort.

Therefore I'm going to merge this to fix the direct bug as reported and wait to see how many more examples of these situations we'll find before making a decision about whether to change cty itself. Hopefully it will be similar to the introduction of the concept of "marks" where once we've found a few historical poor assumptions it's relatively easy to maintain moving forward, but if incorrectly refining cty.DynamicVal remains a common mistake then I'll reconsider.

I think that hcldec line you indicated probably also needs a similar exception, so I'll test that out myself momentarily once I've merged this.

Thanks again!

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.

2 participants