-
Notifications
You must be signed in to change notification settings - Fork 107
Fixes an issue where certain cedar policies couldn't be converted to JSON #601
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
Conversation
5828cc8
to
b77b073
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more comments:
- This approach transforms only INT_MIN. Should we, more generally, represent (-X) as a negative integer literal instead of a negation operation of a positive integer literal, for all X?
- It looks to me that only the CST->EST code was adjusted; is this sufficient? Is there AST->EST code that needs to be adjusted? I assume that EST->AST is OK
perhaps
Yeah, all of those already have the integer literals as |
Do we want to remove this code:
If the intent of the EST is to lossless preserve the policy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve as-is, possible followup per my comment above (transforming all negative literals and not just INT_MIN)
Description of changes
Previously, the Cedar EST code would fail to generate an EST for any policy containing the constant
-9223372036854775808
(i64::MIN
). This happened because it was turned into the CST representing the unary op-
applied to the constant9223372036854775808
, which outside the range ofi64
. This adds a tiny pass to ensure that unary op is constant folded.Issue #, if available
#596
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy
.I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec
(choose one, and delete the other options):Disclaimer
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.