From 5266200791edfce4d40f6bf61068c4f6a07fde06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 15:41:26 +0200 Subject: [PATCH] perf(codegen,runtime): inline precheck for boxed class-field stores, arguments-registry emptiness latch, declared-type refinement for property reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the `interp` campaign. Three independent changes: A. `expr/property_set.rs` gated the #5093 inline shape precheck on `requires_raw_f64`, so every store into a declared field that is not a `number` paid an unconditional `js_typed_feedback_class_field_set_guard` call — including the synthesized `__AnonShape_*_constructor` behind every closed-shape object literal. The sloppy arm has taken the boxed precheck since #7288 and its argument applies verbatim: the write barrier, layout note and string demote come from `emit_jsvalue_slot_store_pointer_tested`, not from the guard, and a setter in the chain is refused upstream by `class_field_global_index`. Every miss still reaches the unchanged guard call and strict fallback. B. `is_arguments_object` gets the #7474/#7469 emptiness latch. It is a probe run from the by-name property-get tail, array push, the iterator entries and class construction; in a program with no `arguments` it was 2.8% of `interp` — a thread-local resolution (a real `_tlv_get_addr` on Darwin), a `RefCell` borrow and a pointer hash to prove a feature's absence. C. `refine_type_from_init` recovers a property read's type from the receiver's declared annotation through the class / interface / object-alias tables `static_type_of` already consults, after stripping nullish union arms — so `const names = e.names` on `let e: Env | null` stops being `Any` and `names[i]` stops being a `js_dyn_index_get` call. That type is a CLAIM, not a proof. Element reads and stores re-check `GC_TYPE_ARRAY` and tolerate a violated one; `.length` does not, because its `js_value_length_f64` fallback answers 0 where JS answers `undefined`. `declared_only_array_locals` (the #7773 mechanism) keeps these ids off the `.length` fast arm. The sabotage test was written first and failed, which is how that was found. Refutes the round-3 handoff's primary lever: hand-written shape narrowing after a discriminant test converts 19 of 31 generic property diamonds in `evalNode` and is worth 5.2%, not the projected ~20% — the cost is the guard, not the lookup, so narrowing swaps one guarded diamond for another. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --- ...precheck-arguments-latch-declared-array.md | 122 +++++++++++++++ crates/perry-codegen/src/codegen/closure.rs | 1 + crates/perry-codegen/src/codegen/entry.rs | 2 + crates/perry-codegen/src/codegen/function.rs | 1 + crates/perry-codegen/src/codegen/method.rs | 2 + crates/perry-codegen/src/expr/mod.rs | 23 +++ crates/perry-codegen/src/expr/property_get.rs | 9 ++ crates/perry-codegen/src/expr/property_set.rs | 64 +++++--- crates/perry-codegen/src/stmt/let_stmt.rs | 16 ++ crates/perry-codegen/src/type_analysis.rs | 1 + .../perry-codegen/src/type_analysis/refine.rs | 148 +++++++++++++++++- crates/perry-runtime/src/object/arguments.rs | 64 ++++++++ .../src/object/arguments_latch_tests.rs | 96 ++++++++++++ crates/perry-runtime/src/object/mod.rs | 2 + ..._gap_declared_field_type_refine_guarded.ts | 143 +++++++++++++++++ 15 files changed, 669 insertions(+), 25 deletions(-) create mode 100644 changelog.d/7854-interp-round4-store-precheck-arguments-latch-declared-array.md create mode 100644 crates/perry-runtime/src/object/arguments_latch_tests.rs create mode 100644 test-files/test_gap_declared_field_type_refine_guarded.ts diff --git a/changelog.d/7854-interp-round4-store-precheck-arguments-latch-declared-array.md b/changelog.d/7854-interp-round4-store-precheck-arguments-latch-declared-array.md new file mode 100644 index 0000000000..210ba2fee1 --- /dev/null +++ b/changelog.d/7854-interp-round4-store-precheck-arguments-latch-declared-array.md @@ -0,0 +1,122 @@ +### perf(codegen,runtime): boxed class-field stores get the inline precheck, the arguments-object registry gets an emptiness latch, and a property read into an untyped local recovers the receiver's declared type + +Round 4 of the `interp` campaign (3.96 s → 1.893 → 1.499 → 1.237 → **this**). Three +independent changes, each measured on its own; plus one refutation that closes the +lever the previous round's handoff was built around. + +#### A. The strict class-field SET arm now emits the inline precheck for BOXED fields + +`expr/property_set.rs` gated `emit_class_field_inline_precheck` on +`requires_raw_f64`, so every store into a declared field that is *not* a `number` — +a `string`, a class type, a union: most fields of most objects — paid an +unconditional cross-crate `js_typed_feedback_class_field_set_guard` call. That +includes the synthesized `__AnonShape_*_constructor` every closed-shape object +literal runs, which is why `js_typed_feedback_class_field_set_guard` (2.9%) plus +`typed_feedback::guards::class_field_fast_contract` (2.1%) sat near the top of +`interp`'s profile: `{ kind: "bin", op, left, right }` is four stores, three of them +boxed. + +The stated reason for the gate — "its setter-in-chain handling and write barrier +aren't reproduced inline" — was already answered by +`try_lower_sloppy_class_field_boxed_store`, which has taken the boxed inline +precheck since #7288. The write barrier, layout note and string demote come from +`emit_jsvalue_slot_store_pointer_tested` (which the shared fast block calls, with +the identical value-side predicates), not from the guard; a setter anywhere in the +chain is refused upstream by `class_field_global_index`'s `accessor_in_chain`. What +the precheck proves is a strict subset of the runtime's `class_field_fast_contract`, +so on a hit the guard call would have answered "fast" too — this removes a call, it +never changes which store happens. Every miss still lands on the guardcall block and +the unchanged strict fallback. + +Verified live rather than assumed: `interp.ts` goes from 3 to 44 emitted +`PERRY_CLASS_FIELD_INLINE_GUARD_DISABLED` gate loads (41 new prechecks) with the +same 43 guard-call sites, now on the miss arm. + +#### B. `is_arguments_object` gets the #7474/#7469 emptiness latch + +`is_arguments_object` is a *probe*, called from the by-name property-get tail, +`Array.prototype.push`, the array and `Symbol.iterator` iterator entries, +`Array.from`/`concat`, and class construction. In a program that never writes the +identifier `arguments` it was still 2.8% of `interp`: a thread-local resolution +(Darwin has no local-exec TLS, so that is a real `_tlv_get_addr` call — 7.1% of the +same profile), a `RefCell` borrow, and a pointer hash, per call, to prove the +absence of a feature the source does not contain. + +`ARGUMENTS_OBJECTS_EVER_USED` is a process-global `AtomicBool` latched by the one +and only registry insert, checked before the thread-local — the +`EXTERNAL_BUFFERS_NONEMPTY` / `SET_REGISTRY_EVER_USED` idiom verbatim. Global rather +than per-thread on purpose (a `thread_local!` flag would cost the very TLS call this +removes); being global only makes it conservative. + +`object/arguments_latch_tests.rs` pins the subject, not the answer: +`latch_off_is_what_makes_the_probe_cheap` registers a real arguments object, forces +the latch back off, and requires the probe to answer `false` — deliberately the +wrong answer, and the only way to show the short-circuit is the arm being taken +rather than dead code in front of a registry that would have answered anyway. Delete +the early-out and that test goes red. `creating_an_arguments_object_arms_the_latch` +pins the other half (a second insert site added without arming would be a silent +wrong answer). + +#### C. A property read into an unannotated local recovers the receiver's declared type + +`const names = e.names` left `names` at `Any`, so `names[i]` lowered to a +`js_dyn_index_get` call (4.3% of `interp`) whose own miss path calls +`js_array_length` (2.9%), and `names[i] === name` lowered to the fully dynamic +`js_eq` instead of an inline string compare. `refine_type_from_init`'s `PropertyGet` +arm resolves the receiver with `receiver_class_name`, which answers `None` for a +reassigned local and for a union, and then looks only in `ctx.classes` — so a +chain-walking cursor (`let e: Env | null = env; … e = e.parent`) over a +`type Env = { … }` alias resolved to nothing three times over. That is the +`type`-vs-`interface` asymmetry #655 fixed for `static_type_of` but not here, plus +the nullish-union and reassignment gaps. + +`declared_property_type_from_annotation` resolves through the same +class / interface / object-alias tables `static_type_of` already consults, after +stripping `null`/`undefined` from the receiver's union (a read that returns at all +had a non-nullish receiver — reading through `null` throws). It infers exactly the +type a hand-written `const names: string[] = e.names` would have installed. + +**It is a claim, not a proof, and one consumer could not take one.** Element reads +and stores tolerate a violated claim — both re-check `GC_TYPE_ARRAY` and fall back. +`.length` does not: its inline arm is guarded but its fallback, +`js_value_length_f64`, answers **0** for every value that carries no length where JS +answers `undefined` (and where a nullish receiver must throw) — a pre-existing +degradation the runtime documents in place, and one that is therefore already +reachable on `main` through a hand-written annotation (filed as **#7853**). So +`.length` must not be handed a fresh claim: `refined_array_type_is_declared_only` records these ids in +`FnCtx::declared_only_array_locals` (the `declared_only_numeric_locals` mechanism +from #7773) and the `.length` arm in `expr/property_get.rs` refuses them, leaving +them on exactly the generic path the unrefined `Any` local takes today. + +`test-files/test_gap_declared_field_type_refine_guarded.ts` is the sabotage test: +the same `items: string[]` declaration is handed arrays, strings, plain objects +aping arrays, numbers, `null` and `undefined`, through an alias, an interface and a +class, through a nullable reassigned cursor and a nested read chain, and every row +must match node byte for byte. **It was written before the guard and it failed** — +four rows read `len=0` where node says `undefined` or throws — which is how the +`.length` hazard above was found rather than shipped. + +#### Measured + +Quiet M1 mini, best-of-5, exit-checked, outputs byte-compared against +`node --experimental-strip-types`. See the PR body for the full 19-program table. + +#### Refuted: shape narrowing after a discriminant test + +`PROFILE-interp-round3.md` identified `evalNode`'s surviving property-read diamonds +(27.2% of the program) and proposed narrowing `n` to its matching union member +inside `if (n.kind === "bin")` so the reads become class-keyed slot loads. **The +ceiling was measured before building it, and it is ~5%, not ~20%.** + +Three source-level arms of `interp.ts`, identical in every other respect, built from +one compiler: object literals (the original), the same program with each union +member as a real `class` (so allocation, not typing, is isolated), and that program +with hand-written narrowing casts in every `evalNode` arm. The narrowed arm converts +**19 of 31** generic property diamonds into guarded class-field inline reads — and +is 1.226 → 1.162 s against its own control, 5.2%. The reason is structural: the +class-field guarded read is only about a third cheaper than the polymorphic-IC read, +because the cost is the *guard*, not the lookup, and narrowing replaces one guarded +diamond with another. A large win there needs the check hoisted out of the branch +(one shape test, N unguarded slot loads), which is loop-versioning applied to a +discriminant arm — a much bigger build than the handoff assumed, for a lever whose +cheap form is now measured and closed. diff --git a/crates/perry-codegen/src/codegen/closure.rs b/crates/perry-codegen/src/codegen/closure.rs index 69a7431ac5..32b74d7e67 100644 --- a/crates/perry-codegen/src/codegen/closure.rs +++ b/crates/perry-codegen/src/codegen/closure.rs @@ -979,6 +979,7 @@ pub(super) fn compile_closure( shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt, arena_state_slot: None, class_keys_slots: HashMap::new(), diff --git a/crates/perry-codegen/src/codegen/entry.rs b/crates/perry-codegen/src/codegen/entry.rs index b416ea3219..9dcdb02d48 100644 --- a/crates/perry-codegen/src/codegen/entry.rs +++ b/crates/perry-codegen/src/codegen/entry.rs @@ -817,6 +817,7 @@ pub(super) fn compile_module_entry( shadow_slot_map: main_shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt: main_shadow_slot_clears_after_stmt, arena_state_slot: None, class_keys_slots: HashMap::new(), @@ -1487,6 +1488,7 @@ pub(super) fn compile_module_entry( shadow_slot_map: init_shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt: init_shadow_slot_clears_after_stmt, arena_state_slot: None, class_keys_slots: HashMap::new(), diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index 98281412c0..6c47c7bf8c 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -777,6 +777,7 @@ pub(super) fn compile_function( shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt, shadow_slots_bound: bound_param_slots, temp_roots: crate::rooting::TempRootPool::default(), diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index eadaff919c..de563f1aad 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -510,6 +510,7 @@ pub(super) fn compile_method( shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt, arena_state_slot: None, class_keys_slots: HashMap::new(), @@ -1574,6 +1575,7 @@ pub(super) fn compile_static_method( shadow_slot_map, persistent_shadow_slots: std::collections::HashSet::new(), declared_only_numeric_locals: std::collections::HashSet::new(), + declared_only_array_locals: std::collections::HashSet::new(), shadow_slot_clears_after_stmt, arena_state_slot: None, class_keys_slots: HashMap::new(), diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index dfd9306052..d48ff06c8b 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -764,6 +764,29 @@ pub(crate) struct FnCtx<'a> { /// the trust into a four-instruction runtime tag test instead. pub declared_only_numeric_locals: std::collections::HashSet, + /// LocalIds whose `Array`/`String` type came from + /// `refine.rs::declared_property_type_from_annotation` — i.e. from the + /// RECEIVER'S ANNOTATION (`type Env = { names: string[] }`), not from an + /// initializer that proves an array (`[...]`, `.split()`, `Object.keys()`). + /// + /// Twin of `declared_only_numeric_locals` (#7773) and there for the same + /// reason: the refinement is load-bearing — without it `const names = + /// e.names` stays `Any` and every `names[i]` is a `js_dyn_index_get` call — + /// but it copies an annotation rather than proving anything, and Perry does + /// not enforce annotations at runtime. + /// + /// Element reads and stores are safe on a claim: they re-check + /// `GC_TYPE_ARRAY` on the receiver and fall back. `.length` is NOT: its + /// slow path (`js_value_length_f64`) answers **0** for every value that + /// carries no length, where JS answers `undefined` (and throws on + /// nullish) — a documented, pre-existing degradation + /// (`value/dynamic_object.rs`, "the generic PropertyGet slow path already + /// degrades to 0 here"). Feeding it a claim would widen a silent wrong + /// answer, so the `.length` arm in `expr/property_get.rs` refuses these + /// ids and lets them take the generic property path — exactly what the + /// unrefined `Any` local does today. + pub declared_only_array_locals: std::collections::HashSet, + /// Cached pointer to this function's `InlineArenaState` slot — /// allocated lazily on the first `new ClassName()` site that uses /// the inline bump-allocator path. The slot lives in the function diff --git a/crates/perry-codegen/src/expr/property_get.rs b/crates/perry-codegen/src/expr/property_get.rs index 69e5bafeab..3c72005a22 100644 --- a/crates/perry-codegen/src/expr/property_get.rs +++ b/crates/perry-codegen/src/expr/property_get.rs @@ -292,6 +292,15 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { Expr::PropertyGet { object, property, .. } if property == "length" + // #7854: a receiver whose Array/String type is a copied ANNOTATION + // rather than a proof must not reach this arm. The inline half is + // guarded, but the `js_value_length_f64` fallback answers 0 where + // JS answers `undefined` (and where a nullish receiver must throw), + // so a violated claim becomes a silent wrong answer instead of a + // slower path. These ids take the generic property route — exactly + // what the same local took before it was refined at all. + // See `FnCtx::declared_only_array_locals`. + && !matches!(object.as_ref(), Expr::LocalGet(id) if ctx.declared_only_array_locals.contains(id)) && (is_array_expr(ctx, object) || is_string_expr(ctx, object) || match crate::type_analysis::static_type_of(ctx, object) { diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index df66f56f8b..1cb86f2f18 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -1253,26 +1253,52 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let fallback_label = ctx.block_label(fallback_idx); let merge_label = ctx.block_label(merge_idx); - // #5093: inline shape pre-check, raw-f64 fields only. The - // boxed-store path keeps the guard call (its setter-in- + // #5093: inline shape pre-check. On a hit this branches + // straight to the store, skipping the call; on a miss the + // guard-call path below runs unchanged. + // + // #7854: this used to be gated on `requires_raw_f64`, + // leaving every BOXED declared field (`string`, a class + // type, a union — i.e. most fields of most objects) paying + // an unconditional cross-crate + // `js_typed_feedback_class_field_set_guard` call per + // store, including the synthesized + // `__AnonShape_*_constructor` that every closed-shape + // object literal runs. The stated reason — "its setter-in- // chain handling and write barrier aren't reproduced - // inline). On a hit this branches straight to the raw - // store, skipping the call; on a miss the guard-call path - // below runs unchanged. - if requires_raw_f64 { - let _guardcall_label = - crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( - ctx, - &obj_bits, - &obj_handle, - &expected_class_id_str, - &expected_keys, - field_index, - true, - Some(&val_bits), - &fast_label, - ); - } + // inline" — is answered by + // `try_lower_sloppy_class_field_boxed_store`, which has + // taken the boxed inline precheck since #7288: the write + // barrier, layout note and string demote come from + // `emit_jsvalue_slot_store_pointer_tested` (which the + // shared `fast_label` block below calls, with the very + // same value-side predicates), NOT from the guard; and a + // setter in the chain is already refused upstream by + // `class_field_global_index`'s `accessor_in_chain`. + // + // What the precheck proves is a strict subset of the + // runtime `class_field_fast_contract`: on a hit the guard + // call would have answered "fast" too, so this only + // removes a call, never changes which store happens. Every + // miss still lands on the guardcall block and the + // unchanged strict fallback, so `[[Set]]` rejection and + // descriptor dispatch are untouched. `require_raw_f64` is + // forwarded rather than hardcoded, so a boxed slot skips + // the plain-finite value test (a boxed slot accepts any + // `JSValue`) but still proves not-frozen / no per-object + // descriptors via `set_value_bits: Some`. + let _guardcall_label = + crate::expr::class_field_inline_guard::emit_class_field_inline_precheck( + ctx, + &obj_bits, + &obj_handle, + &expected_class_id_str, + &expected_keys, + field_index, + requires_raw_f64, + Some(&val_bits), + &fast_label, + ); let guard_ok = ctx.block().call( I32, "js_typed_feedback_class_field_set_guard", diff --git a/crates/perry-codegen/src/stmt/let_stmt.rs b/crates/perry-codegen/src/stmt/let_stmt.rs index 4e23d1b3d4..c7c7b3426e 100644 --- a/crates/perry-codegen/src/stmt/let_stmt.rs +++ b/crates/perry-codegen/src/stmt/let_stmt.rs @@ -311,6 +311,22 @@ pub(crate) fn lower_let( } } + // Same discipline for the array/string half. When the refinement above + // answered `Array`/`String` only because the RECEIVER'S ANNOTATION said so + // (`const names = e.names` on `type Env = { names: string[] }`), record the + // id: element reads re-check `GC_TYPE_ARRAY` and are safe on a claim, but + // the `.length` fast arm's fallback answers 0 where JS answers `undefined`, + // so it must not consume one. See `FnCtx::declared_only_array_locals`. + if matches!( + refined_ty, + perry_hir::types::Type::Array(_) | perry_hir::types::Type::String + ) && init.is_some_and(|e| { + matches!(e, perry_hir::Expr::PropertyGet { .. }) + && crate::type_analysis::refined_array_type_is_declared_only(ctx, e) + }) { + ctx.declared_only_array_locals.insert(id); + } + // Track closure func_id → local_id mapping so the closure // call site in lower_call can look up rest param info. if let Some(perry_hir::Expr::Closure { diff --git a/crates/perry-codegen/src/type_analysis.rs b/crates/perry-codegen/src/type_analysis.rs index ca9c506303..1d1f56bb5f 100644 --- a/crates/perry-codegen/src/type_analysis.rs +++ b/crates/perry-codegen/src/type_analysis.rs @@ -54,6 +54,7 @@ pub(crate) use predicates::tuple_index_literal; pub(crate) use refine::{ compute_auto_captures, is_crypto_digest_chain, is_global_constructor_expr, is_process_namespace_version_property, refine_type_from_init, + refined_array_type_is_declared_only, }; pub(crate) use strings::{ class_name_extends_url_search_params, is_declared_string_expr, is_definitely_string_expr, diff --git a/crates/perry-codegen/src/type_analysis/refine.rs b/crates/perry-codegen/src/type_analysis/refine.rs index d2c652c352..64046c9632 100644 --- a/crates/perry-codegen/src/type_analysis/refine.rs +++ b/crates/perry-codegen/src/type_analysis/refine.rs @@ -30,6 +30,141 @@ pub(crate) fn is_process_namespace_version_property(object: &Expr, property: &st && matches!(object, Expr::NativeModuleRef(module) if is_process_module_ref_name(module)) } +/// Drop `null` / `undefined` / `void` from a union and return the single +/// surviving member, if there is exactly one. +/// +/// `type Env = { … }` + `let e: Env | null` is the ordinary way to write a +/// linked structure in TypeScript, and it is the *only* thing standing between +/// `const names = e.names` and a usable type: a read that returns at all had a +/// non-nullish receiver, because reading a property off `null`/`undefined` +/// throws. So the nullish arms contribute nothing to the RESULT type and can be +/// dropped without weakening anything. +fn strip_nullish_union(ty: &HirType) -> Option<&HirType> { + let HirType::Union(members) = ty else { + return Some(ty); + }; + let mut live = members + .iter() + .filter(|m| !matches!(m, HirType::Null | HirType::Void)); + let first = live.next()?; + if live.next().is_some() { + return None; + } + Some(first) +} + +/// Resolve `.`'s type from the receiver's DECLARED +/// annotation, through the same class / interface / object-type-alias tables +/// [`crate::type_analysis::static_type_of`] already consults. +/// +/// # Why this exists +/// +/// `refine_type_from_init`'s class walk resolves the receiver with +/// `receiver_class_name`, which answers `None` for two shapes that dominate +/// real TypeScript: +/// +/// * a **reassigned** local (`predicates.rs`'s first arm) — e.g. the cursor +/// of a chain walk, `let e: Env | null = env; … e = e.parent;` +/// * a **union** local — `receiver_class_name`'s `LocalGet` arm matches only +/// `Named`/`Generic`, so `Env | null` resolves to nothing. +/// +/// and its table walk then looks only in `ctx.classes`, so a `type X = { … }` +/// alias or an `interface` resolves to nothing either — the `type`-vs- +/// `interface` asymmetry #655 already fixed for `static_type_of` but not here. +/// +/// Measured on `gc-handoff/apps/interp.ts`, whose `lookup` is +/// `const names = e.names; for (i = 0; i < names.length; i++) names[i]`: with +/// `names` left `Any`, `names[i]` lowers to `js_dyn_index_get` (4.3% of the +/// program) and `names.length` misses the property IC into `js_array_length` +/// (2.9%). Writing the annotation by hand — `const names: string[] = e.names` — +/// removes both and is **-14.5%** on the whole benchmark. This infers exactly +/// the type that hand annotation would have written. +/// +/// # It is a CLAIM, and one consumer could not take one +/// +/// This produces a *declared* type, so it is a claim, not a proof — the same +/// claim the author's own `const names: string[] = e.names` would install, +/// through the same `local_types` entry, but Perry enforces no annotation at +/// runtime. Element READS and STORES tolerate that: both re-check +/// `GC_TYPE_ARRAY` on the receiver and fall back, so a violated claim costs a +/// branch and nothing else. +/// +/// **`.length` does not.** Its inline arm is guarded, but its FALLBACK +/// (`js_value_length_f64`) answers **0** for every value that carries no +/// length, where JS answers `undefined` — a pre-existing degradation the +/// runtime documents in place ("the generic PropertyGet slow path already +/// degrades to 0 here", `value/dynamic_object.rs`) and which is therefore +/// reachable on `main` today through a hand-written annotation (#7853). Handing it a +/// freshly inferred claim would widen a silent wrong answer, so +/// `refined_array_type_is_declared_only` records these ids in +/// `FnCtx::declared_only_array_locals` and the `.length` arm in +/// `expr/property_get.rs` refuses them — leaving them on exactly the generic +/// path the unrefined `Any` local takes today. +/// `test_gap_declared_field_type_refine_guarded.ts` pins all of it: the same +/// declaration is handed strings, plain objects, numbers, `null` and +/// `undefined`, and every row must match node. +/// +/// Deliberately conservative: only a NON-generic receiver name whose entry is a +/// class, an interface, or an alias to a closed object type answers, and only +/// the property's own declared type is returned — no inheritance walk beyond +/// what the class table already does, and no index-signature fallback. +pub(crate) fn declared_property_type_from_annotation( + ctx: &FnCtx<'_>, + object: &Expr, + property: &str, +) -> Option { + let declared = match object { + Expr::LocalGet(id) => ctx.local_types.get(id)?, + _ => return None, + }; + match strip_nullish_union(declared)? { + // `let e: Env | null` where `type Env = { … }` / `interface Env` / + // `class Env`. + HirType::Named(name) => { + if let Some(class) = ctx.classes.get(name) { + if let Some(f) = class.fields.iter().find(|f| f.name == property) { + return Some(f.ty.clone()); + } + } + if let Some(iface) = ctx.interfaces.get(name) { + if let Some(p) = iface.properties.iter().find(|p| p.name == property) { + return Some(p.ty.clone()); + } + } + if let Some(HirType::Object(obj)) = ctx.type_aliases.get(name) { + return obj.properties.get(property).map(|p| p.ty.clone()); + } + None + } + // The alias was already expanded in place (`let e: { … } | null`). + HirType::Object(obj) => obj.properties.get(property).map(|p| p.ty.clone()), + _ => None, + } +} + +/// True when `refine_type_from_init` would answer this `PropertyGet` ONLY via +/// [`declared_property_type_from_annotation`] — i.e. the array/string type is a +/// copied annotation and not something an initializer proved. +/// +/// Mirrors `numeric_proof_is_declared_only` (#7773). Callers use it to record +/// the local in `FnCtx::declared_only_array_locals`. +pub(crate) fn refined_array_type_is_declared_only(ctx: &FnCtx<'_>, init: &Expr) -> bool { + let Expr::PropertyGet { + object, property, .. + } = init + else { + return false; + }; + // The pre-existing class walk is the proof-ish arm (a real `class` whose + // field type came from a declaration Perry itself lowered); if it answers, + // this is not a NEW claim and behaviour is unchanged from before #7854. + let via_class = receiver_class_name(ctx, object) + .and_then(|receiver_class| ctx.classes.get(&receiver_class)) + .and_then(|class| class.fields.iter().find(|f| f.name == *property)) + .is_some(); + !via_class && declared_property_type_from_annotation(ctx, object, property).is_some() +} + /// Refine an `Any`-typed local's static type based on its initializer /// expression. Returns Some(Type) when we can statically prove the /// initializer produces a more specific type, so the `Stmt::Let` @@ -396,13 +531,14 @@ pub(crate) fn refine_type_from_init(ctx: &FnCtx<'_>, init: &Expr) -> Option bool { + !ARGUMENTS_OBJECTS_EVER_USED.load(std::sync::atomic::Ordering::Relaxed) +} + pub fn scan_arguments_object_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { let mut moved = Vec::new(); ARGUMENTS_OBJECTS.with(|m| { @@ -176,6 +203,9 @@ pub extern "C" fn js_arguments_object_alloc( ); } + // Latch BEFORE the insert, so no probe can observe a populated registry + // through a `false` flag. + ARGUMENTS_OBJECTS_EVER_USED.store(true, std::sync::atomic::Ordering::Relaxed); ARGUMENTS_OBJECTS.with(|m| { m.borrow_mut().insert( obj as usize, @@ -205,12 +235,46 @@ pub extern "C" fn js_arguments_object_map_index( } pub(crate) fn is_arguments_object(obj: *const ObjectHeader) -> bool { + #[cfg(test)] + TEST_ARGUMENTS_REGISTRY_PROBES.with(|c| c.set(c.get().wrapping_add(1))); + // #7854: nothing has ever been created ⟹ nothing can be found. Checked + // first because it is the only arm that costs neither a thread-local + // resolution nor a hash. See `ARGUMENTS_OBJECTS_EVER_USED`. + if arguments_registry_never_used() { + return false; + } if obj.is_null() { return false; } ARGUMENTS_OBJECTS.with(|m| m.borrow().contains_key(&(obj as usize))) } +/// Every entry into [`is_arguments_object`]. Twin of +/// `set::TEST_SET_REGISTRY_PROBES` — lets a test assert that the latch +/// actually short-circuits rather than merely that nothing threw. +#[cfg(test)] +thread_local! { + static TEST_ARGUMENTS_REGISTRY_PROBES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_arguments_registry_never_used() -> bool { + arguments_registry_never_used() +} + +/// Test-only: drive the process-global latch directly so a test can observe +/// BOTH states deterministically instead of depending on which test in the +/// binary ran first. Callers must restore the previous value. +#[cfg(test)] +pub(crate) fn test_force_arguments_registry_ever_used(value: bool) { + ARGUMENTS_OBJECTS_EVER_USED.store(value, std::sync::atomic::Ordering::Relaxed); +} + +#[cfg(test)] +pub(crate) fn test_arguments_registry_probe_count() -> u64 { + TEST_ARGUMENTS_REGISTRY_PROBES.with(|c| c.get()) +} + pub(crate) unsafe fn arguments_object_get_index( obj: *const ObjectHeader, index: u32, diff --git a/crates/perry-runtime/src/object/arguments_latch_tests.rs b/crates/perry-runtime/src/object/arguments_latch_tests.rs new file mode 100644 index 0000000000..c6b875966c --- /dev/null +++ b/crates/perry-runtime/src/object/arguments_latch_tests.rs @@ -0,0 +1,96 @@ +//! The `#7854` arguments-registry emptiness latch. +//! +//! `is_arguments_object` is a PROBE: it runs on paths that have nothing to do +//! with `arguments` — the by-name property-get tail, `Array.prototype.push`, +//! the array / `Symbol.iterator` iterator entries, `Array.from` / `concat`, +//! class construction. Before the latch every one of those paid a thread-local +//! resolution plus a `RefCell` borrow plus a pointer hash to prove the absence +//! of a feature most programs never use (2.8% of `gc-handoff/apps/interp.ts`). +//! +//! The whole soundness argument is "`ARGUMENTS_OBJECTS` has exactly ONE insert +//! site, and it arms the latch before inserting". These tests pin both halves, +//! and they pin THE SUBJECT rather than the answer: `latch_off_is_what_makes +//! _the_probe_cheap` forces the latch off with a real entry present and requires +//! the probe to answer `false`, so a deleted or never-taken short-circuit turns +//! that test red instead of leaving it quietly green. (A test that only asserted +//! `is_arguments_object(args) == true` would pass with the latch deleted — case +//! 4 of CLAUDE.md's "four ways a gate can be unable to fail".) +//! +//! The latch is process-global on purpose (Darwin has no local-exec TLS, so a +//! `thread_local!` flag would cost the very `_tlv_get_addr` this removes), which +//! makes it only ever CONSERVATIVE: one thread arming it sends every thread back +//! to the registry, i.e. to the pre-#7854 behaviour. + +use super::*; + +fn plain_object() -> *mut ObjectHeader { + js_object_alloc(0, 4) +} + +fn args_object() -> *mut ObjectHeader { + let arr = crate::array::js_array_alloc(2); + let arr = crate::array::js_array_push_f64(arr, 1.0); + let arr = crate::array::js_array_push_f64(arr, 2.0); + let raw_args = crate::value::js_nanbox_pointer(arr as i64); + let callee = f64::from_bits(crate::value::TAG_UNDEFINED); + js_arguments_object_alloc(raw_args, callee, 0) +} + +/// The load-bearing claim: creating an arguments object arms the latch. If a +/// future edit adds a second `ARGUMENTS_OBJECTS` insert site without arming it, +/// `is_arguments_object` starts answering `false` for a real arguments object — +/// a silent wrong answer — and this goes red. +#[test] +fn creating_an_arguments_object_arms_the_latch() { + let args = args_object(); + assert!( + !crate::object::arguments::test_arguments_registry_never_used(), + "js_arguments_object_alloc must arm ARGUMENTS_OBJECTS_EVER_USED before inserting" + ); + assert!( + crate::object::is_arguments_object(args), + "a real arguments object must still be recognised once the latch is armed" + ); + assert!( + !crate::object::is_arguments_object(plain_object()), + "an ordinary object must not be recognised as an arguments object" + ); + assert!( + !crate::object::is_arguments_object(std::ptr::null()), + "a null receiver must answer false" + ); +} + +/// Sabotage: with a REAL arguments object registered, force the latch back off +/// and require the probe to answer `false`. That answer is wrong — deliberately +/// — and it is the proof that the short-circuit is the arm being taken, not +/// dead code sitting in front of a registry lookup that would have answered +/// anyway. Deleting the `arguments_registry_never_used()` early-out makes this +/// test fail. +/// +/// Also asserts the probe was ENTERED (`TEST_ARGUMENTS_REGISTRY_PROBES` moves), +/// so a future refactor that inlines the call away cannot leave this vacuous. +#[test] +fn latch_off_is_what_makes_the_probe_cheap() { + let args = args_object(); + assert!(crate::object::is_arguments_object(args)); + + let restore = !crate::object::arguments::test_arguments_registry_never_used(); + crate::object::arguments::test_force_arguments_registry_ever_used(false); + let before = crate::object::arguments::test_arguments_registry_probe_count(); + let answered = crate::object::is_arguments_object(args); + let probes_moved = crate::object::arguments::test_arguments_registry_probe_count() > before; + crate::object::arguments::test_force_arguments_registry_ever_used(restore); + + assert!(probes_moved, "the probe must have been entered"); + assert!( + !answered, + "with the latch off the probe must short-circuit before the registry — \ + a `true` here means the early-out is gone and every non-`arguments` \ + program is paying the thread-local + hash again" + ); + assert!( + crate::object::is_arguments_object(args), + "restoring the latch must restore the correct answer" + ); +} diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 24de8badbc..6019711722 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -27,6 +27,8 @@ pub(crate) const INLINE_SLOT_FLOOR: usize = 4; // 11.2k-line object.rs. Public re-exports keep FFI symbols stable. mod alloc; mod arguments; +#[cfg(test)] +mod arguments_latch_tests; mod array_object_ops; mod assert; mod async_generator_queue; diff --git a/test-files/test_gap_declared_field_type_refine_guarded.ts b/test-files/test_gap_declared_field_type_refine_guarded.ts new file mode 100644 index 0000000000..ef3751bde9 --- /dev/null +++ b/test-files/test_gap_declared_field_type_refine_guarded.ts @@ -0,0 +1,143 @@ +// #7854: `const xs = obj.field` now refines `xs` from the receiver's DECLARED +// annotation (class / interface / `type X = {…}` alias), seeing through a +// `T | null` union and a reassigned receiver. That is a CLAIM, not a proof — +// Perry does not enforce declared types at runtime — so every consumer the +// refinement newly reaches must be a guarded tier. +// +// This file hands the claim values that violate it and requires the ordinary +// JavaScript answer anyway. If the refined `string[]` ever authorised an +// unguarded element load or an unguarded ArrayHeader length read, the non-array +// rows below would print garbage (or crash) instead of `undefined`. + +type Bag = { items: string[]; label: string }; + +interface IBag { + items: string[]; + label: string; +} + +class CBag { + items: string[]; + label: string; + constructor(items: string[], label: string) { + this.items = items; + this.label = label; + } +} + +// The lie: `v` is whatever the caller passed, stored into a slot the type +// system says is `string[]`. +function mkAlias(v: any): Bag { + return { items: v, label: "alias" }; +} +function mkIface(v: any): IBag { + return { items: v, label: "iface" }; +} +function mkClass(v: any): CBag { + return new CBag(v, "class"); +} + +// Shaped like `interp.ts`'s `lookup`: a nullable, REASSIGNED cursor, so the +// refinement has to see through both `Bag | null` and `reassigned_locals`. +function readAlias(head: Bag | null): string { + let e: Bag | null = head; + let out = ""; + while (e !== null) { + const items = e.items; + out = out + e.label + "|len=" + items.length; + for (let i = 0; i < 2; i++) { + out = out + "|" + i + "=" + items[i]; + } + e = null; + } + return out; +} + +function readIface(head: IBag | null): string { + let e: IBag | null = head; + let out = ""; + while (e !== null) { + const items = e.items; + out = out + e.label + "|len=" + items.length + "|0=" + items[0]; + e = null; + } + return out; +} + +function readClass(head: CBag | null): string { + let e: CBag | null = head; + let out = ""; + while (e !== null) { + const items = e.items; + out = out + e.label + "|len=" + items.length + "|0=" + items[0]; + e = null; + } + return out; +} + +// Honest row first — this is the one the optimization exists for. +console.log(readAlias(mkAlias(["a", "b", "c"]))); +console.log(readIface(mkIface(["x"]))); +console.log(readClass(mkClass(["y", "z"]))); + +// Every row below violates the declared type. +console.log(readAlias(mkAlias("hello"))); // string: has .length, indexes to chars +console.log(readAlias(mkAlias({ length: 7, 0: "zero" }))); // plain object aping an array +console.log(readAlias(mkAlias(42))); // number: no .length, no index + +// A nullish field value must still THROW on `.length`, not read a header. +function readCaught(v: any): string { + try { + return readAlias(mkAlias(v)); + } catch (err) { + return "threw:" + (err instanceof TypeError); + } +} +console.log(readCaught(null)); +console.log(readCaught(undefined)); + +console.log(readIface(mkIface({ length: 3 }))); +console.log(readClass(mkClass("qq"))); + +// A nested read chain: the refined local feeds another declared-type read. +type Outer = { inner: Bag; tag: string }; +function readOuter(o: Outer | null): string { + let e: Outer | null = o; + let out = ""; + while (e !== null) { + const inner = e.inner; + const items = inner.items; + out = out + e.tag + "/" + inner.label + "/" + items.length + "/" + items[0]; + e = null; + } + return out; +} +console.log(readOuter({ inner: mkAlias(["deep"]), tag: "o" })); +console.log(readOuter({ inner: mkAlias(3.5), tag: "o" })); + +// Mutation through the refined local must still go through the normal +// (guarded) array store path when the receiver really is an array, and must +// stay a plain property write when it is not. +function pushish(b: Bag, v: string): string { + const items = b.items; + items[0] = v; + return "" + items[0] + "/" + items.length; +} +console.log(pushish(mkAlias(["old"]), "new")); +console.log(pushish(mkAlias({ length: 1 }), "new")); + +// An element read whose DECLARED element type is `string` but whose runtime +// value is not: `===` against a string literal must not take a string-only +// comparison, and `+` must not take a concat-only lowering. +function scan(b: Bag, needle: string): string { + const items = b.items; + let out = ""; + for (let i = 0; i < 3; i++) { + const v = items[i]; + out = out + "|" + (v === needle) + "," + (v === 7) + "," + typeof v + "," + (v + "!"); + } + return out; +} +console.log(scan(mkAlias(["a", "7", "c"]), "a")); +console.log(scan(mkAlias([1, 7, true]), "a")); +console.log(scan(mkAlias([null, undefined, { z: 1 }]), "a"));