hclsyntax: conditional refinements of collections must use Range() #633
Conversation
There was a problem hiding this comment.
This seems plausible but given our earlier experience with a different part of the same function last week I'd suggest also making sure there's a variant of this test case where the true and false values are both marked, to make sure it handles that combination of marks and refinements correctly.
|
Yes, I'll probably add an additional test if I don't come back with another PR. I'm tracking down some refinements+marks oddities right now. |
jbardin
force-pushed
the
jbardin/conditional-length-refinements
branch
from
b6b3dba
to
14b68b0
Compare
|
OK, the upstream problems I was tracking are taken care of in the latest cty, so I updated the version to match, and extended this PR to cover the marked cases. |
There was a problem hiding this comment.
This looks good to me!
One small hazard here is that IIRC we'd left HCL's dependency to cty on v1.13 to avoid forcing callers to adopt the Unicode 15 variant of grapheme cluster segmentation, though it's probably been long enough since Go 1.21 came out for that to not be as much of a concern anymore. (The concern was pretty narrow anyway: it only applies to those who care about Unicode consistency between different components and who need to remain on an older version of Go for some reason.)
Therefore I think this is probably fine though maybe we should make this be an HCL minor release (rather than a patch) to help communicate that upgrading could have some minor behavior changes beyond just fixing this bug.
When attempting to determine the final length range for a conditional expression with collections, the length values may still be unknown. Always use `Range()` to get the lower and upper bounds.
Correct mark handling for some conditional values. Find correct refinement for overlapping ranges which could not have been compared with `GreaterThan`. Also map inclusive flags for numeric ranges. Correct handling of DefinitelyNotNull collections. Return a known null early when both conditional branches are null.
jbardin
force-pushed
the
jbardin/conditional-length-refinements
branch
from
14b68b0
to
bad33d5
Compare
|
Now with more refined refinements! (and dropped the cty upgrade, since the tests no longer directly depend on it to pass) |
There was a problem hiding this comment.
Ahh, this version seems much nicer!
I guess when I was originally working on this I hadn't completed the Value.Range API in cty yet, since I don't know why else I would've manually implemented all of that finagling with the lower-level API. Using the range API throughout seems to make this much easier to follow.
| falseResult, falseResultMarks := falseResult.Unmark() | ||
|
|
||
| // use a value to merge marks | ||
| _, resMarks := cty.DynamicVal.WithMarks(condResultMarks, trueResultMarks, falseResultMarks).Unmark() |
There was a problem hiding this comment.
This is an interesting trick! Took me some squinting at it to understand what was going on here but I guess it's reasonable. Part of me wants to suggest a more elaborate comment explaining what's going on here but I have trouble imagining what would be useful to say. 🤷♂️
There was a problem hiding this comment.
Hehe, I thought that would get noticed ;) As soon as I started looking at conditional handling I wanted set operations on ValueMarks; maybe later.
|
I'm going to hold off tagging a new release until closer to when Terraform will pull the version, in case we find any more enhancement fixes. |
When attempting to determine the final length range for a conditional expression with collections, the length values may still be unknown. Always use
Range()to get the lower and upper bounds. Calls toRange()must also always be made with unmarked values.