fix(hir): scope a bare-assignment native-instance tag to the binding, not the name (#9847) - #9857
Conversation
…erryTS#9847) `lower_assign` handles `X = <native module>.<method>(...)`. Its class-name match ends in a catch-all, and it registered the result through `push_module_native_instance` — keyed on the identifier TEXT and scoped to the whole module, never truncated. So any method on any recognised native module, assigned to a variable, claimed that spelling for the rest of the program. Minified bundles reuse single letters everywhere, which makes the collision the normal case rather than a corner. `cli_2.1.112.js` (claude-code) compiles as one module, imports `child_process` as `fA1`, contains `let O; try { O = fA1.spawn(z.file, z.args, z.options) }` inside one helper, and binds the name `O` 5,381 times. Every later `O` was typed `child_process::Instance` — including the `for (let { segment: O } of ...)` binding in `string-width` that holds a grapheme STRING, whose `O.codePointAt(0)` lowered as `NativeMethodCall { module: "child_process", class_name: Some("Instance") }` and reached the right answer only because native-instance dispatch falls through to a generic path on a string receiver — once per grapheme, in the loop that dominates a claude-code turn. The tag now keys on the `LocalId` the assignment target resolves to. This is the same correction PerryTS#7775 already made in this file for `new Proxy` bindings (`proxy_locals` -> `proxy_local_ids`) after a proxy bound to `a` in one function made every other function's `a.prop` lower to `js_proxy_get`. Scope-truncating the assignment path would NOT have worked: the module-wide table exists for the cross-function case — a module-level `let client;` assigned inside `init()` and read inside `handler()` — and truncating at scope exit would have dropped exactly that. Keyed on the binding it reaches just as far, because both functions resolve `client` to the same `LocalId`, while a same-named binding in another scope is simply a different binding. `lookup_native_instance` gains an id-keyed arm ahead of the module-wide one, short-circuited when the module has no bare-assignment handle at all (the arm sits on the miss path of every identifier property access). A target that resolves to no local — a bare global — still registers and resolves by name; that is the same hole PerryTS#7775 documented, kept for the same reason and strictly no worse than the previous behaviour, which used it for every assignment. Nothing pattern-matches `child_process` or `codePointAt`: the mislowered call disappears because the tag never reaches that binding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe compiler now keys native-instance tags by resolved ChangesNative instance binding scope
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Native-instance lowering now follows the resolved binding rather than identifier spelling, preventing unrelated minified locals from receiving native dispatch while retaining valid native-handle dispatch. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
Follow-up filed as #9858: the two sibling registration forms in the same It carries the split of the 28 Also from the keystroke lane's segment-view counter, run against this branch: |
…y, and its NAMES
Perrymaster's round-1 run refuted two readings and left one question, and
this answers the question rather than re-asking it.
REFUTED, and recorded here because the retraction is the useful part:
* EXTERNAL_CARRIER-never-cleared is dead. Every examined id is retired
(~51%) or skip_keep (~49%); skip_cache, skip_external, skip_both and
skip_norecord are ZERO in all four draws and the five sum to examined.
No id is un-retirable. The flag may indeed never be cleared; it is
simply not what fills this family.
* retire_owned_shape_siblings is NOT the caller. retire_len_max = 16 in
every draw -- retirement never sees the big list. The removals come
from the dead-owner prune, which is the chain the original profile
named; the earlier attribution came from this file's own counter block
landing in retire's loop.
THE OPEN QUESTION is that bytes moved does not decide the mode: one draw
moved 335 GB and was as fast as one that moved 16 GB, while 848 and 643 GB
draws cost 4.5 s more. Since elems_moved is already the tail handed to the
memmove per removal (len - 1 - pos, not the list length), the definition is
not the escape and the live hypothesis is WHEN. So:
* a per-removal TAIL histogram (0, 1, 2-15, ... 1M+), which says whether
the bytes arrive as many small shifts or few enormous ones;
* the first family to cross 100k is latched by KEY, and every removal
from it is charged separately -- "is it all one list?" becomes a
number rather than an inference from len_max;
* removals from that family PER MINOR, printed and then reset, with the
peak kept. A total spread evenly across a turn and the same total
delivered in a few bursts are different costs, and only the per-minor
figure separates them.
And the crossing lines now print the KEY NAMES of the array. One keys array
carrying ~500k descriptors that all share its key list is the signature of
PerryTS#9847's mis-typed native-instance objects -- a fresh descriptor minted per
call over the same keys -- which is why PerryTS#9857 removes the whole thing. If
the names say so, the growth is EXPLAINED and belongs in an issue ("a family
of N descriptors with identical keys should not exist -- shape dedupe"),
not in a fix.
cargo fmt clean; not compiled (disk).
Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp
|
Landed on |
1 participant
Fixes #9847.
What the compiler was emitting, in one table
Every call site in
cli_2.1.112.js(claude-code) that lowered toNativeMethodCall { module: "child_process", class_name: Some("Instance"), … },counted from the whole-bundle HIR — 194 sites, 76 distinct method names, and
not one of them is a
ChildProcessmethod:sliceisFilestartsWithisDirectoryendsWithsubstringincludesmaptoLowerCaseisSymbolicLinkindexOftoUpperCase,replace,next,gettrimgetAttribute,keys,join,somesplit,bindcloneNode,isEqualNode,normalize, …matchcodePointAttestString methods,
fs.Statspredicates and DOM calls, dispatched through achild_processinstance method table. The singlecodePointAtisstring-width's grapheme loop — the loop that dominates a claude-code turn,executing that mislowered dispatch once per grapheme.
Mechanism
lower_assignhandlesX = <native module>.<method>(...). Its class-name matchends in a catch-all (
_ => Some("Instance")), and it registered the result withpush_module_native_instance— keyed on the identifier text, scoped to thewhole module, never truncated. So any method on any recognised native module,
assigned to a variable, claimed that spelling for the rest of the program.
Minified bundles reuse single letters everywhere, which makes the collision the
normal case rather than a corner.
cli_2.1.112.jscompiles as one module, doesimport fA1 from "node:child_process", containsand binds the name
O5,381 times. EveryOlowered afterWUwas typedchild_process::Instance— including thefor (let { segment: O } of … )binding that holds a grapheme string.
The shape matters, which is why this took two failed reproductions to pin: it is
the assignment form (
let O; … O = cp.spawn(…)), not the declaration form,and the module must be a recognised native import — a CJS
require()nevercreates the tag, so the earlier probes could not produce the precondition they
were testing.
Blast radius, and the control
Whole-bundle count of class-bearing
NativeMethodCallnodes(
perry compile --no-auto-optimize --trace hironcli_2.1.112.js), 410total before the fix:
child_process::Instancepath::Instancefs::Instancehttps::Instancestream::Instance::InstancesubtotalHeaders::Headersnet::Sockethttp::ClientRequesthttp::HttpServerevents::EventEmitternet::Servernet::BlockListreadline::Interfacehttp::IncomingMessagefetch::Responsestream::PassThroughFormData::FormDataThe 70 named-class rows are a registered control: they come from
registration paths this PR does not touch, and the prediction that they would
not move was written down before the fixed compiler was built.
From the other direction, the keystroke lane's registry instrumentation counted
795 native-instance registrations on the same bundle, the most-registered
identifiers all single letters (
Y71,z65,K65,_65,A54,O52,w37,q35), with several names claimed by mutually exclusive classes —Oalone is registered as
stream::Instance,child_process::Instance,transform_stream::TransformStreamandreadable_stream::ReadableStream. Notethese are two different denominators: 795 registrations against the 410
call-site nodes counted here, not the same quantity. (The empty-
module::classregister lines in that instrumentation — probably the
shadow_native_instancetombstone path — are uninvestigated and are not claimed either way.)
The fix
The tag now keys on the
LocalIdthe assignment target resolves to.This is the same correction #7775 already made in this file for
new Proxybindings —
proxy_locals(name, module-wide, scope-blind) becameproxy_local_ids(the resolved binding) after a proxy bound toain onefunction made every other function's
a.proplower tojs_proxy_get. Samedefect class, same table shape, same fix.
Scope-truncating the assignment path would not have worked, and that is why
the fix is id-keying rather than something narrower: the module-wide table
exists for the cross-function case — a module-level
let client;assignedinside
init()and read insidehandler()— and truncating at scope exit wouldhave dropped exactly that. Keyed on the binding it reaches just as far, because
both functions resolve
clientto the sameLocalId, while a same-namedbinding in another scope is simply a different binding.
lookup_native_instancegains an id-keyed arm ahead of the module-wide one,short-circuited when the module has no bare-assignment handle at all (the arm
sits on the miss path of every identifier property access).
Known hole, stated plainly
A target that resolves to no local — a bare global with no binding — still
registers, and resolves, by name. That is the hole #7775 documented for proxies,
kept for the same reason: dropping it would regress genuine handles that reach
the lowering only through a name, and it is strictly no worse than the previous
behaviour, which used that path for every assignment.
Deliberately not changed
_ => Some("Instance")) stays. It is over-broad, butthat is a separate question from scoping, and narrowing it would change which
calls dispatch natively.
x = new NativeClass(...)(which at least also registers scoped) and thevariable-to-variable
x = ypropagation. Same shape as this defect. Leftalone because codegen: a string's .codePointAt in cc's hottest loop lowers to NativeMethodCall{module:"child_process", class_name:"Instance"} #9847's A/B does not exercise them and widening would put
behaviour outside the evidence — flagging for a reviewer's call.
child_processorcodePointAt. The misloweredcall disappears because the tag never reaches that binding.
Evidence
All four were registered before the fixed compiler existed.
1. The issue's one-identifier A/B. Two files differing only in whether the
spawner's variable is spelled
O. On the pre-fix binary arm A loweredCall{PropertyGet(O,"codePointAt")}and arm B loweredNativeMethodCall{module:"child_process", class_name:Some("Instance"), method:"codePointAt"}. On the fixed binary both arms lowerPropertyGet { object: LocalGet(13), property: "codePointAt" }— same node,same local id. Encoded as a test that compares the two lowered bodies with only
source
byte_offsetnormalised (the rename shifts every offset in the file);the structural diff is zero lines.
2.
N$6in the real 13 MB bundle.perry compile --no-auto-optimize --trace hir --focus 'N$6' cli_2.1.112.js. The issue reportedand the same node now reads
Same binding (
118611), correct node.3. A real handle and an unrelated same-named local in one module. One
fixture carries all three cases; on the fixed binary the module-level
cross-function handle still lowers
NativeMethodCall{child_process, "kill"},the function-local handle still lowers
NativeMethodCall{child_process, "kill"}, and only the unrelatedfor…ofbinding changes, toPropertyGet{…,"codePointAt"}.4.
cargo test -p perry-hir -p perry-codegen— 74 suites, 2,535 passed, 0failed, 9 ignored.
What did NOT go to zero, stated plainly
28 catch-all
::Instancesites survive. About half look genuine —stream::on(4),
stream::pipe,stream::once,https::on(3),https::destroy(3),https::write,https::endare all plausible real handles. The other 14 arenot fixed by this PR and I am not claiming they are:
path::startsWith(7),path::slice(3),path::replace(2),path::toLowerCase,path::split,path::match— string methods on a module that has no instances at all. Thosereach their binding either through the unresolvable-name fallback above or
through the two sibling registration forms this PR deliberately leaves alone.
So the honest number is 312 of 326 wrong sites removed, not a clean sweep.
Why the existing test did not catch this
crates/perry-hir/tests/destructured_binding_native_hygiene.rspins theopposite contract — a
childProcess.spawnSynchandle namedzmust not makea destructured
const { install: z }inherit the tag — and it passes bothbefore and after this change. It is protected by the tombstone path
(
shadow_native_instance_if_present), which aconst/letbinding goesthrough and a
for…ofdestructuring head does not. So the guard existed, itjust had a hole shaped exactly like the hot loop. The new test uses the
for…ofform for that reason.
The order dependence, and a test that could not have failed
The tag only reaches functions lowered after the assignment that creates it.
My first fixture put the spawner second — it lowered correctly on the unfixed
compiler, i.e. it could not have failed. The shipped fixture puts the spawner
first and was verified to leak on a pre-fix binary before anything was asserted
about it; the doc comment says so, so nobody reorders it for readability.
Correctness
This is a mis-typing / missed-optimisation fix, not a wrong-answer fix: the
mislowered calls fall through to a generic path on a string receiver, so
claude-code renders correctly today. But "it currently degrades gracefully" is
not "it is safe" — a
child_process::Instancemethod table that ever gained aslice,startsWith,testorcodePointAtentry would have silentlycaptured 194 string, fs and DOM calls.
There is also a possible runtime connection worth naming without claiming: the
non-GC profile of a claude-code turn puts
js_native_call_method, andget_accessor_descriptorbeneath it, at the top of the genuinely non-GC list. Aprogram-wide population of mis-typed native-instance calls falling through to
generic dispatch is a plausible producer of that. This PR does not measure it —
a runtime measurement after this lands is the follow-up.
Summary by CodeRabbit
Bug Fixes
Tests