Skip to content

Commit 0065945

Browse files
proggeramlugRalph Küpper
andauthored
fix(codegen): scalar replacement ignored own-method writes and prototype mutation (#5872) (#6324)
* fix(codegen): scalar replacement ignored own-method writes and prototype mutation (#5872) #5466 taught codegen's escape check to keep a `new C()` receiver scalar-replaced across a method call when `simple_scalar_method_summary` proves the method body is a numeric read of `this.<field>`. That summary proves the method *body* is safe to inline; it does not prove that `obj.method` still RESOLVES to that class method at the call site. JS lets user code break the lookup after construction, and both shapes reached the fast path: * own-property shadow — `(obj as any).getValue = () => 99` writes an own property that wins over the prototype method. The escape check treats a literal-key property write on a candidate as a plain field store, so the receiver stayed scalar-replaced and `obj.getValue()` folded to the field value: 14 instead of 99. * prototype mutation — `C.prototype.getValue = fn`, `Object.defineProperty(C.prototype, …)`, `C.prototype[key] = fn`, or a helper/constructor/field-initializer that does any of those. None of it touches the receiver local, so a per-function walk cannot see it. Before #5466 every method call escaped its receiver, so the only layer that folded `obj.m()` into a field read was perry-transform's exact-receiver inliner — which invalidates its facts on exactly these mutations. Restores the missing half with two narrow fact sets: * `ModuleDispatchFacts` (module-scoped): every class whose prototype is *named* anywhere in the module — top-level init, any function body, any constructor / field initializer / method / accessor / computed member. Naming is enough, because a named prototype can be aliased and written through later. A prototype named through an expression that can't be pinned to a declared class marks the whole module opaque. Function-classic prototypes (`function F(){}; F.prototype.m = …`) are keyed by a synthetic class id and cannot rewrite a declared class's table, so they don't count. * per-candidate own-property writes (function-scoped): the literal property names written directly on each scalar-replacement candidate. A summarized method call keeps its receiver off the heap only when the receiver's class chain has a stable prototype and no own write shadows the called method; otherwise the receiver escapes and takes the ordinary heap-allocate + dispatch path, which observes the shadow exactly like Node. Only receivers of summarized method calls are affected — plain field scalar replacement is untouched. Also repairs the IR guard itself, which has been failing on main since #5294 independently of this bug: it asserted the unsafe receivers allocate by grepping for `js_inline_arena_state`, but #5294 outlined the per-new-site inline bump allocator, so class instances now allocate via `js_object_alloc_class_inline_keys`. The guard therefore reported "scalarized instead of allocated" even for correctly heap-allocated receivers. It now accepts the whole allocation-helper family — and still fails on unfixed main, where `ownMethodWrite()` genuinely emits no allocation at all. test-files/test_issue_945_scalar_method_guards.ts is byte-identical to `node --experimental-strip-types` again (own method write 99, computed method write 101, loop mutation receiver 132). * test(codegen): unit-cover the #5872 scalar-method dispatch guard compile-smoke (the IR guard) and parity only run on tag pushes / labeled PRs, which is why #5466 could land this regression green. These run in cargo-test, the PR gate: * an own-property write shadowing the called method escapes the receiver (the #5872 repro), including when the write is nested in a loop; * a write to a different property name does not; * a prototype mutation anywhere in the module — including from an unrelated function — escapes the receiver, while an unrelated class's mutation does not; * merely naming a class prototype marks it unstable, an unattributable prototype access marks the whole module opaque, and a default fact set is conservative. Also fixes the two existing hir_facts tests for the new fact-graph arity. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 parent 7a3b48a commit 0065945

11 files changed

Lines changed: 741 additions & 4 deletions

File tree

crates/perry-codegen/src/codegen/closure.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,7 @@ pub(super) fn compile_closure(
738738
module_globals,
739739
classes,
740740
&cross_module.compile_time_constants,
741+
&cross_module.module_dispatch,
741742
);
742743

743744
let mut ctx = FnCtx {

crates/perry-codegen/src/codegen/entry.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,7 @@ pub(super) fn compile_module_entry(
552552
module_globals,
553553
classes,
554554
&cross_module.compile_time_constants,
555+
&cross_module.module_dispatch,
555556
);
556557
let mut init_local_types: HashMap<u32, perry_types::Type> = HashMap::new();
557558
crate::boxed_vars::collect_let_types_in_stmts(&hir.init, &mut init_local_types);
@@ -1112,6 +1113,7 @@ pub(super) fn compile_module_entry(
11121113
module_globals,
11131114
classes,
11141115
&cross_module.compile_time_constants,
1116+
&cross_module.module_dispatch,
11151117
);
11161118
let mut ctx = FnCtx {
11171119
func: init_fn,

crates/perry-codegen/src/codegen/function.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,7 @@ pub(super) fn compile_function(
456456
module_globals,
457457
classes,
458458
&cross_module.compile_time_constants,
459+
&cross_module.module_dispatch,
459460
);
460461

461462
let mut ctx = FnCtx {

crates/perry-codegen/src/codegen/method.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,7 @@ pub(super) fn compile_method(
371371
module_globals,
372372
classes,
373373
&cross_module.compile_time_constants,
374+
&cross_module.module_dispatch,
374375
);
375376

376377
let mut ctx = FnCtx {
@@ -1295,6 +1296,7 @@ pub(super) fn compile_static_method(
12951296
module_globals,
12961297
classes,
12971298
&cross_module.compile_time_constants,
1299+
&cross_module.module_dispatch,
12981300
);
12991301

13001302
let mut ctx = FnCtx {

crates/perry-codegen/src/codegen/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1535,6 +1535,7 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result<Vec<u8>>
15351535
compile_time_constants,
15361536
target_triple: triple.clone(),
15371537
app_metadata: opts.app_metadata.clone(),
1538+
module_dispatch: crate::collectors::collect_module_dispatch_facts(hir),
15381539
clamp3_functions: hir
15391540
.functions
15401541
.iter()

crates/perry-codegen/src/codegen/opts.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -728,6 +728,12 @@ pub(crate) struct CrossModuleCtx {
728728
pub target_triple: String,
729729
/// App metadata backing compile-time `perry/system` introspection APIs.
730730
pub app_metadata: AppMetadata,
731+
/// Issue #5872: module-wide record of the prototype mutations that can
732+
/// change what `obj.method` resolves to. Scalar replacement consults this
733+
/// before it lets a summarized method call keep its receiver off the heap —
734+
/// the mutation usually lives in a *different* function (or a constructor,
735+
/// or a field initializer) than the `new`, so a per-function walk misses it.
736+
pub module_dispatch: crate::collectors::ModuleDispatchFacts,
731737
/// Functions with a 3-param clamp pattern: fid → true. Call sites
732738
/// emit `@llvm.smax.i32` + `@llvm.smin.i32` instead of a function call.
733739
pub clamp3_functions: std::collections::HashSet<u32>,

crates/perry-codegen/src/collectors/escape_news.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ pub fn collect_non_escaping_news(
77
boxed_vars: &HashSet<u32>,
88
module_globals: &std::collections::HashMap<u32, String>,
99
classes: &std::collections::HashMap<String, &perry_hir::Class>,
10+
module_dispatch: &super::ModuleDispatchFacts,
1011
) -> std::collections::HashMap<u32, String> {
1112
// Pass 1: find candidates — Let bindings of New that aren't boxed/global.
1213
let mut candidates: std::collections::HashMap<u32, String> = std::collections::HashMap::new();
@@ -51,6 +52,24 @@ pub fn collect_non_escaping_news(
5152
}
5253
}
5354

55+
// Pass 4 (issue #5872): a method call only keeps its receiver scalar-
56+
// replaced when `simple_scalar_method_summary` accepts it (see the
57+
// `Expr::Call` arm of `check_escapes_in_expr`). The summary proves the
58+
// method *body* is a numeric read of `this.<field>` — it does NOT prove
59+
// that `obj.method` still RESOLVES to that class method. An own-property
60+
// write (`(obj as any).getValue = () => 99`) or a prototype mutation
61+
// (`C.prototype.getValue = fn`, possibly from another function or from the
62+
// constructor) replaces the target, and the inlined field read then returns
63+
// the wrong value. Escape those receivers so they take the ordinary
64+
// heap-allocate + dispatch path.
65+
super::mark_unstable_scalar_method_receivers(
66+
stmts,
67+
&candidates,
68+
classes,
69+
module_dispatch,
70+
&mut escaped,
71+
);
72+
5473
candidates.retain(|id, _| !escaped.contains(id));
5574
candidates
5675
}

crates/perry-codegen/src/collectors/hir_facts.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ pub(crate) fn collect_type_facts(
306306
module_globals: &HashMap<u32, String>,
307307
classes: &HashMap<String, &perry_hir::Class>,
308308
compile_time_constants: &HashMap<u32, f64>,
309+
module_dispatch: &super::ModuleDispatchFacts,
309310
) -> TypeFacts {
310311
let integer_locals = super::integer_locals::collect_integer_locals(
311312
stmts,
@@ -323,8 +324,13 @@ pub(crate) fn collect_type_facts(
323324
clamp_fn_ids,
324325
);
325326
let known_noalias_buffer_locals = collect_known_noalias_buffer_locals(stmts);
326-
let non_escaping_news =
327-
super::escape_news::collect_non_escaping_news(stmts, boxed_vars, module_globals, classes);
327+
let non_escaping_news = super::escape_news::collect_non_escaping_news(
328+
stmts,
329+
boxed_vars,
330+
module_globals,
331+
classes,
332+
module_dispatch,
333+
);
328334
let non_escaping_new_used_fields =
329335
super::escape_news::collect_non_escaping_new_used_fields(stmts, &non_escaping_news);
330336
let non_escaping_arrays =
@@ -415,6 +421,7 @@ pub(crate) fn collect_native_region_fact_graph(
415421
module_globals: &HashMap<u32, String>,
416422
classes: &HashMap<String, &perry_hir::Class>,
417423
compile_time_constants: &HashMap<u32, f64>,
424+
module_dispatch: &super::ModuleDispatchFacts,
418425
) -> NativeRegionFactGraph {
419426
collect_type_facts(
420427
stmts,
@@ -426,6 +433,7 @@ pub(crate) fn collect_native_region_fact_graph(
426433
module_globals,
427434
classes,
428435
compile_time_constants,
436+
module_dispatch,
429437
)
430438
}
431439

@@ -447,6 +455,9 @@ pub(crate) fn collect_hir_facts(
447455
&HashMap::new(),
448456
&HashMap::new(),
449457
&HashMap::new(),
458+
// No class table here, so no scalar-method summary can apply; the
459+
// conservative default keeps it that way if one ever could.
460+
&super::ModuleDispatchFacts::default(),
450461
)
451462
}
452463

@@ -1653,6 +1664,7 @@ mod tests {
16531664
&HashMap::new(),
16541665
&HashMap::new(),
16551666
&constants,
1667+
&crate::collectors::ModuleDispatchFacts::default(),
16561668
);
16571669

16581670
assert!(graph.known_noalias_buffer_locals().contains(&1));
@@ -1742,6 +1754,7 @@ mod tests {
17421754
&HashMap::new(),
17431755
&HashMap::new(),
17441756
&HashMap::new(),
1757+
&crate::collectors::ModuleDispatchFacts::default(),
17451758
);
17461759

17471760
assert!(graph.integer_locals().contains(&1));

crates/perry-codegen/src/collectors/mod.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ mod local_refs;
2121
mod mutation;
2222
mod pointer_locals;
2323
mod refs;
24+
mod scalar_method_dispatch;
2425
mod scalar_methods;
2526
mod shadow_slots;
2627
mod this_as_value;
@@ -75,6 +76,9 @@ pub(crate) use pointer_locals::collect_pointer_typed_locals;
7576
pub(crate) use refs::{
7677
collect_let_ids, collect_ref_ids_in_expr, collect_ref_ids_in_stmts, is_clamp_call,
7778
};
79+
pub(crate) use scalar_method_dispatch::{
80+
collect_module_dispatch_facts, mark_unstable_scalar_method_receivers, ModuleDispatchFacts,
81+
};
7882
pub(crate) use scalar_methods::simple_scalar_method_summary;
7983
pub(crate) use shadow_slots::{
8084
collect_declared_shadow_slots_in_stmts, collect_shadow_slot_clear_points,

0 commit comments

Comments
 (0)