Open
Conversation
- preserve numeric type hints through assignments and captures - enable typed arithmetic for inferred locals and annotated params - avoid unnecessary for-of handlers while keeping abrupt-close paths
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds helpers to infer/store non-strict local type hints, centralizes strict-local type checks, updates assignment and compound-assignment paths to maintain/clear hints, reorders parameter strict-type binding, and emits for...of iterator-close handlers only when the loop body requires closing. ChangesLocal Type Inference & Assignment
Iterator-Close Optimization for For-Of
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelsbug 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 18 minutes and 15 seconds. Comment |
coderabbitai
Bot
added
implement-feedback
performance
Contributor
Suite TimingTest Runner (interpreted: 8,515 passed; bytecode: 8,515 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 10 improved · 🔴 333 regressed · 64 unchanged · avg -6.2% arraybuffer.js — Interp: 🟢 1, 🔴 12, 1 unch. · avg -4.9% · Bytecode: 🟢 7, 7 unch. · avg +1.8%
arrays.js — Interp: 🔴 19 · avg -7.3% · Bytecode: 🟢 2, 🔴 2, 15 unch. · avg -0.2%
async-await.js — Interp: 🔴 4, 2 unch. · avg -5.9% · Bytecode: 🔴 3, 3 unch. · avg -2.4%
async-generators.js — Interp: 🔴 2 · avg -10.5% · Bytecode: 2 unch. · avg -0.1%
base64.js — Interp: 🔴 10 · avg -8.7% · Bytecode: 🔴 5, 5 unch. · avg -2.3%
classes.js — Interp: 🔴 26, 5 unch. · avg -6.1% · Bytecode: 🟢 5, 🔴 6, 20 unch. · avg -0.7%
closures.js — Interp: 🔴 11 · avg -8.1% · Bytecode: 🟢 2, 9 unch. · avg +0.6%
collections.js — Interp: 🔴 11, 1 unch. · avg -7.7% · Bytecode: 🟢 2, 🔴 5, 5 unch. · avg -0.7%
csv.js — Interp: 🔴 13 · avg -6.4% · Bytecode: 🔴 2, 11 unch. · avg -1.0%
destructuring.js — Interp: 🔴 22 · avg -7.1% · Bytecode: 🟢 1, 🔴 3, 18 unch. · avg -0.9%
fibonacci.js — Interp: 🔴 7, 1 unch. · avg -7.3% · Bytecode: 🟢 1, 🔴 2, 5 unch. · avg -0.6%
float16array.js — Interp: 🟢 3, 🔴 22, 7 unch. · avg -3.5% · Bytecode: 🟢 11, 🔴 1, 20 unch. · avg +2.4%
for-of.js — Interp: 🔴 5, 2 unch. · avg -4.0% · Bytecode: 🟢 3, 4 unch. · avg +6.0%
generators.js — Interp: 🔴 4 · avg -6.0% · Bytecode: 🟢 2, 2 unch. · avg +2.8%
iterators.js — Interp: 🔴 33, 9 unch. · avg -6.6% · Bytecode: 🟢 1, 🔴 13, 28 unch. · avg -1.7%
json.js — Interp: 🔴 20 · avg -8.2% · Bytecode: 🟢 3, 🔴 7, 10 unch. · avg -2.1%
jsx.jsx — Interp: 🔴 13, 8 unch. · avg -4.7% · Bytecode: 🟢 11, 10 unch. · avg +3.0%
modules.js — Interp: 🔴 8, 1 unch. · avg -7.3% · Bytecode: 🟢 2, 🔴 1, 6 unch. · avg +0.7%
numbers.js — Interp: 🔴 11 · avg -9.2% · Bytecode: 🔴 8, 3 unch. · avg -4.1%
objects.js — Interp: 🔴 7 · avg -6.5% · Bytecode: 🟢 4, 3 unch. · avg +2.8%
promises.js — Interp: 🔴 4, 8 unch. · avg -1.7% · Bytecode: 🟢 2, 10 unch. · avg +1.9%
regexp.js — Interp: 🔴 11 · avg -7.2% · Bytecode: 🔴 5, 6 unch. · avg -2.0%
strings.js — Interp: 🔴 19 · avg -10.6% · Bytecode: 🟢 2, 🔴 2, 15 unch. · avg +0.4%
tsv.js — Interp: 🔴 3, 6 unch. · avg -4.1% · Bytecode: 9 unch. · avg -0.2%
typed-arrays.js — Interp: 🟢 3, 🔴 15, 4 unch. · avg -0.0% · Bytecode: 🟢 3, 🔴 13, 6 unch. · avg -15.7%
uint8array-encoding.js — Interp: 🔴 11, 7 unch. · avg -13.2% · Bytecode: 🟢 15, 3 unch. · avg +49.3%
weak-collections.js — Interp: 🟢 3, 🔴 10, 2 unch. · avg -1.0% · Bytecode: 🟢 5, 🔴 2, 8 unch. · avg +10.3%
Deterministic profile difffloat16array
for-of
generators
typed-arrays
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
test262 Conformance
Areas closest to 100%
Compatibility roadmap skips
Non-blocking. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.Compiler.Expressions.pas (2)
604-608:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the local type hint before the global-backed early return.
This branch exits before the non-strict refresh at Lines 620-625. Because
ExpressionTypeinsource/units/Goccia.Compiler.Statements.passtill trustsLocal.TypeHintfor non-captured locals, a top-levellet x = 1; x = 'a'; x + 1can keep taking the typed-numeric path after the string assignment.Suggested fix
if Local.IsGlobalBacked then begin + if not Local.IsStrictlyTyped then + begin + ValueType := InferredExpressionType(ACtx.Scope, AExpr.Value); + ACtx.Scope.SetLocalTypeHint(LocalIdx, ValueType); + ACtx.Template.SetLocalType(Slot, ValueType); + end; EmitInstruction(ACtx, EncodeABx(OP_SET_GLOBAL, ADest, ACtx.Template.AddConstantString(AExpr.Name))); Exit; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Expressions.pas` around lines 604 - 608, Before exiting the Local.IsGlobalBacked branch, update the local's type hint using the same non-strict refresh logic used later (lines ~620-625) so ExpressionType doesn't keep stale Local.TypeHint for non-captured locals; i.e., after emitting OP_SET_GLOBAL (EmitInstruction/EncodeABx/OP_SET_GLOBAL/ADest/ACtx.Template.AddConstantString(AExpr.Name)) perform the same Local.TypeHint refresh/assignment that the later non-strict path does (use the same logic that updates Local.TypeHint based on AExpr) before the Exit.
2265-2317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShort-circuit and global-backed compound assignments still leave stale hints behind.
These exits never touch the local's inferred type. After
x ||= 'a',x &&= obj, or a global-backedx += y, the slot can still be recorded as numeric from an earlier write, and laterExpressionType-driven codegen will keep emitting typed arithmetic for a value that is no longer provably numeric. For the short-circuit forms, the safe fallback is usuallysltUntypedunless both branches are known compatible.Also applies to: 2386-2398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Expressions.pas` around lines 2265 - 2317, The short-circuit and global-backed compound assignment paths don't update the local's inferred type, leaving stale TypeHint (e.g., numeric) after assignments like x ||= 'a' or global-backed x += y; locate the branches handling IsShortCircuitAssignment(AExpr.Operator) where ResolveLocal(AExpr.Name) and GetLocal(LocalIdx) are used (the global-backed block that emits OP_GET_GLOBAL/OP_SET_GLOBAL and the local Slot block that emits OP_SET_LOCAL), and after performing the actual assignment (both in the global-backed else branch and the local else branch after ACtx.CompileExpression/AEmit OP_SET_LOCAL) clear or update the local's inferred type (e.g., set GetLocal(LocalIdx).TypeHint := sltUntyped or call the appropriate setter) so subsequent codegen doesn't rely on an invalid prior type; do the same in the short-circuit exit paths where you PatchJumpTarget or Exit to ensure the local's TypeHint is corrected whenever the assignment may change type.
🧹 Nitpick comments (2)
source/units/Goccia.Compiler.Test.pas (2)
633-635: ⚡ Quick winAvoid depending on the current nested-function order.
GetFunction(0)assumes the arrow function is always emitted first. If the compiler starts materializing another helper template ahead of it, this test becomes flaky even though the generated bytecode is still correct.♻️ Suggested guard
+ Expect<Integer>(Module.TopLevel.FunctionCount).ToBe(1); Func := Module.TopLevel.GetFunction(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Test.pas` around lines 633 - 635, The test assumes the arrow function is at GetFunction(0) which is brittle; instead locate the emitted function by content not index. Replace the direct GetFunction(0) usage by scanning Module.TopLevel functions (e.g. iterate Module.TopLevel.Functions or use a FindFunctionByName helper) to pick the function where CountOp(FuncCandidate, OP_ADD_FLOAT) > 0 (or where the arrow function name matches), assign that to Func, then run Expect<Integer>(CountOp(Func, OP_ADD_FLOAT)).ToBe(1) and Expect<Integer>(CountOp(Func, OP_ADD)).ToBe(0); update references to GetFunction(0) accordingly so the test no longer depends on emission order.
613-615: ⚡ Quick winPin the optimization flags in these regression tests.
The opcode checks are still running with const propagation and dead-branch elimination enabled. That makes the tests easier to satisfy accidentally if another optimization pass changes, instead of exercising the typed-arithmetic / handler-emission paths directly.
♻️ Suggested adjustment
- False, False, False, False); + False, False, False, False, False, False);Apply the same explicit
Falsevalues to the other new regression cases as well.Also applies to: 629-631, 645-647, 660-666, 679-681, 694-696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Test.pas` around lines 613 - 615, The regression tests call CompileSource without pinning optimization flags, so enable passing explicit False for the optimization booleans; update each CompileSource invocation (e.g., the call assigning Module := CompileSource('let a = 1; let b = 2; let c = a + b; c;', False, False, False, False)) and apply the same four False arguments to the other new regression cases (the calls around the regions noted: the calls currently at the locations corresponding to 629-631, 645-647, 660-666, 679-681, 694-696) so const-propagation and dead-branch-elimination are disabled and the tests exercise typed-arithmetic/handler-emission paths directly.
🤖 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 774-855: The predicate StatementNeedsIteratorClose is too
permissive (returns False for unrecognized statements) so some exception paths
skip OP_ITER_CLOSE; make it conservative by default: change the final Result
from False to True (so any statement not explicitly proven safe will require
iterator close), and review/adjust any explicit False exits (e.g. the
BlockStatement and SwitchStatement exits) only if you can prove they cannot
raise; refer to the function StatementNeedsIteratorClose and the
CompileForOfStatement paths that rely on it (also re-check similar logic around
lines referenced in the comment).
---
Outside diff comments:
In `@source/units/Goccia.Compiler.Expressions.pas`:
- Around line 604-608: Before exiting the Local.IsGlobalBacked branch, update
the local's type hint using the same non-strict refresh logic used later (lines
~620-625) so ExpressionType doesn't keep stale Local.TypeHint for non-captured
locals; i.e., after emitting OP_SET_GLOBAL
(EmitInstruction/EncodeABx/OP_SET_GLOBAL/ADest/ACtx.Template.AddConstantString(AExpr.Name))
perform the same Local.TypeHint refresh/assignment that the later non-strict
path does (use the same logic that updates Local.TypeHint based on AExpr) before
the Exit.
- Around line 2265-2317: The short-circuit and global-backed compound assignment
paths don't update the local's inferred type, leaving stale TypeHint (e.g.,
numeric) after assignments like x ||= 'a' or global-backed x += y; locate the
branches handling IsShortCircuitAssignment(AExpr.Operator) where
ResolveLocal(AExpr.Name) and GetLocal(LocalIdx) are used (the global-backed
block that emits OP_GET_GLOBAL/OP_SET_GLOBAL and the local Slot block that emits
OP_SET_LOCAL), and after performing the actual assignment (both in the
global-backed else branch and the local else branch after
ACtx.CompileExpression/AEmit OP_SET_LOCAL) clear or update the local's inferred
type (e.g., set GetLocal(LocalIdx).TypeHint := sltUntyped or call the
appropriate setter) so subsequent codegen doesn't rely on an invalid prior type;
do the same in the short-circuit exit paths where you PatchJumpTarget or Exit to
ensure the local's TypeHint is corrected whenever the assignment may change
type.
---
Nitpick comments:
In `@source/units/Goccia.Compiler.Test.pas`:
- Around line 633-635: The test assumes the arrow function is at GetFunction(0)
which is brittle; instead locate the emitted function by content not index.
Replace the direct GetFunction(0) usage by scanning Module.TopLevel functions
(e.g. iterate Module.TopLevel.Functions or use a FindFunctionByName helper) to
pick the function where CountOp(FuncCandidate, OP_ADD_FLOAT) > 0 (or where the
arrow function name matches), assign that to Func, then run
Expect<Integer>(CountOp(Func, OP_ADD_FLOAT)).ToBe(1) and
Expect<Integer>(CountOp(Func, OP_ADD)).ToBe(0); update references to
GetFunction(0) accordingly so the test no longer depends on emission order.
- Around line 613-615: The regression tests call CompileSource without pinning
optimization flags, so enable passing explicit False for the optimization
booleans; update each CompileSource invocation (e.g., the call assigning Module
:= CompileSource('let a = 1; let b = 2; let c = a + b; c;', False, False, False,
False)) and apply the same four False arguments to the other new regression
cases (the calls around the regions noted: the calls currently at the locations
corresponding to 629-631, 645-647, 660-666, 679-681, 694-696) so
const-propagation and dead-branch-elimination are disabled and the tests
exercise typed-arithmetic/handler-emission paths directly.
🪄 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: 688aae8e-4e14-4ce6-a665-597babf8bcef
📒 Files selected for processing (3)
source/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.Test.pas
Make for-of iterator-close analysis conservative for throwing statement forms, refresh stale inferred type hints in global-backed and short-circuit assignment paths, and tighten compiler regression tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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.Expressions.pas (1)
627-632:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing strict-type guard before
OP_SET_GLOBAL.These global-backed branches still bypass
OP_CHECK_TYPE, so a strict top-level local can be reassigned to an incompatible value without trapping, even though the non-global local and upvalue paths enforce it.💡 Minimal direction
if Local.IsGlobalBacked then begin + if Local.IsStrictlyTyped and (Local.TypeHint <> sltUntyped) and + not TypesAreCompatible(InferLocalType(AExpr.Value), Local.TypeHint) then + EmitInstruction(ACtx, EncodeABC(OP_CHECK_TYPE, ADest, + UInt8(Ord(Local.TypeHint)), 0)); EmitInstruction(ACtx, EncodeABx(OP_SET_GLOBAL, ADest, ACtx.Template.AddConstantString(AExpr.Name))); RefreshNonStrictLocalTypeHint(ACtx, LocalIdx, AExpr.Value); Exit; end;Mirror the same check in the short-circuit and non-short-circuit compound-assignment branches, validating the computed result register before the final
OP_SET_GLOBAL.Also applies to: 2294-2313, 2411-2422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Expressions.pas` around lines 627 - 632, The global-backed branch (when Local.IsGlobalBacked) emits OP_SET_GLOBAL without the strict-type guard, allowing reassignment of a strict top-level local to an incompatible value; update the branch in Goccia.Compiler.Expressions (the block emitting EncodeABx(OP_SET_GLOBAL, ADest, ACtx.Template.AddConstantString(AExpr.Name)) and calling RefreshNonStrictLocalTypeHint) to mirror the short-circuit and non-short-circuit compound-assignment paths by inserting the same OP_CHECK_TYPE (or equivalent strict-type validation) for the computed result register before emitting OP_SET_GLOBAL so the value is validated against the local's strict type (and still call RefreshNonStrictLocalTypeHint for non-strict cases); apply the same change to the other two similar blocks noted (around lines 2294-2313 and 2411-2422).
🤖 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.Test.pas`:
- Around line 195-201: FindFunctionWithOp currently skips checking the root
ATemplate and only inspects its children; change the search to first test the
template itself (e.g., if CountOp(ATemplate, AOp) > 0 then Exit(ATemplate))
before iterating children, and when recursing with FindFunctionWithOp(Candidate,
AOp) check the returned Result and Exit(Result) immediately if non-nil so the
found candidate bubbles up (use symbols ATemplate, CountOp, AOp, Candidate,
FindFunctionWithOp).
---
Outside diff comments:
In `@source/units/Goccia.Compiler.Expressions.pas`:
- Around line 627-632: The global-backed branch (when Local.IsGlobalBacked)
emits OP_SET_GLOBAL without the strict-type guard, allowing reassignment of a
strict top-level local to an incompatible value; update the branch in
Goccia.Compiler.Expressions (the block emitting EncodeABx(OP_SET_GLOBAL, ADest,
ACtx.Template.AddConstantString(AExpr.Name)) and calling
RefreshNonStrictLocalTypeHint) to mirror the short-circuit and non-short-circuit
compound-assignment paths by inserting the same OP_CHECK_TYPE (or equivalent
strict-type validation) for the computed result register before emitting
OP_SET_GLOBAL so the value is validated against the local's strict type (and
still call RefreshNonStrictLocalTypeHint for non-strict cases); apply the same
change to the other two similar blocks noted (around lines 2294-2313 and
2411-2422).
🪄 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: 7fba2814-7c7d-430a-84f9-d6fd8561b8a3
📒 Files selected for processing (4)
source/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Compiler.Test.pastests/language/for-of/for-of-iterators.js
Include root templates in function opcode lookup and emit strict type guards before global-backed assignment stores, including short-circuit and compound assignment paths.
Owner
Author
|
Addressed the latest CodeRabbit findings in
Verification run:
I replied directly to the inline CodeRabbit thread as well. Marking the thread resolved still fails because the authenticated GitHub user has exhausted its GraphQL rate limit. |
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.Expressions.pas (1)
2450-2469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck strict local compound-assignment results before writing back to the slot.
EmitInstruction(..., Slot, Slot, RegVal)mutates the local beforeEmitStrictLocalTypeCheck(...)runs. In strict mode,a += "x"will throw, but a surroundingcatchcan still observe the already-corrupted local afterward. Compute into a temp register, validate there, then commit withOP_SET_LOCAL.Proposed fix
- if IsKnownNumeric(LocalType) and - IsKnownNumeric(ValType) and - IsArithmeticCompoundAssign(AExpr.Operator, ArithOp) and - TryFloatOp(ArithOp, FloatOp) then - begin - EmitInstruction(ACtx, EncodeABC(FloatOp, Slot, Slot, RegVal)) - end - else - EmitInstruction(ACtx, EncodeABC(Op, Slot, Slot, RegVal)); + RegResult := ACtx.Scope.AllocateRegister; + if IsKnownNumeric(LocalType) and + IsKnownNumeric(ValType) and + IsArithmeticCompoundAssign(AExpr.Operator, ArithOp) and + TryFloatOp(ArithOp, FloatOp) then + begin + EmitInstruction(ACtx, EncodeABC(FloatOp, RegResult, Slot, RegVal)) + end + else + EmitInstruction(ACtx, EncodeABC(Op, RegResult, Slot, RegVal)); ResultType := sltUntyped; if IsArithmeticCompoundAssign(AExpr.Operator, ArithOp) and IsKnownNumeric(LocalType) and IsKnownNumeric(ValType) then ResultType := sltFloat; - EmitStrictLocalTypeCheck(ACtx, LocalIdx, Slot, ResultType); + EmitStrictLocalTypeCheck(ACtx, LocalIdx, RegResult, ResultType); + EmitInstruction(ACtx, EncodeABx(OP_SET_LOCAL, RegResult, Slot)); if not ACtx.Scope.GetLocal(LocalIdx).IsStrictlyTyped then begin SetNonStrictLocalTypeHint(ACtx, LocalIdx, ResultType); end; if ADest <> Slot then EmitInstruction(ACtx, EncodeABC(OP_MOVE, ADest, Slot, 0)); ACtx.Scope.FreeRegister; + ACtx.Scope.FreeRegister;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Expressions.pas` around lines 2450 - 2469, The current EmitInstruction writes the compound-assignment result directly into the local slot (via EncodeABC(..., Slot, Slot, RegVal)) before EmitStrictLocalTypeCheck runs, allowing strict-mode exceptions to leave the local mutated; instead compute into a temporary register, run EmitStrictLocalTypeCheck against that temp, and only on success commit the temp into the local with a separate set-local instruction; update the block that handles numeric and non-numeric compound assigns (the code using LocalType, ValType, ArithOp, FloatOp, Slot, RegVal) to emit to a temp reg (e.g. RegTemp) via EncodeABC/EmitInstruction, call EmitStrictLocalTypeCheck(ACtx, LocalIdx, RegTemp, ResultType) and then EmitInstruction to write RegTemp into the local (and still call SetNonStrictLocalTypeHint when needed).
🤖 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.Expressions.pas`:
- Around line 2450-2469: The current EmitInstruction writes the
compound-assignment result directly into the local slot (via EncodeABC(...,
Slot, Slot, RegVal)) before EmitStrictLocalTypeCheck runs, allowing strict-mode
exceptions to leave the local mutated; instead compute into a temporary
register, run EmitStrictLocalTypeCheck against that temp, and only on success
commit the temp into the local with a separate set-local instruction; update the
block that handles numeric and non-numeric compound assigns (the code using
LocalType, ValType, ArithOp, FloatOp, Slot, RegVal) to emit to a temp reg (e.g.
RegTemp) via EncodeABC/EmitInstruction, call EmitStrictLocalTypeCheck(ACtx,
LocalIdx, RegTemp, ResultType) and then EmitInstruction to write RegTemp into
the local (and still call SetNonStrictLocalTypeHint when needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98ce0440-77aa-437d-9474-0198f9f44340
📒 Files selected for processing (2)
source/units/Goccia.Compiler.Expressions.passource/units/Goccia.Compiler.Test.pas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
for...ofexception handlers; emit no handler for plain loops and one loop-level handler only when iterator close is neededVerification
./build.pas tests loader benchmarkrunner./build/Goccia.Compiler.Test(27/27passed)./build/GocciaTestRunner tests --asi --unsafe-ffi --no-progress(8484/8484passed)./format.pas --checkgit diff --checkOP_ADD_FLOAT; folded2 + 3 * 4emits no add/mul; nbody emits noOP_PUSH_HANDLER/OP_POP_HANDLER