fix(codegen): an inherited property read no longer folds to undefined on a scalar-replaced object (#10689) - #10705
proggeramlug wants to merge 1 commit into
Conversation
… on a scalar-replaced object (PerryTS#10689) `check_escapes_in_expr`'s `Expr::PropertyGet` arm treated every read on a scalar-replacement candidate as a plain field read, without checking that the class chain declares the key. Scalar replacement allocates a slot per declared field only, so `expr/property_get.rs`'s scalar arm found no slot and folded the read to the constant `undefined`. The effect was silent and order-dependent: const o = { a: 1 }; typeof o.toString // undefined, where node gives "function" o.toString() // correct — a fused call never consults the // elided object It reached `Object.prototype` members read as values (`toString`, `constructor`, `hasOwnProperty`), a class's own prototype method read as a value, and user-added `Object.prototype` properties. Anything that made the receiver escape — passing it to a function, storing it in an array — repaired it, which is what made the bug look like it depended on unrelated earlier statements. This is the READ half of the rule the write arms already apply: PerryTS#9024 for `PropertySet`/`PutValueSet` and PerryTS#9460 for `PropertyUpdate`, plus the sibling literal analysis in `escape_objects.rs`. Only the read arm was missing it. Reads of declared fields still take the no-heap scalar path, which is spec-correct because an own property shadows the chain and OrdinaryGet never reaches the prototype. Reads of undeclared keys now take the ordinary heap path. A fused method call is unaffected: its callee is handled in the `Expr::Call` arm and is no longer routed through `PropertyGet`, so `simple_scalar_method_summary` receivers stay scalar-replaced. Claude-Session: https://claude.ai/code/session_01YaNfLEjMRdhCLtk3SB5MdJ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change updates escape analysis so inherited property reads on scalar-replacement candidates use the ordinary object path. Fused method calls retain scalar method handling. New integration tests cover built-in, class, user-added, absent, and own properties. ChangesInherited member read correctness
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Landed via merge train #10716 (v0.5.1598). All source commits preserve authorship; merged main matches the validated train exactly. |
Fixes #10689.
Silent wrong answer, no error, and order-dependent in a way that made it look like it depended on unrelated earlier statements.
It is escape analysis, not realm population
The issue attributes this to lazy
populate_global_this_builtins, and that is wrong — the two were conflated becauseJSON.stringify(o), used in the issue's repro, both forces the realm and makesoescape. Separating them:for...offorces the realm and does not fix it. Escaping is what matters; the realm is incidental.Mechanism
check_escapes_in_expr'sExpr::PropertyGetarm classified every read on a scalar-replacement candidate as// Plain field read — safe, without checking that the class chain declares the key. The local therefore stayed scalar-replaced — no heap object exists at all — andexpr/property_get.rs's scalar arm, finding no slot for the key, folds the read to the constantundefined. Calls were always right because a fused method call never consults the elided object.The same rule was already in this file three times, on the write arms — #9024 (
PropertySet/PutValueSet), #9460 (PropertyUpdate) — and in the sibling literal analysisescape_objects.rs. Only the read arm was missing it.Blast radius is wider than the issue reported: a class's own prototype method read as a value (
typeof c.m→undefinedwhilec.m()→1) and user-addedObject.prototypemembers were also invisible.The change
One file, +73/−19, no runtime code.
class_chain_has_fieldis false — the same helper the write arms use.PropertyGetarm, sosimple_scalar_method_summaryreceivers stay scalar-replaced.Which reads force, and which do not — every
js_get_global_this()site is byte-identical. Reads of declared fields stay on the no-heap scalar path, which is spec-correct: an own property shadows the chain and OrdinaryGet never reaches the prototype. Reads of undeclared keys take the ordinary heap path, which resolves the chain and forces the realm exactly where it already would.Results
o.toString(),o.hasOwnProperty("a"),"" + o) and genuinely absent keys still readundefined— so a future change cannot "fix" reads by breaking calls.new, literal, and summarised-call receivers, 200k iterations each: all four within ±0.001%. Scalar replacement is not weakened for the cases it is meant to serve.The three programs that were wrong got slower, and that is the point
toStringread firstThey were cheap because they were wrong — the folded
undefinedskipped realm population entirely. The control row is the proof: a program that already forced the realm pays 0.04%. The +24M is #10686's existing constant surfacing where a wrong answer had been dodging it, not a cost this change invents. Fixing #10686 removes it.Gates
cargo fmt✅ ·check_file_size.sh✅ ·gc_runtime_root_holders.py✅ · clippy 457 = 457, byte-identical ·perry-codegen2138 pass / 0 fail ·perry-runtime4039 pass / 2 failures, both pre-existing on base (debug-assert sabotage tests that cannot pass under--release; the repo's ownCargo.tomlsays they want--profile gcaudit, andcargo tree -p perry-runtime | grep -c perry-codegen= 0, so that binary is identical in both arms).Not included
#10686 is not fixed here. The two do not share a mechanism, so the "opposite directions" concern noted on both issues does not arise — this fix never needed anything to start forcing. #10686 is now unblocked rather than in tension with it, and the table above makes it more valuable, not less. It should be its own change with its own sabotage-proved test.
The version and
CLAUDE.mdare deliberately not bumped: they are workspace-wide inputs that would invalidate every artifact these numbers came from, and parallel sessions would collide.Separately found while here, pre-existing and not filed yet:
o.__proto__ === Object.prototypeisfalsein perry andtruein node, reproducing on base with a fully escaped heap object.https://claude.ai/code/session_01YaNfLEjMRdhCLtk3SB5MdJ
Summary by CodeRabbit
Bug Fixes
undefinedresults when reading inherited object properties such asconstructor,toString,hasOwnProperty, and other prototype members.Tests