Add --compat-traditional-for-loop opt-in flag#531
Merged
Conversation
Adds traditional C-style for(init; test; update) loops behind a new compatibility flag, mirroring the existing --compat-var / --compat-function posture. let/const declarations in for-init create per-iteration lexical environments per ES2026 §14.7.4.4 so closure capture pins per iteration. var declarations require both --compat-var and the new flag, hoist out of the loop, and share a single binding. Bytecode mode includes a counted-loop fast path mirroring CompileCountedForOf for for(let i = N; i <op> M; i++|i--) shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (2)
📝 WalkthroughWalkthroughThis PR adds opt-in traditional C-style for-loop support ( ChangesTraditional For-Loop Support
Built-in ECMAScript Enhancements
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI App<br/>(e.g., Bundler)
participant Config as Config<br/>Resolution
participant Parser as Parser
participant Compiler as Compiler
participant Evaluator as Evaluator
participant Engine as Engine
User->>CLI: --compat-traditional-for-loop
CLI->>Config: Resolve per-file/root/CLI flags
Config->>Engine: Set TraditionalForLoopsEnabled
Engine->>Parser: Parser.TraditionalForLoopsEnabled := true
User->>CLI: Source code with for(;;)
CLI->>Parser: ParseSource(...)
Parser->>Parser: LooksLikeTraditionalForHeader?
alt Traditional For Header
Parser->>Parser: ParseTraditionalForBody()
Parser->>Parser: Create TGocciaForStatement
else Not Traditional For
Parser->>Parser: Emit compatibility warning
end
CLI->>Compiler: CompileForStatement(AStmt)
Compiler->>Compiler: TryCompileCountedFor?
alt Counted For Pattern
Compiler->>Compiler: Establish per-iteration scope
Compiler->>Compiler: Setup init/condition/update
Compiler->>Compiler: Emit bytecode with disposal
else Generic For Loop
Compiler->>Compiler: Fall back to existing logic
end
User->>Engine: Execute bytecode
Engine->>Evaluator: EvaluateFor(ForStatement, Context)
Evaluator->>Evaluator: Setup lexical environment
loop Per Iteration
Evaluator->>Evaluator: Create per-iteration scope
Evaluator->>Evaluator: Evaluate condition
alt Condition true
Evaluator->>Evaluator: Execute body
Evaluator->>Evaluator: Evaluate update
else Exit
Evaluator->>Evaluator: Break loop
end
end
Evaluator-->>Engine: ControlFlow result
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 |
coderabbitai
Bot
added
documentation
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 16 improved · 🔴 223 regressed · 168 unchanged · avg -2.5% arraybuffer.js — Interp: 🔴 10, 4 unch. · avg -3.9% · Bytecode: 🟢 9, 🔴 2, 3 unch. · avg +4.2%
arrays.js — Interp: 🔴 11, 8 unch. · avg -3.8% · Bytecode: 🟢 14, 5 unch. · avg +5.1%
async-await.js — Interp: 🔴 2, 4 unch. · avg -3.6% · Bytecode: 🟢 1, 5 unch. · avg +2.0%
async-generators.js — Interp: 🔴 1, 1 unch. · avg -3.3% · Bytecode: 2 unch. · avg -2.2%
base64.js — Interp: 🔴 6, 4 unch. · avg -3.2% · Bytecode: 🟢 8, 2 unch. · avg +3.9%
classes.js — Interp: 🔴 20, 11 unch. · avg -3.7% · Bytecode: 🟢 14, 🔴 2, 15 unch. · avg +2.9%
closures.js — Interp: 🔴 4, 7 unch. · avg -1.3% · Bytecode: 🟢 5, 🔴 1, 5 unch. · avg +2.9%
collections.js — Interp: 🟢 7, 🔴 2, 3 unch. · avg +2.6% · Bytecode: 🟢 10, 2 unch. · avg +6.2%
csv.js — Interp: 🔴 7, 6 unch. · avg -3.5% · Bytecode: 🟢 12, 1 unch. · avg +9.0%
destructuring.js — Interp: 🔴 14, 8 unch. · avg -3.9% · Bytecode: 🟢 7, 🔴 1, 14 unch. · avg +3.4%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -3.2% · Bytecode: 🟢 4, 4 unch. · avg +4.2%
float16array.js — Interp: 🔴 7, 25 unch. · avg -1.7% · Bytecode: 🟢 17, 🔴 5, 10 unch. · avg +1.1%
for-of.js — Interp: 🔴 1, 6 unch. · avg -0.7% · Bytecode: 🔴 2, 5 unch. · avg -1.2%
generators.js — Interp: 🔴 4 · avg -3.7% · Bytecode: 🟢 3, 1 unch. · avg +5.8%
iterators.js — Interp: 🔴 20, 22 unch. · avg -2.8% · Bytecode: 🟢 5, 🔴 13, 24 unch. · avg -1.1%
json.js — Interp: 🔴 6, 14 unch. · avg -1.9% · Bytecode: 🟢 20 · avg +12.3%
jsx.jsx — Interp: 🔴 20, 1 unch. · avg -5.0% · Bytecode: 🟢 17, 4 unch. · avg +7.2%
modules.js — Interp: 🔴 5, 4 unch. · avg -4.0% · Bytecode: 🟢 9 · avg +14.3%
numbers.js — Interp: 🔴 6, 5 unch. · avg -3.1% · Bytecode: 🟢 10, 1 unch. · avg +7.2%
objects.js — Interp: 🔴 3, 4 unch. · avg -1.9% · Bytecode: 🟢 7 · avg +10.5%
promises.js — Interp: 🔴 11, 1 unch. · avg -4.0% · Bytecode: 🟢 7, 5 unch. · avg +5.7%
regexp.js — Interp: 🔴 9, 2 unch. · avg -4.2% · Bytecode: 🟢 11 · avg +6.1%
strings.js — Interp: 🔴 15, 4 unch. · avg -4.7% · Bytecode: 🟢 19 · avg +9.9%
tsv.js — Interp: 🔴 5, 4 unch. · avg -3.8% · Bytecode: 🟢 9 · avg +10.2%
typed-arrays.js — Interp: 🔴 15, 7 unch. · avg -13.9% · Bytecode: 🟢 15, 🔴 5, 2 unch. · avg +23.5%
uint8array-encoding.js — Interp: 🟢 3, 🔴 9, 6 unch. · avg +5.0% · Bytecode: 🟢 2, 🔴 15, 1 unch. · avg -24.2%
weak-collections.js — Interp: 🟢 6, 🔴 4, 5 unch. · avg +17.1% · Bytecode: 🟢 13, 2 unch. · avg +30.1%
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
Suite TimingTest Runner (interpreted: 8,667 passed; bytecode: 8,667 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
test262 Conformance
Areas closest to 100%
Per-test deltas (+359 / -353)Newly failing (353):
Newly passing (359):
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
scripts/test-cli-config.ts (1)
611-629: ⚡ Quick winAdd an explicit bytecode assertion for the flag-off no-op path.
This block validates default mode only. Adding the same check with
--mode=bytecodewould close a small regression gap for disabled behavior.💡 Suggested test addition
const stderr = await $`${LOADER} ${join(tmp, "no-flag.js")} 2>&1`.text(); if (!stderr.includes("Traditional 'for(;;)' loops are not supported")) { throw new Error(`Loader without flag should emit warning, got: ${stderr}`); } if (stderr.includes("should not run")) { throw new Error(`Loader without flag should not execute the loop body, got: ${stderr}`); } + + const stderrBc = await $`${LOADER} ${join(tmp, "no-flag.js")} --mode=bytecode 2>&1`.text(); + if (!stderrBc.includes("Traditional 'for(;;)' loops are not supported")) { + throw new Error(`Bytecode loader without flag should emit warning, got: ${stderrBc}`); + } + if (stderrBc.includes("should not run")) { + throw new Error(`Bytecode loader without flag should not execute the loop body, got: ${stderrBc}`); + }🤖 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 `@scripts/test-cli-config.ts` around lines 611 - 629, Add a parallel assertion that runs the same "no-flag.js" scenario with bytecode mode to ensure the default no-op behavior is preserved under --mode=bytecode: after the existing block that creates no-flag.js and invokes `${LOADER} ${join(tmp, "no-flag.js")}`, add a try/finally scoped invocation using `${LOADER} --mode=bytecode ${join(tmp, "no-flag.js")} 2>&1` (use the same tmp, makeTmp/clean, writeFileSync helpers) and assert the stderr includes "Traditional 'for(;;)' loops are not supported" and does NOT include "should not run" (throw errors with clear messages on failure), mirroring the checks in the existing block so the bytecode-disabled path is explicitly tested.
🤖 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 2143-2145: The synthetic iteration locals created with
ACtx.Scope.BeginScope / DeclareLocal (e.g., the pattern StartReg :=
ACtx.Scope.DeclareLocal(LoopName, False) followed by EmitInstruction(...
OP_LOAD_INT ...)) must copy the original header binding's type metadata and
flags so ResolveLocal sees the same TypeHint, strictness, array-typing and
annotations as the loop variable declared in the header; update the
synthetic-local creation to locate the header binding (the variable declared by
the loop header / CompileVariableDeclaration), and transfer its TypeHint,
strictness/flags, array typing and any annotations onto the new scope entry
after DeclareLocal, and apply the same fix at the other synthetic-binding sites
referenced (the other blocks using the same DeclareLocal pattern around
iteration creation and the counted fast-path) so type-driven
checks/optimizations continue to apply.
- Around line 2153-2158: The fast-path currently snapshots the loop bound by
calling ACtx.CompileExpression(CondExpr.Right, LimitReg) once before entering
the loop (see LimitReg, ACtx.CompileExpression, CondExpr.Right), which breaks
semantics when the RHS has side effects or is mutable; fix by either moving the
ACtx.CompileExpression(CondExpr.Right, LimitReg) into the loop body so the RHS
is re-evaluated each iteration, or add a guard before selecting the fast path
that only allows it when CondExpr.Right is provably invariant (e.g.,
literal/constant or a syntactic pure expression) and fall back to the safe path
otherwise. Ensure any change references LimitReg, OneReg, CmpReg and preserves
register allocation semantics and deallocation in ACtx.Scope.
- Around line 2093-2099: The early-return condition currently exits for both
IsConst and IsVarKeyword, preventing `var` loops from reaching the per-iteration
(lexical) fallback; change the conditional so only const immediately Exit (keep
"if IsConst then Exit") and remove IsVarKeyword from this unconditional
fast-path rejection, allowing the later code that builds per-iteration bindings
for non-IsVar to run and for compatibility flags to decide whether `var` should
be treated like `let`; apply the same fix in the second occurrence around the
2363-2384 region so both sites use IsConst-only for the early Exit and let the
downstream logic handle IsVarKeyword/compat flags.
- Around line 2184-2206: Create a dedicated per-iteration cleanup target
immediately before the ACtx.Scope.EndScope(...) / OP_CLOSE_UPVALUE loop (i.e.,
place an "iteration cleanup" label right where ClosedLocals/ClosedCount are
processed) and change the BreakJumps patching so PatchJumpTarget(ACtx,
BreakJumps[I]) targets that iteration-cleanup label instead of the final
ExitJump; then let the cleanup code run EndScope and emit the OP_CLOSE_UPVALUE
loop and only after that jump/patch to ExitJump as before—apply the same change
to the other occurrence around lines 2454-2487. This uses PatchJumpTarget,
BreakJumps, ExitJump, ACtx.Scope.EndScope and the OP_CLOSE_UPVALUE/ClosedLocals
loop as the anchor points.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1936-2009: The classic for-loop evaluator (EvaluateFor) must
persist and restore loop phase and per-iteration bindings across generator
suspension so Init/Condition/Body/Update aren't re-run; before any call that may
yield (calls to EvaluateStatement(EvaluatEExpression) and
EvaluateLoopBodyStatement) capture a suspension record containing the current
loop phase (InitDone/Condition/Body/Update), the IterContext/HeaderContext
references, and the PerIterNames' values (PrevValues) and any binding type
hints, and store it on the current execution frame; on resume detect and consume
that record to restore IterScope/HeaderScope and PrevValues and jump to the
correct phase so you skip Init if already completed and continue at
Condition/Body/Update as appropriate (update the code paths around
AForStatement.Init, AForStatement.Condition, EvaluateLoopBodyStatement, and
AForStatement.Update to save before calls that may yield and to restore before
proceeding).
- Around line 1993-2008: The current loop copies iteration bindings into
HeaderScope before executing the for-loop Update, causing closures created in
Update to close over the shared header slot and allowing evaluator write-back to
fail; change the order so EvaluateExpression(AForStatement.Update, IterContext)
is executed while the per-iteration bindings remain in IterScope (use
IterContext), then after Update completes force-copy each PerIterNames binding's
final value from IterScope.GetBinding(Name).Value back into HeaderScope via a
direct copy/AssignBinding to update the header slot for the next iteration; keep
the IsLexical check around the per-iteration binding handling but ensure Update
is always evaluated in IterContext and the write-back occurs only after
EvaluateExpression returns.
In `@source/units/Goccia.Parser.pas`:
- Around line 4020-4022: The parser currently gates recognizing a gttVar token
in traditional for init on FVarDeclarationsEnabled; change the branch condition
around Check(gttLet)/Check(gttConst) so that Check(gttVar) is accepted
unconditionally (remove the FVarDeclarationsEnabled guard) so `for (var ... )`
is parsed rather than skipped; ensure you make the same change in the analogous
block handling lines 4087-4094 and leave the later compatibility/hoisting
decision to the code that inspects the declaration kind after parsing (do not
emit the compat warning or skip parsing here).
- Around line 232-233: The TraditionalForLoopsEnabled flag added as the property
(FTraditionalForLoopsEnabled / TraditionalForLoopsEnabled) must be propagated
into any nested/child parser instances created inside
ParseInterpolationExpression and the template call sites; update
ParseInterpolationExpression where it constructs child parsers (copying
ASI/var/function flags) to also copy FTraditionalForLoopsEnabled into the new
parser instance, and likewise pass FTraditionalForLoopsEnabled from the tagged
and non-tagged template call sites so nested template expressions inherit the
traditional-for-loops setting.
---
Nitpick comments:
In `@scripts/test-cli-config.ts`:
- Around line 611-629: Add a parallel assertion that runs the same "no-flag.js"
scenario with bytecode mode to ensure the default no-op behavior is preserved
under --mode=bytecode: after the existing block that creates no-flag.js and
invokes `${LOADER} ${join(tmp, "no-flag.js")}`, add a try/finally scoped
invocation using `${LOADER} --mode=bytecode ${join(tmp, "no-flag.js")} 2>&1`
(use the same tmp, makeTmp/clean, writeFileSync helpers) and assert the stderr
includes "Traditional 'for(;;)' loops are not supported" and does NOT include
"should not run" (throw errors with clear messages on failure), mirroring the
checks in the existing block so the bytecode-disabled path is explicitly tested.
🪄 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: 28a9db76-8768-438b-9a89-a257dc1ea3e2
📒 Files selected for processing (30)
docs/decision-log.mddocs/goals.mddocs/language-tables.mddocs/language.mdscripts/test-cli-config.tssource/app/Goccia.CLI.Application.passource/app/GocciaBenchmarkRunner.dprsource/app/GocciaBundler.dprsource/app/GocciaREPL.dprsource/app/GocciaScriptLoader.dprsource/app/GocciaTestRunner.dprsource/shared/CLI.Options.passource/units/Goccia.AST.BindingPatterns.passource/units/Goccia.AST.Statements.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.Interpreter.passource/units/Goccia.Modules.Loader.passource/units/Goccia.Parser.pastests/language/for-loop/basic-counter.jstests/language/for-loop/break-continue.jstests/language/for-loop/destructuring-init.jstests/language/for-loop/empty-parts.jstests/language/for-loop/goccia.jsontests/language/for-loop/let-per-iteration.jstests/language/for-loop/return-from-loop.jstests/language/for-loop/with-var/goccia.jsontests/language/for-loop/with-var/var-shared-binding.js
The bare loader has its own option-parsing and engine-config path, so the flag has to be threaded through it explicitly (mirrors the existing --compat-var / --compat-function plumbing). Without this, test262 (which runs through the bare loader with --compat-all) was not picking up the flag and only one extra traditional-for test passed across the suite. With the wiring, language/statements/for/ goes from 60/385 to 325/385 in bytecode mode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TryCompileCountedFor now bails when the init has a type annotation or the condition RHS isn't a literal/identifier the body doesn't reassign; the spec re-evaluates the test each iteration, so snapshotting a side-effecting or mutable RHS once is wrong (#531 review). - EvaluateFor's iteration→header sync now uses ForceUpdateBinding so a body that runs to completion under `for (const ...)` doesn't trip the evaluator's own writability check on the next iteration's snapshot. - ParseInterpolationExpression threads TraditionalForLoopsEnabled into child parsers so an IIFE inside a template `${...}` can use traditional for when the surrounding source has it enabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 5, 2026
Engine: bytecode break skips per-iteration OP_CLOSE_UPVALUE in for-of and traditional for loops
#537
Closed
Fixes a regression caused by my own dispatch — `for (lhs of expr)` heads
where `lhs` is an assignment target (not a declaration) were previously
warned-and-skipped, but now fell through to ParseTraditionalForBody and
syntax-errored on `Expected ';'` before the `of`. Same shape:
`for ([a, b] of arr)`, `for ({x} of objs)`, etc. Several test262
suites use these forms in harness/feature tests.
LooksLikeTraditionalForHeader is a pure peek that returns True only when
a top-depth ';' exists inside the for-header parens. for-of/for-in
heads contain no top-level ';', so they fall back to the
warn-and-skip path. Also handles the edge case in
language/statements/for/head-init-async-of.js where `for (async of => {};
...; ...)` is a traditional for whose init is an arrow with parameter
named `of` — the prior heuristic would have misclassified that as a
for-of shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
source/units/Goccia.Compiler.Statements.pas (1)
2163-2165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynthetic iteration bindings still drop the init binding's metadata.
The recreated per-iteration locals are plain
DeclareLocal(...); OP_MOVE ...bindings. They never copy the header binding's type hint, strict flag, or annotation/array metadata, so the body resolvesito an untyped shadow instead of the initialized binding metadata.Also applies to: 2205-2207, 2463-2471
🤖 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 `@source/units/Goccia.Compiler.Statements.pas` around lines 2163 - 2165, Synthetic per-iteration locals created with ACtx.Scope.DeclareLocal (e.g., the StartReg/LoopName pattern) are missing the init/header binding metadata; update the spots where you recreate the loop-local (lines around the StartReg/LoopName code and the other occurrences noted) to copy the original binding's type hint, strict flag, and annotation/array metadata onto the new local after DeclareLocal (retrieve the header binding from the scope or the symbol for the loop variable, then assign its TypeHint, IsStrict (or equivalent), Annotations and ArrayMetadata fields to the newly declared local) so the body resolves the per-iteration variable with the same metadata as the initializer.
🤖 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 2201-2221: The per-iteration lexical scope is opened too late:
BeginScope/DeclareLocal (ACtx.Scope.BeginScope and ACtx.Scope.DeclareLocal) are
called only around the loop body so the test and afterthought compile against
the shared canonical slot; move the per-iteration scope so it encloses the test,
body and update. Concretely, open the scope (call ACtx.Scope.BeginScope and
create the iteration Slot via ACtx.Scope.DeclareLocal) immediately after
computing StartReg/LimitReg/CmpReg (before EmitInstruction of ExitOpcode and
before compiling the test and update), use that Slot for the body and for the
update/step emission (where currently OuterSlot/Slot are used), and only call
ACtx.Scope.EndScope/EmitInstruction(OP_CLOSE_UPVALUE...) after the update/jump
so closures in test/update capture the per-iteration binding; keep existing
PatchJumpTarget(ContinueJumps) and loop jump logic (LoopStart, ExitJump,
ContinueJumps) but adjust their positions to match the moved
BeginScope/EndScope.
- Around line 2124-2131: The current fast-path wrongly trusts a bare
TGocciaIdentifierExpression on CondExpr.Right because ForBodyAssignsIdentifier
doesn't detect nested writes (IIFEs, assignment expressions, call-returned
assignments); make the guard conservative: only allow the fast-path when
CondExpr.Right is a TGocciaLiteralExpression (remove/disable the identifier-RHS
branch) or else fall back to the safe slow path; apply the same change where the
same pattern appears (the block around ForBodyAssignsIdentifier and the
identical checks in the 2248-2356 region) so LimitReg is only computed once when
the RHS is provably constant.
---
Duplicate comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 2163-2165: Synthetic per-iteration locals created with
ACtx.Scope.DeclareLocal (e.g., the StartReg/LoopName pattern) are missing the
init/header binding metadata; update the spots where you recreate the loop-local
(lines around the StartReg/LoopName code and the other occurrences noted) to
copy the original binding's type hint, strict flag, and annotation/array
metadata onto the new local after DeclareLocal (retrieve the header binding from
the scope or the symbol for the loop variable, then assign its TypeHint,
IsStrict (or equivalent), Annotations and ArrayMetadata fields to the newly
declared local) so the body resolves the per-iteration variable with the same
metadata as the initializer.
🪄 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: 6b53b757-1d98-4b24-ac08-4f1c6c7b1f46
📒 Files selected for processing (3)
source/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.pas
Bundling traditional `for(;;)` into --compat-all re-runs loop bodies that warn-and-skipped on baseline, surfacing 367 unrelated engine gaps (Atomics, Intl, Math.pow edge cases, RegExp tests) that were trivially passing before. Test262 runs --compat-all unconditionally through the bare loader, so the bundling forced a -31 net merge-blocking regression. Make the flag strictly opt-in: callers who want traditional `for(;;)` must pass --compat-traditional-for-loop explicitly (or set the config / engine property). --compat-all continues to OR in --compat-var and --compat-function as before. Test262 baseline is restored. Also tighten TryCompileCountedFor's condition-RHS guard to literals only (#531 review): `ForBodyAssignsIdentifier` doesn't see writes through IIFEs/callbacks/property setters, so `i < limit` was unsafe to fast-path even when the body looked clean. Falls through to the general path where the condition is re-evaluated each iteration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/units/Goccia.Compiler.Statements.pas (1)
2446-2452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynthetic per-iteration locals still drop the header binding’s type metadata.
These inner bindings only copy the slot value. Inside the body,
ResolveLocalnow hits the synthetic local, so typed loop vars lose theirTypeHint, strictness, and array/type-annotation metadata for the body even though the counted fast path now bails out for annotated headers.Capture the outer local record before
BeginScopeand mirror its metadata onto eachDeclareLocalresult here, not just the value.Also applies to: 2457-2465
🤖 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 `@source/units/Goccia.Compiler.Statements.pas` around lines 2446 - 2452, The synthetic per-iteration locals currently copy only the slot value (OuterSlots/InnerSlots assignment using ACtx.Scope.GetLocal(ACtx.Scope.ResolveLocal(Name)).Slot), which drops the header binding’s type metadata; fix by capturing the outer local record (call ACtx.Scope.GetLocal(ACtx.Scope.ResolveLocal(Name)) once before BeginScope) and when creating each synthetic local via DeclareLocal mirror the captured record’s metadata (TypeHint, strictness, array/type-annotation fields) onto the new local instead of only copying Slot; apply the same change to the second similar loop (the block around lines 2457-2465) so both per-iteration synthetic locals preserve the original header metadata.
🤖 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 2117-2125: The current fast-path guard only checks that
CondExpr.Right is a TGocciaLiteralExpression but the emitted code uses integer
ops (OP_GTE_INT/OP_LTE_INT), so update the guard to require an integral numeric
literal: when CondExpr.Right is a TGocciaLiteralExpression also verify it
represents a numeric value with no fractional part (e.g., IsNumber and
Trunc(value)=value or an explicit IsInteger helper) and only then take the fast
path; apply the same stricter check at the other occurrence noted around lines
2171-2172 so both fast-path branches only accept integral numeric literals
compatible with OP_GTE_INT/OP_LTE_INT.
---
Duplicate comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 2446-2452: The synthetic per-iteration locals currently copy only
the slot value (OuterSlots/InnerSlots assignment using
ACtx.Scope.GetLocal(ACtx.Scope.ResolveLocal(Name)).Slot), which drops the header
binding’s type metadata; fix by capturing the outer local record (call
ACtx.Scope.GetLocal(ACtx.Scope.ResolveLocal(Name)) once before BeginScope) and
when creating each synthetic local via DeclareLocal mirror the captured record’s
metadata (TypeHint, strictness, array/type-annotation fields) onto the new local
instead of only copying Slot; apply the same change to the second similar loop
(the block around lines 2457-2465) so both per-iteration synthetic locals
preserve the original header metadata.
🪄 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: fc47aa34-e3b5-417d-9031-99ca4d32492f
📒 Files selected for processing (5)
docs/decision-log.mddocs/language.mdsource/app/Goccia.CLI.Application.passource/app/GocciaScriptLoaderBare.dprsource/units/Goccia.Compiler.Statements.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/app/GocciaScriptLoaderBare.dpr
This reverts commit cd0f7bb.
Brings the bot review fix forward over the decouple revert. The identifier-RHS branch in TryCompileCountedFor used ForBodyAssignsIdentifier as a safety check, but that walker doesn't see writes through IIFEs, callbacks, property setters, or eval-like reaches — so a bare-identifier RHS was unsafe for `i < limit` shapes even when the body looked clean. Falls through to the general path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/decision-log.md`:
- Around line 20-21: The decision log entry for the new flag
--compat-traditional-for-loop contradicts the PR objectives: update the decision
text so it matches the implementation and PR intent by stating that
--compat-traditional-for-loop is included (ORed) into --compat-all; specifically
edit the decision-log.md paragraph to change the "Not bundled into --compat-all"
wording to reflect that the flag is ORed into --compat-all (and note the
reverted decoupling mentioned in the PR objectives), ensuring the language
references the flag name --compat-traditional-for-loop and --compat-all so
readers can verify consistency with the PR objectives.
🪄 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: a162039a-839c-44ef-a639-ffda69a5d888
📒 Files selected for processing (4)
docs/decision-log.mddocs/language.mdsource/app/Goccia.CLI.Application.passource/app/GocciaScriptLoaderBare.dpr
🚧 Files skipped from review as they are similar to previous changes (1)
- source/app/Goccia.CLI.Application.pas
Both bugs were silently passing test262 because traditional for(;;) loops in their test bodies warn-and-skipped on baseline. Restoring --compat-all coverage in #531 surfaced both as honest regressions; fix at the source. Math.pow (§6.1.6.1.3 Number::exponentiate) Previous impl forwarded directly to FreePascal's `Power(B, E)`, which uses `exp(E * ln(B))` and returns NaN for any base ≤ 0. Spec has 30+ special cases: NaN/+0/-0 exponent, ±∞ base, ±∞ exponent, signed-zero/Infinity sign rules driven by "exponent is an odd integer". Adds explicit branches for §6.1.6.1.3 steps 1-13 and guards Round() against doubles above 2^53 (oddness can only be detected up to that magnitude — Round(1.8e308) overflows Int64). Fixes Math/pow/* tests for ±Infinity^x and x^±Infinity. parseFloat (§21.1.2.13.1 StrUnsignedDecimalLiteral) Previous impl parsed integer + fractional digits but no ExponentPart, so `parseFloat("0.1e1")` returned 0.1 instead of 1. Adds optional `(e|E)[+-]?DecimalDigits` parsing after the fraction; malformed exponent (stray 'e' with no digits) returns the mantissa alone per the longest-valid-prefix rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ES2026 §11.2 WhiteSpace and §11.3 LineTerminator include Unicode characters beyond ASCII space — Tab/LF/CR/VT/FF/SP/NBSP/USP plus the ZWNBSP and LS/PS line terminators. parseInt/parseFloat call TrimString(string, start) which uses StrWhiteSpace, so a Unicode whitespace prefix like `\u3000` (ideographic space) must be skipped before scanning digits. Goccia's parseInt/parseFloat were calling FreePascal's `Trim()`, which only strips ASCII whitespace (#0..#32). `parseInt("\u20001")` returned NaN instead of 1. Switch both to `TrimECMAScriptWhitespace` from `TextSemantics`, the same helper String.prototype.trim and BigInt parsing already use. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Number.prototype.toString (§21.1.3.6 / §6.1.6.1.20) Previous impl only handled radix 10 (default ToString) and radix 16 (IntToHex two's-complement, which gave `(-255).toString(16) = 'ffffffffffffff01'` instead of `-ff`). Every radix in 11-15, 17-36 fell through to decimal output. Implements the generalized decimal-to-radix algorithm: optional sign + integer-part-digits + optional `.fraction-digits`, using `0-9a-z` for digits 10-35. IntegerToRadixString and FractionToRadixString are file-local helpers shared with the integer path; the fraction conversion uses 52 digits (enough to round-trip a binary64 mantissa) with trailing-zero trimming. Fixes Number/prototype/toString/a-z and any test using non-10 / non-16 radix. Math.ceil (§21.3.2.10) Per spec step 3, when the argument is in (-1, 0) the result is -0𝔽 (preserves signed-zero). Goccia returned `+0` because it skipped straight to FreePascal's Ceil(), which collapses signed zeros. Adds the explicit "negative-fractional → -0" branch so `Math.ceil(-0.1) === -Math.floor(0.1)` per Object.is. Fixes Math/ceil/S15.8.2.6_A7 (the 2000-iteration ceil(x) ≡ -floor(-x) identity). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ES2026 §21.3.2.16: Math.floor(-0𝔽) is -0𝔽, not +0𝔽. Goccia was passing through to FreePascal's `Floor()` which collapses signed zeros. Returns the argument directly when it is -0 or +0 so the ceil(x) ≡ -floor(-x) identity (Math.floor/S15.8.2.6_A7) holds at the zero boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both methods had two divergences from ES2026 §23.1.3.{29,34}:
1. Validation order: spec step 1 throws TypeError when comparefn is
non-undefined and non-callable, BEFORE LengthOfArrayLike reads
`this.length`. Goccia read length first, so a getter that throws on
length was observed even when the comparator argument was already
bad. test262 specifically covers this with a `getLengthThrow` object
plus invalidComparators list (null/true/false/""/regex/number/
bigint/[]/{}/Symbol).
2. Error type: ThrowError was throwing a generic Error rather than
TypeError, so `assert.throws(TypeError, ...)` saw the wrong
constructor. Switch to ThrowTypeError to match the spec's
"throw a TypeError exception".
Also pull comparator extraction up so the undefined-comparator branch
falls through to the default ascending compare without re-checking
AArgs.Length downstream.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 5, 2026
`parseFloat("0.1e-1")` returned `0.010000000000000002` instead of
`0.01`. Multiplying 0.1 by IntPower(10, -1) = 0.1 yields the imprecise
product (0.1 * 0.1 has FP error). Dividing by IntPower(10, 1) = 10
lands exactly on the closest IEEE 754 double to 0.01.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.Builtins.GlobalNumber.pas (1)
190-325:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp exponent parsing to prevent
Integeroverflow.On line 116-129,
ExpValueaccumulates unbounded into anIntegerwithout saturation. Inputs with lengthy exponents like1e2147483648overflow before reaching theIntPowercall. In PRODUCTION mode (overflow checks off), this wraps silently to an incorrect exponent; in DEBUG mode, it raises an exception—both violate ECMAScript parseFloat semantics, which should saturate toInfinityor0.Cap exponent accumulation to a safe threshold (≈324) that covers Double underflow/overflow boundaries:
ExpValue := 0; HasExpDigits := False; while I <= Length(InputStr) do begin C := InputStr[I]; if (C >= '0') and (C <= '9') then begin - ExpValue := ExpValue * 10 + (Ord(C) - Ord('0')); HasExpDigits := True; + if ExpValue < 324 then + begin + ExpValue := ExpValue * 10 + (Ord(C) - Ord('0')); + if ExpValue > 324 then + ExpValue := 324; + end; Inc(I); endThen treat any clamped positive exponent as saturation when applying it to
Mantissa.source/units/Goccia.Values.NumberObjectValue.pas (1)
279-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat explicit
undefinedas the default radix.Line 281 coerces the first argument before checking whether it is
undefined, sonum.toString(undefined)falls into the non-decimal path instead of the Step 2 default-to-10 path. Right now that returns''for a valid call shape.Suggested fix
- if AArgs.Length > 0 then + if (AArgs.Length > 0) and not (AArgs.GetElement(0) is TGocciaUndefinedLiteralValue) then begin Radix := Trunc(AArgs.GetElement(0).ToNumberLiteral.Value); // Step 3: If radixMV < 2 or radixMV > 36, throw a RangeError exception if (Radix < 2) or (Radix > 36) then begin Result := TGocciaStringLiteralValue.Create(''); Exit; end; // Step 2: If radix is undefined or 10, return ! ToString(x) if Radix = 10 then begin Result := Prim.ToStringLiteral; Exit; end;🤖 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 `@source/units/Goccia.Values.NumberObjectValue.pas` around lines 279 - 293, The code currently coerces the first argument with AArgs.GetElement(0).ToNumberLiteral before checking for undefined, causing num.toString(undefined) to be handled incorrectly; change the logic in the method that builds Radix so you first inspect AArgs.Length and then check if AArgs.GetElement(0).IsUndefined (or equivalent) and, if so, treat it as the default radix 10 and return Prim.ToStringLiteral; only after confirming the argument is not undefined should you call ToNumberLiteral and assign Radix, then apply the existing range check that returns TGocciaStringLiteralValue.Create('') for out-of-range values.
🧹 Nitpick comments (2)
docs/language.md (1)
774-774: ⚡ Quick winDocument the literal-only RHS guard in the counted-loop fast path.
Line 774 currently describes the fast path shape, but it omits the literal-only RHS safety restriction mentioned in this PR’s behavior notes. Please add that non-literal bounds fall back to the general path.
🤖 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 `@docs/language.md` at line 774, Update the docs sentence describing the counted-loop fast path (referencing CompileCountedForOf and the BeginScope/EndScope with OP_CLOSE_UPVALUE behavior) to state that the RHS bound must be a literal constant; if the loop's upper bound is not a literal the compiler will reject the fast path and fall back to the general outer-scope/per-iteration path. Keep references to the same symbols (CompileCountedForOf, BeginScope, EndScope, OP_CLOSE_UPVALUE) so readers can locate the implementation.source/units/Goccia.Builtins.Math.pas (1)
334-335: 💤 Low valueNit: Misleading variable name
IntPart.The variable stores the result of
Power(B, E)which is a general floating-point result, not an integer part. Consider renaming toPowResultfor clarity.♻️ Suggested rename
- IntPart: Double; + PowResult: Double; ExpIsOddInteger: Boolean;And at line 440:
- IntPart := Power(B, E); - Result := TGocciaNumberLiteralValue.Create(IntPart); + PowResult := Power(B, E); + Result := TGocciaNumberLiteralValue.Create(PowResult);🤖 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 `@source/units/Goccia.Builtins.Math.pas` around lines 334 - 335, The variable IntPart incorrectly implies "integer part" even though it holds Power(B, E); rename IntPart to PowResult across the unit to improve clarity: update the declaration (IntPart: Double) to PowResult: Double and replace every reference to IntPart (including the expression that assigns Power(B, E) and subsequent uses) with PowResult; ensure any associated comments or tests referring to IntPart are updated to the new symbol so compilation and semantics remain unchanged (look for references in functions/procedures in Goccia.Builtins.Math that compute Power(B, E)).
🤖 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 `@docs/language.md`:
- Line 787: The sentence conflates break and continue by implying both apply to
switch; update the wording so it explicitly states that break is valid in switch
statements while continue applies only to iteration constructs (for...of, for
await...of, traditional for(;;) when enabled) or to labeled loops. Edit the line
referencing `break` and `continue` to split into two clauses: one that declares
`break` is available inside `switch` statements, and another that declares
`continue` is available only inside loop constructs (and when applicable,
labeled iteration targets), mentioning `for...of`, `for await...of`, and the
optional `for(;;)` flag by name.
In `@source/units/Goccia.Values.NumberObjectValue.pas`:
- Around line 225-258: The FractionToRadixString function is incorrectly capped
at 52 fractional digits causing loss of exactness for some binary64 values;
replace the fixed MAX_DIGITS cap with a computed limit based on binary64
mantissa bits (use 53 bits) and the target radix (e.g. maxDigits := Ceil(53 /
Log2(ARadix))) and then loop until V = 0 or the computed maxDigits is reached;
apply the same change to the analogous routine referenced around lines 309–313
so both fractional-conversion paths use the dynamic mantissa-based digit limit
instead of the hard-coded 52.
- Around line 204-223: IntegerToRadixString currently takes an Int64 and will
overflow for finite numeric whole parts > Int64.MaxValue; change it to accept an
unsigned 64-bit type (e.g., QWord/UInt64) by updating the signature
IntegerToRadixString(const AValue: QWord; const ARadix: Integer): string and the
local V to QWord, and adjust the caller site that now casts the whole part using
an unsigned conversion instead of Round(IntPart) (e.g., use
QWord(Trunc(IntPart)) or an appropriate unsigned cast) so very large finite
whole parts are handled correctly when formatting non-decimal radices.
---
Outside diff comments:
In `@source/units/Goccia.Values.NumberObjectValue.pas`:
- Around line 279-293: The code currently coerces the first argument with
AArgs.GetElement(0).ToNumberLiteral before checking for undefined, causing
num.toString(undefined) to be handled incorrectly; change the logic in the
method that builds Radix so you first inspect AArgs.Length and then check if
AArgs.GetElement(0).IsUndefined (or equivalent) and, if so, treat it as the
default radix 10 and return Prim.ToStringLiteral; only after confirming the
argument is not undefined should you call ToNumberLiteral and assign Radix, then
apply the existing range check that returns TGocciaStringLiteralValue.Create('')
for out-of-range values.
---
Nitpick comments:
In `@docs/language.md`:
- Line 774: Update the docs sentence describing the counted-loop fast path
(referencing CompileCountedForOf and the BeginScope/EndScope with
OP_CLOSE_UPVALUE behavior) to state that the RHS bound must be a literal
constant; if the loop's upper bound is not a literal the compiler will reject
the fast path and fall back to the general outer-scope/per-iteration path. Keep
references to the same symbols (CompileCountedForOf, BeginScope, EndScope,
OP_CLOSE_UPVALUE) so readers can locate the implementation.
In `@source/units/Goccia.Builtins.Math.pas`:
- Around line 334-335: The variable IntPart incorrectly implies "integer part"
even though it holds Power(B, E); rename IntPart to PowResult across the unit to
improve clarity: update the declaration (IntPart: Double) to PowResult: Double
and replace every reference to IntPart (including the expression that assigns
Power(B, E) and subsequent uses) with PowResult; ensure any associated comments
or tests referring to IntPart are updated to the new symbol so compilation and
semantics remain unchanged (look for references in functions/procedures in
Goccia.Builtins.Math that compute Power(B, E)).
🪄 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: cb2aa9a1-a176-4381-b722-2354624f87a1
📒 Files selected for processing (8)
docs/decision-log.mddocs/language.mdsource/app/Goccia.CLI.Application.passource/app/GocciaScriptLoaderBare.dprsource/units/Goccia.Builtins.GlobalNumber.passource/units/Goccia.Builtins.Math.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.NumberObjectValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/app/Goccia.CLI.Application.pas
- TryCompileCountedFor now also rejects non-integer literal RHS so `for(let i=0; i<3.5; i++)` doesn't take the OP_GTE_INT/OP_LTE_INT fast path; falls through to the general path which preserves IEEE 754 compare semantics. (#531 review) - IntegerToRadixString takes a Double, so values above Int64.MaxValue (which still produce a finite Number) no longer overflow on Round() and stringify via repeated div/mod in floating-point. The trailing-bit alignment for values past 2^53 is implementation-defined per ES2026 §6.1.6.1.20. - FractionToRadixString sizes the digit budget per radix as `Ceil(53 / Log2(R)) + 4` so binary64-precision values capture their mantissa in any base 2..36 (the previous fixed 52-digit cap was too short for radix 2 and overshot for radix 36). Full subnormal range remains future work — a proper Steele-White / Grisu implementation is tracked separately. - docs/language.md: clarify `continue` only applies to iteration constructs, never `switch`. - docs/decision-log.md: spell out that `--compat-traditional-for-loop` is bundled into `--compat-all` and reference the engine-gap issues surfaced (#540 / #541 / #542). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/language.md (1)
806-818:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the labeled-statements rationale; it’s now outdated.
Line 817 says traditional loops are excluded and labels serve no purpose, but traditional
for(;;)is now available behind--compat-traditional-for-loop. This sentence should be revised to avoid contradicting the new loop section.🤖 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 `@docs/language.md` around lines 806 - 818, The docs' "Labeled Statements" rationale is outdated: update the paragraph that says "Since GocciaScript excludes traditional loops, labels serve no purpose." to acknowledge the new --compat-traditional-for-loop option and explain that labels remain relevant when that compatibility flag enables traditional for-loops; adjust the example references (`myLabel: x = 2;`, `outer: for (...)`) and the warning wording ("Warning: Labeled statements are not supported in GocciaScript") so the text clarifies that labels are stripped/unwarned only when traditional loops are disabled, but may be meaningful when --compat-traditional-for-loop is enabled.
🤖 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 `@docs/language.md`:
- Around line 10-12: Update the doc sentence about "Graceful handling" to
clarify that only excluded loop forms are treated as no-ops: specify that
`while` and `do...while` loops (and the traditional `for(init; test; update)`
loop only when `--compat-traditional-for-loop` is not enabled) parse
successfully but execute as a no-op with a warning/suggestion; retain the note
about parser-recognized excluded syntax (`==`, `with`) while scoping the "no-op"
claim to those excluded loop forms.
In `@source/units/Goccia.Values.NumberObjectValue.pas`:
- Around line 223-231: The loop uses Trunc(...) which returns an Int64 and
overflows for very large Double values (e.g. 1e100); replace Trunc(V / ARadix)
and Trunc(V - Q * ARadix) with Int(V / ARadix) and Int(V - Q * ARadix)
respectively so Q and D stay as Double-compatible values and no Int64
intermediate is produced; update any local declarations for Q/D if needed to be
Double (or the same type used by V) and keep the rest of the logic that uses
DIGITS and Result unchanged.
- Around line 321-322: The Trunc(V) call can overflow for values beyond Int64
range; update the code around IntPart := Trunc(V); FracPart := V - IntPart to
first check whether V is within Int64 bounds (compare against High(Int64) and
Low(Int64)) and only call Trunc when it is safe; if V is outside that range,
skip Trunc and route the large-integer path (produce the integer part by using
the big-integer/string conversion flow instead of Trunc—e.g. call the routine
that eventually uses IntegerToRadixString or otherwise format the whole-number
portion directly) and set FracPart accordingly, so Trunc is never invoked on
out-of-range V.
---
Outside diff comments:
In `@docs/language.md`:
- Around line 806-818: The docs' "Labeled Statements" rationale is outdated:
update the paragraph that says "Since GocciaScript excludes traditional loops,
labels serve no purpose." to acknowledge the new --compat-traditional-for-loop
option and explain that labels remain relevant when that compatibility flag
enables traditional for-loops; adjust the example references (`myLabel: x = 2;`,
`outer: for (...)`) and the warning wording ("Warning: Labeled statements are
not supported in GocciaScript") so the text clarifies that labels are
stripped/unwarned only when traditional loops are disabled, but may be
meaningful when --compat-traditional-for-loop is enabled.
🪄 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: cee0f6e5-d89b-41bc-9440-d3ea4767c5f9
📒 Files selected for processing (4)
docs/decision-log.mddocs/language.mdsource/units/Goccia.Compiler.Statements.passource/units/Goccia.Values.NumberObjectValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Compiler.Statements.pas
- IntegerToRadixString and the call site at NumberToString now use Int() instead of Trunc(). Trunc() returns Int64 and tripped a range-check error for any double past 2^63 (e.g. (1e100).toString(16)). Int() returns the truncated value as Double so arbitrarily large finite numbers stringify; Round() narrows just the small modulo digit. (#531 review) - docs/language.md: replace "loops" in the Graceful Handling bullet with the precise excluded list — `while`/`do...while` always, and traditional `for(;;)` only when --compat-traditional-for-loop is off. The previous wording implied all loops are no-op which now conflicts with opt-in traditional-for support. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/language.md`:
- Around line 789-792: Update the doc text under the "### `while` and
`do...while`" section so it no longer claims traditional loops are globally
excluded; instead state that traditional for(;;) loops are opt-in via the
--compat-traditional-for-loop flag (or that traditional for is optional). Locate
the sentence that currently asserts "GocciaScript excludes traditional loops"
and change it to describe current behavior: parser accepts those syntaxes but
traditional for requires the compat flag and while/do...while are treated as
no-op unless the appropriate compatibility option is enabled. Make sure to
reference the existing phrasing around "for(;;) (with
`--compat-traditional-for-loop`), `for...of`, or array methods" and update that
explanatory line accordingly.
🪄 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: 2d390e95-ac1e-4639-97a9-c42bf2f5e58b
📒 Files selected for processing (2)
docs/language.mdsource/units/Goccia.Values.NumberObjectValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Values.NumberObjectValue.pas
Two follow-on doc consistencies after opt-in traditional `for(;;)`: - docs/language.md § Labeled Statements: "Since GocciaScript excludes traditional loops, labels serve no purpose" was outdated — traditional for(;;) is now available behind --compat-traditional-for-loop. Rewrite to call out which loop forms exist and that labeled break/continue isn't implemented for any of them. - docs/language-tables.md while/do...while row: list traditional for(;;) (with --compat-traditional-for-loop) alongside for-of and array methods as the suggested alternatives, matching the prose in language.md § while and do...while. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`tests/language/for-loop/with-var/` was a nested subdir; the goccia.json
inside it correctly only applied to its own tests, so isolation was
empirically fine. But the layout made it look like compat-var was a
sub-concern of for-loop, when it's really an orthogonal flag combination.
Move to a sibling at `tests/language/for-loop-var/` so the directory tree
makes the concern split obvious:
tests/language/for-loop/ — only --compat-traditional-for-loop
(let/const tests, default features)
tests/language/for-loop-var/ — --compat-traditional-for-loop +
--compat-var (combination tests)
Each dir's goccia.json controls its own scope; running either, both,
or neither still gives the same per-test config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
This was referenced May 7, 2026
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
for(init; test; update)loops behind a new--compat-traditional-for-loopflag, mirroring the existing--compat-var/--compat-functionposture (off by default;--compat-allORs it in).let/constin for-init create a per-iteration lexical environment per ES2026 §14.7.4.4, so closures captured during iteration N pin to that iteration's binding (the textbookfns.push(() => i)case yields[0, 1, 2], not[3, 3, 3]).varin for-init requires both--compat-varand the new flag, hoists out of the loop, and is shared across iterations.CompileCountedForOffor the commonfor(let i = N; i <op> M; i++ | i--)shape; the general path uses an outer scope for the canonical loop slot plus a per-iterationBeginScope/EndScopecycle withOP_CLOSE_UPVALUEfor captures.whileanddo...whileremain excluded — they share the same stub status but were intentionally split into a separate iteration.Testing
tests/language/for-loop/directory: 26/26 in interpreter + bytecode (basic-counter, empty-parts, let-per-iteration, break-continue, return-from-loop, destructuring-init, with-var/var-shared-binding).scripts/test-cli-config.ts: per-file config across loader (interp + bytecode), test runner (interp + bytecode), bundler, and benchmark runner; plus a flag-off → warning + no-op assertion.tests/fixtures/ffi/libfixture.dylibis not built locally).tests/language/var,tests/language/for-of,tests/language/function-keyword): 239/239 in both modes.docs/language.md§ Traditional for Loop,docs/language-tables.md,docs/goals.md,docs/decision-log.md2026-05-05 entry)🤖 Generated with Claude Code