Unify blueprint bridge on ConvertBlueprintToClassValue#100
Conversation
Fixes #99. The UnwrapToGocciaValue path previously used the simpler BlueprintToClassValue which omitted getters/setters, static fields, private members, and field initializers. If a blueprint was first seen via value unwrapping, the cached simpler bridge could be reused by Construct, dropping richer semantics. Unify both paths on ConvertBlueprintToClassValue so the cache always holds the full-fidelity class value. Remove the now-unused BlueprintToClassValue function. Add targeted tests for bridge consistency covering static methods, getters/setters, private fields, and instanceof identity across value-unwrap and construction paths. Made-with: Cursor
|
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)
📝 WalkthroughWalkthroughThe change removes the public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Remove implementation-detail test file (blueprint-bridge-consistency.js). Merge the two uncovered scenarios into existing language-level tests: - Class reference preserving instanceof → basic-class-declaration.js - Getters/setters with private backing field → getters-setters.js Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/classes/blueprint-bridge-consistency.js`:
- Around line 21-101: Add a deterministic regression test that first captures a
class as a value and only afterwards constructs it to reproduce the “value path
first, construct path second” failure: create a class (e.g., RichClass) with a
static method/property, a private field and an accessor, then assign it to a
variable (e.g., RichClassRef = RichClass) to simulate the value-bridge path,
construct an instance via new RichClassRef(), and assert that instance
instanceof RichClass, instance.__proto__.constructor === RichClass, static
members (RichClass.increment or a static getter) behave correctly, and private
field accessors (instance.reveal or getter) return expected values; add this as
a new test case alongside the existing class tests (use a descriptive test name
like "value-then-construct preserves prototype, statics and privates") so it
runs deterministically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4af3664d-79e6-4215-9780-21defe1bc21a
📒 Files selected for processing (2)
tests/language/classes/blueprint-bridge-consistency.jsunits/Goccia.Runtime.Operations.pas
Benchmark Results254 benchmarks Interpreted: 🔴 1 regressed · 253 unchanged · avg +0.6% arraybuffer.js — Interp: 14 unch. · avg +0.4% · Bytecode: 14 unch. · avg +0.5%
arrays.js — Interp: 19 unch. · avg +1.0% · Bytecode: 19 unch. · avg +0.1%
async-await.js — Interp: 6 unch. · avg +1.9% · Bytecode: 6 unch. · avg +0.1%
classes.js — Interp: 31 unch. · avg +1.6% · Bytecode: 🟢 1, 30 unch. · avg +1.2%
closures.js — Interp: 11 unch. · avg +0.2% · Bytecode: 🟢 5, 6 unch. · avg +6.7%
collections.js — Interp: 12 unch. · avg -0.7% · Bytecode: 12 unch. · avg -1.4%
destructuring.js — Interp: 22 unch. · avg +2.0% · Bytecode: 🟢 1, 21 unch. · avg +3.0%
fibonacci.js — Interp: 8 unch. · avg -0.7% · Bytecode: 🟢 1, 7 unch. · avg +1.8%
for-of.js — Interp: 7 unch. · avg +3.2% · Bytecode: 🔴 1, 6 unch. · avg +0.5%
iterators.js — Interp: 20 unch. · avg +1.5% · Bytecode: 🟢 1, 🔴 1, 18 unch. · avg +0.3%
json.js — Interp: 20 unch. · avg +1.5% · Bytecode: 🟢 1, 19 unch. · avg -0.6%
jsx.jsx — Interp: 21 unch. · avg +0.5% · Bytecode: 21 unch. · avg +3.4%
numbers.js — Interp: 11 unch. · avg -1.9% · Bytecode: 11 unch. · avg -0.5%
objects.js — Interp: 7 unch. · avg -1.3% · Bytecode: 7 unch. · avg +1.7%
promises.js — Interp: 12 unch. · avg +0.6% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg -0.4%
strings.js — Interp: 🔴 1, 10 unch. · avg -1.8% · Bytecode: 11 unch. · avg -0.6%
typed-arrays.js — Interp: 22 unch. · avg -1.0% · Bytecode: 22 unch. · avg -1.0%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Suite Timing
Measured on ubuntu-latest x64. |
Fixes #99.
Summary
UnwrapToGocciaValueandConstruct) onConvertBlueprintToClassValue, eliminating the cache cross-contamination where a simpler bridge from the value path could be reused by the construct pathBlueprintToClassValuefunction (the simpler converter that omitted getters/setters, static fields, private members, and field initializers)instanceofidentityApproach
Option C from the issue: unify on the richer converter everywhere. Options A (split caches) and B (keyed cache) were explored but split caches caused
instanceoffailures because construction and value unwrapping produced differentTGocciaClassValueinstances with different prototypes, soinstanceofcould not match them. Unifying on the richer converter ensures one class value per blueprint with consistent prototype identity.Test plan
Made with Cursor
Summary by CodeRabbit
Tests
Refactor