Types

[alexsnaps]

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]

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]

@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.

[alexsnaps]

@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]

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

[alexsnaps]

@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]

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]

@alexsnaps no concerns

[alexsnaps]

@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]

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

[alexsnaps]

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]

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

[clarkmcc]

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

[Copilot]

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]

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

[clarkmcc]

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]

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]

@alexsnaps feel free to merge whenever you're ready