Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Cedar 3.0] Desugaring of greater-than operator changes evaluation order #112

Closed
3 tasks done
mattmccutchen-amazon opened this issue Jun 6, 2023 · 0 comments · Fixed by #402
Closed
3 tasks done
Labels
3.0 bug Something isn't working. This is as high priority issue.

Comments

@mattmccutchen-amazon
Copy link

Before opening, please confirm:

Bug Category

Cedar Parser

Describe the bug

The CST-AST conversion desugars a > b into b < a and a >= b into b <= a. That changes the evaluation order, which is visible to users via its effect on which error gets reported if both a and b raise an error and possibly via performance differences as well. The effect on error reporting could be confusing to users: Cedar's evaluation order is generally left to right, so if an expression a op b raises an error from b, I think an average user might think that means a evaluated successfully and might get led down a wrong path during debugging.

Expected behavior

When an expression of the form a > b is evaluated, a should be evaluated before b, as with most other Cedar binary operators.

Reproduction steps

This can be reproduced using the cedar command-line tool:

% cedar evaluate 'context.nosuch < (4294967296 * 4294967296)'

error while evaluating the expression: record does not have the required attribute: nosuch

% cedar evaluate 'context.nosuch > (4294967296 * 4294967296)'

error while evaluating the expression: integer overflow while attempting to multiply 4294967296 by 4294967296

In both commands of this example, both the left and the right operand raise an error. The < operator propagates the error from the left operand, while the > operator propagates the error from the right operand.

Code Snippet

Included in the "reproduction steps".

Log output

Included in the "reproduction steps".

Additional configuration

No response

Operating System

Linux

Additional information and screenshots

A possible solution that doesn't require adding any more operators to the AST: change the desugaring of a > b to !(a <= b), and change the desugaring of a >= b to !(a < b).

@mattmccutchen-amazon mattmccutchen-amazon added the pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. label Jun 6, 2023
@andrewmwells-amazon andrewmwells-amazon added the bug Something isn't working. This is as high priority issue. label Jun 6, 2023
@khieta khieta added pending-review A Cedar maintainer has looked at this, but believes it needs review by more of the core team backlog and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. pending-review A Cedar maintainer has looked at this, but believes it needs review by more of the core team labels Jun 6, 2023
@andrewmwells-amazon andrewmwells-amazon changed the title Desugaring of greater-than operator changes evaluation order [Cedar 3.0] Desugaring of greater-than operator changes evaluation order Jul 12, 2023
john-h-kastner-aws added a commit that referenced this issue Nov 3, 2023
Fixes the evaluation order of operands to `>` and `>=` so that an error
in the left operand will always be reported instead of any error which
may exist in the right operand.

`a > b` is defined as `!(a <= b)` instead of `b < a` and `a >= b` is
defined as `!(a > b)` instead of `b <= a`.
@john-h-kastner-aws john-h-kastner-aws mentioned this issue Nov 3, 2023
11 tasks
john-h-kastner-aws added a commit that referenced this issue Nov 6, 2023
Fixes #112.
These evaluated operands right to left instead of left
to right due to how the CST to AST transformation
was implemented.
@sarahcec sarahcec moved this to Todo in Cedar 3.2 Status Nov 14, 2023
@sarahcec sarahcec added the 3.0 label Nov 27, 2023
@sarahcec sarahcec added 3.0 and removed 3.0 labels Nov 27, 2023
@sarahcec sarahcec moved this from Proposed to Done in Cedar 3.2 Status Nov 27, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
3.0 bug Something isn't working. This is as high priority issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants