-
Notifications
You must be signed in to change notification settings - Fork 43
Types #154
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
Types #154
Conversation
cacf9bc
to
ff8b7da
Compare
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. |
@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 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 " |
@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 |
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 " I'll try to have most of that in the |
@alexsnaps no concerns |
e6f323f
to
0959ca9
Compare
@clarkmcc as part of this, I'm thinking of moving the module 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? |
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! |
Splitting this up, to keep steady progress and avoiding too much pain given the ongoing moves/restructuring. |
Thanks, I'll take a look at this most likely tomorrow. Got a lot on the plate today :) |
There was a problem hiding this 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
tocommon
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 withCelVal
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 |
63f226b
to
5ef5252
Compare
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>
@clarkmcc Anything left that you'd want me to address? |
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. |
@alexsnaps feel free to merge whenever you're ready |
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
ref.Val
equivalent traitVal
Val
uesFollow up PRs
stdlib
:Val
uesOptional
in Optional support... in the parser #137Optional
(if enabled)Mutable-
Lister
&Mapper