fix: flatten spread elements in array and object literals#53
Conversation
Spread was a silent no-op: [...a, x] produced a nested rather than a
flattened array, and {...o} underflowed the stack. Spread now pushes a
transient Value::Spread marker that CreateArray/CreateObject recognize
and flatten (arrays, strings, and objects; anything else is a TypeError).
The marker never crosses the public API boundary; the JS, Python, and
WASM bindings unwrap it defensively.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesSpread Value Implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Bytecode
participant Dispatch
participant Stack
participant Value
Bytecode->>Dispatch: Instruction::Spread
Dispatch->>Stack: pop top value
Dispatch->>Value: wrap in Value::Spread(Box::new(v))
Dispatch->>Stack: push Value::Spread
Bytecode->>Dispatch: Instruction::CreateArray(count)
Dispatch->>Stack: pop count items
Dispatch->>Value: detect Value::Spread, flatten array/string
Dispatch->>Stack: push Value::Array(arr)
Bytecode->>Dispatch: Instruction::CreateObject(count)
Dispatch->>Stack: pop count items
Dispatch->>Value: classify as Entry::Kv or Entry::Spread
Dispatch->>Stack: merge enumerables, push Value::Object
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zapcode-core/src/vm/mod.rs (1)
1393-1452: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame unbounded-growth concern applies to
CreateObjectspread merging.The single
track_allocationcheck at Line 1394 doesn't bound the number of keys inserted byEntry::Spreadin the object/array/string branches (Lines 1428-1445). Spreading a large object/array/string repeatedly can build an arbitrarily largeIndexMapbefore any further limit check occurs. Same fix pattern (checking allocation per inserted entry) applies here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zapcode-core/src/vm/mod.rs` around lines 1393 - 1452, The CreateObject path currently only calls tracker.track_allocation once before expanding spreads, so Entry::Spread can still grow the IndexMap without further bounds. Update the Instruction::CreateObject handling in vm::mod so each inserted property from Object, Array, and String spread branches also performs an allocation/limit check before inserting, using the same tracker/limits pattern already used elsewhere. Keep the fix localized to the CreateObject entry-merging logic and the Entry::Spread branches.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
crates/zapcode-core/tests/objects_arrays.rs (1)
229-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing edge-case and error-path tests for spread.
The new tests only cover the positive path (flatten, multiple spreads, object merge, override precedence). There's no test for:
- Spreading a non-iterable value into an array (should trigger
ZapcodeError::TypeErrorper theCreateArraydispatch logic).- Empty-source spread (e.g.
[...[]],{...{}}).- Spreading a string into an object (character-indexed keys).
As per coding guidelines, "Every language feature must have a positive test (correct execution), edge case tests (boundary conditions, empty inputs), and a sandbox escape test where applicable."
✅ Suggested additional tests
#[test] fn test_array_spread_non_iterable_errors() { let err = eval_ts("const x = 5; [...x]").unwrap_err(); assert!(err.to_string().contains("not iterable")); } #[test] fn test_array_spread_empty() { let r = eval_ts("[...[]].length").unwrap(); assert_eq!(r, Value::Int(0)); } #[test] fn test_object_spread_string() { let r = eval_ts("const o = { ...'ab' }; o['0'] + o['1']").unwrap(); assert_eq!(r, Value::String("ab".into())); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zapcode-core/tests/objects_arrays.rs` around lines 229 - 255, The spread tests in objects_arrays.rs only cover happy paths, so add edge-case coverage around the existing array/object spread assertions. Extend the test module with cases for CreateArray dispatch failures when spreading a non-iterable into an array, empty-source spreads like [...[]] and {...{}}, and object spread from a string to verify character-indexed keys. Use the existing eval_ts-based tests such as test_array_spread_flattens and test_object_spread as anchors for where to place the new assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/zapcode-core/src/vm/mod.rs`:
- Around line 1364-1392: The CreateArray path in the VM dispatcher only checks
allocation limits once before flattening, so spread expansion can build an
arbitrarily large Vec in host memory. Update Instruction::CreateArray in
vm::mod.rs to enforce resource limits incrementally during the flattening loop,
especially in the Value::Spread branches that handle Array and String, by
checking the tracker before or as items are appended. Keep the final
self.push(Value::Array(arr)) call, but do not rely on it as the only safeguard;
the limit must be applied while constructing arr.
---
Outside diff comments:
In `@crates/zapcode-core/src/vm/mod.rs`:
- Around line 1393-1452: The CreateObject path currently only calls
tracker.track_allocation once before expanding spreads, so Entry::Spread can
still grow the IndexMap without further bounds. Update the
Instruction::CreateObject handling in vm::mod so each inserted property from
Object, Array, and String spread branches also performs an allocation/limit
check before inserting, using the same tracker/limits pattern already used
elsewhere. Keep the fix localized to the CreateObject entry-merging logic and
the Entry::Spread branches.
---
Nitpick comments:
In `@crates/zapcode-core/tests/objects_arrays.rs`:
- Around line 229-255: The spread tests in objects_arrays.rs only cover happy
paths, so add edge-case coverage around the existing array/object spread
assertions. Extend the test module with cases for CreateArray dispatch failures
when spreading a non-iterable into an array, empty-source spreads like [...[]]
and {...{}}, and object spread from a string to verify character-indexed keys.
Use the existing eval_ts-based tests such as test_array_spread_flattens and
test_object_spread as anchors for where to place the new assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 769afd32-d06d-49ed-b6a0-01a567d35714
📒 Files selected for processing (7)
crates/zapcode-core/src/value.rscrates/zapcode-core/src/vm/builtins.rscrates/zapcode-core/src/vm/mod.rscrates/zapcode-core/tests/objects_arrays.rscrates/zapcode-js/src/lib.rscrates/zapcode-py/src/lib.rscrates/zapcode-wasm/src/lib.rs
Value semantics make spread an amplifier: [...a, ...a] doubles the payload for O(1) stack pushes, so the single up-front track_allocation in CreateArray/CreateObject could not bound the flattened result. Every element flattened into an array and every entry merged into an object now ticks the allocation tracker. Adds two sandbox-escape regression tests (array doubling loop and string-into-object spread under a low max_allocations) plus spread edge-case tests: non-iterable spread TypeError, empty sources, and string-into-object char-index keys.
1 participant
Summary
Spread is listed as supported (AGENTS.md), but
[...a, x]silently produced a non-flattened result and{...o}underflowed the stack. Silent wrong answers are the worst failure mode for agent-generated code — the agent gets no signal that anything went wrong and continues on corrupted data.Changes
Value::Spreadmarker;CreateArray/CreateObjectrecognize and flatten it (arrays, strings, and objects; spreading anything else is aTypeError).tests/objects_arrays.rs.Test plan
cargo test)make lintclean across the workspace (all binding crates compile)Related issues
None open. We run this patch in production via ex_zapcode.
Summary by CodeRabbit
New Features
Bug Fixes
Tests