Add for-in compatibility flag#697
Open
frostney wants to merge 4 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements JavaScript Changesfor...in Loop Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite TimingTest Runner (interpreted: 9,909 passed; bytecode: 9,909 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 8 improved · 🔴 395 regressed · 4 unchanged · avg -21.6% arraybuffer.js — Interp: 🔴 13, 1 unch. · avg -20.9% · Bytecode: 🟢 1, 🔴 11, 2 unch. · avg -8.5%
arrays.js — Interp: 🔴 19 · avg -25.0% · Bytecode: 🔴 19 · avg -10.6%
async-await.js — Interp: 🔴 6 · avg -22.7% · Bytecode: 🔴 4, 2 unch. · avg -7.4%
async-generators.js — Interp: 🔴 2 · avg -24.4% · Bytecode: 🔴 1, 1 unch. · avg -9.8%
base64.js — Interp: 🔴 10 · avg -22.2% · Bytecode: 🟢 7, 🔴 2, 1 unch. · avg +1.4%
classes.js — Interp: 🔴 31 · avg -20.3% · Bytecode: 🔴 26, 5 unch. · avg -7.7%
closures.js — Interp: 🔴 11 · avg -19.3% · Bytecode: 🔴 11 · avg -11.4%
collections.js — Interp: 🔴 12 · avg -19.1% · Bytecode: 🔴 9, 3 unch. · avg -6.6%
csv.js — Interp: 🔴 13 · avg -20.5% · Bytecode: 🔴 13 · avg -11.6%
destructuring.js — Interp: 🔴 22 · avg -23.2% · Bytecode: 🔴 22 · avg -10.6%
fibonacci.js — Interp: 🔴 8 · avg -21.1% · Bytecode: 🔴 8 · avg -12.8%
float16array.js — Interp: 🔴 32 · avg -22.0% · Bytecode: 🟢 1, 🔴 28, 3 unch. · avg -7.0%
for-of.js — Interp: 🔴 7 · avg -26.7% · Bytecode: 🔴 7 · avg -12.4%
generators.js — Interp: 🔴 4 · avg -16.3% · Bytecode: 🔴 4 · avg -7.0%
iterators.js — Interp: 🔴 42 · avg -20.4% · Bytecode: 🔴 42 · avg -10.4%
json.js — Interp: 🔴 20 · avg -19.9% · Bytecode: 🔴 19, 1 unch. · avg -7.1%
jsx.jsx — Interp: 🔴 21 · avg -22.7% · Bytecode: 🔴 21 · avg -10.0%
modules.js — Interp: 🔴 9 · avg -23.8% · Bytecode: 🔴 9 · avg -16.2%
numbers.js — Interp: 🔴 11 · avg -20.0% · Bytecode: 🔴 10, 1 unch. · avg -6.8%
objects.js — Interp: 🔴 7 · avg -21.4% · Bytecode: 🔴 7 · avg -9.1%
promises.js — Interp: 🔴 12 · avg -22.2% · Bytecode: 🔴 12 · avg -8.2%
regexp.js — Interp: 🔴 11 · avg -22.3% · Bytecode: 🟢 6, 🔴 3, 2 unch. · avg +3.6%
strings.js — Interp: 🔴 19 · avg -21.7% · Bytecode: 🔴 19 · avg -10.0%
tsv.js — Interp: 🔴 9 · avg -25.5% · Bytecode: 🔴 9 · avg -10.7%
typed-arrays.js — Interp: 🟢 1, 🔴 20, 1 unch. · avg -28.1% · Bytecode: 🟢 12, 🔴 7, 3 unch. · avg +22.1%
uint8array-encoding.js — Interp: 🟢 3, 🔴 15 · avg -15.1% · Bytecode: 🔴 17, 1 unch. · avg -25.2%
weak-collections.js — Interp: 🟢 4, 🔴 9, 2 unch. · avg -17.8% · Bytecode: 🔴 13, 2 unch. · avg -17.8%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+216 / -6)Newly failing (6):
Newly passing (216):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/language/for-in-loop/control-flow.js (1)
24-30: ⚡ Quick winAdd a
returncontrol-flow case.This suite covers
breakandcontinue, but notreturn, which is another dedicated path in the newfor...inexecution logic. A small helper test here would guard both interpreter and bytecode propagation.➕ Suggested test
test("closures capture per-iteration lexical bindings", () => { const fns = []; for (const key in { a: 1, b: 2, c: 3 }) { fns.push(() => key); } expect(fns.map(fn => fn())).toEqual(["a", "b", "c"]); }); + +test("return exits the loop and propagates the value", () => { + const run = () => { + for (const key in { a: 1, b: 2, c: 3 }) { + if (key === "b") return key; + } + return "miss"; + }; + expect(run()).toBe("b"); +});🤖 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 `@tests/language/for-in-loop/control-flow.js` around lines 24 - 30, Add a small test that verifies the `return` control-flow path inside a `for...in` loop also preserves per-iteration lexical bindings: replicate the existing test pattern (the `test("closures capture per-iteration lexical bindings", ...)` setup with `fns` and a `for (const key in {...})`) but include a `return` from within the loop on a specific key so the loop exits early and ensure the pushed closure still captures the correct per-iteration `key` values for iterations executed before the return; update or add a new test name to reflect the `return` case so the interpreter and bytecode propagation for `return` are exercised.tests/language/statements/unsupported-features.js (1)
19-31: ⚡ Quick winAdd the skipped destructuring-head case.
The parser has a separate
for...inbinding-pattern path, but this negative coverage only exercises identifier and assignment targets. A destructuring head here would protect the other compat-gated syntax path too.➕ Suggested test
test("code after skipped for-in assignment target loop executes", () => { let x = 1; let key; for (key in { a: 1 }) { x = 99; } expect(x).toBe(1); expect(key).toBe(undefined); }); + + test("code after skipped destructuring for-in loop executes", () => { + let x = 1; + for (const [key] in { ab: 1 }) { x = 99; } + expect(x).toBe(1); + expect(typeof key).toBe("undefined"); + });🤖 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 `@tests/language/statements/unsupported-features.js` around lines 19 - 31, Add a new test that covers the skipped destructuring-head path: mirror the existing for-in negative tests but use a binding-pattern in the loop head (e.g., test name "code after skipped for-in destructuring-head executes" and a loop like for (const [k] in { a: 1 }) { x = 99; }), assert x stays 1 after the loop to ensure the binding-pattern path in the parser is exercised (reference the existing tests "code after skipped for-in loop executes" and "code after skipped for-in assignment target loop executes" to match style and expectations).
🤖 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 `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 2681-2688: The code only treats a for-in loop's BindingName as a
write when ForIn.IsVar is true, which lets TryCompileCountedFor incorrectly keep
the counted-for fast path; change the check so any for-in loop that binds the
identifier is considered a definite write: in the TGocciaForInStatement branch,
replace the condition "if (ForIn.BindingName = AName) and ForIn.IsVar then
Exit(True)" with a check that treats BindingName match unconditionally (e.g., if
ForIn.BindingName = AName then Exit(True)); keep the existing calls to
ForBodyAssignsIdentifier(ForIn.ObjectExpression, AName) and
ForBodyAssignsIdentifier(ForIn.Body, AName) unchanged.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 2056-2083: The loop state (EntriesArray/EntryIndex/CurrentValue)
is only saved after AssignPattern runs, but AssignPattern can trigger
EGocciaGeneratorYield (computed names/defaults), so a yielded generator will
restart the for...in header; to fix, mirror EvaluateForOf's two-phase head
handling: call Continuation.SaveLoopState with the simple iteration state
(EntriesArray, EntryIndex, CurrentValue) before performing
binding/destructuring, then after the binding finishes replace/update the saved
state with the scopeful state (IterScope/IterContext.Scope/whatever
Continuation.SaveLoopState expects for the per-iteration scope). Ensure you move
the initial SaveLoopState call to just after computing CurrentValue and before
AssignPattern, and keep the existing SaveLoopState call (or update it) to store
the final scopeful state once bindings complete.
- Around line 1910-1935: CreateForInEntriesArray currently allocates Result,
calls ToObject (which may box a primitive) and then allocates EntryObj and key
values without rooting them, allowing a re-entrant GC to reclaim partially built
values; fix by temp-rooting the snapshot objects during the build: ensure Result
and the boxed Obj (result of ToObject) are placed on the GC rootset before
entering the loop and that each newly created EntryObj and its key/value are
rooted (or created within a TempRoot scope) until they are safely stored into
Result.Elements; update CreateForInEntriesArray to push/pop these temporaries
(or use the unit's TempRoot helper) around allocations so the GC cannot collect
them mid-construction.
In `@source/units/Goccia.Parser.pas`:
- Around line 4466-4495: The parser currently fails when a for...in target is a
private member because ConvertToPattern lacks a branch for
TGocciaPrivateMemberExpression; update ConvertToPattern to recognize
TGocciaPrivateMemberExpression and return a valid assignment/pattern node (or
wrap it as a simple assignment target) instead of raising "Invalid destructuring
target", and ensure any pattern validation paths (used by ConvertToPattern and
the for-in handling in the for/in parsing code that calls AssignmentTarget :=
ConvertToPattern(TargetExpr)) allow private-member targets; also verify
TGocciaForInStatement creation (and any downstream destructuring/assignment
logic) accepts the resulting target type so constructs like for (this.#x in obj)
{} parse and compile without error.
In `@source/units/Goccia.Values.ObjectValue.pas`:
- Around line 1116-1134: The traversal over Prototype in the loop starting with
Current := Self needs the same MAX_PROTOTYPE_CHAIN_DEPTH guard used elsewhere to
detect cycles or excessive depth; add a depth counter (e.g., Depth := 0) that
increments each iteration and if Depth >= MAX_PROTOTYPE_CHAIN_DEPTH stop the
loop and fail fast (raise the same exception or error used elsewhere in this
unit) to avoid infinite loops when Prototype is cyclic. Apply this guard inside
the while Assigned(Current) loop before calling
Current.GetAllPropertyNames/GetOwnPropertyDescriptor and reference Current,
Prototype, MAX_PROTOTYPE_CHAIN_DEPTH, Visited, and Names when implementing the
check so it matches the existing pattern in the unit.
---
Nitpick comments:
In `@tests/language/for-in-loop/control-flow.js`:
- Around line 24-30: Add a small test that verifies the `return` control-flow
path inside a `for...in` loop also preserves per-iteration lexical bindings:
replicate the existing test pattern (the `test("closures capture per-iteration
lexical bindings", ...)` setup with `fns` and a `for (const key in {...})`) but
include a `return` from within the loop on a specific key so the loop exits
early and ensure the pushed closure still captures the correct per-iteration
`key` values for iterations executed before the return; update or add a new test
name to reflect the `return` case so the interpreter and bytecode propagation
for `return` are exercised.
In `@tests/language/statements/unsupported-features.js`:
- Around line 19-31: Add a new test that covers the skipped destructuring-head
path: mirror the existing for-in negative tests but use a binding-pattern in the
loop head (e.g., test name "code after skipped for-in destructuring-head
executes" and a loop like for (const [k] in { a: 1 }) { x = 99; }), assert x
stays 1 after the loop to ensure the binding-pattern path in the parser is
exercised (reference the existing tests "code after skipped for-in loop
executes" and "code after skipped for-in assignment target loop executes" to
match style and expectations).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 296a26c2-24fc-432c-b425-9e5ff6af290f
📒 Files selected for processing (31)
README.mddocs/build-system.mddocs/decision-log.mddocs/embedding.mddocs/errors.mddocs/goals.mddocs/language-tables.mddocs/language.mddocs/test262.mddocs/tutorial.mdscripts/run_test262_suite.tsscripts/test-cli.tsscripts/test262_compatibility_roadmap.jsonsource/app/Goccia.CLI.Options.passource/units/Goccia.AST.BindingPatterns.passource/units/Goccia.AST.Statements.passource/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.SourcePipeline.passource/units/Goccia.VM.passource/units/Goccia.Values.ObjectValue.pastests/language/for-in-loop/basic-enumeration.jstests/language/for-in-loop/control-flow.jstests/language/for-in-loop/goccia.jsontests/language/for-in-loop/var/goccia.jsontests/language/for-in-loop/var/var-shared-binding.jstests/language/statements/unsupported-features.js
💤 Files with no reviewable changes (1)
- scripts/test262_compatibility_roadmap.json
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
--compat-for-in-loop/"compat-for-in-loop"/cfForInas a standalone JavaScript compatibility flag forfor...inloops.for...intests.Testing
Commands run:
./build.pas loader testrunner./build.pas loaderbare benchmarkrunner bundler repl./build/GocciaTestRunner tests/language/for-in-loop./build/GocciaTestRunner tests/language/for-in-loop --mode=bytecode./build/GocciaTestRunner testsbun scripts/test-cli.tsbun scripts/run_test262_suite.ts --helpGocciaScriptLoaderBare --compat-for-in-loopstdin smoke./format.pas --checkgit diff --checkNot run: full external test262 suite.