Add tuple types to CEL (literals, .N indexing, CStackList interop)#33
Merged
Conversation
Covers tuple literals, .N indexing, the in-place type-erased runtime representation on RawStack, and a generalized signature-matching mechanism for interop with user-supplied Rust operations.
Fold the leading paren into tuple_or_group instead of splitting it across two top-level alternatives — same five cases, easier to read.
Nine-task plan covering RawStack primitives (push_raw/repack/drop_sized), StackInfo/AssociatedType metadata, tuple construction and .N indexing, parser grammar, generalized tuple-shaped op registration, and the CStackList interop bridge.
…ngs for RawStack - Restore CRLF line endings in raw_stack.rs (previous commit accidentally rewrote the whole file to LF, inflating the diff). - Add #[allow(clippy::len_without_is_empty)] to RawStack::len (RawStack is stack machinery, not a general collection; an is_empty() method would be unused). - Document in push_raw's # Safety section that src must not overlap the stack's internal buffer, matching the copy_nonoverlapping precondition.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… primitives copy_from's safety doc was missing the dst-non-overlap clause (uses copy_nonoverlapping); copy_from/truncate_to/drop_sized had no debug_assert! for their arithmetic preconditions, unlike sibling push_raw. Addresses Task 2 review findings.
Forward-order copying could overwrite a later element's not-yet-read source bytes whenever dest_base > ambient_start (i.e. almost any tuple whose first element's alignment is smaller than a later element's), silently corrupting tuple contents. Reverse order is provably safe: given dest_base >= ambient_start and same-order src/dest layouts, each element's destination is always at or after every earlier element's source end. Adds a regression test that fails under the old ordering and passes under the fix.
…ing-padded tuples extract_tuple_element previously truncated past the tuple's own leading pad (using tuple_padding) and then re-pushed the extracted element, but tuple_index's parse-time bookkeeping computed the result's padding flag and stack_index relative to tuple_start (dest_base) instead of the true post-truncation base — a mismatch whenever the tuple had leading padding, corrupting subsequent pops/unwinds once the extracted element's alignment was smaller than the tuple's own alignment. Fix: never strip the tuple's own leading pad during indexing. tuple_base is already aligned for every element inside the tuple (the tuple's alignment is the max of all elements'), so truncating only down to tuple_base and re-pushing the target from there introduces no new padding — the extracted element simply inherits the tuple's own leading-padding relationship unchanged. Adds a regression test with an element whose alignment is smaller than the tuple's own, which fails under the old bookkeeping and passes now.
…adding tests Both tuple_layout_is_independent_of_ambient_stack_depth and tuple_index_result_inherits_leading_padding_when_element_align_is_smaller were asserting on the just-extracted element, whose own read doesn't depend on the padding/offset bookkeeping the tests exist to guard. Assert on the deeper (pre-tuple) sentinel instead, which does.
others_match indexed operand_type_ids[i] directly, which panics if a registered TupleOpSignature's operand_type_ids is shorter than num_operands at a non-tuple position — reachable by any external registrant (TupleOpSignature/register_tuple_op are pub, unlike the private, macro-constructed OpSignature). Use .get(i) so a missing entry simply fails to match instead of crashing the parser.
…ns-cell layout make_tuple and push_tuple computed a tuple's per-element offsets using a flat #[repr(C)] struct formula, but CStackList<H,T> is a *nested* cons cell where each level pads itself up to the max alignment of everything nested inside it so far before the next field is appended. For shapes where a higher-alignment element is followed by two or more lower-alignment elements (e.g. (u32, u8, u8): flat gives offsets [0,4,5]/size 8, CStackList's real layout gives [0,4,8]/size 12), the two formulas diverge, making pop_tuple_as/push_tuple relabel bytes at the wrong offsets — an out-of-bounds read/write only caught by a debug-only size assertion in pop_tuple_as, and not caught at all in push_tuple. Fix both offset-accumulation loops to additionally round the running offset up to the maximum alignment seen so far after placing each element, matching CStackList's real layout for every shape (not just non-decreasing-alignment ones). Leave RawStack::repack and the ambient (already-pushed) offset formula in make_tuple untouched, since neither depends on this convention. Adds round-trip tests through both bridge directions (push_tuple and pop_tuple_as) against a genuinely divergent (u32, u8, u8) shape, verified to fail under the old formula and pass under the fix.
Source text like .0.1 (intended as two chained tuple-index operations,
e.g. (t).0.1) tokenizes as Punct('.') followed by a single Lit::Float
token 0.1 — Rust's own lexer maximally munches the digits after the
second '.'. is_postfix_expression only handled Token::Literal(Int(..))
after a '.', so it rejected any chained index with "expected integer
after '.'", even though the grammar's repetition and the tuple design
spec both intend .0.1 to work.
Detect the float case and split its base10_digits() (e.g. "0.1") on the
decimal point back into two integer indices, applying each in sequence
via a new apply_tuple_index helper shared with the existing
single-index path. A suffix on the whole float token (e.g. 0.1i32)
is still rejected by the existing unsuffixed-integer rule.
…ed tuple index The chained .N.M indexing fix assumed a float token's base10_digits() always contains a '.', splitting on it via .expect(). Scientific notation (e.g. `1e2`) normalizes to digits with no '.' at all, so `t.1e2` panicked instead of returning a parse error -- a crash reachable from ordinary malformed script text, violating this crate's no-panics-on-malformed-input contract. Return a graceful ParseError instead, checked before consuming the token so the error span still points at it.
…age collect branches
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class tuple support to the CEL implementation, spanning parsing, runtime stack representation, tuple extraction/indexing, and operator dispatch/interop. It extends the existing DynSegment/RawStack model to support tuple-shaped values in-place (with metadata) and introduces parse-time .N indexing and tuple-operand signature matching for registered operations.
Changes:
- Add low-level
RawStackprimitives (push_raw, repack/drop/copy/truncate helpers) needed for building and tearing down tuple layouts in-place. - Extend
cel-runtimetype metadata (StackInfo/AssociatedType) and implement tuple construction/indexing plusCStackListrelabeling (push_tuple/pop_tuple_as). - Extend
cel-parserto parse tuple literals and.Nindexing (including chained indexing) and add tuple-shaped signature dispatch inOpLookup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-07-07-cel-tuples-design.md | Design spec documenting tuple grammar, runtime layout, indexing semantics, and interop goals. |
| docs/superpowers/plans/2026-07-07-cel-tuples.md | Step-by-step implementation plan and test strategy for the tuple feature set. |
| cel-runtime/src/raw_stack.rs | Adds raw byte push/copy/drop/truncate and repack primitives to support in-place tuple layout operations. |
| cel-runtime/src/dyn_segment.rs | Introduces tuple metadata, tuple construction/indexing ops, unwind refactor to size-based drops, and CStackList bridge APIs. |
| cel-parser/src/op_table.rs | Adds tuple-shaped operation signature registration and matching in OpLookup. |
| cel-parser/src/lib.rs | Adds tuple literal parsing and .N postfix indexing (including chained .N.M handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Updated the `push_raw` and `copy_from` methods to accept and operate on `MaybeUninit<u8>` instead of `u8` to handle potential uninitialized bytes safely. - Adjusted the corresponding tests to ensure compatibility with the new type. - Improved safety by ensuring that raw byte operations do not inadvertently read uninitialized memory.
…element extract_tuple_element left the tuple's leading alignment padding as dead, untracked bytes on the stack after tuple_index, so a later tuple literal built from the extracted result computed its element offsets against the wrong ambient start and could read garbage. Truncate past the tuple's own pad using its recorded padding flag, and recompute the extracted element's own padding flag fresh (relative to the recovered ambient offset) instead of inheriting the tuple's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pe, in join2 Every tuple shares the same erased DynTuple TypeId, so join2's plain type_id comparison accepted if/else (and short-circuit &&/||) branches whose tuple results had different arity or element types, silently merging incompatible layouts. Add a recursive shape comparison that inspects each tuple's associated element types (and nested tuples within them) instead of relying on the shared marker type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ab3e10e. Configure here.
…nt nested-shape gap Add a regression test confirming repack's ptr::copy-based relocation (without dropping the vacated source) never leaks or double-drops a non-Copy element, even when the element is actually moved rather than left in place — the same memmove pattern std itself uses (e.g. Vec::remove), sound because make_tuple's bookkeeping only ever tracks the new location afterward. Document TupleOpSignature's flat `shape` field: it has no way to express a nested tuple's inner arity/element types, so a nested-tuple operand position can only ever be recorded as "any tuple." No current caller relies on nested-shape precision; a real fix would need a recursive shape type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds tuple support to CEL: literal syntax (
(a, b, c),(x,)for 1-tuples),.Nfield indexing (including chained.N.M), and a generalized mechanism for registering Rust operations that take or return tuple-shaped values, bridging soundly to a concreteCStackList<...>type.docs/superpowers/specs/2026-07-07-cel-tuples-design.mddocs/superpowers/plans/2026-07-07-cel-tuples.mdTuples are represented as a type-erased, in-place layout directly on
RawStack— no heap allocation for the persistent representation. The internal layout matchesCStackList's real nested cons-cell layout exactly (not a flat#[repr(C)]struct — those differ whenever a higher-alignment element is followed by lower-alignment ones), so the interop bridge (pop_tuple_as/push_tuple) is a genuine zero-cost relabel, sound for every shape.Key design points
RawStackgainspush_raw,copy_from/drop_at/truncate_to/drop_sized, andrepack— primitives for building/tearing down tuples without touching the stack's other values.StackInfo/AssociatedTypewidened withsize/align/raw_dropperso tuple element metadata can describe offset, size, alignment, and an in-place dropper per element.DynSegment::make_tuple/tuple_indeximplement construction and indexing; indexing pops the whole tuple and pushes back just the extracted element (rather than leaving stale bytes behind), so it composes correctly with surrounding operations.OpLookupgains a generalization of the existing built-in operator dispatch (OpSignature/BuiltinScope) for tuple-shaped operands, viaTupleOpSignature/register_tuple_op.Process notes
Built via
superpowers:subagent-driven-development— 9 tasks, each implemented and independently reviewed by fresh subagents, plus a final whole-branch review. Two real, silently-corrupting bugs were caught and fixed during per-task review (a copy-ordering bug inrepack, and a padding/offset bookkeeping bug intuple_index), and the final whole-branch review caught a third, more fundamental issue — the erased-tuple layout didn't actually matchCStackList's real (nested, not flat) layout for divergent-alignment shapes — plus two smaller issues in the fix for that (a parsing gap and a panic on scientific-notation float tokens), all fixed and re-verified before this PR.Test plan
cargo test --workspace— all crates greencargo test --doc --workspace— all doctests greencargo clippy --workspace --exclude begin -- -D warnings— cleancargo clippy -p begin --no-default-features -- -D warnings— cleancargo fmt --all -- --check— clean🤖 Generated with Claude Code
Note
High Risk
Large changes to unsafe stack layout, repack/indexing, and parser evaluation paths; incorrect padding or drop order could corrupt evaluation or cause UB, though coverage is extensive.
Overview
Adds tuple literals and
.Nindexing to CEL:(a, b)/(x,)1-tuples vs(x)grouping, postfix.N(including chained indices when the lexer merges digits like.1.0), with parse-time arity and range checks.Runtime (
cel-runtime): Tuples live onRawStackas type-erasedDynTuplevalues with per-element metadata onStackInfo/AssociatedType(size,align,RawDropper). New stack ops includemake_tuple,tuple_index,pop_tuple_as/push_tupleforCStackListinterop, plusRawStack::repackand related raw push/copy/drop helpers. Stack byte offsets are derived frombase_stack_indexinstead of a cached index;join2compares tuple shapes, not only the sharedDynTupleTypeId.Parser (
cel-parser): Grammar updates fortuple_or_groupand postfix.integer indexing;OpLookupgainsTupleOpSignature/register_tuple_opfor tuple-shaped builtins.Also adds design/plan docs, optional
playgroundfeatures on several crates, a macro playground test, VS Codeplaygroundfeature for rust-analyzer, and a CLAUDE.md note that plaincargo build/testmust be warning-free.Reviewed by Cursor Bugbot for commit cac47b2. Bugbot is set up for automated code reviews on this repo. Configure here.