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

check for typed nulls in flatten func #129

Merged
merged 1 commit into from Aug 22, 2022

Conversation

jbardin
Copy link
Contributor

@jbardin jbardin commented Aug 8, 2022

@apparentlymart
Copy link
Collaborator

Hi @jbardin! Thanks for looking into this and sorry for the delay in reviewing it.

I split the tests up into some smaller parts because I wanted to make sure I understood the behavior here. I notice in doing so that you implemented it to treat a null list, tuple. or set as if it were an empty one, rather than leaving the null in place as this function would do for all other nulls.

I feel a bit ambivalent about that inconsistency: cty operations and functions don't normally vary their behavior based on the type of a null, and in the few cases where I did that historically (such as the Equal function, which only allows nulls of the same type to be equal) it's typically been considered confusing or "buggy" despite it having been intentional.

I think it might also technically be a violation of "the rules for unknown values" -- albeit an awkward one because there aren't actually any unknown values directly involved -- because cty.NullVal(cty.DynamicPseudoType) might appear because the true type of the null isn't known yet, but then a subsequent evaluation with more information might replace it with a typed null. Per the usual rules, that would mean that flatten would need to return an unknown value when given untyped nulls in order to give it license to return a different type/value once more information arrives, but that wouldn't be an acceptable behavior to retrofit because it would turn what was previously a known result into an unknown one.

Given that I find myself leaning towards treating all nulls in the same way -- leaving them in the output as null on account of them not being real sequences -- so that the result is consistent even if re-evaluated with better type information.

I've pushed the test cases I was using to your branch, with failures indicating the cases where it's currently treating null values differently depending on type. However, I know (thanks to out of band information) you're not available this week so I'm going to see about making those test cases pass myself now, and if I succeed then I'll merge my slightly modified version just so that the Terraform team can adopt it sooner.

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.

Panic in flatten() function
2 participants