Conversation
- Add `compat-var` flag across CLI, engine, parser, interpreter, and compiler - Support function-scoped `var` hoisting, redeclaration, and destructuring - Add language tests for `var` behavior
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds opt-in ECMAScript Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Config
participant Engine as Engine
participant Parser as Parser
participant Compiler as Compiler
participant Interpreter as Interpreter
participant ModuleLoader as ModuleLoader
CLI->>Engine: set CompatVar (--compat-var)
Engine->>Parser: VarDeclarationsEnabled = CompatVar
Engine->>ModuleLoader: VarEnabled = CompatVar
alt compile path
Parser->>Compiler: AST (nodes with IsVar)
Compiler->>Compiler: HoistVarLocals into function/module scope
Compiler->>VM: emit bytecode using var depth-0 slots / reuse
else interpret path
Parser->>Interpreter: AST (nodes with IsVar)
Interpreter->>Interpreter: HoistVarDeclarations into call/module scope
Interpreter->>Evaluator: execute with function-scoped var semantics
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
new feature
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 89 improved · 🔴 181 regressed · 116 unchanged · avg -3.8% arraybuffer.js — Interp: 🔴 11, 3 unch. · avg -7.1% · Bytecode: 🟢 9, 🔴 1, 4 unch. · avg +4.4%
arrays.js — Interp: 🟢 3, 🔴 15, 1 unch. · avg -12.4% · Bytecode: 🟢 18, 1 unch. · avg +10.2%
async-await.js — Interp: 🟢 1, 🔴 3, 2 unch. · avg -5.2% · Bytecode: 🟢 5, 1 unch. · avg +11.2%
base64.js — Interp: 🟢 2, 🔴 4, 4 unch. · avg -2.7% · Bytecode: 🟢 2, 🔴 4, 4 unch. · avg -2.1%
classes.js — Interp: 🔴 11, 20 unch. · avg -2.6% · Bytecode: 🟢 15, 16 unch. · avg +5.6%
closures.js — Interp: 🔴 6, 5 unch. · avg -5.2% · Bytecode: 🟢 11 · avg +9.4%
collections.js — Interp: 🟢 8, 🔴 3, 1 unch. · avg +3.9% · Bytecode: 🟢 12 · avg +10.1%
csv.js — Interp: 🟢 10, 🔴 1, 2 unch. · avg +7.1% · Bytecode: 🟢 13 · avg +10.6%
destructuring.js — Interp: 🟢 1, 🔴 12, 9 unch. · avg -4.3% · Bytecode: 🟢 22 · avg +8.6%
fibonacci.js — Interp: 🔴 7, 1 unch. · avg -5.9% · Bytecode: 🟢 8 · avg +11.2%
float16array.js — Interp: 🟢 12, 🔴 13, 7 unch. · avg -2.5% · Bytecode: 🟢 25, 🔴 5, 2 unch. · avg +3.8%
for-of.js — Interp: 🟢 7 · avg +8.3% · Bytecode: 🟢 6, 1 unch. · avg +9.6%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 32, 10 unch. · avg -8.6% · Bytecode: 🟢 41, 1 unch. · avg +9.5%
json.js — Interp: 🟢 14, 🔴 1, 5 unch. · avg +5.8% · Bytecode: 🟢 17, 3 unch. · avg +7.9%
jsx.jsx — Interp: 🟢 2, 🔴 9, 10 unch. · avg -1.1% · Bytecode: 🟢 21 · avg +8.5%
modules.js — Interp: 🔴 9 · avg -16.1% · Bytecode: 🟢 9 · avg +13.7%
numbers.js — Interp: 🟢 4, 7 unch. · avg +3.0% · Bytecode: 🟢 11 · avg +7.1%
objects.js — Interp: 🟢 3, 🔴 1, 3 unch. · avg +1.0% · Bytecode: 🟢 6, 1 unch. · avg +7.1%
promises.js — Interp: 🟢 3, 🔴 5, 4 unch. · avg -3.3% · Bytecode: 🟢 7, 5 unch. · avg +2.9%
regexp.js — Interp: 🟢 9, 2 unch. · avg +8.2% · Bytecode: 🟢 11 · avg +10.1%
strings.js — Interp: 🔴 13, 6 unch. · avg -16.3% · Bytecode: 🟢 19 · avg +6.0%
tsv.js — Interp: 🟢 5, 4 unch. · avg +4.3% · Bytecode: 🟢 9 · avg +8.4%
typed-arrays.js — Interp: 🟢 5, 🔴 11, 6 unch. · avg -2.1% · Bytecode: 🟢 13, 🔴 5, 4 unch. · avg +2.3%
uint8array-encoding.js — Interp: 🔴 14, 4 unch. · avg -15.3% · Bytecode: 🟢 16, 2 unch. · avg +43.6%
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 Timing
Measured on ubuntu-latest x64. |
![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: 10
🧹 Nitpick comments (1)
tests/language/var/closure-over-var.js (1)
6-24: Make one case escape the defining function.Both tests call
getter()beforefn()returns, so they never verify that the capturedvarenvironment survives after the outer activation is gone. A broken implementation that tears down the environment eagerly can still pass here. Consider returning the closure, mutating first, and asserting after the outer function returns.Based on learnings
Applies to tests/**/*.{js,jsx,ts,tsx,mjs} : JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/var/closure-over-var.js` around lines 6 - 24, Change one of the tests so the closure escapes the defining function: update either the "closure captures var from enclosing function" or "var assignment visible to previously-created closure" test so that fn returns the closure (getter) instead of invoking it inside fn, perform the mutation to the var (x or value) before returning the closure, then call the returned closure from the test scope and assert it sees the updated value; reference the functions and vars by name (fn, getter, x/value) so the change is applied to the correct test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Compiler.pas`:
- Around line 307-358: Hoisting currently only walks a subset of container
nodes, so HoistVarLocals misses var bindings from destructuring, export
declarations, and for-await-of bodies; update HoistVarLocals to also handle
TGocciaForAwaitOfStatement (visit its Body), TGocciaExportNamedDeclaration
(visit its Declaration), and any node types that can carry
TGocciaVariableDeclaration children (e.g., inspect
TGocciaVariableDeclarator/destructuring inside declarations) so that whenever a
TGocciaVariableDeclaration or a node wrapping one is encountered you call
AScope.DeclareVarLocal for each Variables[i].Name; modify the existing branches
in HoistVarLocals to include these node types and/or add a generic traversal
that examines child declaration fields to ensure all var declarators (including
destructured and exported ones) are visited.
In `@source/units/Goccia.Compiler.Scope.pas`:
- Around line 157-165: The new local is being initialized with Depth := 0 which
breaks scope unwinding in nested scopes; update the local's Depth to the current
scope depth instead of 0 (use the variable that tracks current scope depth in
this unit — e.g., FCurrentDepth or FScopeDepth) when assigning
FLocals[FLocalCount].Depth so EndScope can correctly pop locals; keep the
FNextSlot overflow check and resizing logic for FLocals and ensure EndScope
still matches against that depth when unwinding captured/non-captured locals.
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 476-479: In CompileVariableDeclaration, the no-initializer branch
currently emits OP_LOAD_UNDEFINED unconditionally and thus overwrites an
existing value when AStmt.IsVar uses ACtx.Scope.DeclareVarLocal to reuse a slot
for a redeclared var; modify the logic so after calling
ACtx.Scope.DeclareVarLocal(Info.Name) you detect whether it returned an existing
slot (i.e., a redeclaration without new storage) and skip emitting
OP_LOAD_UNDEFINED in that case; reference the functions/methods
CompileVariableDeclaration, AStmt.IsVar, ACtx.Scope.DeclareVarLocal and the
OP_LOAD_UNDEFINED emission point, and add a unit test that covers the redeclared
var-without-initializer case (e.g., var x = 1; var x;) to ensure the previous
value is preserved.
In `@source/units/Goccia.Engine.pas`:
- Around line 1467-1474: TGocciaEngine.SetVarEnabled correctly flips the cfVar
flag and sets FInterpreter.VarEnabled but the module execution path in
TGocciaInterpreterExecutor.EvaluateModuleBody still skips a `var` hoist pass;
update EvaluateModuleBody to run the same hoisting logic used for scripts when
`cfVar`/FInterpreter.VarEnabled is true (i.e., perform a var-declaration
collection/hoist before evaluating module statements) so imported modules get
proper `var` hoisting semantics; reference the compatibility flag
(FCompatibility/cfVar), the engine setter (TGocciaEngine.SetVarEnabled) and the
executor method (TGocciaInterpreterExecutor.EvaluateModuleBody) when adding the
hoist pass and ensure the hoist respects existing interpreter state
(FInterpreter.VarEnabled).
In `@source/units/Goccia.Evaluator.pas`:
- Around line 161-211: CollectVarNamesFromNode currently only adds simple
TGocciaVariableDeclaration variable names and recurses TGocciaForOfStatement
bodies, so destructured declarations and for-await-of bodies are skipped; update
CollectVarNamesFromNode to (1) handle destructuring by iterating each
TGocciaVariableDeclaration.Variables (TGocciaVariableDeclarator) and when a
declarator has no simple Name, walk its pattern nodes (e.g.,
TGocciaObjectPattern, TGocciaArrayPattern and any nested Property/Element
patterns) to extract and ANames.Add each identifier found, and (2) add a branch
for TGocciaForAwaitOfStatement (like TGocciaForOfStatement) to recurse into its
Body so var declarations inside for-await-of are collected. Ensure you use the
existing CollectVarNamesFromNode recursion for nested pattern elements and
reference TGocciaVariableDeclaration, TGocciaVariableDeclarator,
TGocciaObjectPattern, TGocciaArrayPattern, and TGocciaForAwaitOfStatement when
implementing.
In `@source/units/Goccia.Interpreter.pas`:
- Around line 138-139: The interpreter currently only hoists var declarations
for the main program body (HoistVarDeclarations(AProgram.Body, FGlobalScope)),
so standalone modules loaded via TGocciaModuleLoader.LoadModule (which creates
an skModule child scope) never get var hoisting; update
TGocciaInterpreter.EvaluateModuleBody to, when FVarEnabled, call
HoistVarDeclarations on the module's statement list using the module's execution
scope (the skModule scope created by TGocciaModuleLoader.LoadModule) instead of
always using FGlobalScope so that var reads-before-declaration in imported
modules are initialized to undefined.
In `@source/units/Goccia.Parser.pas`:
- Around line 2869-2873: The parser branch that handles const/let/var currently
only sets IsConst and then Advance, so the loop binding loses whether it was a
var; modify the branch in Goccia.Parser.pas to capture the actual binding kind
(e.g., introduce an IsVar or BindingKind variable) by checking Check(gttVar)
(and FVarDeclarationsEnabled) as well as gttLet/gttConst, set the appropriate
flag before calling Advance, and ensure that the resulting AST node for the
for...of loop records that flag (instead of only IsConst) so downstream code can
distinguish var vs let/const semantics.
- Around line 195-196: ParseInterpolationExpression creates a new TGocciaParser
with default flags so nested template interpolations ignore the outer parser's
VarDeclarationsEnabled; update ParseInterpolationExpression so after
constructing the nested TGocciaParser instance (the one used to parse the
interpolation) you assign its VarDeclarationsEnabled :=
Self.VarDeclarationsEnabled (or otherwise propagate the outer parser's
FVarDeclarationsEnabled) so the nested parser honors the outer parser's
compat-var setting.
- Around line 2777-2784: The recovery for unsupported "var" uses
SkipUntilSemicolon which ignores ASI and block boundaries and can consume
closing braces (affecting Depth); replace the current SkipUntilSemicolon call in
the FVarDeclarationsEnabled fallback so it uses an ASI-aware recovery routine
(or implement one) that stops on an explicit ';', on an automatic-semicolon
insertion point (newline before a closing token/EOF), or before a '}' token so
Depth is not decremented; update the path that issues AddWarning and returns
TGocciaEmptyStatement to invoke that ASI-aware skipper instead of
SkipUntilSemicolon.
In `@tests/language/var/hoisting.js`:
- Around line 26-30: The test currently checks hoisting inside the test(...)
callback (function scope) rather than module scope; move the pre-declaration
read and the var declaration to file (module) scope so the read happens before
the var statement at module level, then inside the existing test callback assert
that the pre-read value is undefined and the post-declaration value equals
"assigned"; update references to hoistedVar and any local variables accordingly
(look for the test function named "var hoisted at module top level" and the
identifier hoistedVar).
---
Nitpick comments:
In `@tests/language/var/closure-over-var.js`:
- Around line 6-24: Change one of the tests so the closure escapes the defining
function: update either the "closure captures var from enclosing function" or
"var assignment visible to previously-created closure" test so that fn returns
the closure (getter) instead of invoking it inside fn, perform the mutation to
the var (x or value) before returning the closure, then call the returned
closure from the test scope and assert it sees the updated value; reference the
functions and vars by name (fn, getter, x/value) so the change is applied to the
correct test case.
🪄 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: 9ab09815-7c8f-40f4-9e92-c9e995a2b41a
📒 Files selected for processing (26)
source/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.Statements.passource/units/Goccia.Compiler.Scope.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.passource/units/Goccia.Scope.passource/units/Goccia.Values.FunctionValue.pastests/language/var/basic-declaration.jstests/language/var/closure-over-var.jstests/language/var/destructuring-var.jstests/language/var/function-scoping.jstests/language/var/goccia.jsontests/language/var/hoisting.jstests/language/var/multiple-declarations.jstests/language/var/redeclaration.js
Comment on lines
+2777
to
+2784
| if not FVarDeclarationsEnabled then | ||
| begin | ||
| AddWarning('''var'' declarations are not supported in GocciaScript', | ||
| 'Use ''let'' or ''const'' instead', | ||
| Line, Column); | ||
| SkipUntilSemicolon; | ||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| Exit; |
Contributor
There was a problem hiding this comment.
Unsupported-var recovery breaks on ASI/block boundaries.
SkipUntilSemicolon() only stops on an explicit ;. In this fallback path, var x followed by a newline and } will consume the closing brace, drive Depth negative, and can skip the rest of the file instead of producing one warned no-op. Please use an ASI-aware recovery path here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/units/Goccia.Parser.pas` around lines 2777 - 2784, The recovery for
unsupported "var" uses SkipUntilSemicolon which ignores ASI and block boundaries
and can consume closing braces (affecting Depth); replace the current
SkipUntilSemicolon call in the FVarDeclarationsEnabled fallback so it uses an
ASI-aware recovery routine (or implement one) that stops on an explicit ';', on
an automatic-semicolon insertion point (newline before a closing token/EOF), or
before a '}' token so Depth is not decremented; update the path that issues
AddWarning and returns TGocciaEmptyStatement to invoke that ASI-aware skipper
instead of SkipUntilSemicolon.
Owner
Author
There was a problem hiding this comment.
Acknowledged. The SkipUntilSemicolon recovery path is only hit when --compat-var is disabled (the "var not supported" warning path). In that mode, ASI is also typically disabled, so the semicolon-based skip is sufficient for the common case. Will track as a follow-up improvement for edge cases where both ASI and unsupported-var recovery interact.
Contributor
There was a problem hiding this comment.
@frostney, understood — that reasoning holds. Since the SkipUntilSemicolon fallback is only reachable when --compat-var is explicitly off, the user has already opted out of var support, and in practice they're almost certainly not running with --asi either. The edge case (ASI on, --compat-var off, var statement with no explicit ;) is a very narrow combination.
Would you like me to open a follow-up GitHub issue to track the ASI-aware recovery improvement so it doesn't get lost?
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Parser.pas:2780-2787
Timestamp: 2026-04-20T16:59:47.231Z
Learning: In `source/units/Goccia.Parser.pas`, the `SkipUntilSemicolon` call in `TGocciaParser.VarStatement`'s unsupported-`var` warning path (when `FVarDeclarationsEnabled = False`) is intentionally not ASI-aware. This path is only reachable when `--compat-var` is disabled, and ASI is also typically disabled in that mode, making the semicolon-based skip sufficient for the common case. The edge-case interaction (ASI enabled + `--compat-var` disabled) is acknowledged as a known gap and deferred to a follow-up improvement. Do NOT re-flag `SkipUntilSemicolon` in this specific fallback path until that follow-up is addressed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T15:16:53.061Z
Learning: Applies to **/Goccia.Parser.pas : The parser must accept unsupported syntax but treat it as a no-op — `function` declarations produce `TGocciaEmptyStatement`, `function` expressions evaluate to `undefined`, `==`/`!=` expressions evaluate to `undefined`, labeled statements strip the label and execute the labeled statement normally. A warning must be emitted to stdout with a suggestion
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Parser.pas:949-961
Timestamp: 2026-04-11T09:44:42.181Z
Learning: In frostney/GocciaScript (units/Goccia.AST.Node.pas, units/Goccia.AST.Statements.pas): `TGocciaProgram` has no destructor and does NOT free its `FBody: TObjectList<TGocciaStatement>` when `Free` is called. `TGocciaExpressionStatement` similarly has no destructor and does NOT free its `FExpression: TGocciaExpression`. Therefore, extracting a child expression from a temporary `TGocciaProgram` (e.g., via `TGocciaExpressionStatement(ProgramNode.Body[0]).Expression`) and then calling `ProgramNode.Free` is safe — the extracted expression is NOT freed and remains valid. This pattern is intentional and is used in `ParseTaggedTemplate` and similar parsing helpers. Do NOT flag this as a dangling-pointer or use-after-free issue.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T07:58:33.798Z
Learning: Applies to **/{Parser,AST}*.pas : type/interface declarations and import type/export type must produce TGocciaEmptyStatement (no-op at runtime)
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1010-1023
Timestamp: 2026-04-12T09:44:50.402Z
Learning: In frostney/GocciaScript (units/Goccia.Parser.pas, SplitTemplateAtBoundaries and the original ParseTaggedTemplate), the brace-counting used to find the closing `}` of a template interpolation expression (`${...}`) is NOT lexically aware — it does not account for `{`/`}` inside string literals, comments, or nested templates within the expression. This is a known pre-existing limitation that predates this codebase area; it was present in the original `ParseTaggedTemplate` on `main` and is carried forward. Fixing it properly requires lexer-time segmentation (embedding boundary markers during `ScanTemplate` while escape/string context is available). Do NOT re-flag the naive brace counter in SplitTemplateAtBoundaries or ParseTaggedTemplate as a regression or new bug — it is intentionally deferred to a follow-up. The limitation exists because `ScanTemplate` stores the entire template (including `${...}` expressions) as a single flat token with no expression-level lexical analysis.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Do not add support for these JavaScript features: `var` declarations, `function` keyword, `==` and `!=` loose equality, `eval()` and `arguments` object, automatic semicolon insertion, `with` statement, traditional loops (`for`, `while`, `do...while`), default imports/exports, or global `parseInt`, `parseFloat`, `isNaN`, `isFinite`. The parser may accept some unsupported syntax as no-ops with warnings, but these features must not be implemented.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:0-0
Timestamp: 2026-03-08T04:50:29.540Z
Learning: In format.pas (GocciaScript formatter), ParseParamNames previously misidentified parenthesized arguments inside one-liner function bodies as parameter names, causing the `A` prefix to be prepended to them. The fix (f5b2f23) makes ParseParamNames ignore parentheses appearing after the first semicolon. Be cautious when reviewing one-liner procedure/function bodies in .pas files for incorrectly prefixed identifiers — this was the root cause of the AGC_ANY_CONVERT_EXTERN → AGC_ANY_CONVERT_EXTERN typos in souffle/Souffle.Wasm.Emitter.pas.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Builtins.GlobalPromise.pas:56-57
Timestamp: 2026-04-15T06:26:35.760Z
Learning: In frostney/GocciaScript (PR `#306`), ~30 builtin units (e.g., Goccia.Builtins.GlobalPromise.pas, Goccia.Builtins.GlobalArray.pas, Goccia.Builtins.Math.pas, and ~27 others) use `threadvar FStaticMembers: TArray<TGocciaMemberDefinition>` in their `implementation` sections. FPC does NOT finalize managed threadvar contents on thread exit, so the TArray shell (~few hundred bytes per unit) leaks per worker thread. This is an accepted/deferred trade-off: (1) threads are short-lived spawn-and-join workers, (2) the GC-managed TGocciaMemberDefinition objects are cleaned up by ShutdownThreadRuntime, (3) OS reclaims all memory on process exit, (4) a proper fix requires a registration-based cleanup system or per-unit finalization hooks since the threadvars are in implementation sections. Do NOT re-flag this managed-threadvar leak until long-lived thread pools are introduced and a cleanup registry is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 290
File: units/Goccia.Compiler.Statements.pas:613-649
Timestamp: 2026-04-13T00:22:03.392Z
Learning: In frostney/GocciaScript (units/Goccia.Compiler.Statements.pas), `CompileUsingDeclaration` intentionally does not apply `Info.TypeAnnotation`, strict-type hints, or emit `OP_CHECK_TYPE` for `using`/`await using` bindings. This is a known, deferred gap — runtime validation via `OP_USING_INIT` covers correctness. The typed-local pipeline for `using` declarations will be wired up in a follow-up PR. Do NOT re-flag this gap in `CompileUsingDeclaration` until that follow-up lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/language.md:118-118
Timestamp: 2026-04-14T13:18:31.918Z
Learning: In frostney/GocciaScript (units/Goccia.Lexer.pas, ScanTemplate / ProcessEscapeSequence), cooked=undefined support for tagged templates with malformed escape sequences (e.g., \u{, \xG1) landed in PR `#287` (TC39 Template Literal Revision / ES2018). Tagged templates now correctly set the cooked segment to undefined for malformed escapes while preserving the raw source text; untagged templates still throw SyntaxError. Do NOT re-flag this as missing or deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:3436-3437
Timestamp: 2026-04-15T15:41:51.609Z
Learning: Repo frostney/GocciaScript — units/Goccia.VM.pas:
For TGocciaVM.HasPropertyValue’s “in” operator error path (primitives on the RHS), the project intentionally uses SSuggestCheckNullBeforeAccess for all primitive cases (undefined, null, boolean, number, string). Branching the suggestion by value kind is deferred as low priority; do not re-flag this as an issue unless a dedicated follow-up changes that policy.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:13.250Z
Learning: In frostney/GocciaScript, `EscapeJSONString` (escaping `\`, `"`, `\n`, `\r`, `\t` only) is intentionally duplicated across ~5 CLI tool files (e.g., TestRunner.dpr, BenchmarkRunner.dpr). The incomplete control-character coverage (missing U+0000–U+001F beyond the five named characters) is a known pre-existing issue tracked for fix as part of a future consolidation into a shared helper. Do NOT re-flag missing control-character escaping in individual per-tool copies of `EscapeJSONString` until that consolidation PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 300
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-14T06:31:04.274Z
Learning: In frostney/GocciaScript (units/Goccia.VM.pas), the OP_CHECK_TYPE handler intentionally calls `VMStrictTypeCheckRegisterValue(GetRegister(A), TGocciaLocalType(B))`, which boxes the register via `GetRegister(A)` before the type guard runs. A register-native fast path (passing `TGocciaRegister` directly and only materializing `TGocciaValue` for error text) is acknowledged as a potential optimization but is explicitly deferred to a future follow-up PR. Do NOT re-flag the `GetRegister(A)` boxing in the OP_CHECK_TYPE handler as a performance issue until that follow-up is scoped.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 222
File: scripts/test262_syntax_filter.py:430-434
Timestamp: 2026-04-08T14:59:30.654Z
Learning: In frostney/GocciaScript, `scripts/test262_syntax_filter.py` SKIP_PATH_SEGMENTS: the `for-in`, `eval-code`, and `generators` path segments are intentional categorical exclusions — every test under those directories requires a feature GocciaScript excludes by design (`for...in` loops, `eval()`, and generators respectively). Do not flag these as incorrect skips. Only the `asi/` segment may warrant conditional handling based on the `--asi` runner flag.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Type annotations in GocciaScript (TC39 Types as Comments proposal) are parsed but ignored at runtime. Raw type strings are preserved on AST nodes for potential future optimization. Use parser helpers `CollectTypeAnnotation(Terminators)` and `CollectGenericParameters` for type handling.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.Coverage.pas:225-249
Timestamp: 2026-04-07T20:24:40.351Z
Learning: In `units/Goccia.Coverage.pas` `BuildExecutableLineFlags`: `interface Foo {` opening lines are intentionally marked as executable (`AFlags[I] := True`) because they produce `TGocciaEmptyStatement` which the engine does evaluate; interface body lines are suppressed via `TypeBodyDepth`. Single-line `type` aliases (e.g. `type Point = { x: number };`) are also correctly counted as executable for the same reason. The only known gap is multi-line `type` aliases spanning several lines — those continuation lines are not currently suppressed, but this is considered rare in practice and acceptable for now. Do not flag the `interface` opening-line executable marking or single-line `type` handling as bugs.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Use `Goccia.Keywords.Reserved` and `Goccia.Keywords.Contextual` constants (`KEYWORD_THIS`, `KEYWORD_SUPER`, `KEYWORD_GET`, `KEYWORD_TYPE`, etc.) instead of hardcoded string literals when referencing JavaScript keywords.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.694Z
Learning: Applies to **/*.pas : Use `Goccia.Keywords.Reserved` and `Goccia.Keywords.Contextual` constants instead of hardcoded string literals when referencing JavaScript keywords (e.g., `KEYWORD_THIS`, `KEYWORD_SUPER`, `KEYWORD_GET`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Evaluator.Arithmetic.pas:49-55
Timestamp: 2026-04-18T21:08:03.166Z
Learning: In frostney/GocciaScript (commit 3344dede, PR `#343`), `TGocciaBigIntObjectValue` was added to support BigInt boxing via `Object(1n)`. `Box` in `ClassHelper.pas` now returns a `TGocciaBigIntObjectValue` for `TGocciaBigIntValue` primitives. `ToPrimitive` calls were added to `EvaluateSubtraction`, `EvaluateMultiplication`, `EvaluateDivision`, `EvaluateModulo`, and `EvaluateExponentiation` in `source/units/Goccia.Evaluator.Arithmetic.pas` so that boxed BigInts (e.g. `Object(1n) - 1n`) are correctly unboxed via `valueOf()` before BigInt type checks. `Object()` was also fixed to call `Box` for primitive arguments per ES2026 §20.1.1.1. Future reviews of arithmetic operators SHOULD expect `ToPrimitive` before BigInt checks.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 276
File: tests/built-ins/Goccia/build.js:6-7
Timestamp: 2026-04-11T18:29:34.210Z
Learning: In frostney/GocciaScript, the pattern `const hasGoccia = typeof Goccia !== "undefined"` combined with `describe.runIf(hasGoccia)(...)` is intentional and used across the entire `tests/` suite. It allows test files to run safely in non-GocciaScript environments (e.g., Vitest compatibility checks) where GocciaScript-specific globals like `Goccia` are not present. Do NOT flag this conditional-skip guard as a regression risk or suggest making suites unconditional — the portability requirement is by design.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 323
File: docs/language-tables.md:0-0
Timestamp: 2026-04-17T13:02:51.268Z
Learning: In frostney/GocciaScript, `atob`/`btoa` and `structuredClone` are NOT ECMAScript features — they belong to the WHATWG HTML spec. Since PR `#323` (commit 933ac45d), `docs/language-tables.md` has a dedicated **Web Platform APIs** section listing all 10 WHATWG/W3C APIs (`console`, `structuredClone`, `DOMException`, `atob`/`btoa`, `queueMicrotask`, `URL`, `URLSearchParams`, `TextEncoder`, `TextDecoder`, `performance`). `Goccia.Spec.pas` exposes a `WHATWG_FEATURES` array as `Goccia.spec.whatwg` on the runtime spec object. Do NOT flag `atob`/`btoa` or `structuredClone` as ECMAScript features (any edition) in future reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:2797-2828
Timestamp: 2026-03-23T15:57:21.755Z
Learning: Repo frostney/GocciaScript — PR `#109`: Native Map/Set scaffolding
- Current behavior: ToSouffleValue wraps TGocciaMapValue/TGocciaSetValue as TGocciaWrappedValue; TGocciaSouffleMap/TGocciaSouffleSet instances are not produced in normal execution.
- Pre-wired fast paths: GetProperty, GetIterator, and IteratorNext recognize TGocciaSouffleMap/TGocciaSouffleSet and their iterator types; these are staged for future activation.
- Deferred work: When Construct is updated to produce native collection types, add corresponding fast paths in HasProperty, SetProperty, and DeleteProperty.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Builtins.GlobalBigInt.pas:213-223
Timestamp: 2026-04-18T20:48:02.918Z
Learning: In `source/units/Goccia.Builtins.GlobalBigInt.pas` (frostney/GocciaScript, landed in commit 477b5609 on PR `#343`), `BigInt.asIntN` and `BigInt.asUintN` use proper `ToIndex` abstract-op coercion for the `bits` parameter: `undefined`/`NaN` → 0, `Infinity`/negative/oversized → `RangeError`. This is fully implemented and tested. Do not flag missing `ToIndex` coercion for these methods.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: tests/built-ins/global-properties/goccia-gc-memory-limit.js:85-106
Timestamp: 2026-04-19T15:42:42.176Z
Learning: In frostney/GocciaScript (PR `#351`, tests/built-ins/global-properties/goccia-gc-memory-limit.js), the `maxBytes vs suggestedMaxBytes` describe block is intentionally dual-mode: it derives `isOverridden = maxBytes !== suggestedMaxBytes` from observed runtime values and conditionally validates only the active path. This is by design — hard coverage for a broken `--max-memory` flag is provided by dedicated CI integration steps (in .github/workflows/pr.yml and ci.yml) that assert exact numeric output and exit codes. Do NOT re-flag this conditional pattern as hiding failures.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: units/Goccia.Spec.pas:30-51
Timestamp: 2026-04-12T15:33:54.442Z
Learning: In frostney/GocciaScript, `Proxy` is fully supported — it was implemented in PR `#226`. The `Proxy` entry in `ES2015_FEATURES` in `units/Goccia.Spec.pas` is correct and intentional. Do not flag `Proxy` as unimplemented in future reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:57.064Z
Learning: Repo: frostney/GocciaScript PR: 58
Context: units/Goccia.Runtime.Operations.pas (BridgePromiseStatic, NativePromiseStaticResolve, NativePromiseStaticReject) and interpreter Promise statics in Goccia.Builtins.GlobalPromise.pas
Learning: GocciaScript’s Promise statics are intentionally NOT species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 333
File: source/shared/OrderedMap.Test.pas:387-401
Timestamp: 2026-04-18T03:14:07.382Z
Learning: For Free Pascal (FPC) in Delphi mode (`{$mode delphi}`) used in this repo (Pascal `.pas` files), do not treat a bare indexed-property read used as a standalone statement (e.g., `Map[999];`) as a compile error. In this mode, the getter call is executed and its result is silently discarded; if the getter raises an exception, it still propagates to the surrounding `except` handler as expected. For readability/portability, assigning the result to a dummy local (e.g., `Dummy := Map[999];`) is preferred, but bare indexed-property reads should remain unflagged as long as they are valid under FPC Delphi mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 337
File: source/units/Goccia.Builtins.TSV.pas:382-388
Timestamp: 2026-04-18T13:58:20.551Z
Learning: When reviewing GocciaScript Pascal code, do not flag missing `Args.Free`/`try/finally` around `TGocciaArgumentsCollection` instances. `TGocciaArgumentsCollection` (in `source/units/Goccia.Arguments.Collection.pas`) descends from `TInterfacedObject` and is reference-counted, so objects created via `CreateWithCapacity` or `Create` are automatically freed when the last reference goes out of scope. An explicit `Free` call (or wrapping in `try/finally`) is not appropriate for this type.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: source/units/Goccia.Engine.pas:1364-1368
Timestamp: 2026-04-19T15:42:49.575Z
Learning: In GocciaScript (e.g., source/units/Goccia.Engine.pas), treat `Goccia.gc()` / `TGocciaEngine.GocciaGC` as intentionally thread-safe: it should be a no-op on worker threads by guarding the manual `GC.Collect` call with `not GIsWorkerThread`. This prevents races on shared singleton state that uses a single, non-thread-safe `FGCMark` field during mark-sweep. Reviewers should not flag the `GIsWorkerThread` guard as a regression or as a missing manual-GC path; only the full/manual collection triggered by `Goccia.gc()` is skipped on workers, while other worker-thread protections (e.g., the memory-limit ceiling check in `TGocciaValue.AfterConstruction`) should still apply.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 354
File: source/units/Goccia.Values.TemporalPlainDate.pas:668-680
Timestamp: 2026-04-19T18:53:31.289Z
Learning: For this project (frostney/GocciaScript), do not raise review findings at individual call sites that use `Arg.ToStringLiteral.Value` on user-supplied arguments for missing Symbol guards (e.g., ensuring `Arg is TGocciaSymbolValue`). This implicit Symbol→string coercion gap is intentionally deferred to a project-wide fix that should be implemented in `ToStringLiteral` itself (or a shared coercion helper). Until that `ToStringLiteral` fix lands, suppress/skip re-flagging the same issue in methods like `DateWithCalendar` and other `withCalendar`/`withTimeZone`-style methods in `source/units`.
Owner
Author
There was a problem hiding this comment.
Please create a follow up issue for that - marked for 0.12.0 milestone
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find milestone for 0.12.0
gh api repos/frostney/GocciaScript/milestones --jq '.[] | {number: .number, title: .title}'
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Create the follow-up issue
MILESTONE=$(gh api repos/frostney/GocciaScript/milestones --jq '.[] | select(.title == "0.12.0") | .number')
echo "Milestone number: $MILESTONE"
gh issue create \
--repo frostney/GocciaScript \
--title "Parser: ASI-aware recovery for unsupported \`var\` statement skip (--compat-var disabled)" \
--body "## Summary
When \`--compat-var\` is **disabled**, \`TGocciaParser.VarStatement\` falls back to \`SkipUntilSemicolon\` to skip the unsupported \`var\` declaration before emitting a warning and returning \`TGocciaEmptyStatement\`. The current \`SkipUntilSemicolon\` implementation is not ASI-aware and will not stop on a closing \`}\` or EOF — it decrements the internal depth counter and can skip past closing braces, consuming more tokens than intended.
## Affected path
\`source/units/Goccia.Parser.pas\` — \`TGocciaParser.VarStatement\`, the \`not FVarDeclarationsEnabled\` branch (lines ~2777–2784).
## Expected behavior
The skip should stop on:
- An explicit \`;\`
- An ASI point (newline preceding a closing token or EOF)
- A \`}\` token (to avoid consuming closing braces)
## Notes
- This is only reachable when \`--compat-var\` is explicitly off, and in practice ASI is also typically disabled in that mode, so the current behavior is sufficient for the common case.
- The edge case (ASI enabled + \`--compat-var\` disabled + \`var\` statement with no explicit \`;\`) is a narrow combination but should be handled correctly.
- Backlinks: PR `#368`, comment https://github.com/frostney/GocciaScript/pull/368#discussion_r3112218334
- Reported by: `@frostney`
" \
${MILESTONE:+--milestone "$MILESTONE"}
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Create issue without milestone first
ISSUE_URL=$(gh issue create \
--repo frostney/GocciaScript \
--title "Parser: ASI-aware recovery for unsupported \`var\` statement skip (--compat-var disabled)" \
--body "## Summary
When \`--compat-var\` is **disabled**, \`TGocciaParser.VarStatement\` falls back to \`SkipUntilSemicolon\` to skip the unsupported \`var\` declaration before emitting a warning and returning \`TGocciaEmptyStatement\`. The current \`SkipUntilSemicolon\` implementation is not ASI-aware and will not stop on a closing \`}\` or EOF — it decrements the internal depth counter and can skip past closing braces, consuming more tokens than intended.
## Affected path
\`source/units/Goccia.Parser.pas\` — \`TGocciaParser.VarStatement\`, the \`not FVarDeclarationsEnabled\` branch (lines ~2777–2784).
## Expected behavior
The skip should stop on:
- An explicit \`;\`
- An ASI point (newline preceding a closing token or EOF)
- A \`}\` token (to avoid consuming closing braces)
## Notes
- This is only reachable when \`--compat-var\` is explicitly off, and in practice ASI is also typically disabled in that mode, so the current behavior is sufficient for the common case.
- The edge case (ASI enabled + \`--compat-var\` disabled + \`var\` statement with no explicit \`;\`) is a narrow combination but should be handled correctly.
- Backlinks: PR `#368`, comment https://github.com/frostney/GocciaScript/pull/368#discussion_r3112218334
- Reported by: `@frostney`
")
echo "Issue URL: $ISSUE_URL"
# Extract issue number from URL
ISSUE_NUM=$(echo "$ISSUE_URL" | grep -oE '[0-9]+$')
echo "Issue number: $ISSUE_NUM"
# Add milestone via API (milestone number is 9)
gh api --method PATCH "repos/frostney/GocciaScript/issues/$ISSUE_NUM" \
--field milestone=9 \
--jq '{number: .number, title: .title, milestone: .milestone.title, url: .html_url}'
Length of output: 274
@frostney, done! I've opened #371 — "Parser: ASI-aware recovery for unsupported var statement skip (--compat-var disabled)" — and assigned it to the 0.12.0 milestone.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Evaluator.pas:1344-1355
Timestamp: 2026-04-20T19:42:50.788Z
Learning: In frostney/GocciaScript (`source/units/Goccia.Evaluator.pas` and `source/units/Goccia.Scope.pas`), var-scoped bindings are tracked in a dedicated `FVarBindings` map on the scope. `AssignBinding` uses this map to route writes to the correct hoisted var slot on the function/module scope without needing to swap `AContext.Scope` to the function scope. In for-of var destructuring (`EvaluateForOf`/`EvaluateForAwaitOf`), `AssignPattern(..., AIsDeclaration=False)` is used so that `AssignIdentifierPattern` calls `AssignBinding`, which traverses `FVarBindings` to find the hoisted binding, while computed keys and default values still resolve against the block scope. Do NOT add scope-swap patterns (redirecting `IterContext.Scope` to `FindFunctionOrModuleScope`) for var destructuring in loops; `AssignBinding` handles the scope-chain walk automatically via `FVarBindings`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Compiler.Statements.pas:479-491
Timestamp: 2026-04-20T19:43:00.895Z
Learning: In frostney/GocciaScript (PR `#368`, commit 1098b659), the interpreter-side `var` redeclaration handling was refactored to use a dedicated `FVarBindings` map with a `DefineVarBinding(Name, Value, HasInitializer)` method. This method handles redeclaration semantics natively (preserving the existing value when `HasInitializer = false` and the binding already exists). The interpreter no longer uses `ForceUpdateBinding` or `ContainsOwnLexicalBinding` for `var` redeclarations. The compiler path (bytecode) uses `IsVarRedeclaration: Boolean` (set before `DeclareVarLocal` is called) to suppress `OP_LOAD_UNDEFINED` emission for re-declarations without real initializers; the compiler scope's `DeclareVarLocal` returns the existing slot on redeclaration and does NOT trigger the `IsTopLevelGlobalBacked` re-resolve path. Do NOT flag `LocalIdx` overwrite concerns in the `CompileVariableDeclaration` var-path — the `IsVarRedeclaration` boolean correctly captures the pre-declaration state.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T15:16:53.061Z
Learning: Applies to **/Goccia.Parser.pas : The parser must accept unsupported syntax but treat it as a no-op — `function` declarations produce `TGocciaEmptyStatement`, `function` expressions evaluate to `undefined`, `==`/`!=` expressions evaluate to `undefined`, labeled statements strip the label and execute the labeled statement normally. A warning must be emitted to stdout with a suggestion
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Parser.pas:2780-2787
Timestamp: 2026-04-20T16:59:47.231Z
Learning: In `source/units/Goccia.Parser.pas`, the `SkipUntilSemicolon` call in `TGocciaParser.VarStatement`'s unsupported-`var` warning path (when `FVarDeclarationsEnabled = False`) is intentionally not ASI-aware. This path is only reachable when `--compat-var` is disabled, and ASI is also typically disabled in that mode, making the semicolon-based skip sufficient for the common case. The edge-case interaction (ASI enabled + `--compat-var` disabled) is acknowledged as a known gap and deferred to a follow-up improvement. Do NOT re-flag `SkipUntilSemicolon` in this specific fallback path until that follow-up is addressed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Parser.pas:949-961
Timestamp: 2026-04-11T09:44:42.181Z
Learning: In frostney/GocciaScript (units/Goccia.AST.Node.pas, units/Goccia.AST.Statements.pas): `TGocciaProgram` has no destructor and does NOT free its `FBody: TObjectList<TGocciaStatement>` when `Free` is called. `TGocciaExpressionStatement` similarly has no destructor and does NOT free its `FExpression: TGocciaExpression`. Therefore, extracting a child expression from a temporary `TGocciaProgram` (e.g., via `TGocciaExpressionStatement(ProgramNode.Body[0]).Expression`) and then calling `ProgramNode.Free` is safe — the extracted expression is NOT freed and remains valid. This pattern is intentional and is used in `ParseTaggedTemplate` and similar parsing helpers. Do NOT flag this as a dangling-pointer or use-after-free issue.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Evaluator.pas:227-239
Timestamp: 2026-04-20T19:43:32.162Z
Learning: In frostney/GocciaScript (source/units/Goccia.Evaluator.pas), the `CollectVarNamesFromNode` hoisting pre-scan is intentionally scoped to `var` declarations lexically inside function/loop bodies (e.g. `var x` inside a for-of body). For the for-of loop binding itself (`for (var x of arr)`), frostney's design intent is that the binding is created at execution time via `DefineVarBinding` in the `EvaluateForOf` IsVar branch. However, this requires careful handling for empty iterables and pre-loop access: if no iterations occur, `x` is never bound, causing a ReferenceError instead of `undefined`. The fix in commit 38284eb4 pre-declares the loop binding name during the hoist scan to cover this edge case. Do NOT remove the `ForOf.IsVar` binding-collection block from `CollectVarNamesFromNode` without also ensuring empty-iterable and pre-loop-access behaviour is correct.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T07:58:33.798Z
Learning: Applies to **/{Parser,AST}*.pas : type/interface declarations and import type/export type must produce TGocciaEmptyStatement (no-op at runtime)
Learnt from: frostney
Repo: frostney/GocciaScript PR: 368
File: source/units/Goccia.Compiler.Scope.pas:157-165
Timestamp: 2026-04-20T16:59:07.204Z
Learning: In `source/units/Goccia.Compiler.Scope.pas` (frostney/GocciaScript), `DeclareVarLocal` intentionally sets `Depth := 0` on newly declared var-hoisted locals. `EndScope` uses exact equality (`FLocals[FLocalCount-1].Depth = FDepth`) to pop locals, so depth-0 locals are never popped by any inner-scope unwind. `DeclareVarLocal` is only called during the hoisting pre-scan (before `BeginScope` increments depth) or from `CompileVariableDeclaration` where the var binding was already hoisted at depth 0. Do NOT flag `Depth := 0` in `DeclareVarLocal` as a scope-unwinding bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:43-46
Timestamp: 2026-04-14T13:31:59.106Z
Learning: In frostney/GocciaScript (docs/contributing/code-style.md and related docs), units like `BaseMap.pas`, `HashMap.pas`, `OrderedMap.pas`, `OrderedStringMap.pas` are described as "shared infrastructure units that are not GocciaScript-specific" and intentionally live outside the `Goccia.*` namespace. They are NOT "legacy units." Only project-specific units follow the `Goccia.<Category>.<Name>.pas` convention. The naming convention for these shared infrastructure units is tracked in issue `#104`. Do not describe these units as "legacy" in review comments.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:57.064Z
Learning: Repo: frostney/GocciaScript PR: 58
Context: units/Goccia.Runtime.Operations.pas (BridgePromiseStatic, NativePromiseStaticResolve, NativePromiseStaticReject) and interpreter Promise statics in Goccia.Builtins.GlobalPromise.pas
Learning: GocciaScript’s Promise statics are intentionally NOT species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:6259-6271
Timestamp: 2026-03-23T15:59:01.630Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#109`)
As of PR `#109`, Map/Set instances are still wrapped as TGocciaWrappedValue by ToSouffleValue; no construction/wrapping path produces TGocciaSouffleMap/TGocciaSouffleSet yet. The native Map/Set fast paths (e.g., NativeMapDelete/Clear/ForEach) are dormant scaffolding and only trigger for Souffle-backed instances. Therefore, falling back to InvokeGocciaMethod for Map/Set is correct and safe today; do not re-flag this as an invalid cast or behavior until the construction path starts creating TGocciaSouffleMap/TGocciaSouffleSet instances.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 261
File: units/Goccia.Builtins.GlobalObject.pas:0-0
Timestamp: 2026-04-11T06:43:41.435Z
Learning: In frostney/GocciaScript (units/Goccia.Builtins.GlobalObject.pas), all Object.* static methods (Object.keys, Object.values, Object.entries, Object.getOwnPropertyNames, Object.getOwnPropertyDescriptors, Object.getOwnPropertyDescriptor, Object.assign, etc.) intentionally check `is TGocciaObjectValue` and throw for primitives instead of calling ToObject coercion. Primitive coercion (wrapping 42/"str"/true to their wrapper objects) is a known project-wide gap deferred to a dedicated follow-up PR. Do not flag individual Object.* methods for missing ToObject coercion until the project-wide fix lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 0
File: :0-0
Timestamp: 2026-04-11T06:43:54.605Z
Learning: In frostney/GocciaScript, `ToObject` coercion for primitive arguments (e.g., passing a number or string to `Object.getOwnPropertyDescriptor`, `Object.getOwnPropertyDescriptors`, and other `Object.*` static methods) is intentionally deferred. This is a project-wide gap affecting 20+ `Object.*` methods in `units/Goccia.Builtins.GlobalObject.pas`, not specific to any single method. Do not flag missing `ToObject` coercion in individual `Object.*` method PRs as a blocking issue; reference this project-wide deferral instead.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:3436-3437
Timestamp: 2026-04-15T15:41:51.609Z
Learning: Repo frostney/GocciaScript — units/Goccia.VM.pas:
For TGocciaVM.HasPropertyValue’s “in” operator error path (primitives on the RHS), the project intentionally uses SSuggestCheckNullBeforeAccess for all primitive cases (undefined, null, boolean, number, string). Branching the suggestion by value kind is deferred as low priority; do not re-flag this as an issue unless a dedicated follow-up changes that policy.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 354
File: source/units/Goccia.Values.TemporalPlainDate.pas:668-680
Timestamp: 2026-04-19T18:53:31.289Z
Learning: For this project (frostney/GocciaScript), do not raise review findings at individual call sites that use `Arg.ToStringLiteral.Value` on user-supplied arguments for missing Symbol guards (e.g., ensuring `Arg is TGocciaSymbolValue`). This implicit Symbol→string coercion gap is intentionally deferred to a project-wide fix that should be implemented in `ToStringLiteral` itself (or a shared coercion helper). Until that `ToStringLiteral` fix lands, suppress/skip re-flagging the same issue in methods like `DateWithCalendar` and other `withCalendar`/`withTimeZone`-style methods in `source/units`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.Values.TemporalPlainDateTime.pas:144-145
Timestamp: 2026-04-16T06:33:07.065Z
Learning: In frostney/GocciaScript Temporal unit implementations (e.g., units/Goccia.Values.TemporalPlainDateTime.pas, units/Goccia.Values.TemporalPlainDate.pas, units/Goccia.Values.TemporalPlainTime.pas, units/Goccia.Values.TemporalPlainYearMonth.pas, units/Goccia.Values.TemporalPlainMonthDay.pas, units/Goccia.Values.TemporalInstant.pas, units/Goccia.Values.TemporalZonedDateTime.pas, units/Goccia.Builtins.Temporal.pas), the correction of ThrowTypeError → ThrowRangeError for invalid ISO/time/duration string parsing failures (per TC39 Temporal spec) is intentionally deferred to a dedicated follow-up PR. PRs that only centralize error message constants and add suggestion metadata must not change error types (TypeError vs RangeError). Do not re-flag the ThrowTypeError usage for ISO/time/duration parse failures in these Temporal units until the dedicated behavioral-change PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T13:53:21.272Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper (escaping only `\`, `"`, `\n`, `\r`, `\t`, leaving other C0 control characters U+0000–U+001F unescaped) is tracked for consolidation into a shared helper and full fix in issue `#303`. During code review, do not re-flag missing control-character escaping in individual per-tool `EscapeJSONString` copies (e.g., TestRunner.dpr, BenchmarkRunner.dpr) until the consolidation PR landing issue `#303` is resolved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:691-693
Timestamp: 2026-04-17T14:59:54.438Z
Learning: In frostney/GocciaScript (units/Goccia.Values.TemporalDuration.pas), TC39 Temporal Duration magnitude validation (calendar units < 2^32, normalized seconds < 2^53) is intentionally deferred to a follow-up PR. The validation belongs in the `TGocciaTemporalDurationValue` constructor and `DurationFromObject`, not in `DurationEndDate`, `AddMonthsToDate`, or `DurationTotal`. Do not re-flag missing magnitude validation in `DurationEndDate` or `total()` path — the arithmetic there is safe for spec-compliant inputs; the gap is upstream validation not yet implemented.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:13.250Z
Learning: In frostney/GocciaScript, `EscapeJSONString` (escaping `\`, `"`, `\n`, `\r`, `\t` only) is intentionally duplicated across ~5 CLI tool files (e.g., TestRunner.dpr, BenchmarkRunner.dpr). The incomplete control-character coverage (missing U+0000–U+001F beyond the five named characters) is a known pre-existing issue tracked for fix as part of a future consolidation into a shared helper. Do NOT re-flag missing control-character escaping in individual per-tool copies of `EscapeJSONString` until that consolidation PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 324
File: units/Goccia.Values.TemporalDuration.pas:630-656
Timestamp: 2026-04-17T14:39:13.057Z
Learning: In frostney/GocciaScript, `ParseRelativeTo` in `units/Goccia.Values.TemporalDuration.pas` intentionally supports only `TGocciaTemporalPlainDateValue` and ISO date/date-time string inputs for `relativeTo` in `Duration.prototype.total()`. Support for `TGocciaTemporalZonedDateTimeValue` and property-bag/object inputs is deferred to a dedicated follow-up PR, as it requires importing the ZonedDateTime unit, extracting the plain date, and handling DST-aware day lengths. Do not re-flag missing ZonedDateTime/property-bag handling in `ParseRelativeTo` until that follow-up PR lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:0-0
Timestamp: 2026-03-08T04:50:29.540Z
Learning: In format.pas (GocciaScript formatter), ParseParamNames previously misidentified parenthesized arguments inside one-liner function bodies as parameter names, causing the `A` prefix to be prepended to them. The fix (f5b2f23) makes ParseParamNames ignore parentheses appearing after the first semicolon. Be cautious when reviewing one-liner procedure/function bodies in .pas files for incorrectly prefixed identifiers — this was the root cause of the AGC_ANY_CONVERT_EXTERN → AGC_ANY_CONVERT_EXTERN typos in souffle/Souffle.Wasm.Emitter.pas.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 51
File: units/Goccia.AST.Expressions.pas:1022-1025
Timestamp: 2026-03-08T04:34:30.723Z
Learning: In units/Goccia.AST.Expressions.pas: `TGocciaTemplateWithInterpolationExpression` is never instantiated by the parser. All template literals (with and without interpolation) are parsed as `TGocciaTemplateLiteralExpression`. The `TemplateWithInterpolation` variant exists only for the bytecode compiler pipeline (`Goccia.Compiler.Expressions.pas`), which has its own dispatch. The `Evaluate` override returning `undefined` in the interpreter is intentional — it is unreachable code that preserves the pre-existing fall-through behavior. Do NOT flag this as a bug.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Do not add support for these JavaScript features: `var` declarations, `function` keyword, `==` and `!=` loose equality, `eval()` and `arguments` object, automatic semicolon insertion, `with` statement, traditional loops (`for`, `while`, `do...while`), default imports/exports, or global `parseInt`, `parseFloat`, `isNaN`, `isFinite`. The parser may accept some unsupported syntax as no-ops with warnings, but these features must not be implemented.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/language.md:118-118
Timestamp: 2026-04-14T13:18:31.918Z
Learning: In frostney/GocciaScript (units/Goccia.Lexer.pas, ScanTemplate / ProcessEscapeSequence), cooked=undefined support for tagged templates with malformed escape sequences (e.g., \u{, \xG1) landed in PR `#287` (TC39 Template Literal Revision / ES2018). Tagged templates now correctly set the cooked segment to undefined for malformed escapes while preserving the raw source text; untagged templates still throw SyntaxError. Do NOT re-flag this as missing or deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1010-1023
Timestamp: 2026-04-12T09:44:50.402Z
Learning: In frostney/GocciaScript (units/Goccia.Parser.pas, SplitTemplateAtBoundaries and the original ParseTaggedTemplate), the brace-counting used to find the closing `}` of a template interpolation expression (`${...}`) is NOT lexically aware — it does not account for `{`/`}` inside string literals, comments, or nested templates within the expression. This is a known pre-existing limitation that predates this codebase area; it was present in the original `ParseTaggedTemplate` on `main` and is carried forward. Fixing it properly requires lexer-time segmentation (embedding boundary markers during `ScanTemplate` while escape/string context is available). Do NOT re-flag the naive brace counter in SplitTemplateAtBoundaries or ParseTaggedTemplate as a regression or new bug — it is intentionally deferred to a follow-up. The limitation exists because `ScanTemplate` stores the entire template (including `${...}` expressions) as a single flat token with no expression-level lexical analysis.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Type annotations in GocciaScript (TC39 Types as Comments proposal) are parsed but ignored at runtime. Raw type strings are preserved on AST nodes for potential future optimization. Use parser helpers `CollectTypeAnnotation(Terminators)` and `CollectGenericParameters` for type handling.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.Coverage.pas:225-249
Timestamp: 2026-04-07T20:24:40.351Z
Learning: In `units/Goccia.Coverage.pas` `BuildExecutableLineFlags`: `interface Foo {` opening lines are intentionally marked as executable (`AFlags[I] := True`) because they produce `TGocciaEmptyStatement` which the engine does evaluate; interface body lines are suppressed via `TypeBodyDepth`. Single-line `type` aliases (e.g. `type Point = { x: number };`) are also correctly counted as executable for the same reason. The only known gap is multi-line `type` aliases spanning several lines — those continuation lines are not currently suppressed, but this is considered rare in practice and acceptable for now. Do not flag the `interface` opening-line executable marking or single-line `type` handling as bugs.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: units/Goccia.Compiler.Statements.pas:569-570
Timestamp: 2026-04-07T17:12:43.685Z
Learning: In frostney/GocciaScript (units/Goccia.Compiler.Statements.pas, units/Goccia.Engine.Backend.pas, units/Goccia.Scope.Redeclaration.pas), the `EmitGlobalDefine` / `OP_DEFINE_GLOBAL_CONST` calls in `CompileVariableDeclaration`, `CompileClassDeclaration`, `CompileDestructuringDeclaration`, and `CompileEnumDeclaration` intentionally execute *after* the RHS/initializer has been evaluated. Redeclaration protection is handled up-front by `CheckTopLevelRedeclarations` (an AST-level pre-flight walk) called from `TGocciaBytecodeBackend.CompileToModule` (when `GlobalBackedTopLevel` is true) and from `TGocciaEngine.Execute` (after parsing, before execution). This catches all top-level `let`/`const`/class/enum redeclarations before any RHS side effects can run, matching V8 early-error behaviour. Do NOT re-flag the post-evaluation placement of `EmitGlobalDefine` as a side-effect ordering bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 343
File: source/units/Goccia.Evaluator.Arithmetic.pas:49-55
Timestamp: 2026-04-18T21:08:03.166Z
Learning: In frostney/GocciaScript (commit 3344dede, PR `#343`), `TGocciaBigIntObjectValue` was added to support BigInt boxing via `Object(1n)`. `Box` in `ClassHelper.pas` now returns a `TGocciaBigIntObjectValue` for `TGocciaBigIntValue` primitives. `ToPrimitive` calls were added to `EvaluateSubtraction`, `EvaluateMultiplication`, `EvaluateDivision`, `EvaluateModulo`, and `EvaluateExponentiation` in `source/units/Goccia.Evaluator.Arithmetic.pas` so that boxed BigInts (e.g. `Object(1n) - 1n`) are correctly unboxed via `valueOf()` before BigInt type checks. `Object()` was also fixed to call `Box` for primitive arguments per ES2026 §20.1.1.1. Future reviews of arithmetic operators SHOULD expect `ToPrimitive` before BigInt checks.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:2797-2828
Timestamp: 2026-03-23T15:57:21.755Z
Learning: Repo frostney/GocciaScript — PR `#109`: Native Map/Set scaffolding
- Current behavior: ToSouffleValue wraps TGocciaMapValue/TGocciaSetValue as TGocciaWrappedValue; TGocciaSouffleMap/TGocciaSouffleSet instances are not produced in normal execution.
- Pre-wired fast paths: GetProperty, GetIterator, and IteratorNext recognize TGocciaSouffleMap/TGocciaSouffleSet and their iterator types; these are staged for future activation.
- Deferred work: When Construct is updated to produce native collection types, add corresponding fast paths in HasProperty, SetProperty, and DeleteProperty.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 276
File: tests/built-ins/Goccia/build.js:6-7
Timestamp: 2026-04-11T18:29:34.210Z
Learning: In frostney/GocciaScript, the pattern `const hasGoccia = typeof Goccia !== "undefined"` combined with `describe.runIf(hasGoccia)(...)` is intentional and used across the entire `tests/` suite. It allows test files to run safely in non-GocciaScript environments (e.g., Vitest compatibility checks) where GocciaScript-specific globals like `Goccia` are not present. Do NOT flag this conditional-skip guard as a regression risk or suggest making suites unconditional — the portability requirement is by design.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 333
File: source/shared/OrderedMap.Test.pas:387-401
Timestamp: 2026-04-18T03:14:07.382Z
Learning: For Free Pascal (FPC) in Delphi mode (`{$mode delphi}`) used in this repo (Pascal `.pas` files), do not treat a bare indexed-property read used as a standalone statement (e.g., `Map[999];`) as a compile error. In this mode, the getter call is executed and its result is silently discarded; if the getter raises an exception, it still propagates to the surrounding `except` handler as expected. For readability/portability, assigning the result to a dummy local (e.g., `Dummy := Map[999];`) is preferred, but bare indexed-property reads should remain unflagged as long as they are valid under FPC Delphi mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 337
File: source/units/Goccia.Builtins.TSV.pas:382-388
Timestamp: 2026-04-18T13:58:20.551Z
Learning: When reviewing GocciaScript Pascal code, do not flag missing `Args.Free`/`try/finally` around `TGocciaArgumentsCollection` instances. `TGocciaArgumentsCollection` (in `source/units/Goccia.Arguments.Collection.pas`) descends from `TInterfacedObject` and is reference-counted, so objects created via `CreateWithCapacity` or `Create` are automatically freed when the last reference goes out of scope. An explicit `Free` call (or wrapping in `try/finally`) is not appropriate for this type.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 351
File: source/units/Goccia.Engine.pas:1364-1368
Timestamp: 2026-04-19T15:42:49.575Z
Learning: In GocciaScript (e.g., source/units/Goccia.Engine.pas), treat `Goccia.gc()` / `TGocciaEngine.GocciaGC` as intentionally thread-safe: it should be a no-op on worker threads by guarding the manual `GC.Collect` call with `not GIsWorkerThread`. This prevents races on shared singleton state that uses a single, non-thread-safe `FGCMark` field during mark-sweep. Reviewers should not flag the `GIsWorkerThread` guard as a regression or as a missing manual-GC path; only the full/manual collection triggered by `Goccia.gc()` is skipped on workers, while other worker-thread protections (e.g., the memory-limit ceiling check in `TGocciaValue.AfterConstruction`) should still apply.
- Fix hoisting to handle destructuring var declarations (walk patterns) - Fix OP_LOAD_UNDEFINED not emitted for var redeclaration without initializer - Add var hoisting to module body evaluation (imported modules) - Propagate VarDeclarationsEnabled to nested parser in template interpolation - Add IsVar to for-of statement for `for (var x of xs)` semantics - Fix module-level hoisting test to actually be module-scoped - Add --compat-var to CI/PR test commands - Add test for var redeclaration without initializer preserving value Co-Authored-By: Claude Opus 4.6 (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)
source/units/Goccia.Evaluator.pas (1)
1437-1440:⚠️ Potential issue | 🟠 MajorMissing
IsVarhandling inEvaluateForAwaitOf.
EvaluateForOfwas updated withIsVarsupport (lines 1339-1355), butEvaluateForAwaitOflacks equivalent handling.for await (var x of asyncIterable)will treat bindings as lexical instead of function-scoped.The same pattern needs to be applied at both binding sites (lines 1437-1440 for async iterators and lines 1478-1481 for sync fallback).
🔧 Proposed fix for async iterator path
- if AForAwaitOfStatement.BindingPattern <> nil then - AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) - else - IterScope.DefineLexicalBinding(AForAwaitOfStatement.BindingName, CurrentValue, DeclarationType); + if AForAwaitOfStatement.IsVar then + begin + if AForAwaitOfStatement.BindingPattern <> nil then + begin + IterContext.Scope := AContext.Scope.FindFunctionOrModuleScope; + AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext, False); + IterContext.Scope := IterScope; + end + else + AContext.Scope.FindFunctionOrModuleScope.ForceUpdateBinding( + AForAwaitOfStatement.BindingName, CurrentValue); + end + else if AForAwaitOfStatement.BindingPattern <> nil then + AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) + else + IterScope.DefineLexicalBinding(AForAwaitOfStatement.BindingName, CurrentValue, DeclarationType);Apply the same pattern to the sync fallback path at lines 1478-1481.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 1437 - 1440, EvaluateForAwaitOf is missing IsVar handling so bindings from "for await" are always treated as lexical; update the two binding sites in EvaluateForAwaitOf (the async iterator path where AForAwaitOfStatement.BindingPattern / BindingName are handled and the sync fallback path) to mirror the logic added in EvaluateForOf: when AForAwaitOfStatement.IsVar is true define function-scoped bindings (use IterScope.DefineBinding or the equivalent function-scoped define call used in EvaluateForOf) instead of IterScope.DefineLexicalBinding, and when a pattern is present call AssignPattern with the same IsVar-aware arguments (passing CurrentValue, IterContext, True, DeclarationType/IsVar behavior consistent with EvaluateForOf). Ensure both the async path (where AssignPattern / DefineLexicalBinding are invoked) and the sync fallback path receive the same IsVar-aware branching so "for await (var x of ...)" is function-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 479-489: The code overwrites LocalIdx after saving a redeclaration
check, so the subsequent logic (e.g., emitting OP_LOAD_UNDEFINED) sees the wrong
value; add a dedicated boolean (e.g., WasRedeclaration) set right after LocalIdx
:= ACtx.Scope.ResolveLocal(Info.Name) / immediately after calling
ACtx.Scope.DeclareVarLocal(Info.Name) when AStmt.IsVar, preserve that boolean
through the IsTopLevelGlobalBacked branch where LocalIdx may be reassigned via
ResolveLocal, and replace uses of LocalIdx >= 0 in the later checks (the
conditions around OP_LOAD_UNDEFINED and the checks at the locations referenced)
with the new boolean (or use both where appropriate) so the original
redeclaration state is not lost by Declare/ResolveLocal calls.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1339-1355: The var-destructuring branch uses AssignPattern with
AIsDeclaration=True causing lexical-style defines; change it to use assignment
semantics by calling AssignPattern with AIsDeclaration=False when handling
AForOfStatement.IsVar (inside the block that sets IterContext.Scope :=
AContext.Scope.FindFunctionOrModuleScope), mirroring the simple binding case
which uses ForceUpdateBinding; update the call that currently passes True to
instead pass False so AssignPattern performs updates rather than
DefineLexicalBinding.
- Around line 227-232: The for-of branch misses hoisting loop-declared vars:
when ANode is TGocciaForOfStatement and ForOf.IsVar is true you must also
collect binding names from the loop's BindingName or BindingPattern before
traversing the body; update the TGocciaForOfStatement branch to, if ForOf.IsVar,
call CollectVarNamesFromNode on ForOf.BindingName (or ForOf.BindingPattern when
present) in addition to CollectVarNamesFromNode(ForOf.Body) so var bindings like
for(var x of y) get pre-declared into ANames.
---
Outside diff comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1437-1440: EvaluateForAwaitOf is missing IsVar handling so
bindings from "for await" are always treated as lexical; update the two binding
sites in EvaluateForAwaitOf (the async iterator path where
AForAwaitOfStatement.BindingPattern / BindingName are handled and the sync
fallback path) to mirror the logic added in EvaluateForOf: when
AForAwaitOfStatement.IsVar is true define function-scoped bindings (use
IterScope.DefineBinding or the equivalent function-scoped define call used in
EvaluateForOf) instead of IterScope.DefineLexicalBinding, and when a pattern is
present call AssignPattern with the same IsVar-aware arguments (passing
CurrentValue, IterContext, True, DeclarationType/IsVar behavior consistent with
EvaluateForOf). Ensure both the async path (where AssignPattern /
DefineLexicalBinding are invoked) and the sync fallback path receive the same
IsVar-aware branching so "for await (var x of ...)" is function-scoped.
🪄 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: baf123ca-8910-439d-a627-92ed2bcb833f
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/pr.ymlsource/units/Goccia.AST.Statements.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.passource/units/Goccia.Evaluator.passource/units/Goccia.Interpreter.passource/units/Goccia.Parser.pastests/language/var/hoisting.jstests/language/var/redeclaration.js
✅ Files skipped from review due to trivial changes (1)
- tests/language/var/redeclaration.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/language/var/hoisting.js
- source/units/Goccia.Compiler.pas
Instead of requiring --compat-var on the global CLI (which conflicts
with the unsupported-features tests), the TestRunner now discovers
goccia.json per test file. Files in tests/language/var/ pick up
{"compat-var": true} from their directory's config automatically.
Reverts the --compat-var addition to CI/PR workflow commands since
it is no longer needed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use dedicated IsVarRedeclaration boolean instead of LocalIdx which gets overwritten by later ResolveLocal calls - Collect for-of loop binding names during var hoisting scan - Use assignment semantics (IsDeclaration=False) for var destructuring in for-of loops since bindings are already hoisted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1346-1359: In EvaluateForAwaitOf replicate the same var semantics
used in EvaluateForOf: when AForAwaitOfStatement.IsVar is true, set
IterContext.Scope to AContext.Scope.FindFunctionOrModuleScope and call
AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext,
False) for destructuring or use
FindFunctionOrModuleScope.ForceUpdateBinding(AForAwaitOfStatement.BindingName,
CurrentValue) for a simple name; reset IterContext.Scope back to IterScope
afterwards. Otherwise (not IsVar) keep the existing behavior of binding through
IterScope. Update both binding sites around the lines that currently always use
IterScope so for await(var ...) hoists to function/module scope consistently.
- Around line 265-267: The TStringList used to collect hoist names (Names) is
case-insensitive by default, causing distinct identifiers like "foo" and "Foo"
to be merged; after creating the list (TStringList.Create) set
Names.CaseSensitive := True before calling CollectVarNames or performing
IndexOf/deduplication, and apply the same change to the other TStringList
instance used for hoisting at the 281-283 region so all hoist/name-collection
lists are case-sensitive.
🪄 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: 9f2d7af1-8a5b-4569-a468-8f186ad3bfad
📒 Files selected for processing (2)
source/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.pas
Both the async iterator path and sync fallback path now mirror the IsVar-aware branching from EvaluateForOf: when IsVar is true, bindings are defined in the function scope via ForceUpdateBinding (simple name) or AssignPattern with IsDeclaration=False (destructuring pattern). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/units/Goccia.Evaluator.pas (1)
265-267:⚠️ Potential issue | 🟠 MajorMake the hoist-name set case-sensitive.
Names.IndexOf(...)is doing JavaScript identifier dedupe here, but theseTStringListinstances still use the default case-insensitive comparison. That merges distinct bindings likefooandFoo, so only one name gets hoisted/predeclared.Suggested fix
procedure HoistVarDeclarations(const AStatements: TObjectList<TGocciaStatement>; const AScope: TGocciaScope); var Names: TStringList; I: Integer; begin Names := TStringList.Create; + Names.CaseSensitive := True; try CollectVarNames(AStatements, Names);procedure HoistVarDeclarations(const ANodes: TObjectList<TGocciaASTNode>; const AScope: TGocciaScope); var Names: TStringList; I: Integer; begin Names := TStringList.Create; + Names.CaseSensitive := True; try for I := 0 to ANodes.Count - 1 doAlso applies to: 281-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 265 - 267, The TStringList "Names" is created with default case-insensitive comparison so IndexOf merges identifiers like "foo" and "Foo"; after creating Names (and likewise any other TStringList created near lines 281–283), set its CaseSensitive property to True before calling CollectVarNames or using IndexOf so hoisting/predeclaration treats distinct-case identifiers separately (i.e., set Names.CaseSensitive := True immediately after Names := TStringList.Create and do the same for the other TStringList instances).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 1351-1353: The current fix replaces the entire evaluation scope
(IterContext.Scope / VarContext.Scope) with the function/module scope before
calling AssignPattern, which causes computed keys and default initializers to
resolve against the wrong scope; instead preserve the existing evaluation
context and only redirect the binding target: restore
IterContext.Scope/VarContext.Scope to the original value for expression
evaluation, and pass the function/module scope as the explicit target scope to
AssignPattern (i.e., use a separate target scope variable such as
IterScope/VarScope rather than assigning into IterContext.Scope), adjusting
calls to AssignPattern at the sites using IterContext.Scope :=
AContext.Scope.FindFunctionOrModuleScope (and the analogous changes at the other
mentioned locations) so evaluation uses the original scope while identifier
writes go to the function/module scope.
---
Duplicate comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 265-267: The TStringList "Names" is created with default
case-insensitive comparison so IndexOf merges identifiers like "foo" and "Foo";
after creating Names (and likewise any other TStringList created near lines
281–283), set its CaseSensitive property to True before calling CollectVarNames
or using IndexOf so hoisting/predeclaration treats distinct-case identifiers
separately (i.e., set Names.CaseSensitive := True immediately after Names :=
TStringList.Create and do the same for the other TStringList instances).
🪄 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: 53da0a5b-16dd-4605-afe6-8c5ef21b3606
📒 Files selected for processing (1)
source/units/Goccia.Evaluator.pas
Remove scope swaps (IterContext.Scope := FindFunctionOrModuleScope) before calling AssignPattern for var destructuring. Since we use IsDeclaration=False, AssignLexicalBinding walks the parent chain to find the hoisted binding in the function scope. Keeping the block scope as the evaluation context ensures computed keys and default initializers resolve against the correct lexical environment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Architectural rework: var bindings now live in a dedicated FVarBindings map on function/module/global scopes, separate from the lexical binding system. This eliminates the impedance mismatch between var semantics (function-scoped, redeclarable, no TDZ) and lexical semantics (block-scoped, no redeclaration, TDZ). Changes: - Add dtVar to TGocciaDeclarationType (always writable, no TDZ) - Add FVarBindings: TGocciaScopeBindingMap to TGocciaScope (lazily allocated) - Add DefineVarBinding(Name, Value, HasInitializer) API that handles create-or-update semantics and redeclaration preservation natively - Modify GetLexicalBinding to check FVarBindings at each scope level - Modify AssignLexicalBinding to check FVarBindings before parent walk - Update MarkReferences and Contains for GC and lookup correctness - Simplify TGocciaVariableDeclaration.Execute to just call DefineVarBinding - Simplify HoistVarDeclarations to populate the var map directly - Replace ForceUpdateBinding/FindFunctionOrModuleScope hacks in for-of with clean DefineVarBinding calls This removes the need for: - ForceUpdateBinding for var redeclaration - ContainsOwnLexicalBinding guards before DefineLexicalBinding - IsDeclaration=False parent-chain-walk trick for destructuring - Scope swaps (IterContext.Scope := FindFunctionOrModuleScope) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
t3code/8d1b55e1
branch
from
1694706 to
ea8f3ba
Compare
…/AssignIdentifierBinding These methods resolve an identifier name to its binding across the full scope chain, checking both lexical (let/const) and var binding maps. The name "Identifier" makes it clear these resolve a named identifier regardless of binding type. The define/create operations remain properly separated by binding kind: - DefineLexicalBinding — creates let/const/parameter in lexical map - DefineVarBinding — creates var in var map The identifier resolution operations check both maps: - GetIdentifierBinding — walks chain, checks lexical + var at each scope - AssignIdentifierBinding — walks chain, updates in lexical or var map Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
force-pushed
the
t3code/8d1b55e1
branch
from
ea8f3ba to
487a101
Compare
…ding Rename the binding resolution functions to reflect that they resolve any binding type (lexical or variable), not just lexical bindings: - GetIdentifierBinding -> GetBinding - AssignIdentifierBinding -> AssignBinding - DefineVarBinding -> DefineVariableBinding The naming now follows a clear pattern: - DefineLexicalBinding — creates a let/const/parameter binding - DefineVariableBinding — creates a var binding - GetBinding — resolves any binding by name (walks scope chain) - AssignBinding — assigns to any binding by name (walks scope chain) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace all `AssignPattern(..., False)` calls in var paths with a
dedicated `DefineVariablePattern` procedure that walks destructuring
patterns and calls `DefineVariableBinding` for each leaf identifier.
This fixes the case where `for (var {a, b} of arr)` would fail because
the loop binding names aren't pre-hoisted — `AssignPattern` with
`IsDeclaration=False` calls `AssignBinding` which throws if the name
doesn't exist. `DefineVariablePattern` calls `DefineVariableBinding`
which creates-or-updates, so it works correctly on first iteration
without requiring prior hoisting of the loop binding.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follows GocciaScript's Define vs Assign convention: Define creates new bindings, Assign updates existing ones. The pattern walker assigns destructured values into variable bindings (which may already exist from hoisting or prior iterations), so Assign is the correct verb. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
source/units/Goccia.Evaluator.pas (1)
262-290:⚠️ Potential issue | 🟠 MajorUse a case-sensitive name set while hoist-deduping.
These
TStringListinstances still rely on defaultIndexOfsemantics while deduplicating JS identifiers. That merges distinct names likefooandFoo, so only one gets predeclared and the other stays unhoisted.Suggested fix
procedure HoistVarDeclarations(const AStatements: TObjectList<TGocciaStatement>; const AScope: TGocciaScope); var Names: TStringList; I: Integer; begin Names := TStringList.Create; + Names.CaseSensitive := True; try CollectVarNames(AStatements, Names);procedure HoistVarDeclarations(const ANodes: TObjectList<TGocciaASTNode>; const AScope: TGocciaScope); var Names: TStringList; I: Integer; begin Names := TStringList.Create; + Names.CaseSensitive := True; try for I := 0 to ANodes.Count - 1 doIn Free Pascal / Delphi RTL, does TStringList.IndexOf use case-insensitive matching by default unless CaseSensitive := True?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 262 - 290, The TStringList used for collecting/deduping variable names in both HoistVarDeclarations routines is case-insensitive by default; change the creation of Names in each HoistVarDeclarations (the TStringList variable named Names) to enable case-sensitive comparison (e.g. after Names := TStringList.Create set Names.CaseSensitive := True) so identifiers like "foo" and "Foo" are treated as distinct when collecting and defining bindings in AScope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 3929-3957: The TGocciaArrayDestructuringPattern branch only
handles TGocciaArrayValue, causing string and iterable sources to be treated as
undefined; update this branch (inside the APattern is
TGocciaArrayDestructuringPattern handling) to mirror AssignArrayPattern's logic:
detect when AValue is a string or a generic iterable/iterator (in addition to
TGocciaArrayValue), convert those into a sequence you can index/iterate (e.g.,
build a temporary TGocciaArrayValue or enumerate an iterator into elements),
then proceed with the same loop that handles Rest patterns and per-index
assignment via AssignVariablePattern; ensure fallback to
TGocciaUndefinedLiteralValue when an index is out of range and reuse the same
symbols TGocciaArrayDestructuringPattern, TGocciaArrayValue,
TGocciaUndefinedLiteralValue, and AssignVariablePattern so string/iterator
sources produce the same hoisted bindings as arrays.
- Around line 3916-3927: The object-destructuring branch in
AssignVariablePattern (handling TGocciaObjectDestructuringPattern) must mirror
AssignObjectPattern: first guard that AValue is an object and not null and raise
a JS TypeError if not; when iterating ObjPat.Properties handle computed keys
that evaluate to Symbol (use the evaluated key as-is rather than
ToStringLiteral) so symbol-keyed properties work like let/const; and implement
object-rest collection by building a new object with properties from AValue
excluding all assigned keys and then AssignVariablePattern on ObjPat.Rest (or
equivalent rest field) with that object. Ensure you reuse the same helper calls
and property-key extraction logic used by AssignObjectPattern to keep semantics
identical.
---
Duplicate comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 262-290: The TStringList used for collecting/deduping variable
names in both HoistVarDeclarations routines is case-insensitive by default;
change the creation of Names in each HoistVarDeclarations (the TStringList
variable named Names) to enable case-sensitive comparison (e.g. after Names :=
TStringList.Create set Names.CaseSensitive := True) so identifiers like "foo"
and "Foo" are treated as distinct when collecting and defining bindings in
AScope.
🪄 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: 1f9016da-b45f-4ff3-8a8c-5d4ffa484bcc
📒 Files selected for processing (6)
source/units/Goccia.AST.Expressions.passource/units/Goccia.AST.Statements.passource/units/Goccia.Evaluator.passource/units/Goccia.Scope.BindingMap.passource/units/Goccia.Scope.passource/units/Goccia.VM.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.AST.Statements.pas
Comment on lines
+3916
to
+3927
| else if APattern is TGocciaObjectDestructuringPattern then | ||
| begin | ||
| ObjPat := TGocciaObjectDestructuringPattern(APattern); | ||
| for I := 0 to ObjPat.Properties.Count - 1 do | ||
| begin | ||
| if ObjPat.Properties[I].Computed and Assigned(ObjPat.Properties[I].KeyExpression) then | ||
| PropValue := (AValue as TGocciaObjectValue).GetProperty( | ||
| EvaluateExpression(ObjPat.Properties[I].KeyExpression, AContext).ToStringLiteral.Value) | ||
| else | ||
| PropValue := (AValue as TGocciaObjectValue).GetProperty(ObjPat.Properties[I].Key); | ||
| AssignVariablePattern(ObjPat.Properties[I].Pattern, PropValue, AContext); | ||
| end; |
Contributor
There was a problem hiding this comment.
Mirror AssignObjectPattern semantics in the var helper.
This object branch skips the non-object/null guard, symbol-key handling, and object-rest collection that AssignObjectPattern already implements. That makes cases like var {...rest} = obj and var {[sym]: x} = obj behave differently from the let/const path, and a non-object RHS can surface a raw cast failure instead of the expected JS TypeError. Because EvaluateDestructuringDeclaration, EvaluateForOf, and EvaluateForAwaitOf all route var patterns through this helper, the regression is broad.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/units/Goccia.Evaluator.pas` around lines 3916 - 3927, The
object-destructuring branch in AssignVariablePattern (handling
TGocciaObjectDestructuringPattern) must mirror AssignObjectPattern: first guard
that AValue is an object and not null and raise a JS TypeError if not; when
iterating ObjPat.Properties handle computed keys that evaluate to Symbol (use
the evaluated key as-is rather than ToStringLiteral) so symbol-keyed properties
work like let/const; and implement object-rest collection by building a new
object with properties from AValue excluding all assigned keys and then
AssignVariablePattern on ObjPat.Rest (or equivalent rest field) with that
object. Ensure you reuse the same helper calls and property-key extraction logic
used by AssignObjectPattern to keep semantics identical.
Documentation: - Update language.md: var is now "opt-in" with feature documentation - Update language-tables.md: var status changed from "Excluded" to "Opt-in" - Add decision-log.md entry explaining the architectural choice Code fixes from review: - Set CaseSensitive := True on TStringList in HoistVarDeclarations - Add null/undefined guards to AssignVariablePattern - Add string source handling to AssignVariablePattern array branch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coderabbitai
Bot
added
documentation
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)
docs/language.md (1)
9-9:⚠️ Potential issue | 🟡 MinorUpdate Executive Summary to reflect
varas opt-in.Line 9 lists
varunder "Excluded by design," but the detailed section (line 566) now describes it as "Opt-in. Excluded by default" with--compat-varsupport. This creates an inconsistency. Consider movingvarout of the "Excluded by design" bullet or rephrasing to clarify it's available as a compatibility mode.📝 Suggested revision
-- **Excluded by design** — `var`, `function` keyword, `==`/`!=`, `eval`, `arguments`, traditional loops, `with`, default imports/exports +- **Excluded by design** — `function` keyword, `==`/`!=`, `eval`, `arguments`, traditional loops, `with`, default imports/exports +- **Opt-in compatibility** — `var` (via `--compat-var`), ASI (via `--asi`)Or alternatively, update line 11:
-- **Opt-in toggles** — ASI (`--asi` / `Engine.ASIEnabled := True`) +- **Opt-in toggles** — ASI (`--asi` / `Engine.ASIEnabled := True`), `var` declarations (`--compat-var` / `Engine.VarEnabled := True`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/language.md` at line 9, Update the Executive Summary's "Excluded by design" bullet so it no longer lists `var` as wholly excluded; instead reflect that `var` is excluded by default but available opt-in via the `--compat-var` compatibility flag (or move `var` out of that bullet and add a parenthetical note about `--compat-var`), ensuring the summary matches the detailed section that describes `var` as "Opt-in. Excluded by default."
♻️ Duplicate comments (2)
source/units/Goccia.Evaluator.pas (2)
3918-3933:⚠️ Potential issue | 🟠 MajorMirror
AssignObjectPatternsemantics in thevarhelper.This branch still diverges from
AssignObjectPattern: onlynull/undefinedare guarded, so non-object RHS values can still blow up on the cast at Line 3928; computed symbol keys are coerced to strings; and object rest is never collected. That breaks cases likevar {[sym]: x} = objandvar {...rest} = obj.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 3918 - 3933, The var-branch for TGocciaObjectDestructuringPattern must mirror AssignObjectPattern: first guard against any non-object RHS (not just null/undefined) before casting AValue to TGocciaObjectValue; for computed property keys use the same ToPropertyKey/coercion path used in AssignObjectPattern (evaluate the KeyExpression and convert to a property key instead of ToStringLiteral.Value) so symbols are handled correctly; and implement collection of the object rest properties into the rest pattern (handle TGocciaObjectRestProperty like AssignObjectPattern does) before assigning individual properties. Update the branch around TGocciaObjectDestructuringPattern, referencing ObjPat, GetProperty and AssignVariablePattern to follow the same checks and helper calls used by AssignObjectPattern.
3935-3994:⚠️ Potential issue | 🟠 MajorKeep
vararray destructuring iterable-aware.Unlike
AssignArrayPattern, this path never falls back toGetIteratorFromValue, sovar [x] = someIterable()andfor (var [x] of someIterator())silently leave the hoisted binding atundefinedinstead of consuming the iterator. The array-rest branch also copies raw holes, while the lexical path normalizes them toundefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Evaluator.pas` around lines 3935 - 3994, The TGocciaArrayDestructuringPattern branch currently only handles TGocciaArrayValue and TGocciaStringLiteralValue and therefore fails to consume generic iterables; update the else branch in the TGocciaArrayDestructuringPattern handling so that when AValue is neither TGocciaArrayValue nor TGocciaStringLiteralValue you call GetIteratorFromValue(AValue, AContext) and iterate it to produce element values exactly as AssignArrayPattern does (including producing TGocciaUndefinedLiteralValue for holes and collecting remaining iterator results into a TGocciaArrayValue for TGocciaRestDestructuringPattern), then call AssignVariablePattern for each element/rest argument; ensure the rest branch uses the iterator (not raw holes) and normalizes holes to undefined so hoisted var bindings are consumed and mirror AssignArrayPattern behavior.
🤖 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 `@docs/language.md`:
- Line 9: Update the Executive Summary's "Excluded by design" bullet so it no
longer lists `var` as wholly excluded; instead reflect that `var` is excluded by
default but available opt-in via the `--compat-var` compatibility flag (or move
`var` out of that bullet and add a parenthetical note about `--compat-var`),
ensuring the summary matches the detailed section that describes `var` as
"Opt-in. Excluded by default."
---
Duplicate comments:
In `@source/units/Goccia.Evaluator.pas`:
- Around line 3918-3933: The var-branch for TGocciaObjectDestructuringPattern
must mirror AssignObjectPattern: first guard against any non-object RHS (not
just null/undefined) before casting AValue to TGocciaObjectValue; for computed
property keys use the same ToPropertyKey/coercion path used in
AssignObjectPattern (evaluate the KeyExpression and convert to a property key
instead of ToStringLiteral.Value) so symbols are handled correctly; and
implement collection of the object rest properties into the rest pattern (handle
TGocciaObjectRestProperty like AssignObjectPattern does) before assigning
individual properties. Update the branch around
TGocciaObjectDestructuringPattern, referencing ObjPat, GetProperty and
AssignVariablePattern to follow the same checks and helper calls used by
AssignObjectPattern.
- Around line 3935-3994: The TGocciaArrayDestructuringPattern branch currently
only handles TGocciaArrayValue and TGocciaStringLiteralValue and therefore fails
to consume generic iterables; update the else branch in the
TGocciaArrayDestructuringPattern handling so that when AValue is neither
TGocciaArrayValue nor TGocciaStringLiteralValue you call
GetIteratorFromValue(AValue, AContext) and iterate it to produce element values
exactly as AssignArrayPattern does (including producing
TGocciaUndefinedLiteralValue for holes and collecting remaining iterator results
into a TGocciaArrayValue for TGocciaRestDestructuringPattern), then call
AssignVariablePattern for each element/rest argument; ensure the rest branch
uses the iterator (not raw holes) and normalizes holes to undefined so hoisted
var bindings are consumed and mirror AssignArrayPattern behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78740ab6-cb1e-45a2-b81c-333f6119e7ed
📒 Files selected for processing (4)
docs/decision-log.mddocs/language-tables.mddocs/language.mdsource/units/Goccia.Evaluator.pas
✅ Files skipped from review due to trivial changes (2)
- docs/decision-log.md
- docs/language-tables.md
- Resolve decision-log.md conflict (keep both entries) - Update executive summary: move var from "excluded" to opt-in toggles - Add generic iterable fallback to AssignVariablePattern array branch (mirrors AssignArrayPattern for iterators beyond Array/String) - Add Iterator/IterResult vars to AssignVariablePattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
source/units/Goccia.Compiler.Statements.pas (1)
2797-2811:⚠️ Potential issue | 🟠 MajorTop-level global-backed
vardestructuring bindings are never marked or emitted.When
AStmt.IsVardestructuring is compiled at global scope withGlobalBackedTopLevel = true,CollectDestructuringVarBindingscallsDeclareVarLocalfor each identifier. SinceDeclareVarLocalis idempotent (returns existing slot on redeclaration), and hoisting has already pre-declared all identifiers duringDoCompileFunctionBody, the second call inCompileDestructuringDeclarationadds no new locals. ThusLocalCountBefore == ACtx.Scope.LocalCountafter the call, leaving bothMarkGlobalBackedandEmitGlobalDefineloops with an empty iteration range. The hoisted bindings are skipped entirely.
CompileVariableDeclarationavoids this by re-resolving each binding after the slot allocation path completes (rather than relying on a limited range).CompileDestructuringDeclarationrequires the same treatment: re-resolve each leaf identifier in the pattern and callMarkGlobalBackedandEmitGlobalDefineindividually, matching the single-variable path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Statements.pas` around lines 2797 - 2811, CompileDestructuringDeclaration currently assumes new locals appear in the ACtx.Scope range between LocalCountBefore and ACtx.Scope.LocalCount, but when hoisting/DeclareVarLocal pre-declared slots this range can be empty and top-level var destructuring globals are skipped; change the end of CompileDestructuringDeclaration (after EmitDestructuring/FreeRegister) to walk the destructuring pattern's leaf identifiers and for each identifier re-resolve its Local via the same lookup used in CompileVariableDeclaration, then call ACtx.Scope.MarkGlobalBacked(LocalIndex) and EmitGlobalDefine(ACtx, Local.Slot, Local.Name, AStmt.IsConst) for each leaf; reference functions/idents: CompileDestructuringDeclaration, CollectDestructuringVarBindings, DeclareVarLocal, DoCompileFunctionBody, EmitDestructuring, ACtx.Scope.MarkGlobalBacked, EmitGlobalDefine.
🧹 Nitpick comments (1)
source/units/Goccia.Parser.pas (1)
2851-2897: Consider extracting shared declaration parsing logic to reduce drift risk.
VarStatementandDeclarationStatementnow duplicate most declarator/destructuring parsing behavior. A shared helper would make future semantics changes safer and easier to keep in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Parser.pas` around lines 2851 - 2897, The code duplicates declarator/destructuring parsing between VarStatement and DeclarationStatement; extract the shared logic into a single helper (e.g., ParseDeclarators or ParseDeclarationDeclarators) that handles: detecting destructuring via Check(gttLeftBracket)/Check(gttLeftBrace) and calling ParsePattern, optional type via CollectTypeAnnotation, consuming initializer with Consume(gttAssign) or creating an undefined literal, and consuming the trailing semicolon via ConsumeSemicolonOrASI; have the helper return a discriminated result (pattern + initializer + TypeAnnotation or a list of variable records) so callers can construct TGocciaDestructuringDeclaration or TGocciaVariableDeclaration, preserving uses of ParsePattern, CollectTypeAnnotation, Consume, ConsumeSemicolonOrASI, TGocciaDestructuringDeclaration and TGocciaVariableDeclaration.
🤖 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 `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 2797-2811: CompileDestructuringDeclaration currently assumes new
locals appear in the ACtx.Scope range between LocalCountBefore and
ACtx.Scope.LocalCount, but when hoisting/DeclareVarLocal pre-declared slots this
range can be empty and top-level var destructuring globals are skipped; change
the end of CompileDestructuringDeclaration (after
EmitDestructuring/FreeRegister) to walk the destructuring pattern's leaf
identifiers and for each identifier re-resolve its Local via the same lookup
used in CompileVariableDeclaration, then call
ACtx.Scope.MarkGlobalBacked(LocalIndex) and EmitGlobalDefine(ACtx, Local.Slot,
Local.Name, AStmt.IsConst) for each leaf; reference functions/idents:
CompileDestructuringDeclaration, CollectDestructuringVarBindings,
DeclareVarLocal, DoCompileFunctionBody, EmitDestructuring,
ACtx.Scope.MarkGlobalBacked, EmitGlobalDefine.
---
Nitpick comments:
In `@source/units/Goccia.Parser.pas`:
- Around line 2851-2897: The code duplicates declarator/destructuring parsing
between VarStatement and DeclarationStatement; extract the shared logic into a
single helper (e.g., ParseDeclarators or ParseDeclarationDeclarators) that
handles: detecting destructuring via Check(gttLeftBracket)/Check(gttLeftBrace)
and calling ParsePattern, optional type via CollectTypeAnnotation, consuming
initializer with Consume(gttAssign) or creating an undefined literal, and
consuming the trailing semicolon via ConsumeSemicolonOrASI; have the helper
return a discriminated result (pattern + initializer + TypeAnnotation or a list
of variable records) so callers can construct TGocciaDestructuringDeclaration or
TGocciaVariableDeclaration, preserving uses of ParsePattern,
CollectTypeAnnotation, Consume, ConsumeSemicolonOrASI,
TGocciaDestructuringDeclaration and TGocciaVariableDeclaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46104987-ffa2-4eee-aafd-89ff6a3c7a55
📒 Files selected for processing (10)
docs/decision-log.mddocs/language.mdsource/units/Goccia.AST.Expressions.passource/units/Goccia.AST.Statements.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.VM.passource/units/Goccia.Values.FunctionValue.pas
✅ Files skipped from review due to trivial changes (2)
- source/units/Goccia.VM.pas
- source/units/Goccia.AST.Expressions.pas
🚧 Files skipped from review as they are similar to previous changes (6)
- source/units/Goccia.Values.FunctionValue.pas
- docs/language.md
- source/units/Goccia.Engine.pas
- docs/decision-log.md
- source/units/Goccia.AST.Statements.pas
- source/units/Goccia.Evaluator.pas
When var destructuring runs at top-level with global backing enabled, DeclareVarLocal reuses pre-hoisted slots so LocalCountBefore equals LocalCount — the existing loop does nothing. Add EmitGlobalDefinesForPattern that walks the pattern tree, resolves each leaf identifier, and emits MarkGlobalBacked + EmitGlobalDefine per binding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
added a commit
that referenced
this pull request
Apr 21, 2026
Resolve conflict in GocciaREPL.dpr: incorporate --compat-var support from main (#368) into the CLI.Application-based rewrite. CreateEngine handles VarEnabled on the engine; added Parser.VarDeclarationsEnabled in the bytecode path's manual parser creation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 21, 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
--compat-var/CompatVarsupport to enablevardeclarations through the CLI, engine, interpreter, parser, compiler, bundler, REPL, loader, benchmark runner, and test runner.varhoisting and redeclaration semantics in both interpreter and compiler paths, including destructuring support and function/module-scope binding behavior.vardeclaration behavior, scoping, hoisting, closures, redeclarations, and destructuring.