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

ext/typeexpr: Avoid refinements on dynamic values #625

Merged

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