fix(runtime): throw for noncallable own builtin method shadows (#11006) - #11011
Closed
proggeramlug wants to merge 22 commits into
Closed
proggeramlug wants to merge 22 commits into
proggeramlug wants to merge 22 commits into
Conversation
…S#10943 layer 2) — RED Committed BEFORE the fix. 17 of its 30 rows are wrong on pristine main v0.5.1633: map.get/has/set/delete on a proven local own -> 1 / true / undefined / true map.get via param, any-annot, array elem, own -> 1 call result, class field set.has / set.add own -> true / undefined date.setHours own -> 10800000 array.push / indexOf / slice own -> 2 / 1 / "3,1" map.get before a later delete own -> native perry lowers a method call to a direct native call whenever it can prove the receiver's KIND, and an own property that shadows the method leaves that proof intact: `const m = new Map(); m.get = () => "own"` is still provably a Map. PerryTS#10476 already fixed this for UNPROVEN receivers — their runtime kind picks the builtin or the universal dispatcher, "which finds an own or inherited user method". The proven-receiver branch never got the same treatment. EVERY CALL IN THIS FILE PASSES AN ARGUMENT, deliberately. The previous attempt's differential used zero-argument calls throughout and went green against a fix that only covered zero-argument calls — a differential built from the spelling the fix covers proves nothing. A call with an argument is what real code writes and is exactly what gets specialised away from the dispatcher: the HIR fold (`lower/expr_call/local_array_methods.rs:948`) is gated on `!args.is_empty()`, and codegen's `map_set.rs` arms on `args.len() == 1`/`== 2`. The receiver forms are spread on purpose. A bare parameter is NOT a defence: HIR monomorphisation gives the clone that receives a Map a concrete `Map` local type. The `poly-plain` / `poly-map` pair proves it — one function, correct for a plain object and wrong for a Map, in the same program. The 13 rows that already pass are what make the 17 a contradiction rather than a design choice: every `native` row shows the builtin still works when nothing shadows it (so the fix cannot be "stop dispatching natively"), the subclass `get()` override resolves, deleting the own property restores the builtin, and `hasOwn` / `typeof` already agree the own property is there.
…#10943 layer 2, step 1) Runtime half only; no lowering site consults it yet, so this commit changes no behaviour. Split out so a half-finished state is a handover rather than a loss. `js_receiver_may_own_named_method(recv, name)` is the condition of a diamond the CALLER emits: 0 takes the direct builtin call, 1 takes the universal method dispatcher. It only ever chooses a branch. It deliberately does NOT resolve the property and call it — an own slot can hold a builtin thunk that dispatches by name again, and the previous attempt at this fix did exactly that and overflowed the stack on `test_bound_timer_dispatch_roots_args_during_async_hook_init_gc`. A fail-closed answer may decline a fast path; it may not substitute an action of its own. It never answers 0 for anything it cannot prove. A wrong 0 is a silent wrong value; a wrong 1 is only slower. Three tiers, cheapest first: * a primitive receiver, or no readable GC header -> 0 / 1 respectively; * an ARRAY is answered exactly off the cell, with no global consulted: `GC_ARRAY_NAMED_PROPS` already records this fact and is already monotonic; * everything else consults one relaxed load of a process-global arm, and only if it is set does the authoritative `js_object_has_own` run. Why a global arm and not a per-cell bit: `GcHeader::_reserved` has no free bits (`gc/types.rs`'s map says so, and bits 12/13 are actively ERASED by `set_layout_state` — PerryTS#8690 and PerryTS#10842 each lost a flag there). Map/Set/Date/ RegExp keep own named properties in a per-thread side table, so there is no per-cell bit to read and their `ObjectMeta` is usually null, which would mean materialising a record to read an almost-always-clear flag. The global is the `accessors_in_use` idiom the read path already uses. The arm is set-only and over-approximating, both on purpose. Set-only because clearing on delete would reopen delete-then-shadow, exactly as `GC_ARRAY_NAMED_PROPS` is monotonic. Over-approximating because it is armed at the TOP of `field_set_by_name`'s exotic-store gauntlet, above that gauntlet's per-kind branches: there is no single install funnel down there — buffers, stream handles and the meta/expando paths each store their own way — and a missed installer is the same silent wrong value. Arming early covers every kind including ones added later, and a spurious arm costs only the slow side. Named keys only; an index write is not a method shadow. `object_ops::has_own` becomes `pub(crate)` so the guard can ask the predicate behind `Object.hasOwn` rather than re-deriving own-ness from a shape descriptor — which is what fails here, since a Map/Set/Array cell's `+4` word is `capacity` and not a ShapeId.
layer 2) ECMA-262 resolves `recv.m(a)` as `Get(recv, "m")` then `Call`, so an own `m` wins. perry lowers a method call to a DIRECT native call whenever it can prove the receiver's KIND, and an own property that shadows the method leaves that proof intact: `const m = new Map(); m.get = () => 1` is still provably a Map. The result was a silent, plausible wrong value. PerryTS#10476 fixed this for UNPROVEN receivers. The guard could not simply be extended there: `try_lower_property_get_method_call` is an ORDERED CHAIN and `builtin_kind_guard`'s diamond is near its end, so every proven receiver is claimed upstream and never reaches it — extending it emitted a guard nothing executed (symbol present, call count 0). The test has to be above the chain. Only the RECEIVER is hoisted. The condition needs its value before the branch, so an arm re-lowering it would evaluate an effectful receiver (`make().get(k)`) twice; it is materialised once and re-read in both arms. ARGUMENTS are not hoisted: a diamond runs one arm, so each argument is still evaluated exactly once at runtime, and two emitted copies cost code size rather than semantics. That is what keeps every arm's signature unchanged. The receiver EXPRESSION is passed down unchanged rather than rewritten to a synthetic local, because every arm's proof is keyed on it (`is_array_expr`, `receiver_class_name`, `is_date_receiver`, the Ptr<Shape> facts); a synthetic local would erase those and un-specialise every one of these calls. `rooting::with_materialized_receiver` is the combinator: rooted for the window because everything below it allocates, re-read at each use rather than handed out as a register (PerryTS#7211), released on the way out. `lower_expr` consults it, which is the one funnel every operand lowering in the compiler already passes through. The guard only CHOOSES A BRANCH. It does not resolve the property and call it: an own slot can hold a builtin thunk that dispatches by name again, and an earlier attempt at this fix overflowed the stack doing exactly that. The other side is the universal dispatcher, which already finds an own or inherited user method.
… layer 3) The chain guard covers what codegen's ordered chain lowers. Most of PerryTS#10943 is not there: HIR folds m.get(k), s.has(v), a.push(x) on a proven receiver into dedicated nodes that lower straight to the native helper, so with only the chain guarded the differential emitted 3 guard calls and 16 of 30 rows stayed wrong. The same diamond is applied at lower_expr's dispatch, the one place every folded node passes through, with a per-variant table of (receiver, method name, arguments).
The predicate's ABI is (recv, name_ptr, name_len) — it asks Object.hasOwn's own predicate, which needs a real key. Both guards were passing a static dispatch id in the pointer slot, which faulted inside js_string_from_bytes the first time a receiver reached the authoritative tier. The parked attempt had the same mismatch and never surfaced it, because its guard was never reached: inert code can be wrong code.
… takes a named property (PerryTS#10943) Layer 1 armed at the top of field_set_by_name's exotic gauntlet, chosen as the funnel because that file has no single installer. It is not the funnel for the spelling the bug is reported with: m.get = () => 1 on a proven Map local lowers to js_put_value_set_dyn_ic, which never enters the gauntlet, so the predicate answered 0 and the builtin still won — 17 of 30 differential rows stayed red with both codegen guards in place and correct. exotic_expando::value_store is where the property is actually installed, whatever lowering asked for it. Arming there covers every spelling and every kind, and keeps the gauntlet's arm as the over-approximating outer net.
…PerryTS#10943) The codegen guards route a receiver that may own a shadowing method to the universal dispatcher — and the dispatcher had the same bug. Every kind dispatcher in js_native_call_method resolves a method by NAME against the receiver's kind and none consults its own properties, so the own arm landed on js_map_get anyway: measured under gdb, the guard answered 1, the own arm was taken, and the call still reached collection_methods::dispatch_map_set. Resolved and called in Get-then-Call order above the kind dispatchers, the way the Proxy arm already does it. A BORROWED builtin is not a user method and falls through to the native arms, which is what stops the recursion an earlier attempt hit.
js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
…erryTS#10943) js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
…path never sets (PerryTS#10943) js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
…tative predicate (PerryTS#10943) js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
…nts (PerryTS#10943) js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
) js_object_get_field_by_name_f64 walks the prototype chain, so on a Set the own-method arm found Set.prototype.has and called the builtin thunk through the dispatcher — the same bug one layer further in. Traced: js_native_call_method -> js_native_call_value -> set_proto_has_thunk -> js_set_has. An inherited method is not an override; only an own one beats the builtin, so this asks js_object_get_own_field_or_undef.
…ryTS#10943) Four temp_root_coverage::set_receiver rows failed, and they were right to: the hoist put the receiver in a SECOND rooted slot, so the consuming call re-read that slot instead of the one the receiver lives in. Both are roots and both are re-read after the allocating operand, so the emission was safe — but it added a hop for nothing and moved the re-read off the slot the invariant is stated about. A LocalGet/This receiver is already in a slot the collector rewrites, and lowering it again emits a LOAD from that slot, which IS the re-read PerryTS#7114/PerryTS#9523 demand. So materialise only what cannot be evaluated twice — a call result, a property or element read — and let every consumer of THAT re-read its slot. The four rows are the witness that the hoist cannot hand a consumer a register held across an allocation.
…ead (PerryTS#10943)" This reverts commit 5bde5ad.
…read (PerryTS#10943)" This reverts commit 997ef8f.
…eave ReadonlySet alone (PerryTS#10943) Two failures were mine, not the tests': * array_pop and entry_block_alloca panicked on my own expect("the receiver was materialised above") — the gate deliberately skips materialising a local, and the read has to cope with that. It re-lowers instead, which for a local IS a read of its rooted slot. * the three readonly_collection rows were right: js_readonly_set_has brand-checks and otherwise preserves JavaScript dispatch, which already reaches an own method, so a diamond there only adds the generic tower to the common native case — which those tests exist to forbid. ReadonlySet comes back out of the gate; the declared-Map case that needed it stays.
…ns the new contract It asserted `!ir.contains(DISPATCH)` — "a proven receiver must not pay for method dispatch" — and that premise IS the bug: proving the receiver's KIND proves nothing about an own property, so `d.getTime = () => 'own'` ran Date.prototype.getTime and returned a timestamp. The dispatcher now appears as the other side of one own-override diamond. What replaces it is the cost claim still worth pinning: exactly one predicate test and exactly one dispatch call, reached only when that test says the receiver may own the name. The two assertions above it — the builtin is still called directly, and the getter is called once — are unchanged and still pass.
…ts measurement Two gates asserted NO rooted temporary for a Map/Set call whose value cannot collect. The own-override guard tests the receiver before it branches, and the receiver stays rooted across that call because the predicate allocates today (js_string_from_bytes on its authoritative tier), so it is not a GC leaf and the root is not optional. Measured on that exact shape (s.has(2) in a hot loop, same compiler with Set in and out of the guard's gate, min of 3, fitted 500k -> 5M): 778.27 vs 778.26 instructions per iteration, +0.01. LLVM hoists the test, the branch and the slot traffic out of the loop; the cost is emitted shape, not runtime. Counted rather than deleted: assert_temp_rooting_count keeps a ratchet where there was a check, so a SECOND slot reddens. PerryTS#10957 removes this one for real by passing the interned key instead of (ptr, len), which takes the allocation out of the predicate, makes the GC-leaf claim provable rather than assumed, and returns both gates to assert_no_temp_rooting.
The builtin arm re-enters `lower_expr` for the node being guarded, and the
suppression that keeps it from forming a second diamond around itself was a
DEPTH COUNTER. That re-entry lowers the node's ARGUMENTS too, so a folded
builtin nested in an argument was suppressed as well and ran its native
helper with no diamond:
m1.set("k", m2.get("k")) // m2.get is an own property
node -> own:k
perry -> native
while the same call in statement position took the own method. The two arms
of one diamond disagreed for the same source.
Verified before fixing, on two binaries: the spelling is wrong identically on
this branch AND on main (a022cf2), so it is PerryTS#10943 surviving in a spelling
the differential did not contain, not a regression this guard introduced.
A depth counter cannot express "this node"; an identity can. `SUPPRESSED_NODE`
holds the address of the node being re-lowered, `Suppressed` saves and
restores the previous one so nested diamonds nest, and `try_lower` declines
only for that exact node.
Seven rows added to the differential, which is now 37 of 37 byte-identical to
node: the call in a `Map.set` argument, in an `Array.push` argument, in a
nested constructor argument, twice in one argument list, inside a concat
argument, a `Set.has` in an argument, and the unshadowed control that must
stay native.
Found by review on PerryTS#10958. The reviewer reasoned from the code; this is the
run.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
Author
|
Not in merge train 257 (#11039), but NOT a conflict with main: this applies cleanly to main on its own and only conflicted with a sibling PR in the same train. That sibling is in 257, so rebase once 257 lands (v0.5.1639) and ping me. No action until then. |
Contributor
Author
|
Held out of train 260 only because of its base: the #10943 stack in #10958 pushes |
proggeramlug
force-pushed
the
fix/10943-chain-guard
branch
from
September 23, 2026 08:13
262a6ca to
986c28e
Compare
Contributor
Author
|
Rebased onto #10958's repaired head ( Why it needed this: it had forked four commits behind #10958's tip, so its diff was reverting #10958 itself is now green — its four red steps ( Checked on the rebased head: |
Contributor
Author
|
Correction to my previous comment — the rebase I described never reached this PR. This PR's head lives in the The substance of that comment still holds, and it still matters:
Since I cannot push to your fork, either rebase it yourself: or leave it — I can still take it in a merge train by cherry-picking Apologies for the noise. The lesson on my side is to check |
This was referenced Sep 23, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 23, 2026
(cherry picked from commit fcca2ca)
Contributor
Author
|
Landed on Four of this train's seven PRs — including this one, if it is #11055, #11023, #11012 or #11014 — were repaired here because they were stuck: the fixes were cherry-picked from A train rebase gives the commits new SHAs, so GitHub cannot auto-close the source PR; closing by hand. Nothing needed from you. |
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
2 participants
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.
Fixes #11006. Depends on #10958; this PR is based on
fix/10943-chain-guardand should merge after it.An own noncallable method value was treated as though no own method existed, so the universal dispatcher ran the collection builtin. The runtime now confirms own presence, reads the value from the relevant storage path, and throws
TypeErrorbefore native dispatch when the value is not callable. It preserves borrowed native methods and roots the resolved value across the borrowed-method classifier.Verification:
cargo fmt --all -- --checkcargo build --profile perry-dev -p perry-runtime-static -p perrytest_parity_11006_noncallable_own_builtin.tsand the parent PR'stest_parity_own_override_beats_builtin.ts.