fix: Fixed iterator state handling in for...of loops to properly update and advance through iterations#37
Merged
TheUncharted merged 2 commits intomasterfrom Mar 21, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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)
1935-1947:⚠️ Potential issue | 🟠 MajorAdd resource-limit checks before building iterator state arrays.
Value::Array(vec![...])allocates in VM dispatch without an explicit pre-allocation resource check. Please calltrack_allocationbefore constructingnew_iterin both branches.Suggested patch
if idx < arr.len() { let value = arr[idx].clone(); + self.tracker.track_allocation(&self.limits)?; // Update iterator let new_iter = Value::Array(vec![items[0].clone(), Value::Int((idx + 1) as i64)]); // Push updated iterator back, then the value self.push(new_iter)?; self.push(value)?; } else { // Done — increment index past the end so IteratorDone sees idx > len + self.tracker.track_allocation(&self.limits)?; let new_iter = Value::Array(vec![items[0].clone(), Value::Int((idx + 1) as i64)]); self.push(new_iter)?; self.push(Value::Undefined)?; }As per coding guidelines, "Resource limits (memory, time, stack depth, allocation count) must be enforced during execution. Check resource limits before allocations in the VM dispatcher."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zapcode-core/src/vm/mod.rs` around lines 1935 - 1947, The VM is constructing iterator state arrays with Value::Array without checking resource limits; before creating new_iter in both branches (the "if idx < arr.len()" and the "else" branch) call track_allocation with the anticipated allocation size/count and/or check resource limits, then only construct Value::Array(vec![...]) and call self.push(new_iter) if track_allocation succeeded; update the code around the new_iter creation sites and ensure you reference the track_allocation result (and abort/return the appropriate error) before calling Value::Array and self.push.
🧹 Nitpick comments (1)
crates/zapcode-core/tests/async_await.rs (1)
1029-1183: Add an empty-iterablefor...ofsuspend/resume boundary test.Nice regression coverage for normal and multi-external flows. Please add one test for
for (const x of [])asserting immediateVmState::Complete(no suspension), to lock boundary behavior.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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zapcode-core/tests/async_await.rs` around lines 1029 - 1183, Add a new unit test (e.g., test_for_of_empty_array_no_suspend) that uses start_with_externals to run code with an empty iterable like `const items = []; let count = 0; for (const x of items) { count = count + 1; } count;` (use externals Vec::new()). After starting, assert the returned state is VmState::Complete and that the value equals Value::Int(0); do not attempt any resume calls. Reference start_with_externals, VmState::Complete, and Value::Int in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/zapcode-core/src/vm/mod.rs`:
- Around line 1935-1947: The VM is constructing iterator state arrays with
Value::Array without checking resource limits; before creating new_iter in both
branches (the "if idx < arr.len()" and the "else" branch) call track_allocation
with the anticipated allocation size/count and/or check resource limits, then
only construct Value::Array(vec![...]) and call self.push(new_iter) if
track_allocation succeeded; update the code around the new_iter creation sites
and ensure you reference the track_allocation result (and abort/return the
appropriate error) before calling Value::Array and self.push.
---
Nitpick comments:
In `@crates/zapcode-core/tests/async_await.rs`:
- Around line 1029-1183: Add a new unit test (e.g.,
test_for_of_empty_array_no_suspend) that uses start_with_externals to run code
with an empty iterable like `const items = []; let count = 0; for (const x of
items) { count = count + 1; } count;` (use externals Vec::new()). After
starting, assert the returned state is VmState::Complete and that the value
equals Value::Int(0); do not attempt any resume calls. Reference
start_with_externals, VmState::Complete, and Value::Int in the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c541ba2-ea7c-41fa-99ae-cfe317e764ce
📒 Files selected for processing (2)
crates/zapcode-core/src/vm/mod.rscrates/zapcode-core/tests/async_await.rs
Contributor
Benchmark Results |
TheUncharted
changed the title
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
1 participant
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
Bug Fixes
for...ofloops to properly update and advance through iterations.Tests
for...ofloops covering synchronous iteration, async/await scenarios, and multiple external function calls within loop bodies.