Skip to content

Conversation

alexsnaps
Copy link
Collaborator

@alexsnaps alexsnaps commented Jul 15, 2025

This PR (based off the clippy one, sorry for the noise), is slowly introducing typing in a way that's more aligned with the other implementations.

I'd appreciate any feedback and/or questions anyone would have already.

I'll slowly add to this, to implement stdlib's the overloads...

Roadmap for the PR

  • Add ref.Val equivalent trait Val
  • Implement base types
  • Implement base Values

Follow up PRs

  • stdlib:
    • Implement overloads for the base Values
    • Implement container types
    • Implement overloads for container types
  • Leverage Optional in Optional support... in the parser #137
  • Implement overloads for Optional (if enabled)
  • Support Mutable- Lister & Mapper
  • Leverage mutable containers in macro expansions

@alexsnaps alexsnaps force-pushed the types branch 2 times, most recently from cacf9bc to ff8b7da Compare July 15, 2025 10:56
@alexsnaps
Copy link
Collaborator Author

I'm also really starting to wonder whether we should consolidate the two crates into a single one, wdyt @clarkmcc ? I just find it harder to see how to lay the code into files... and mostly it feels like so much going into the parser. All that making it into the public API (pulling e.g. uses cel-parser::Value to then work with the cel-interpreter::Context for instance)

@clarkmcc
Copy link
Collaborator

@alexsnaps the only reason to keep them separate are these use-cases. If it's going to create a maintenance burden or technical debt in order to specifically support the parser being used in non-CEL contexts, then I am comfortable consolidating to a single crate. While it's nice to accommodate other non-CEL use-cases, those use-cases can still use the parser as part of the single interpreter crate.

@clarkmcc clarkmcc requested a review from Copilot July 15, 2025 17:09
Copilot

This comment was marked as outdated.

@alexsnaps
Copy link
Collaborator Author

@alexsnaps the only reason to keep them separate are these use-cases. If it's going to create a maintenance burden or technical debt in order to specifically support the parser being used in non-CEL contexts, then I am comfortable consolidating to a single crate. While it's nice to accommodate other non-CEL use-cases, those use-cases can still use the parser as part of the single interpreter crate.

I sorta get the rational, tho... possibly not. As far as I can tell, supporting these use-cases "only" requires the crate to expose whatever is needed to create an AST to be accessible to the end user. Now that remains the case either way as that's how CEL phases are defined.

If we're talking requiring users of the parser to pull "too much" within the compile unit, then... possibly. Tho all they don't use would be DCE'ed anyways, but arguably that's not a free thing and will make their compilation, if only initially, slower. But that's where I fear things are going to get more obvious as I progress with this: there will be a whole lot more than "just the parser" in the cel-parser crate, as the whole type system is being pulled in. Also note, that the release and API burden does "somewhat" exist today, as there is a hard dependency between parser & interpreter version.

Now I will try to keep the parser "as lightweight as possible" for now and see how far I can get, we can always re-evaluate later. So I'll keep all traits in the parser, along side with the literal types. I think I can possibly split some types out of the parser into the interpreter (thinking of container types here, but I could be wrong). I still think this is somewhat fighting the design of the other reference implementation, where these shared things are in a "common" package/module/subproject, but I would really hate to add a 3rd crate. Until I'm further along, these things will then go into cel-parser::common, is that fine with you?

@clarkmcc
Copy link
Collaborator

@alexsnaps I mean I think you make a compelling case for unification. I'm comfortable with that.

@alexsnaps
Copy link
Collaborator Author

@alexsnaps I mean I think you make a compelling case for unification. I'm comfortable with that.

It's mostly the design that we're porting that from that makes that case. Tho, I'm also trying to avoid too many or certainly unnecessary dynamic dispatches... Which could become the case if I "hide" certain things behind a trait "just" for the sake of the dependency. I'll keep on iterating for a few days, and then we can hopefully make a call informed by actual code.

@alexsnaps
Copy link
Collaborator Author

alexsnaps commented Jul 23, 2025

Quick update:

I've updated the description a bit to reflect how I'm going about this and where things stand. The next step being starting to declare the overloads in a "stdlib", that then the "standard Environment" would use.

I'll try to have most of that in the interpreter, but this is where I expect to see if the splitting in two crates is still sensible or not. Additionally, there is a bit of a tension between separation of concerns and performance here. But I'm thinking that probably addressing performance should become a task on its own (data/benches driven), when more of this work has come together. Any disagreement/alternate proposal maybe?

