Skip to content

Conversation

junlarsen
Copy link
Contributor

@junlarsen junlarsen commented May 9, 2025

Description of changes

Partially implements RFC 71 by adding parser support for trailing commas to the policy parser. I will take care of the schema parser in another PR.

Test diagnostics have been updated where the change in grammar caused the expected token set to change. I have also added simple tests for the cases where trailing commas occur in the policy files.

Issue #, if available

Part of #1605

Checklist for requesting a review

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

  • A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).
  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).
  • A bug fix or other functionality change requiring a patch to cedar-policy.
  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)
  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

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).
  • 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.
  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)
  • Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
  • I'm not sure how my change impacts cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)

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.
  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-docs. PRs should be targeted at a staging-X.Y branch, not main.)
  • I'm not sure how my change impacts the documentation. (Post your PR anyways, and we'll discuss in the comments.)

This change requires updates in the grammar shown in the docs https://docs.cedarpolicy.com/policies/syntax-grammar.html. The changes are already outlined in the RFC itself. I believe? the docs repo is still private so somebody at AWS will have to do this.

@junlarsen junlarsen force-pushed the jun/feat/rfc-71-trailing-commas-for-policies branch from 695ee4a to 33588fb Compare May 9, 2025 17:00
Copy link

github-actions bot commented May 9, 2025

Coverage Report

Head Commit: 695ee4a2638a39b7b20edff6ffe291b25f7173cb

Base Commit: 35182b6f306722ee6f54772ef2854da82695b610

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.75%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3111/4528 68.71% 68.71%
cedar-policy-cli 🔴 571/972 58.74% 58.74%
cedar-policy-core 🟢 11966/14188 84.34% 84.34%
cedar-policy-formatter 🟢 914/1043 87.63% 87.63%
cedar-policy-validator 🟢 8753/10206 85.76% 85.76%
cedar-wasm 🔴 0/29 0.00% 0.00%

Copy link

github-actions bot commented May 9, 2025

Coverage Report

Head Commit: 33588fbde8d0567901b03b11731234541a1f75c7

Base Commit: 35182b6f306722ee6f54772ef2854da82695b610

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.75%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3111/4528 68.71% 68.71%
cedar-policy-cli 🔴 571/972 58.74% 58.74%
cedar-policy-core 🟢 11966/14188 84.34% 84.34%
cedar-policy-formatter 🟢 914/1043 87.63% 87.63%
cedar-policy-validator 🟢 8753/10206 85.76% 85.76%
cedar-wasm 🔴 0/29 0.00% 0.00%

Copy link

github-actions bot commented May 9, 2025

Coverage Report

Head Commit: 9dce92770a4e0c9c65b8cc4da9c8f168911f2e12

Base Commit: 35182b6f306722ee6f54772ef2854da82695b610

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.75%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3111/4528 68.71% 68.71%
cedar-policy-cli 🔴 571/972 58.74% 58.74%
cedar-policy-core 🟢 11966/14188 84.34% 84.34%
cedar-policy-formatter 🟢 914/1043 87.63% 87.63%
cedar-policy-validator 🟢 8753/10206 85.76% 85.76%
cedar-wasm 🔴 0/29 0.00% 0.00%

@@ -84,8 +84,9 @@ match {
}

Comma<E>: Vec<E> = {
<e:E?> => e.into_iter().collect(),
<mut es:(<E> ",")+> <e:E> => {
=> vec![],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason why we have special cases for 0 and 1 elements?

As opposed to defining the macro as:

Comma<E>: Vec<E> = { // (1)
    <mut v:(<E> ",")*> <e:E?> => match e { // (2)
        None => v,
        Some(e) => {
            v.push(e);
            v
        }
    }
},

Copy link
Contributor Author

@junlarsen junlarsen May 13, 2025

Choose a reason for hiding this comment

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

Absolutely not :) Your version is a lot more concise so I'll change to that - thanks!

Copy link

Coverage Report

Head Commit: 17d76548ac89379cdc0973bb1fa727751489d832

Base Commit: 35182b6f306722ee6f54772ef2854da82695b610

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.74%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3111/4528 68.71% 68.71%
cedar-policy-cli 🔴 571/972 58.74% 58.74%
cedar-policy-core 🟢 11974/14209 84.27% 84.34%
cedar-policy-formatter 🟢 914/1043 87.63% 87.63%
cedar-policy-validator 🟢 8841/10307 85.78% 85.76%
cedar-wasm 🔴 0/29 0.00% 0.00%

@john-h-kastner-aws john-h-kastner-aws self-requested a review June 2, 2025 17:22
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Looks great, thanks

junlarsen added 3 commits June 3, 2025 17:33
Partially implements RFC 71 by adding parser support for trailing commas
to the policy parser.

Part of cedar-policy#1605

Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
@junlarsen junlarsen force-pushed the jun/feat/rfc-71-trailing-commas-for-policies branch from 17d7654 to 15bf6d0 Compare June 3, 2025 08:33
Copy link

github-actions bot commented Jun 3, 2025

Coverage Report

Head Commit: 15bf6d04a433a9175aabc6381d296358bbc46b0e

Base Commit: 7bbe30e3e26ae6f67c57ad5cb2a62586c8dbe430

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 100.00%

Status: PASSED ✅

Details
File Status Covered Coverage Missed Lines

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 81.92%

Status: PASSED ✅

Details
Package Status Covered Coverage Base Coverage
cedar-policy 🔴 3223/4654 69.25% 69.25%
cedar-policy-cli 🔴 571/972 58.74% 58.74%
cedar-policy-core 🟢 21009/24728 84.96% 84.96%
cedar-policy-formatter 🟢 1074/1206 89.05% 89.05%
cedar-wasm 🔴 0/29 0.00% 0.00%

@john-h-kastner-aws john-h-kastner-aws merged commit acd5a00 into cedar-policy:main Jun 3, 2025
20 checks passed
mishjude pushed a commit to mishjude/cedar that referenced this pull request Jun 17, 2025
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Signed-off-by: Mish Jude <mishiej@amazon.com>
cdisselkoen pushed a commit that referenced this pull request Jun 26, 2025
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
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