Fix class expression super() bytecode crash in decorator wrapping benchmark#56
Conversation
CompileClassExpression used AllocateRegister() for the super class
register, making it invisible to upvalue capture. Constructors in
class expressions that called super() would resolve __super__ to nil,
causing a fatal error at runtime.
Mirror CompileClassDeclaration by using DeclareLocal('__super__', False)
so that nested method scopes can capture the super binding via upvalue.
This fixes the 'class decorator (wrapping)' benchmark producing 0 ops/sec
in bytecode mode.
Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughA new test suite validates class expressions extending dynamic bases with various inheritance patterns, including constructor super() calls, spread arguments, implicit super behavior, additional methods, and decorator wrapping. The compiler's superclass reference handling is modified to use a dedicated local variable slot instead of a temporary register. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 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 |
Benchmark Results254 benchmarks (no baseline) arraybuffer.js — 14 benchmarks
arrays.js — 19 benchmarks
async-await.js — 6 benchmarks
classes.js — 31 benchmarks
closures.js — 11 benchmarks
collections.js — 12 benchmarks
destructuring.js — 22 benchmarks
fibonacci.js — 8 benchmarks
for-of.js — 7 benchmarks
iterators.js — 20 benchmarks
json.js — 20 benchmarks
jsx.jsx — 21 benchmarks
numbers.js — 11 benchmarks
objects.js — 7 benchmarks
promises.js — 12 benchmarks
strings.js — 11 benchmarks
typed-arrays.js — 22 benchmarks
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
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)
units/Goccia.Compiler.Statements.pas (1)
1903-1906:⚠️ Potential issue | 🟠 MajorRemove stale
FreeRegistercall forSuperReg.Line 1906 calls
FreeRegisterforSuperReg, but this is incorrect now thatSuperRegis allocated viaDeclareLocal(line 1806) rather thanAllocateRegister. Locals declared withDeclareLocalare managed by the scope system and freed viaEndScope, notFreeRegister.Compare with
CompileClassDeclaration(lines 1757-1760) which does not callFreeRegisterfor itsSuperReg.🐛 Proposed fix: remove the FreeRegister call
if HasSuper then - begin - CompileDecoratorAndAccessorPass(ACtx, ADest, ClassDef, SuperReg); - ACtx.Scope.FreeRegister; - end + CompileDecoratorAndAccessorPass(ACtx, ADest, ClassDef, SuperReg) else CompileDecoratorAndAccessorPass(ACtx, ADest, ClassDef, -1);🤖 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 1903 - 1906, The call ACtx.Scope.FreeRegister after CompileDecoratorAndAccessorPass should be removed because SuperReg is now created via DeclareLocal (not AllocateRegister) and is managed by the scope system; locate the HasSuper branch in CompileDecoratorAndAccessorPass/CompileClassDeclaration where SuperReg is used and delete the ACtx.Scope.FreeRegister invocation so SuperReg is freed via EndScope like other locals.
🤖 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 `@units/Goccia.Compiler.Statements.pas`:
- Around line 1903-1906: The call ACtx.Scope.FreeRegister after
CompileDecoratorAndAccessorPass should be removed because SuperReg is now
created via DeclareLocal (not AllocateRegister) and is managed by the scope
system; locate the HasSuper branch in
CompileDecoratorAndAccessorPass/CompileClassDeclaration where SuperReg is used
and delete the ACtx.Scope.FreeRegister invocation so SuperReg is freed via
EndScope like other locals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44c0e38b-33bb-4588-b008-31b3d23c5043
📒 Files selected for processing (2)
tests/language/classes/class-expression-extends.jsunits/Goccia.Compiler.Statements.pas
Fix a fatal error in bytecode mode for class expressions extending dynamic values with
super()calls.The
CompileClassExpressionincorrectly usedAllocateRegister()for the super register, preventing__super__from being resolved via upvalue capture in constructors, leading to a crash and the "class decorator (wrapping)" benchmark reporting 0 ops/sec.Summary by CodeRabbit
Release Notes
Tests
Refactor