Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Propagate some source locations from cedar syntax schema parser to JSON syntax structures #1405

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Propagates source locations on the Cedar schema format's EntityDecl, ActionDecl, and TypeDecl to the corresponding structures in json_schema. Partially addresses #1084, although we probably need to propagate source locations on even more types in order to fully resolve #1084.

Issue #, if available

Partial resolution of #1084

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

  • 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.)

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.

…ON syntax structures

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@shaobo-he-aws
Copy link
Contributor

A high level question: Should we have a Node struct instead? It seems that we will have to add a loc field wherever and whenever we want to propagate source locations.

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 good to me. Maybe add a test case demonstrating the new source locations?

@cdisselkoen
Copy link
Contributor Author

Should we have a Node struct instead?

This is a great idea. It would mean that we have to change the serde representation of Node though, in order to not break the existing schema JSON format. Specifically we'd have to serde(skip) the loc field and serde(flatten) the node field. Node is already Deserialize and Serialize, I wonder where that's currently used

@cdisselkoen
Copy link
Contributor Author

Well the answer is, it's not used anywhere in cedar repo, at least cargo test --all-features builds and passes even with Serialize and Deserialize removed from Node. So probably breaking the serialized representation of Node is OK

@cdisselkoen
Copy link
Contributor Author

Ah, quickly discovered the problem: Node has a non-optional Loc. We need an Option<Loc> here. We could create another flavor of Node that carries Option<Loc>, but is that worth it?

@shaobo-he-aws
Copy link
Contributor

Ah, quickly discovered the problem: Node has a non-optional Loc. We need an Option<Loc> here. We could create another flavor of Node that carries Option<Loc>, but is that worth it?

I'm fine as it is. We can always refactor the code when it's deemed necessary.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen cdisselkoen merged commit ebc0785 into main Jan 3, 2025
18 of 19 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/propagate-some-source-locs branch January 3, 2025 14:30
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.

Carry source info through schema structures
3 participants