Skip to content

TPE: Use value.get_as_x methods when available#2323

Merged
luxas merged 1 commit into
cedar-policy:mainfrom
luxas:tpe-get-as-x
May 6, 2026
Merged

TPE: Use value.get_as_x methods when available#2323
luxas merged 1 commit into
cedar-policy:mainfrom
luxas:tpe-get-as-x

Conversation

@luxas

@luxas luxas commented Apr 22, 2026

Copy link
Copy Markdown
Member

Description of changes

Broken out from #2162

Using the get_as_x methods in TPE minimizes the delta to the concrete evaluator (which uses them), and gives us automatically (later) the right error types for type mismatches (which should not happen if the Residual is validated, but anyways).

Issue #, if available

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

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

  • Does not update the CHANGELOG because my change does not significantly impact released code.

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

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

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.

@luxas luxas changed the title TPE: Use get_as_x methods when available TPE: Use value.get_as_x methods when available Apr 22, 2026

@john-h-kastner-aws john-h-kastner-aws left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is good, but I feel like it could probably be more aggressive

Comment thread cedar-policy-core/src/tpe/evaluator.rs Outdated
@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: b471c2d4aed133a4295b2f0fad5f6eec0f46373d

Base Commit: c50137867110c87b8e7e2c80ea603a9542197c72

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 75.00%

Status: FAILED ❌

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/tpe/evaluator.rs 🟡 15/20 75.00% 103, 150, 201, 216, 238

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 87.70%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟡 4247/5352 79.35% --
cedar-policy-cli 🟡 1222/1614 75.71% --
cedar-policy-core 🟢 23963/27241 87.97% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6760/7270 92.98% --
cedar-wasm 🔴 0/28 0.00% --

@luxas

luxas commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

Answered in #2323 (comment):

I held off at first, as it actually introduced a small change: before, && was left as-is, now && => && . However, this is kinda expected anyways, and should never happen on validated residuals.

Left are only the Residual::Concrete matches against HasAttr and GetAttr, which either match an entity or record, I think it makes the most sense to leave those as-is, as the concrete evaluator also uses similar match statements, and then if none of them match, return a custom type_error

However, now I updated the RHS matching for And and Or to use get_as_bool as well, and improved the and and or tests to follow the same order as the match statements, such that we cover all cases exhaustively (some small edge cases were not covered before, I checked the coverage report). Also using insta to assert inner structure of the residual, this could be done for other TPE tests as well in a follow-up.

},
// Cannot simplify "<left-residual> && <right-residual>" or "<left-residual> && <error>"
// The latter expression could become a Residual::PartialError later.
Residual::Partial { .. } | Residual::Error(_) => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the explicit notion here instead of _ for reader clarity.

@github-actions

Copy link
Copy Markdown

Coverage Report

Head Commit: c44bd6e8b415b84545561609e6ac555e888522b9

Base Commit: a2756b66050db5fdc6423933441939cb57ee566e

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 90.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/tpe/evaluator.rs 🟢 45/50 90.00% 103, 149, 199, 214, 236

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 87.73%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% 92.55%
cedar-policy 🟡 4247/5352 79.35% 79.35%
cedar-policy-cli 🟡 1222/1614 75.71% 75.71%
cedar-policy-core 🟢 23965/27241 87.97% 87.97%
cedar-policy-formatter 🟢 914/1088 84.01% 84.01%
cedar-policy-symcc 🟢 6769/7266 93.16% 93.16%
cedar-wasm 🔴 0/28 0.00% 0.00%

Comment thread cedar-policy-core/src/tpe/evaluator.rs Outdated
Signed-off-by: Lucas Käldström <lucas.kaldstrom@upbound.io>
@luxas luxas requested a review from victornicolet May 6, 2026 15:49
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown

Coverage Report

Head Commit: acfdcf9de5e68d180ebdd8c08c840ca168b75fc4

Base Commit: 844cdb54feb2875f3a9f958c4d78b514f08a327f

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 90.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines
cedar-policy-core/src/tpe/evaluator.rs 🟢 45/50 90.00% 104, 150, 200, 215, 237

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 87.73%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-language-server 🟢 4722/5102 92.55% --
cedar-policy 🟡 4446/5589 79.55% --
cedar-policy-cli 🟡 1222/1614 75.71% --
cedar-policy-core 🟢 24324/27638 88.01% --
cedar-policy-formatter 🟢 914/1088 84.01% --
cedar-policy-symcc 🟢 6769/7266 93.16% --
cedar-wasm 🔴 0/28 0.00% --

@luxas luxas merged commit 7229edc into cedar-policy:main May 6, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants