add datastructure for public syntax tree, display and ExprBuilder by victornicolet · Pull Request #2170 · cedar-policy/cedar

victornicolet · 2026-02-18T19:55:28Z

Description of changes

This is a first PR for the implementation of a public AST (motivated by #816), the "PST".
This PR defines:

basic data structures for the PST,
a basic display for the PST expressions,
an implementation of ExprBuilder for the PST.

The goal in the design of the PST here was to decouple the public representation from internal data structures and data structures meant for serialization/deserialization.

where types are similar to the types reused in both AST and EST, we implement From<...> functions,
most types define a new representation for policies and expressions.

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.

Copilot

Pull request overview

Introduces an initial Public Syntax Tree (PST) module in cedar-policy-core to support ergonomic programmatic policy construction, including core PST data structures, basic expression Display, and an ExprBuilder implementation.

Changes:

Adds new pst module with expression, constraint, and policy data structures.
Implements Display for PST expressions.
Implements ExprBuilder for PST expression construction (internal builder).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`cedar-policy-core/src/lib.rs`	Exposes the new `pst` module publicly from the crate root.
`cedar-policy-core/src/pst/mod.rs`	Defines the PST module structure and re-exports key PST types.
`cedar-policy-core/src/pst/expr.rs`	Defines PST expression-related types, conversions, `ExprBuilder` impl, and `Display` + tests.
`cedar-policy-core/src/pst/constraints.rs`	Adds PST scope constraint data structures.
`cedar-policy-core/src/pst/policy.rs`	Adds PST policy representation (effect, clauses, annotations, scope constraints).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

victornicolet · 2026-02-19T14:54:44Z

Getting coverage for the changes in this PR won't be very meaningful, coverage will increase once we start adding functionality to convert / manipulate the PST.

cdisselkoen · 2026-02-19T20:19:49Z

+    /// In set of actions (single action is just length 1)
+    In(nonempty::NonEmpty<EntityUID>),


Do the AST/EST have the nonempty constraint here? Is permit(principal, action in [], resource); a valid Cedar policy?

They do not, good catch.

cdisselkoen · 2026-02-19T20:21:09Z

+pub enum SlotId {
+    /// Principal slot
+    Principal,
+    /// Resource slot
+    Resource,
+}


Discussion question: do we want to try to make the PST structures such that the modifications that will be needed for RFC 98 are backwards-compatible? is that an achievable goal?

I think it depends on what we define as "backwards compatible"? We can also force clients to not depend on exhaustively matching on the enums.

I think we can achieve compile-time backwards compatible changes if we want to extend types, whether it's because we have less restrictive types (have a variant that accepts any name) or because we force non-exhaustive matches (i.e. #non_exhaustive). By compile-time backwards compatible, I mean that a change in the reprsentation will not cause clients that previously compiled to fail.

I think the design needs to be able to maintain compiler-level backwards compatibility of the PST for backwards compatible changes to the Cedar language. We're not going to want to release new library major version for minor language versions.

At least, that's the attitude we've had so far. I've argued before that we should be comfortable with the library and language version getting out of sync.

This is kind of unfortunate since I might like to force consumers of the library to update their code if we add a new operator/slot kind.

I think RFC 98 will probably require library breaking changes in other places (i.e., a cedar-policy 5.0 using language version 4.x). However, it might be nice if consumers of the PST-specifically were not broken, if we have the opportunity to do that. OTOH, if we're breaking consumers of other parts of the public API it's ok to break consumers of the PST too.

Do you agree the non-exhaustive attribute on the public structs and enums would be sufficient to ensure compiler backwards compatibility, and probably the better option over not having enums?

On the other hand, if there are language characteristics that we know would require breaking changes in other places, it would be nice that a library update forces the client to update their code. I can't predict that with my lack of experience here.

cdisselkoen · 2026-02-19T20:23:41Z

+    Not(Arc<Expr>),
+    /// Arithmetic negation
+    Neg(Arc<Expr>),
+    /// Binary operation
+    BinaryOp {
+        /// The operator
+        op: BinaryOp,
+        /// Left operand
+        left: Arc<Expr>,
+        /// Right operand
+        right: Arc<Expr>,
+    },


Any reason to flatten the unary ops (Not/Neg) into Expr, but not the binary ops?

I guess the fact that there are a lot more binary operators, and using "not" is very common and it's marginally more convenient to have it as a top level enum, but we could move it inside a UnaryOp with Neg and IsEmpty as well.

cdisselkoen · 2026-02-19T20:24:40Z

+    /// Function call (builtin or extension). Syntactically, this can be either a function-style or
+    /// method-style call depending on the extension.
+    FuncCall {
+        /// Function name
+        name: SmolStr,
+        /// Arguments
+        args: Vec<Arc<Expr>>,
+    },


The comment says "builtin or extension", but builtin calls like .containsAny() or .getTag() are actually handled above in BinaryOp. Maybe this should be ExtFuncCall and specifically for extension functions?

Yes, some confusion on my part on what is a builtin and what is an extension from the point of view of a user. Is "extension" mostly an internal concept? Should we actually move the .containsAny and .getTag to be functions instead of binary operators, or vice-versa?

I think that's worth a small discussion, and from what I can see in the doc everything is an "operation" and it would make sense to treat all those uniformly.

The split mostly followed the AST/EST's shapes.

name: SmolStr feels like the wrong design here. The actual ought to be part of an enum (BinOp). The extension functions could use SmolStr as well, but it would also be reasonable to represent them as an enum in the PST (committing to the notion that extensions can't be user defined, but we've already done that for the most part)

Signed-off-by: Victor Nicolet <victornl@amazon.com>

…e implementation Signed-off-by: Victor Nicolet <victornl@amazon.com>

…2173) Signed-off-by: Victor Nicolet <victornl@amazon.com>

Signed-off-by: Victor Nicolet <victornl@amazon.com>

cdisselkoen

This is in good shape; a few more comments

cdisselkoen · 2026-02-25T14:23:56Z

+    /// Representation of an unknown for partial evaluation
+    Unknown {
+        /// Name of the unknown
+        name: SmolStr,
+    },


discussion: should we remove this, and just fail PST conversion for ASTs that contain Unknowns? once we fully remove partial evaluation in favor of TPE, we won't have this node in the AST, right?

Is the Unknown something the client should never see and construct?
From a practical standpoint, currently the ExprBuilder interface cannot fail on unknown construction (unless we panic of course).
Is that a change that would require removing Unknowns everywhere (AST, EST, the "unknown" in the extension functions), and will it be breaking?

Removing it from the AST should be nonbreaking because the AST is not exposed publicly. If the EST currently has an Unknown node, then removing it is breaking. Does the EST have an Unknown node, or does it represent unknowns via an extension function unknown()?

From a quick look we should be able to remove the unknown in the AST, leaving the extension function representation. IIRC, that's how the EST does it.

It'd be a larger change though.

The EST does represent unknowns with an extension function unknown(). Although I think removing the "unknown" extension supported in the PST to represent unknowns would still be a breaking change? And not having the Unknown node in the PST would make the language EST and PST can represent different?

I would lean towards having Unknown in the PST for now since it's also documented in the JSON policy format

+ fix handling of Unknown + eliminate clones Signed-off-by: Victor Nicolet <victornl@amazon.com>

github-actions · 2026-02-25T20:14:10Z

Coverage Report

Head Commit: a3bdea48cccdd1870ca79d2273090d2d5bb52677

Base Commit: 39cd0e81b21c9768218ba71e21c37df673d71bfd

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 89.66%

Status: PASSED ✅

Details

File	Status	Covered	Coverage	Missed Lines
cedar-policy-core/src/ast/entity.rs	🟢	4/5	80.00%	118
cedar-policy-core/src/pst/constraints.rs	🟢	34/34	100.00%
cedar-policy-core/src/pst/expr.rs	🟢	453/489	92.64%	133-135, 137-139, 304, 373, 405, 596-599, 604-607, 618-620, 644, 646-647, 649-651, 653-655, 891-894, 896-897, 951
cedar-policy-core/src/pst/policy.rs	🔴	3/23	13.04%	74-77, 79, 83-86, 88, 92, 96-100, 102-103, 106-107

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 86.38%

Status: PASSED ✅

Details

Package	Status	Covered	Coverage	Base Coverage
cedar-language-server	🟢	4722/5102	92.55%	--
cedar-policy	🟡	3776/5137	73.51%	--
cedar-policy-cli	🔴	794/1209	65.67%	--
cedar-policy-core	🟢	22567/25886	87.18%	--
cedar-policy-formatter	🟢	914/1088	84.01%	--
cedar-policy-symcc	🟢	6628/7161	92.56%	--
cedar-wasm	🔴	0/28	0.00%	--

victornicolet · 2026-02-25T21:50:25Z

A summary of the important discussion topics in this PR:

regarding backwards compatibility, some enum types are marked as #non_exhaustive (e.g unary and binary operations, the main Expr enum, the Literal enum, and for RFC 98, the SlotId)
the PST has Unknown and Error nodes for now, which are there mostly to make conversion from other representations non-faillible. An Error cannot be constructed by a client, and a client cannot inspect the contents of an error node. Unknown can be constructed for now, we are also discussing whether we should even have it.

@john-h-kastner-aws and @cdisselkoen what do you think? Is there anything that needs resolving before merging?
We can also keep this PR open and I can make PRs to this branch for the rest of the code.

cdisselkoen · 2026-02-26T14:05:44Z

I'm good to merge this as-is, and if necessary we can continue the discussion going forward. I also think it's easier to merge this now and not make a whole bunch of PRs into this branch -- among other things, it will make it easier to ensure that this work doesn't keep diverging from main and creating annoying merge conflicts.

john-h-kastner-aws

some smaller comments

john-h-kastner-aws · 2026-02-27T14:06:23Z

+    Resource,
+}
+
+impl From<ast::SlotId> for SlotId {


nit: we'll probably want #[doc(hidden)] on conversions from internal types

john-h-kastner-aws · 2026-02-27T14:07:30Z

+        Name {
+            id: id.into_smolstr(),
+            namespace: Arc::new(
+                Arc::try_unwrap(path)


Could this be Arc::unwrap_or_clone?

john-h-kastner-aws · 2026-02-27T14:09:22Z

+///
+/// Represents the type of an entity in Cedar (e.g., `User`, `Photo`, `Namespace::Resource`)
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct EntityType(pub Name);


Is there a way this could be the existing EntityTypeName struct?

I guess it represents the same thing (and EntityTypeName wraps ast::EntityType) although I don't think you can construct the name, you can only parse it? One goal was also to make the PST's representation completely decoupled from the internal representations, but this of course creates duplication. Not sure what's the best solution in this case.
This solution allows creating a name from a list of strings for the namespace and doesn't require parsing.

I'll keep it as-is in this PR, we can change our mind later.

john-h-kastner-aws · 2026-02-27T14:14:55Z

+                got,
+            });
+        }
+        Ok(match args.len() {


Might be able to write this with match args.as_slice() and avoid unwraps

I don't know if I can without adding a .clone() because .as_slice() gives a reference. Current solution is a little verbose but doesn't clone

john-h-kastner-aws · 2026-02-27T14:16:39Z

+            Expr::Literal(lit) => match lit {
+                Literal::Bool(b) => write!(f, "{}", b),
+                Literal::Long(i) => write!(f, "{}", i),
+                Literal::String(s) => write!(f, "\"{}\"", s.escape_default()),


We've used escape_debug more often in other places. Not sure how they're different

Documentation indicates they're handling unicode differently. I'll use escape_debug for consistency then.

Signed-off-by: Victor Nicolet <victornl@amazon.com>

github-actions · 2026-02-27T15:48:54Z

Coverage Report

Head Commit: 27041fd8703669dc55bbf78366cf7b98a411bd13

Base Commit: 39cd0e81b21c9768218ba71e21c37df673d71bfd

Download the full coverage report.

Coverage of Added or Modified Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 89.64%

Status: PASSED ✅

Details

File	Status	Covered	Coverage	Missed Lines
cedar-policy-core/src/ast/entity.rs	🟢	4/5	80.00%	118
cedar-policy-core/src/pst/constraints.rs	🟢	34/34	100.00%
cedar-policy-core/src/pst/expr.rs	🟢	452/488	92.62%	134-136, 138-140, 308, 377, 409, 601-604, 609-612, 623-625, 649, 651-652, 654-656, 658-660, 896-899, 901-902, 956
cedar-policy-core/src/pst/policy.rs	🔴	3/23	13.04%	74-77, 79, 83-86, 88, 92, 96-100, 102-103, 106-107

Coverage of All Lines of Rust Code

Required coverage: 80.00%

Actual coverage: 86.38%

Status: PASSED ✅

Details

Package	Status	Covered	Coverage	Base Coverage
cedar-language-server	🟢	4722/5102	92.55%	92.55%
cedar-policy	🟡	3776/5137	73.51%	73.51%
cedar-policy-cli	🔴	794/1209	65.67%	65.67%
cedar-policy-core	🟢	22566/25885	87.18%	87.09%
cedar-policy-formatter	🟢	914/1088	84.01%	84.01%
cedar-policy-symcc	🟢	6628/7161	92.56%	92.56%
cedar-wasm	🔴	0/28	0.00%	0.00%

victornicolet force-pushed the public-syntax-tree-data branch from c32bc61 to d63a006 Compare February 18, 2026 20:48

victornicolet marked this pull request as draft February 18, 2026 21:09

victornicolet force-pushed the public-syntax-tree-data branch from d63a006 to 90b438d Compare February 18, 2026 21:13

victornicolet requested a review from Copilot February 18, 2026 21:13

Copilot started reviewing on behalf of victornicolet February 18, 2026 21:14 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

victornicolet force-pushed the public-syntax-tree-data branch from 90b438d to b66cd0b Compare February 18, 2026 21:53

victornicolet requested a review from Copilot February 18, 2026 22:05

Copilot started reviewing on behalf of victornicolet February 18, 2026 22:06 View session

This comment was marked as outdated.

Sign in to view

Copilot AI reviewed Feb 18, 2026

View reviewed changes

Comment thread cedar-policy-core/src/pst/expr.rs Outdated

Comment thread cedar-policy-core/src/pst/mod.rs Outdated

Comment thread cedar-policy-core/src/pst/expr.rs

victornicolet force-pushed the public-syntax-tree-data branch from b66cd0b to a05fb1b Compare February 18, 2026 22:48

victornicolet marked this pull request as ready for review February 18, 2026 22:57

This comment was marked as outdated.

Sign in to view

victornicolet requested review from cdisselkoen and john-h-kastner-aws February 19, 2026 13:43

cdisselkoen reviewed Feb 19, 2026

View reviewed changes

victornicolet and others added 3 commits February 24, 2026 13:55

add datastructure for public syntax tree, display and ExprBuilder

1108855

Signed-off-by: Victor Nicolet <victornl@amazon.com>

add a few test, refactor based on suggestions and insights from futur…

a4b1f9c

…e implementation Signed-off-by: Victor Nicolet <victornl@amazon.com>

refactor extension functions in PST into binary and unary operators (#…

c01b981

…2173) Signed-off-by: Victor Nicolet <victornl@amazon.com>

victornicolet force-pushed the public-syntax-tree-data branch 2 times, most recently from 8ebc2f0 to ea91805 Compare February 24, 2026 19:10

cedar-policy deleted a comment from github-actions Bot Feb 24, 2026

add allow private interface, cleanup lints

8561350

Signed-off-by: Victor Nicolet <victornl@amazon.com>

victornicolet force-pushed the public-syntax-tree-data branch from ea91805 to 8561350 Compare February 24, 2026 19:34

This comment was marked as outdated.

Sign in to view

cdisselkoen approved these changes Feb 25, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

victornicolet force-pushed the public-syntax-tree-data branch from d3ac405 to dad5926 Compare February 25, 2026 19:44

improve Name and EntityType conversions, minor comments

a3bdea4

+ fix handling of Unknown + eliminate clones Signed-off-by: Victor Nicolet <victornl@amazon.com>

victornicolet force-pushed the public-syntax-tree-data branch from dad5926 to a3bdea4 Compare February 25, 2026 19:57

john-h-kastner-aws approved these changes Feb 27, 2026

View reviewed changes

hide doc for conversions to internal types, minor improvements

27041fd

Signed-off-by: Victor Nicolet <victornl@amazon.com>

victornicolet requested review from cdisselkoen and john-h-kastner-aws February 27, 2026 15:51

cdisselkoen approved these changes Feb 27, 2026

View reviewed changes

john-h-kastner-aws approved these changes Feb 27, 2026

View reviewed changes

victornicolet merged commit 8118788 into main Feb 27, 2026
30 of 35 checks passed

victornicolet deleted the public-syntax-tree-data branch February 27, 2026 18:38

		/// In set of actions (single action is just length 1)
		In(nonempty::NonEmpty<EntityUID>),

Conversation

victornicolet commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

victornicolet commented Feb 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornicolet Feb 19, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornicolet Feb 20, 2026 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornicolet Feb 19, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornicolet Feb 19, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

cdisselkoen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornicolet commented Feb 18, 2026 •

edited

Loading

victornicolet Feb 19, 2026 •

edited

Loading

victornicolet Feb 20, 2026 •

edited

Loading

victornicolet Feb 19, 2026 •

edited

Loading

victornicolet Feb 19, 2026 •

edited

Loading

john-h-kastner-aws Feb 25, 2026 •

edited

Loading

victornicolet Feb 25, 2026 •

edited

Loading