remove HierarchyNotRespected validation error
#1355
Conversation
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
There was a problem hiding this comment.
I'm happy to see this changes, though I'm not 100% sure it will pass DRT. It might be returning Bool when the lean impl gives a more precise False type. Fixing any discrepancy there should also be a backwards compatible change, but we should take a close look at what both are doing with in.
If it does pass, then we might be able to use AgreeOnAll here if this is the only error the Rust reports but Lean doesn't.
https://github.com/cedar-policy/cedar-spec/blob/main/cedar-drt/src/lean_impl.rs#L516
| if !self.mode.is_strict() { | ||
| TypecheckAnswer::success(type_of_in) | ||
| } else if matches!(type_of_in.data(), Some(Type::False)) { | ||
| if self.mode.is_strict() && matches!(type_of_in.data(), Some(Type::False)) { | ||
| TypecheckAnswer::success( | ||
| ExprBuilder::with_data(Some(Type::False)).val(false), |
There was a problem hiding this comment.
I don't think rewriting to the literal false should be required anymore. We could just return the original annotated expression if it simplifies things further
There was a problem hiding this comment.
I'll do that in a separate PR since this one is a little delicate as-is and the change you suggest is technically orthogonal
There was a problem hiding this comment.
I think that change was done in #1462?
There was a problem hiding this comment.
Updated this PR, now simplified further as you say. (we just return success(type_of_in) in all cases)
I looked at this Rust code some, and I think/hope that the logic I removed was already performed in |
IIUC, the DRT currently passes if Rust returns an errors when lean doesn't. So, if rust has stopped returning errors here, there could be a precision difference in the non-error response. |
…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
|
Corpus tests are failing in CI, probably because there are edge cases where this PR changes the validation behavior from fail -> pass (as noted in the changelog entry). Adjusting the corpus test expectation will have to be a different PR against cedar-integration-tests |
…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
|
note on the status of this: have been waiting for cedar-policy/cedar-integration-tests#14 to be merged first, to avoid conflicts in updating the corpus tests |
…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Description of changes
As suggested in #638, removes the
HierarchyNotRespectedvalidation error (well, except from the public error enum, where we leave it to avoid a breaking change, but adjust documentation to indicate it is never returned). This simplifies the typechecker code considerably.I'm a little unclear from the discussion on #638 whether the Lean model needs any corresponding changes or not.
Issue #, if available
Fixes #638
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):I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):