remove `HierarchyNotRespected` validation error #1355

cdisselkoen · 2024-12-05T15:13:08Z

Description of changes

As suggested in #638, removes the HierarchyNotRespected validation 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):

A bug fix or other functionality change requiring a patch to cedar-policy.

I confirm that this PR (choose one, and delete the other options):

Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

Not sure about changes to the Lean model, see above

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

john-h-kastner-aws

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

cedar-policy-validator/src/diagnostics/validation_errors.rs

john-h-kastner-aws · 2024-12-05T15:56:04Z

cedar-policy-validator/src/typecheck.rs

-                    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),


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

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

I think that change was done in #1462?

Updated this PR, now simplified further as you say. (we just return success(type_of_in) in all cases)

cdisselkoen · 2024-12-05T17:25:25Z

It might be returning Bool when the lean impl gives a more precise False type.

I looked at this Rust code some, and I think/hope that the logic I removed was already performed in type_of_var_in_entity_literals() and friends. The logic I removed only ever results in HierarchyNotRespected or success(type_of_in); it never returns False when type_of_in wasn't already False. So removing this logic shouldn't stop us from returning False in any situation where we were already returning False before this PR.

john-h-kastner-aws · 2024-12-05T18:16:04Z

It might be returning Bool when the lean impl gives a more precise False type.

I looked at this Rust code some, and I think/hope that the logic I removed was already performed in type_of_var_in_entity_literals() and friends. The logic I removed only ever results in HierarchyNotRespected or success(type_of_in); it never returns False when type_of_in wasn't already False. So removing this logic shouldn't stop us from returning False in any situation where we were already returning False before this PR.

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>

john-h-kastner-aws

This looks good to me. The concerns I mention will be fully addressed just by running the diff tests with this change merged, so we just need another review to look this over.

cdisselkoen · 2024-12-12T16:19:34Z

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>

cdisselkoen · 2025-01-13T14:45:18Z

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>

github-actions · 2025-03-25T20:00:03Z

Coverage Report

Head Commit: c318efbd1a40ff596d8ca40d22780dc1892a4b1a

Base Commit: ea9b3afefaca34e0a2ac9574c1d8be809a9c87e9

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 50.00%

Status: FAILED ❌

Details

File	Status	Covered	Coverage	Missed Lines
cedar-policy-validator/src/diagnostics/validation_errors.rs	🔴	0/1	0.00%	479
cedar-policy-validator/src/typecheck.rs	🟢	1/1	100.00%

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.49%

Status: PASSED ✅

Details

Package	Status	Covered	Coverage	Base Coverage
cedar-policy	🔴	3055/4403	69.38%	--
cedar-policy-cli	🔴	566/976	57.99%	--
cedar-policy-core	🟢	11657/13899	83.87%	--
cedar-policy-formatter	🟢	914/1043	87.63%	--
cedar-policy-validator	🟢	8296/9701	85.52%	--
cedar-wasm	🔴	0/29	0.00%	--

cdisselkoen · 2025-03-25T20:06:07Z

Ready for re-review. Corpus test failure is expected, see comments above; we will need to regenerate corpus tests because of this PR, which will be a separate PR to cedar-integration-tests. Also, DRT failure is because as of this writing we have merged #1535 but not cedar-policy/cedar-spec#587.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

remove HierarchyNotRespected validation error

ef44b64

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

cdisselkoen requested review from shaobo-he-aws and john-h-kastner-aws December 5, 2024 15:13

john-h-kastner-aws approved these changes Dec 5, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into cdisselkoen/simplify-…

478a481

…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

john-h-kastner-aws approved these changes Dec 12, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into cdisselkoen/simplify-…

9be9906

…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

cdisselkoen added 3 commits January 16, 2025 09:31

Merge remote-tracking branch 'origin/main' into cdisselkoen/simplify-…

302f074

…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

Merge remote-tracking branch 'origin/main' into cdisselkoen/simplify-…

ab17ce0

…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

Merge remote-tracking branch 'origin/main' into cdisselkoen/simplify-…

c318efb

…typechecker Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

john-h-kastner-aws approved these changes Mar 25, 2025

View reviewed changes

cdisselkoen requested a review from adpaco-aws March 25, 2025 20:16

shaobo-he-aws approved these changes Mar 25, 2025

View reviewed changes

cdisselkoen merged commit 1dcf194 into main Mar 25, 2025
17 of 20 checks passed

cdisselkoen deleted the cdisselkoen/simplify-typechecker branch March 25, 2025 20:21

cdisselkoen mentioned this pull request Mar 26, 2025

relax trait bounds on structs and enums where possible #1554

Merged

4 tasks

jv-garcia pushed a commit to jv-garcia/cedar that referenced this pull request May 6, 2025

remove HierarchyNotRespected validation error (cedar-policy#1355)

6de5db3

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

remove `HierarchyNotRespected` validation error #1355

remove `HierarchyNotRespected` validation error #1355

Uh oh!

cdisselkoen commented Dec 5, 2024

Uh oh!

john-h-kastner-aws left a comment •

edited

Loading

Uh oh!

Uh oh!

john-h-kastner-aws Dec 5, 2024

Uh oh!

cdisselkoen Dec 5, 2024

Uh oh!

cdisselkoen Feb 12, 2025

Uh oh!

cdisselkoen Feb 12, 2025

Uh oh!

cdisselkoen commented Dec 5, 2024

Uh oh!

john-h-kastner-aws commented Dec 5, 2024

Uh oh!

john-h-kastner-aws left a comment

Uh oh!

cdisselkoen commented Dec 12, 2024

Uh oh!

cdisselkoen commented Jan 13, 2025

Uh oh!

github-actions bot commented Mar 25, 2025

Uh oh!

cdisselkoen commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

remove HierarchyNotRespected validation error #1355

remove HierarchyNotRespected validation error #1355

Uh oh!

Conversation

cdisselkoen commented Dec 5, 2024

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

john-h-kastner-aws left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

john-h-kastner-aws Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

cdisselkoen Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

cdisselkoen Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

cdisselkoen Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

cdisselkoen commented Dec 5, 2024

Uh oh!

john-h-kastner-aws commented Dec 5, 2024

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

cdisselkoen commented Dec 12, 2024

Uh oh!

cdisselkoen commented Jan 13, 2025

Uh oh!

github-actions bot commented Mar 25, 2025

Coverage Report

Coverage of Added or Modified Lines of Rust Code

Coverage of All Lines of Rust Code

Uh oh!

cdisselkoen commented Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

remove `HierarchyNotRespected` validation error #1355

remove `HierarchyNotRespected` validation error #1355

john-h-kastner-aws left a comment •

edited

Loading