Allow 'if' as HCL attribute name#6473
Conversation
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
|
Thanks for looking into this @Gautam-aman ; are you sure you pushed all the changes you were after? As I'm seeing no reference that |
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
|
This PR is now ready for review. The grammar and visitor changes allow Local build and tests pass:
Happy to address any feedback. |
|
|
||
| identifierLike | ||
| : Identifier | ||
| | IF |
There was a problem hiding this comment.
@greg-at-moderne any idea whether if is the only keyword that would need special handling? Or could there be others too?
There was a problem hiding this comment.
That's a good point. I think it's not the only one.
Another real-life example is in, see this real-world mention:
https://github.com/terraform-aws-modules/terraform-aws-fsx/blob/ac8fe39a0d2428c30be6ed96a2d13026e0306e41/examples/lustre/main.tf#L73
So I think eventually we need to allow all (or at least nearly all) keywords.
Having said that, I think this PR is a net improvement.
@Gautam-aman - would you like to continue the effort and change the impl to support keywords too, not if specifically?
There was a problem hiding this comment.
As a potentially larger change we can optionally rework the parser to specify where it should expect identifiers using boolean fields as also seen on
I've found Claude to be quite efficient to pick up this work, so you might not want to tackle all of that manually.
There was a problem hiding this comment.
Thanks, that makes sense.
I agree the broader keyword handling and parser rework would be a larger change. For this PR, I’d prefer to keep the scope limited to if so it remains small and safe.
I’m happy to look into a follow-up PR for broader keyword support or parser refactoring if that would be useful.
There was a problem hiding this comment.
It's more that your current change would likely result in an out-of-place keyword IF and LST element, where an identifier is expected. Let me try quickly if I can change your test to show that issue.
There was a problem hiding this comment.
Thanks for checking that makes sense.
If the test exposes an unintended case where IF is accepted outside identifier positions, I’m happy to adjust the implementation accordingly once we see the result.
…/JsonPathParserVisitor.java Co-authored-by: Tim te Beek <timtebeek@gmail.com>
…erVisitor.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
All review feedback has been addressed and the PR is ready. |
|
Thanks for the review and merge , appreciate it! |
|
Thanks! There's one open question that I'd like @greg-at-moderne to weigh in on; please don't resolve that comment before then: Mostly I'm wondering if the fix here is too specific for the problem that was reported, whereas I think there might be similar cases that we'd want to tackle here as well. |
|
I agree it’s worth considering whether this should be generalized beyond the reported case. I’m happy to wait for @greg-at-moderne’s input and adjust the approach if needed. Let me know how you’d like to proceed. |
…/hcl-if-as-property
Fixes parsing failure when if is used as an attribute name in HCL.
Terraform allows keywords as attribute names, but the current grammar only accepts Identifier, causing valid HCL to fail parsing.
This change allows if in attribute position only and adds a regression test.
ifas property name #6446