@clarkmcc
Copy link
Collaborator

@alexsnaps no concerns

@clarkmcc clarkmcc added this to the 0.11.0 milestone Jul 25, 2025
@alexsnaps alexsnaps force-pushed the types branch 2 times, most recently from e6f323f to 0959ca9 Compare July 26, 2025 12:57
@alexsnaps
Copy link
Collaborator Author

@clarkmcc as part of this, I'm thinking of moving the module ast under common (just like in the go implementation actually), as while it's produced by the parser (eventually amongst other), it is not a "parser internal", if that makes sense.

Would you object to doing that, and then open this PR for review? It's actually mostly still dead code, but it's been a pain to rebase and I fear other PRs will suffer the same pain twice (i.e. now and again when this PR eventually get merged, when all of the stdlib is part of it).

I'm thinking it may make everyone's life easier if we iterate a little quicker on these API revamps, wdyt?

@clarkmcc
Copy link
Collaborator

All sounds good to me. Move fast and break things is fine, especially since we already broke things this week.😃

@alexsnaps
Copy link
Collaborator Author

All sounds good to me. Move fast and break things is fine, especially since we already broke things this week.😃

No better time than now... they say! But in the very case indeed, now is really the best time to break things given where we are!

@alexsnaps alexsnaps marked this pull request as ready for review July 28, 2025 11:05
@alexsnaps
Copy link
Collaborator Author

Splitting this up, to keep steady progress and avoiding too much pain given the ongoing moves/restructuring.

@alexsnaps alexsnaps requested a review from clarkmcc July 28, 2025 12:27
@clarkmcc clarkmcc requested a review from Copilot July 28, 2025 15:52
@clarkmcc
Copy link
Collaborator

Thanks, I'll take a look at this most likely tomorrow. Got a lot on the plate today :)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new type system architecture for CEL (Common Expression Language) that aligns with other implementations. It reorganizes the codebase by moving AST and reference types from the parser module to a common module, and introduces a comprehensive type system with traits and value types.

  • Moves AST and reference definitions from parser to common module for better organization
  • Introduces a new Val trait and comprehensive type system with base types (Bool, Int, String, etc.)
  • Replaces the old Val enum with CelVal enum that includes more type variants

Reviewed Changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
cel/src/parser/references.rs Updates import paths from parser to common module
cel/src/parser/reference.rs Removes old Val enum definition (moved to common)
cel/src/parser/parser.rs Updates imports and replaces Val with CelVal throughout
cel/src/parser/mod.rs Removes ast and reference modules, updates imports
cel/src/parser/macros.rs Updates imports from parser to common module
cel/src/objects.rs Updates imports and From implementation for CelVal
cel/src/magic.rs Updates import path for Expr
cel/src/lib.rs Adds common module and updates public exports
cel/src/common/types/*.rs New type system with Val trait implementations for each type
cel/src/common/traits.rs Defines trait constants for type capabilities
cel/src/common/reference.rs New CelVal enum and Val trait definition
cel/src/common/mod.rs Module definition for common package
cel/src/common/ast/mod.rs Updates Literal type from Val to CelVal

@alexsnaps alexsnaps force-pushed the types branch 2 times, most recently from 63f226b to 5ef5252 Compare August 1, 2025 13:27
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
@alexsnaps
Copy link
Collaborator Author

@clarkmcc Anything left that you'd want me to address?

@clarkmcc
Copy link
Collaborator

clarkmcc commented Aug 1, 2025

I'm on my phone but I think there's just the file rename unresolved thread. Once all discussions are resolved I'm good to approve

@alexsnaps
Copy link
Collaborator Author

I'm on my phone but I think there's just the file rename unresolved thread. Once all discussions are resolved I'm good to approve

That's done... but when you have time. There are some "unresolved" comments, where I left a note. If we agree, then they can be resolved and this can then be merged I think. I hope to open the stdlib PR if not this weekend, early next next week.

@clarkmcc
Copy link
Collaborator

clarkmcc commented Aug 1, 2025

@alexsnaps feel free to merge whenever you're ready

@alexsnaps alexsnaps merged commit 21add78 into cel-rust:master Aug 1, 2025
3 checks passed
@alexsnaps alexsnaps deleted the types branch August 1, 2025 16:39
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