Conversation
- Add `--mode=bytecode` and `--timing` output for REPL runs - Teach the bytecode backend and VM to track top-level global bindings - Update compiler emission for top-level globals, classes, destructuring, and enums
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a REPL Changes
Sequence DiagramsequenceDiagram
participant REPL as REPL Loop
participant Lexer as Lexer
participant Parser as Parser
participant Compiler as Compiler
participant Backend as Bytecode Backend
participant VM as VM
participant MTQ as Microtask Queue
loop Each Input Line (bytecode mode)
REPL->>Lexer: ScanTokens(input)
Lexer-->>Parser: tokens
Parser-->>Compiler: AST
Compiler-->>Backend: module (GlobalBackedTopLevel flag)
Backend->>VM: execute(module bytecode)
VM-->>Backend: result
Backend-->>REPL: formatted result (+timings)
REPL->>MTQ: Drain microtask queue
MTQ-->>REPL: processed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Route OP_DEFINE_GLOBAL_CONST through explicit let/const binding - Remove unused global value helper and stale compiler emission
![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: 4
🧹 Nitpick comments (1)
units/Goccia.VM.pas (1)
4872-4880: Consider named constants for declaration-type flags.The values
1(let) and2(const) are opcode operand conventions. While the coding guidelines exempt trivial literals, introducing named constants likeGLOBAL_DECL_LET = 1andGLOBAL_DECL_CONST = 2inGoccia.Bytecode.paswould improve readability and maintainability, especially for the compiler emission side.💡 Suggested constant definitions (in Goccia.Bytecode.pas or similar)
const GLOBAL_DECL_VAR = 0; // legacy: DefineGlobalValue (upsert semantics) GLOBAL_DECL_LET = 1; // dtLet: strict redeclaration check GLOBAL_DECL_CONST = 2; // dtConst: strict redeclaration check, immutableThen in the VM:
OP_DEFINE_GLOBAL_CONST: begin GlobalName := Template.GetConstantUnchecked(C).StringValue; case B of - 1: DefineGlobalBinding(GlobalName, GetRegister(A), dtLet); - 2: DefineGlobalBinding(GlobalName, GetRegister(A), dtConst); + GLOBAL_DECL_LET: DefineGlobalBinding(GlobalName, GetRegister(A), dtLet); + GLOBAL_DECL_CONST: DefineGlobalBinding(GlobalName, GetRegister(A), dtConst); else DefineGlobalValue(GlobalName, GetRegister(A)); end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.VM.pas` around lines 4872 - 4880, The code uses magic numeric flags (1,2) for global declaration types; introduce named constants (e.g., GLOBAL_DECL_VAR = 0, GLOBAL_DECL_LET = 1, GLOBAL_DECL_CONST = 2) in Goccia.Bytecode.pas and replace the numeric checks in the VM branch that uses Template.GetConstantUnchecked(C).StringValue with those constants so the case becomes: GLOBAL_DECL_LET -> DefineGlobalBinding(..., dtLet), GLOBAL_DECL_CONST -> DefineGlobalBinding(..., dtConst), otherwise -> DefineGlobalValue(...); update any emitter/compiler code that emits these operand values to use the new constants as well to keep encoding/decoding consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@REPL.dpr`:
- Around line 155-168: The timing for the "Lex" bucket currently starts before
StringListToLFText and TGocciaJSXTransformer.Transform, so JSX preprocessing is
being measured as lexing; adjust timing by moving the StartTime (and the
StartTime/lex timer boundary logic) to immediately before the call to
Lexer.ScanTokens (i.e., after StringListToLFText and after
TGocciaJSXTransformer.Transform when ggJSX is enabled), or alternatively add a
distinct timer bucket for the transform step (capture transform start/end around
TGocciaJSXTransformer.Transform and log it separately), and make the same change
for the other occurrence around lines 189-195 that also wraps
StringListToLFText/Transform/ScanTokens.
- Around line 181-187: ResultValue is a Pascal-held temporary that can be
collected during DrainQueue or FormatREPLValue; protect it by calling the
existing AddTempRoot(ResultValue) immediately after Backend.RunModule returns
and call RemoveTempRoot(ResultValue) after the WriteLn/FormatREPLValue is
finished (or if ResultValue is nil skip protection), and when triggering GC from
TGocciaMicrotaskQueue.Instance.DrainQueue use the protected
CollectIfNeeded(AProtect) path so the temp-root is honored; update the block
around ResultValue, DrainQueue and FormatREPLValue to add/remove the temp root
and use CollectIfNeeded(AProtect).
In `@units/Goccia.Compiler.Statements.pas`:
- Around line 571-572: Move the global redeclaration preflight so it runs before
evaluating RHS/initializer: when IsTopLevelGlobalBacked is true, call
EmitGlobalDefine (or an equivalent "predeclare" path using ACtx, Slot,
Info.Name, AStmt.IsConst) before executing the RHS/class
body/destructuring/enum, and then perform the actual initialization after
evaluation completes; update the same pattern at the other occurrences noted
(around the locations corresponding to the blocks at lines ~2074-2075,
~2273-2278, ~2326-2327) so bindings are rejected up-front and side effects in
evaluation cannot run before DefineGlobalBinding rejects redeclarations.
In `@units/Goccia.VM.pas`:
- Around line 2433-2439: DefineGlobalBinding calls DefineLexicalBinding which
can raise TGocciaSyntaxError (e.g. during OP_DEFINE_GLOBAL_CONST redeclaration),
but the VM loop only handles
TGocciaTypeError/TGocciaReferenceError/TGocciaRuntimeError; add a
TGocciaSyntaxError handler in the VM execution loop (alongside the existing
exception handlers) that converts the TGocciaSyntaxError into an
EGocciaBytecodeThrow carrying a SyntaxError object (matching the pattern used
for other errors), or alternatively update DefineGlobalBinding to pre-check for
existing global bindings and raise a caught error type instead; reference
TGocciaSyntaxError, EGocciaBytecodeThrow, DefineGlobalBinding,
DefineLexicalBinding and OP_DEFINE_GLOBAL_CONST when applying the change.
---
Nitpick comments:
In `@units/Goccia.VM.pas`:
- Around line 4872-4880: The code uses magic numeric flags (1,2) for global
declaration types; introduce named constants (e.g., GLOBAL_DECL_VAR = 0,
GLOBAL_DECL_LET = 1, GLOBAL_DECL_CONST = 2) in Goccia.Bytecode.pas and replace
the numeric checks in the VM branch that uses
Template.GetConstantUnchecked(C).StringValue with those constants so the case
becomes: GLOBAL_DECL_LET -> DefineGlobalBinding(..., dtLet), GLOBAL_DECL_CONST
-> DefineGlobalBinding(..., dtConst), otherwise -> DefineGlobalValue(...);
update any emitter/compiler code that emits these operand values to use the new
constants as well to keep encoding/decoding consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f27b729-6c52-46d4-a7ba-41861e083c57
📒 Files selected for processing (7)
AGENTS.mdREPL.dprunits/Goccia.Compiler.Context.pasunits/Goccia.Compiler.Statements.pasunits/Goccia.Compiler.pasunits/Goccia.Engine.Backend.pasunits/Goccia.VM.pas
Comment on lines
+155
to
+168
| StartTime := GetNanoseconds; | ||
| SourceText := StringListToLFText(Source); | ||
|
|
||
| if ggJSX in TGocciaEngine.DefaultGlobals then | ||
| begin | ||
| JSXResult := TGocciaJSXTransformer.Transform(SourceText); | ||
| SourceText := JSXResult.Source; | ||
| JSXResult.SourceMap.Free; | ||
| end; | ||
|
|
||
| Lexer := TGocciaLexer.Create(SourceText, REPL_FILE_NAME); | ||
| try | ||
| Tokens := Lexer.ScanTokens; | ||
| LexEnd := GetNanoseconds; |
Contributor
There was a problem hiding this comment.
The reported Lex time currently includes JSX preprocessing.
StartTime is taken before StringListToLFText and TGocciaJSXTransformer.Transform, but the first bucket is labeled Lex. On JSX inputs that overstates lexing and makes the new phase breakdown misleading. Start the lex timer immediately before ScanTokens, or report transform/preprocess as its own bucket.
Also applies to: 189-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@REPL.dpr` around lines 155 - 168, The timing for the "Lex" bucket currently
starts before StringListToLFText and TGocciaJSXTransformer.Transform, so JSX
preprocessing is being measured as lexing; adjust timing by moving the StartTime
(and the StartTime/lex timer boundary logic) to immediately before the call to
Lexer.ScanTokens (i.e., after StringListToLFText and after
TGocciaJSXTransformer.Transform when ggJSX is enabled), or alternatively add a
distinct timer bucket for the transform step (capture transform start/end around
TGocciaJSXTransformer.Transform and log it separately), and make the same change
for the other occurrence around lines 189-195 that also wraps
StringListToLFText/Transform/ScanTokens.
Owner
Author
There was a problem hiding this comment.
Not a bug — this matches the timing convention in both Engine.Execute and ScriptLoader's ParseSource. All three measure Lex = LexEnd - StartTime where StartTime is captured before JSX transform. This is consistent across every CLI (ScriptLoader, TestRunner, BenchmarkRunner) and the engine itself. The JSX pre-pass is part of the "source → tokens" pipeline, so including it in the Lex bucket is intentional.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the clear explanation. I'll note this convention for future reviews.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: Repo: frostney/GocciaScript — REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute all share the same timing convention: `StartTime` is captured before JSX transformation (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, and `LexEnd` is captured after `ScanTokens`. The "Lex" bucket (`LexEnd - StartTime`) intentionally includes the JSX pre-pass because the JSX transform is considered part of the "source → tokens" pipeline. Do not flag this as a bug or suggest moving `StartTime` to after the JSX transform.
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-06T21:38:39.013Z
Learning: Applies to **/*{Bytecode,VM}*.pas : Bytecode mode uses the same runtime objects as the interpreter. Use `Goccia.Bytecode*` and `Goccia.VM*` names in new code. Use `.gbc`, not `.sbc`. WASM emission removed pre-1.0.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-06T21:38:39.013Z
Learning: Applies to **/*JSX*.pas : JSX is handled by standalone pre-pass transformer (`Goccia.JSX.Transformer.pas`) converting JSX syntax to `createElement` calls. Transformer generates internal source map for correct error reporting. Zero overhead when `ggJSX` not enabled.
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: 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: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:02.668Z
Learning: Repo: frostney/GocciaScript — ScopeMap.pas (units/ScopeMap.pas) was deleted (commit d99cf96) after profiling showed TScopeMap's linear-scan IndexOf was ~2.7× slower than hash-based lookup (CreateBridgedContext samples rose from 24% to 51%). Scope bindings now use TOrderedStringMap<TLexicalBinding> (DJB2 hash, O(1) per level); scope chain walking is done via recursive TGocciaScope-level methods (Contains/GetLexicalBinding) rather than a parent-pointer chain inside the map. Do not flag TScopeMap or its Parent property in future reviews — the type no longer exists.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to **/*.pas : Type annotations in GocciaScript (parsed from source) are preserved as raw strings on AST nodes. In interpreted mode, annotations are ignored at runtime. In bytecode mode, the compiler uses annotations and inferred types to emit `OP_CHECK_TYPE` guards and typed opcodes — reassignment to an incompatible type throws `TypeError` at runtime.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 105
File: docs/optimization-log.md:45-46
Timestamp: 2026-03-18T15:34:27.613Z
Learning: Applies to units/Goccia.Compiler*.pas|units/Goccia.Runtime.Operations.pas|Goccia.Compiler.ExtOps.pas|Goccia.Compiler.Statements.pas: As of PR `#75`, `GOCCIA_EXT_EVAL_CLASS`, `EvaluateClassByIndex`, and `FPendingClasses` are completely removed from the codebase. Decorated classes are NOT deferred to the interpreter. ALL classes (with or without decorators) are compiled natively via `GOCCIA_EXT_BEGIN_DECORATORS` / `APPLY_ELEMENT_DECORATOR` / `FINISH_DECORATORS` in the bytecode pipeline. Do NOT flag decorator compilation as a deferred/bridge path.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 137
File: AGENTS.md:102-109
Timestamp: 2026-03-30T07:34:23.743Z
Learning: In frostney/GocciaScript (units/Goccia.VM*.pas), the bytecode VM register file uses a tagged `TGocciaRegister` type, not `TGocciaValue` directly. Hot scalar kinds (undefined, null, hole, booleans, integers, floats) stay unboxed in registers; object/runtime boundaries materialize `TGocciaValue` instances when needed. Do NOT describe the VM register file as storing `TGocciaValue` directly in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
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: 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: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.
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 7 improved · 🔴 226 regressed · 41 unchanged · avg -5.1% arraybuffer.js — Interp: 🟢 1, 🔴 13 · avg -6.7% · Bytecode: 🟢 1, 🔴 13 · avg -7.4%
arrays.js — Interp: 🔴 17, 2 unch. · avg -6.1% · Bytecode: 🔴 17, 2 unch. · avg -7.0%
async-await.js — Interp: 🔴 6 · avg -7.2% · Bytecode: 🔴 6 · avg -9.1%
classes.js — Interp: 🔴 25, 6 unch. · avg -5.2% · Bytecode: 🔴 12, 19 unch. · avg -3.8%
closures.js — Interp: 🔴 11 · avg -6.0% · Bytecode: 🔴 9, 2 unch. · avg -4.2%
collections.js — Interp: 🔴 9, 3 unch. · avg -5.3% · Bytecode: 🔴 11, 1 unch. · avg -6.3%
destructuring.js — Interp: 🔴 15, 7 unch. · avg -3.6% · Bytecode: 🔴 19, 3 unch. · avg -6.8%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -7.8% · Bytecode: 🔴 7, 1 unch. · avg -10.4%
for-of.js — Interp: 🔴 5, 2 unch. · avg -3.4% · Bytecode: 🔴 7 · avg -9.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 18, 2 unch. · avg -4.8% · Bytecode: 🔴 20 · avg -6.8%
json.js — Interp: 🔴 20 · avg -7.1% · Bytecode: 🔴 19, 1 unch. · avg -9.3%
jsx.jsx — Interp: 🔴 14, 7 unch. · avg -2.9% · Bytecode: 🔴 16, 5 unch. · avg -5.0%
modules.js — Interp: 🔴 9 · avg -5.3% · Bytecode: 🔴 8, 1 unch. · avg -2.3%
numbers.js — Interp: 🔴 10, 1 unch. · avg -8.1% · Bytecode: 🔴 10, 1 unch. · avg -7.6%
objects.js — Interp: 🟢 1, 🔴 1, 5 unch. · avg -0.6% · Bytecode: 🔴 7 · avg -6.3%
promises.js — Interp: 🔴 10, 2 unch. · avg -4.2% · Bytecode: 🔴 12 · avg -6.7%
regexp.js — Interp: 🔴 11 · avg -4.6% · Bytecode: 🔴 9, 2 unch. · avg -7.8%
strings.js — Interp: 🔴 10, 1 unch. · avg -7.6% · Bytecode: 🔴 10, 1 unch. · avg -8.9%
typed-arrays.js — Interp: 🟢 5, 🔴 16, 1 unch. · avg -3.0% · Bytecode: 🟢 6, 🔴 16 · avg -4.3%
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. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
units/Goccia.Compiler.Statements.pas (1)
569-570:⚠️ Potential issue | 🟠 MajorPredeclare the binding before any initializer/class/enum work runs.
Line 569, Line 2072, Line 2271, and Line 2324 still emit the global define only after user code has already executed. In bytecode REPL mode, redeclaring an existing top-level
let/constcan therefore run RHS side effects, decorators,extendsexpressions, destructuring getters, or enum member initializers beforeDefineGlobalBindingrejects the redeclaration. This still breaks lexical declaration semantics and needs a predeclare/preflight step before evaluation.Also applies to: 2072-2073, 2271-2275, 2324-2325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Statements.pas` around lines 569 - 570, The current code calls EmitGlobalDefine(ACtx, Slot, Info.Name, AStmt.IsConst) only after running initializer/class/enum work (guarded by IsTopLevelGlobalBacked), which allows RHS side effects to run before the binding is rejected; to fix, move a pre-declaration step so that for top-level, global-backed lexical declarations you call EmitGlobalDefine (or a new DefineGlobalBindingPredeclare helper) before any initializer, decorator, extends expression, destructuring getter, or enum member initializer logic is executed in the same code paths that currently check IsTopLevelGlobalBacked and then emit later; update all sites that currently call EmitGlobalDefine after evaluation (the occurrences using IsTopLevelGlobalBacked/EmitGlobalDefine) to perform the predeclare first and then proceed with initializer/evaluation, preserving the existing EmitGlobalDefine semantics for final binding setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@units/Goccia.Compiler.Statements.pas`:
- Around line 569-570: The current code calls EmitGlobalDefine(ACtx, Slot,
Info.Name, AStmt.IsConst) only after running initializer/class/enum work
(guarded by IsTopLevelGlobalBacked), which allows RHS side effects to run before
the binding is rejected; to fix, move a pre-declaration step so that for
top-level, global-backed lexical declarations you call EmitGlobalDefine (or a
new DefineGlobalBindingPredeclare helper) before any initializer, decorator,
extends expression, destructuring getter, or enum member initializer logic is
executed in the same code paths that currently check IsTopLevelGlobalBacked and
then emit later; update all sites that currently call EmitGlobalDefine after
evaluation (the occurrences using IsTopLevelGlobalBacked/EmitGlobalDefine) to
perform the predeclare first and then proceed with initializer/evaluation,
preserving the existing EmitGlobalDefine semantics for final binding setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b04b7d3c-b942-4f71-9fae-893aaf688ae7
📒 Files selected for processing (3)
units/Goccia.Compiler.Expressions.pasunits/Goccia.Compiler.Statements.pasunits/Goccia.VM.pas
💤 Files with no reviewable changes (1)
- units/Goccia.Compiler.Expressions.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.VM.pas
- Preserve timing output when execution raises - Store last script timing on the engine for REPL display
- Check global scope bindings before running scripts or compiling bytecode - Surface duplicate top-level declarations from destructuring, classes, enums, and variables
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Engine.Backend.pas (1)
95-180: Consider extracting shared redeclaration logic to avoid duplication.
CollectDestructuringPatternNamesandCheckTopLevelRedeclarationsare nearly identical toCollectPatternRedeclarationsandCheckRedeclarationsinunits/Goccia.Engine.pas. This duplication increases maintenance burden and risks divergence.Consider extracting these to a shared unit (e.g.,
Goccia.Redeclaration.pas) and reusing in bothTGocciaEngine.ExecuteandTGocciaBytecodeBackend.CompileToModule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Engine.Backend.pas` around lines 95 - 180, The two functions here (CollectDestructuringPatternNames and CheckTopLevelRedeclarations) duplicate logic present as CollectPatternRedeclarations and CheckRedeclarations in units/Goccia.Engine.pas; extract the shared redeclaration traversal and error-raising logic into a single reusable unit (e.g., TGocciaRedeclaration or unit Goccia.Redeclaration) that exposes functions like CollectPatternRedeclarations/CheckRedeclarations (or generic names CollectDestructuringPatternNames/CheckTopLevelRedeclarations moved) and then replace the local implementations in both TGocciaEngine.Execute and TGocciaBytecodeBackend.CompileToModule to call the shared routines, updating uses clauses and preserving the exact error messages and parameter lists (APattern, AScope, ASourcePath, TGocciaProgram, etc.) so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Engine.Backend.pas`:
- Around line 95-180: The two functions here (CollectDestructuringPatternNames
and CheckTopLevelRedeclarations) duplicate logic present as
CollectPatternRedeclarations and CheckRedeclarations in units/Goccia.Engine.pas;
extract the shared redeclaration traversal and error-raising logic into a single
reusable unit (e.g., TGocciaRedeclaration or unit Goccia.Redeclaration) that
exposes functions like CollectPatternRedeclarations/CheckRedeclarations (or
generic names CollectDestructuringPatternNames/CheckTopLevelRedeclarations
moved) and then replace the local implementations in both TGocciaEngine.Execute
and TGocciaBytecodeBackend.CompileToModule to call the shared routines, updating
uses clauses and preserving the exact error messages and parameter lists
(APattern, AScope, ASourcePath, TGocciaProgram, etc.) so behavior stays
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e1fbda-cc47-46e5-a947-cb37fb36e8c5
📒 Files selected for processing (3)
REPL.dprunits/Goccia.Engine.Backend.pasunits/Goccia.Engine.pas
- Move program and pattern redeclaration logic into `Goccia.Scope.Redeclaration` - Update engine backend to reuse the shared checker - Keep bytecode and interpreter redeclaration handling aligned
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Scope.Redeclaration.pas (1)
31-38: Line/column information lost for destructuring pattern errors.When a redeclaration is detected in a destructuring pattern, the error is raised with
0, 0for line/column. IfTGocciaDestructuringPatternor its subclasses have position information available, consider propagating it for better error diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Scope.Redeclaration.pas` around lines 31 - 38, The redeclaration error for TGocciaIdentifierDestructuringPattern is being raised with hardcoded line/column 0,0; update the TGocciaSyntaxError.Create call to pass the actual source position from the pattern (e.g. use position/line/column properties available on TGocciaDestructuringPattern or TGocciaIdentifierDestructuringPattern) instead of 0,0, falling back to the existing ASourcePath and the current 0,0 only if no position info exists; modify the code near AScope.ContainsOwnLexicalBinding and the TGocciaSyntaxError.Create invocation to read and propagate the pattern's position fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Scope.Redeclaration.pas`:
- Around line 31-38: The redeclaration error for
TGocciaIdentifierDestructuringPattern is being raised with hardcoded line/column
0,0; update the TGocciaSyntaxError.Create call to pass the actual source
position from the pattern (e.g. use position/line/column properties available on
TGocciaDestructuringPattern or TGocciaIdentifierDestructuringPattern) instead of
0,0, falling back to the existing ASourcePath and the current 0,0 only if no
position info exists; modify the code near AScope.ContainsOwnLexicalBinding and
the TGocciaSyntaxError.Create invocation to read and propagate the pattern's
position fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57410ba2-1119-4ce5-8452-5005006785b2
📒 Files selected for processing (3)
units/Goccia.Engine.Backend.pasunits/Goccia.Engine.pasunits/Goccia.Scope.Redeclaration.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.Engine.Backend.pas
- Root REPL results during timing so microtask draining cannot collect them early - Route TGocciaSyntaxError through bytecode exception handlers as SyntaxError
This was referenced Apr 12, 2026
5 tasks
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
--mode=bytecodeto the REPL so it can run through the bytecode VM instead of the interpreter.--timingoutput for bytecode REPL runs, reporting lex, parse, compile, execute, and total durations per line.let/constsemantics for top-level bindings.Testing
./build.pas REPL./build/REPL --mode=bytecode --timing./build.pas testrunner && ./build/TestRunner testsSummary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation