diff --git a/changelog.d/7139-cjs-scaffolding-barrier-exemption.md b/changelog.d/7139-cjs-scaffolding-barrier-exemption.md new file mode 100644 index 0000000000..cf80f3b22f --- /dev/null +++ b/changelog.d/7139-cjs-scaffolding-barrier-exemption.md @@ -0,0 +1,110 @@ +`Ptr` rule 5 kills all shape promotion in any module containing a +`defineProperty`-family site, regardless of target. #7034 §2 recorded that +`Object.defineProperty(exports, "__esModule", { value: true })` — one line of +transpiler boilerplate — was doing this to half the dependency graph. + +It was worse than that, and Perry was the one doing it. `cjs_wrap`'s preamble +emits `Object.defineProperty(require, 'name', { value: 'require', … })` into +**every** wrapped CommonJS module (`cjs_wrap/wrap.rs:841`, the shared +`cjs_preamble` used by both the IIFE wrap and the flat emit). So +`shape_barrier_sites` was true in **100 %** of the CommonJS dependency graph +before a single line of package source ran — and exempting the `__esModule` +marker alone would have recovered exactly nothing. + +Both sites are now recognised, and only those two. A +`ObjectDefineProperty(target, key, desc)` node is exempt iff `target` is +`LocalGet(id)`, the module binds `id` with a `Stmt::Let` named `exports` (resp. +`require`), **every** binding of `id` has an initializer that is not a fresh +allocation at all (`PropertyGet` / `Closure` / `Undefined` / absent), and `key` +is the string literal `"__esModule"` (resp. `"name"`). + +That initializer test is the soundness hinge, and it is deliberately a +**whitelist**. Rule 1's seed set is a moving target — #7034 §4 already landed +return-shape facts under which a call to a proven function *is* a provenance +seed — so a blacklist of `Expr::New` would silently widen this exemption the +day such a seed is wired into `find_new_candidates`. A whitelist fails closed: +an exempted target can never itself be a `Ptr` local, and rule 2's +containment already keeps every promoted object out of any other binding. It is +checked on the HIR rather than assumed from the wrap template, so a future +template change degrades to *no exemption*, never to an unsound one. + +Every other target, key, computed key, and barrier family (`delete`, +`setPrototypeOf` / `__proto__`, `Proxy`, mutating `Reflect.*`) keeps the +module-wide kill. + +## Eligibility recovered, and promotion gained — separately + +85 self-contained `__esModule` CJS leaves from `scriptc/node_modules`, compiled +as `compilePackages` dependencies by both arms (85/85 compiled in both): + +| | before | after | +|---|---|---| +| modules with a rule-5 denial | **85** | **1** | +| `ptr-shape` denials, rule 5 (module barrier) | 117 | 14 | +| `ptr-shape` denials, rule 1 (provenance) | 154 | 154 | +| `ptr-shape` denials, rule 2 (containment) | 0 | **100** | +| distinct `ptr-shape` selections | **0** | 18 (in 2 modules) | +| `ptr-shape` consumptions | **0** | 16 (in 1 module) | + +**Eligibility recovered: 84 of 85 modules (98.8 %).** The holdout has a genuine +barrier of its own. + +**Promotion gained is small, and that is the finding.** Of the ~103 candidates +the barrier stopped denying, **100 were immediately re-denied by rule 2** — +containment, not the barrier, is the real wall in minified dependency code. The +18 that got through: 15 proven-`this` receivers in +`@eslint-community/regexpp` (16 consumption sites) and 3 anon-shape record +locals in `vscode-jsonrpc`'s `linkedMap.js` that are selected and consumed zero +times. Barrier narrowing is now done; the next win is containment (Track D). + +## Template-change canary + +The `require` / `"name"` arm is coupled to `cjs_wrap`'s preamble template by +nothing but a matching binding name, initializer shape and property key. Rename +the local, change the key, or bind it through anything but a function +declaration and the exemption silently stops applying — nothing breaks, no test +fails, and the barrier re-arms for 100 % of CommonJS modules. Since that site is +the one arming every module, silent drift costs the whole win with no symptom: +the "gate that cannot fail" shape CLAUDE.md documents. + +`cjs_wrap::preamble_canary_tests` runs the real template through the real +recogniser (wrap -> parse -> lower -> `module_has_ptr_shape_barrier`) and +asserts the barrier is not armed, plus two anti-vacuity guards: the wrapped +source must still *contain* the `defineProperty(require, …)` site, so the test +cannot pass trivially the day the template drops it and leaves that recogniser +arm as unnoticed dead code; and a positive control proves the same chain still +reports a genuine `delete`. Verified red under four independent perturbations — +template key renamed, template site deleted, recogniser `REQUIRE_KEY` changed, +recogniser `EXPORTS_KEY` changed — with the positive control green in all four. + +## Adjacent question: does the barrier walk run before or after DCE? + +After — but Perry performs no dead-code elimination in a default build, so the +walk sees everything. `collect_module_dispatch_facts` +(`perry-codegen/src/codegen/mod.rs:1385`) walks `hir.init`, every entry of +`hir.functions` whether called or not, and every class member body. The only +two dead-code passes, `reachability::tree_shake` +(`compile/reachability.rs:45`, module granularity) and +`env_fold::fold_env_branches` (`compile/env_fold.rs:30`, statically-false +`process.env` branches), are both gated on `ctx.tree_shake`, which defaults to +`false` (`compile/types.rs:1146`). + +Quantified before acting: of **1750** barrier sites across 718 files in the +corpus, **12 (0.7 %)** sit inside a statically-dead `if`, and removing all of +them would take **zero** files from barrier-armed to barrier-free. Making the +walk DCE-aware would need an intra-module call graph and buys nothing +measurable, so it is recorded here rather than filed. + +## Validation + +`cargo test -p perry-codegen --lib` 417 passed (11 new) plus 2 canary tests in +`perry` (11 new, sabotage-verified red +against the unfixed rule while the negative tests stay green); census gate +green with `batch` `ptr-shape` = 2 asserted against its floor; +`gc_repsel_matrix.sh --arms all --pressure 8` FAIL=0 with the `requires=move` +arms live; emitted IR byte-identical between arms on pure-TS workloads; IR checked at the call sites — the promoted body +loses 3 `js_typed_feedback_class_field_get_guard`, 1 `…_set_guard`, 3 +`js_typed_feedback_record_fallback_call`, 3 `js_object_get_field_by_name_f64`, +1 `js_method_direct_shape_guard` and 1 `js_native_call_method_by_id`, and drops +from 354 to 136 IR lines; behavioural A/B against Node 26.5.1 byte-identical on +both the synthetic probe and `regexpp` parsing/validating real patterns. diff --git a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs new file mode 100644 index 0000000000..25413ef366 --- /dev/null +++ b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs @@ -0,0 +1,646 @@ +//! CommonJS module-scaffolding `Object.defineProperty` sites (#7139). +//! +//! Narrows the `Ptr` rule-5 module-wide barrier +//! ([`super::ptr_shape`] doc, rule 5) so that the two `defineProperty` calls +//! every `cjs_wrap`-compiled CommonJS module contains — neither of them +//! anything to do with user objects — stop disabling shape promotion for the +//! whole module. +//! +//! ## The two sites +//! +//! 1. **Perry's own CJS preamble.** `cjs_wrap` emits +//! `Object.defineProperty(require, 'name', { value: 'require', … })` into +//! *every* wrapped module (`perry/src/commands/compile/cjs_wrap/wrap.rs`, +//! the `cjs_preamble` literal). This alone means **100 %** of the +//! CommonJS dependency graph carried `shape_barrier_sites = true` +//! regardless of what the package source does. +//! 2. **The transpiled-CJS interop marker.** +//! `Object.defineProperty(exports, "__esModule", { value: true })` — the +//! single line tsc/Babel/esbuild put at the top of every emitted CJS file. +//! +//! Exempting only (2) would have recovered nothing, because (1) fires first +//! and unconditionally. Both are recognised here, and nothing else is. +//! +//! ## Predicate +//! +//! An `Expr::ObjectDefineProperty(target, key, desc)` node is exempt iff +//! +//! * `target` is `Expr::LocalGet(id)`; +//! * the module binds `id` with a `Stmt::Let` named `exports` (resp. +//! `require`); +//! * **every** `Stmt::Let` binding of `id` in the module has an initializer in +//! [`init_is_never_a_seed`]'s whitelist (`PropertyGet` / `Closure` / +//! `Undefined` / absent); +//! * `key` is the string literal `"__esModule"` (resp. `"name"`). +//! +//! Every other `defineProperty` target, every other key, every computed key, +//! and every other barrier family (`delete`, `setPrototypeOf` / `__proto__` +//! write, `new Proxy`, mutating `Reflect.*`) keep the module-wide kill +//! untouched. +//! +//! ## Why it is sound +//! +//! A `defineProperty` can only invalidate a `Ptr` local's proof if the +//! object it mutates *is* the object that local holds. Two independent facts +//! rule that out: +//! +//! * **The target is not a candidate.** `Ptr` candidates are seeded by +//! [`super::find_new_candidates`] from `Stmt::Let { init: Some(Expr::New +//! { .. }), .. }`. The third clause above admits only initializers that are +//! not fresh allocations at all — a field read, a function value, or nothing +//! — so an exempted target can never be promoted, and, being a whitelist, +//! it stays true if the seed set widens (#7034 §4's return-shape calls). +//! The check is on the HIR, not on an expectation about the wrap template, +//! so a future template change degrades to *no exemption* rather than to an +//! unsound one. +//! * **No promoted object can reach the target.** Rule 2 (containment) admits +//! a local only when *every* use of it is a declared-chain field +//! read/write/update or a vetted method call; reassignment, aliasing, +//! capture, and passing it as a call/constructor argument all disqualify. +//! A promoted object therefore never flows into another binding, so it can +//! never be the value of `exports` or `require`. +//! +//! The descriptor argument is deliberately unconstrained. A descriptor is a +//! plain value; if it contains an accessor closure, that closure's body is a +//! `Vec` the module barrier walk descends into on its own +//! (`for_each_expr` recurses through `Expr::Closure`), so a barrier *inside* +//! a descriptor still sets the flag. +//! +//! The flag this feeds (`ModuleDispatchFacts::shape_barrier_sites`) is also +//! read by `ptr_numarray` and `proven_this`. The argument above is about the +//! identity of the mutated object, not about which analysis consults the +//! flag, so it carries over unchanged: `exports` and `require` are likewise +//! never `Ptr` locals nor proven-`this` receivers. + +use std::collections::HashSet; + +use perry_hir::{Expr, Module, Stmt}; + +use super::scalar_method_dispatch::{for_each_expr, for_each_expr_in_stmts}; + +/// Binding name / property-key pairs the exemption recognises. Deliberately +/// exhaustive and literal — see the module doc. +const EXPORTS_BINDING: &str = "exports"; +const EXPORTS_KEY: &str = "__esModule"; +const REQUIRE_BINDING: &str = "require"; +const REQUIRE_KEY: &str = "name"; + +/// The module's CommonJS-scaffolding bindings, resolved to `LocalId`s. +#[derive(Debug, Default)] +pub(super) struct CjsScaffolding { + exports: HashSet, + require: HashSet, +} + +impl CjsScaffolding { + /// Is `expr` one of the two recognised scaffolding `defineProperty` + /// sites? Callers use this to *skip* setting + /// `ModuleDispatchFacts::shape_barrier_sites`; it is never consulted for + /// any other barrier family. + pub(super) fn exempts_shape_barrier(&self, expr: &Expr) -> bool { + let Expr::ObjectDefineProperty(target, key, _descriptor) = expr else { + return false; + }; + let Expr::LocalGet(id) = target.as_ref() else { + return false; + }; + let Expr::String(key) = key.as_ref() else { + return false; + }; + (key == EXPORTS_KEY && self.exports.contains(id)) + || (key == REQUIRE_KEY && self.require.contains(id)) + } +} + +/// Resolve the module's `exports` / `require` scaffolding bindings. +/// +/// Mirrors [`super::scalar_method_dispatch::collect_module_dispatch_facts`]'s +/// coverage: module init, every function body, every class member body, and +/// class field initializers / computed keys — plus every closure body nested +/// in any of them, which is where `cjs_wrap`'s IIFE puts the whole CommonJS +/// body. +pub(super) fn collect(module: &Module) -> CjsScaffolding { + let mut acc = Acc::default(); + note_stmt_root(&module.init, &mut acc); + for function in &module.functions { + note_stmt_root(&function.body, &mut acc); + } + for class in &module.classes { + if let Some(ctor) = &class.constructor { + note_stmt_root(&ctor.body, &mut acc); + } + for method in class + .methods + .iter() + .chain(class.static_methods.iter()) + .chain(class.getters.iter().map(|(_, f)| f)) + .chain(class.setters.iter().map(|(_, f)| f)) + .chain(class.computed_members.iter().map(|m| &m.function)) + { + note_stmt_root(&method.body, &mut acc); + } + for field in class.fields.iter().chain(class.static_fields.iter()) { + for expr in field.init.iter().chain(field.key_expr.iter()) { + note_expr_root(expr, &mut acc); + } + } + for member in &class.computed_members { + note_expr_root(&member.key_expr, &mut acc); + } + } + + CjsScaffolding { + exports: acc.exports.difference(&acc.disqualified).copied().collect(), + require: acc.require.difference(&acc.disqualified).copied().collect(), + } +} + +#[derive(Default)] +struct Acc { + exports: HashSet, + require: HashSet, + /// Every local with an initializer outside [`init_is_never_a_seed`]'s + /// whitelist. Subtracted from both sets. + disqualified: HashSet, +} + +impl Acc { + fn note_let(&mut self, stmt: &Stmt) { + let Stmt::Let { id, name, init, .. } = stmt else { + return; + }; + match name.as_str() { + EXPORTS_BINDING => { + self.exports.insert(*id); + } + REQUIRE_BINDING => { + self.require.insert(*id); + } + _ => {} + } + if !init_is_never_a_seed(init.as_ref()) { + self.disqualified.insert(*id); + } + } +} + +/// Can this initializer never make its local a `Ptr` provenance seed? +/// +/// Deliberately a **whitelist** of the three shapes `cjs_wrap` actually emits +/// for its scaffolding bindings, not a blacklist of `Expr::New`. Rule 1's seed +/// set is a moving target — #7034 §4 added return-shape facts so that a call to +/// a proven function *is* a provenance seed, and that machinery +/// (`ModuleDispatchFacts::return_shape_class`) already exists. A blacklist would +/// silently widen this exemption the day such a seed is wired into +/// [`super::find_new_candidates`]; a whitelist fails closed instead. +/// +/// * `PropertyGet` — `var exports = __cjs_module.exports`. A field read of an +/// existing object, never a fresh allocation. +/// * `Closure` — `function require(specifier) { … }`. A function value. +/// * `Undefined` / no initializer — the hoisted `var` pre-declaration. +fn init_is_never_a_seed(init: Option<&Expr>) -> bool { + matches!( + init, + None | Some(Expr::Undefined) | Some(Expr::PropertyGet { .. }) | Some(Expr::Closure { .. }) + ) +} + +/// A statement list plus every closure body reachable from it. +fn note_stmt_root(stmts: &[Stmt], acc: &mut Acc) { + for_each_stmt(stmts, &mut |stmt| acc.note_let(stmt)); + // `for_each_expr_in_stmts` already recurses through `Expr::Closure`, so + // this yields every closure body at any nesting depth exactly once. + for_each_expr_in_stmts(stmts, &mut |expr| { + if let Expr::Closure { body, .. } = expr { + for_each_stmt(body, &mut |stmt| acc.note_let(stmt)); + } + }); +} + +/// Same, rooted at a bare expression (a class field initializer / computed +/// key). Bindings can only appear inside a closure from here. +fn note_expr_root(expr: &Expr, acc: &mut Acc) { + for_each_expr(expr, &mut |node| { + if let Expr::Closure { body, .. } = node { + for_each_stmt(body, &mut |stmt| acc.note_let(stmt)); + } + }); +} + +/// Every statement in `stmts`, descending through nested statement lists but +/// NOT into closure bodies (`note_stmt_root` reaches those separately). +fn for_each_stmt(stmts: &[Stmt], f: &mut dyn FnMut(&Stmt)) { + for stmt in stmts { + f(stmt); + match stmt { + Stmt::If { + then_branch, + else_branch, + .. + } => { + for_each_stmt(then_branch, f); + if let Some(branch) = else_branch { + for_each_stmt(branch, f); + } + } + Stmt::While { body, .. } | Stmt::DoWhile { body, .. } => for_each_stmt(body, f), + Stmt::For { init, body, .. } => { + if let Some(init) = init { + for_each_stmt(std::slice::from_ref(init.as_ref()), f); + } + for_each_stmt(body, f); + } + Stmt::Try { + body, + catch, + finally, + } => { + for_each_stmt(body, f); + if let Some(catch) = catch { + for_each_stmt(&catch.body, f); + } + if let Some(finally) = finally { + for_each_stmt(finally, f); + } + } + Stmt::Switch { cases, .. } => { + for case in cases { + for_each_stmt(&case.body, f); + } + } + Stmt::Labeled { body, .. } => for_each_stmt(std::slice::from_ref(body.as_ref()), f), + Stmt::Expr(_) + | Stmt::Throw(_) + | Stmt::Return(_) + | Stmt::Let { .. } + | Stmt::Break + | Stmt::Continue + | Stmt::LabeledBreak(_) + | Stmt::LabeledContinue(_) + | Stmt::PreallocateBoxes(_) + | Stmt::PreallocateTdzBoxes(_) => {} + } + } +} + +#[cfg(test)] +mod tests { + use super::super::ptr_shape::collect_shape_proven_ptr_locals; + use super::super::scalar_method_dispatch::collect_module_dispatch_facts; + use super::*; + use perry_hir::types::Type; + use perry_hir::{Class, ClassField, Function}; + use std::collections::HashMap; + + const REQUIRE_ID: u32 = 10; + const CJS_MODULE_ID: u32 = 12; + const EXPORTS_ID: u32 = 7; + const POINT_ID: u32 = 42; + + fn closure(func_id: u32, body: Vec) -> Expr { + Expr::Closure { + func_id, + params: Vec::new(), + return_type: Type::Any, + body, + captures: Vec::new(), + mutable_captures: Vec::new(), + captures_this: false, + captures_new_target: false, + enclosing_class: None, + is_arrow: false, + is_async: false, + is_generator: false, + is_strict: true, + } + } + + fn let_stmt(id: u32, name: &str, init: Expr) -> Stmt { + Stmt::Let { + id, + name: name.to_string(), + ty: Type::Any, + mutable: true, + init: Some(init), + } + } + + fn anon_shape(class_name: &str, args: Vec) -> Expr { + Expr::New { + class_name: class_name.to_string(), + args, + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + } + } + + /// `{ value: true }` — how a literal descriptor reaches HIR. + fn descriptor() -> Expr { + anon_shape("__AnonShape_desc", vec![Expr::Bool(true)]) + } + + fn define_property(target: Expr, key: Expr) -> Stmt { + Stmt::Expr(Expr::ObjectDefineProperty( + Box::new(target), + Box::new(key), + Box::new(descriptor()), + )) + } + + /// The `cjs_wrap` preamble, verbatim in HIR shape: a `require` closure, the + /// `__cjs_module` record, and `exports` read out of it. `extra` is appended + /// to the CommonJS body inside the IIFE. + fn cjs_module(extra: Vec) -> perry_hir::Module { + let mut body = vec![ + let_stmt(REQUIRE_ID, "require", closure(7, Vec::new())), + let_stmt( + CJS_MODULE_ID, + "__cjs_module", + anon_shape( + "__AnonShape_module", + vec![anon_shape("__AnonShape_exports", Vec::new())], + ), + ), + let_stmt( + EXPORTS_ID, + "exports", + Expr::PropertyGet { + byte_offset: 0, + object: Box::new(Expr::LocalGet(CJS_MODULE_ID)), + property: "exports".to_string(), + }, + ), + ]; + body.extend(extra); + let mut module = perry_hir::Module::new("node_modules/dep/index.js"); + module.init.push(let_stmt( + 0, + "_cjs", + Expr::Call { + callee: Box::new(closure(2, body)), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + }, + )); + module + } + + /// The two scaffolding sites every `cjs_wrap` module carries. + fn scaffolding_sites() -> Vec { + vec![ + define_property( + Expr::LocalGet(REQUIRE_ID), + Expr::String(REQUIRE_KEY.to_string()), + ), + define_property( + Expr::LocalGet(EXPORTS_ID), + Expr::String(EXPORTS_KEY.to_string()), + ), + ] + } + + fn barrier(extra: Vec) -> bool { + collect_module_dispatch_facts(&cjs_module(extra)).has_shape_barrier_sites() + } + + #[test] + fn cjs_scaffolding_define_property_sites_do_not_arm_the_module_barrier() { + assert!(!barrier(scaffolding_sites())); + } + + /// Each half on its own — so a regression in either recogniser is named. + #[test] + fn each_scaffolding_site_is_exempt_on_its_own() { + for site in scaffolding_sites() { + assert!(!barrier(vec![site])); + } + } + + /// A module with NO scaffolding recogniser at all still has to arm — this + /// is the anti-vacuity check for every assertion above. + #[test] + fn a_define_property_on_an_unrelated_target_still_arms_the_barrier() { + let mut sites = scaffolding_sites(); + sites.push(define_property( + Expr::LocalGet(99), + Expr::String(EXPORTS_KEY.to_string()), + )); + assert!(barrier(sites)); + } + + /// Only the two recognised keys. `defineProperty(exports, "foo", …)` is a + /// real named-export install and keeps the kill. + #[test] + fn another_key_on_the_exports_binding_still_arms_the_barrier() { + assert!(barrier(vec![define_property( + Expr::LocalGet(EXPORTS_ID), + Expr::String("someNamedExport".to_string()), + )])); + } + + #[test] + fn another_key_on_the_require_binding_still_arms_the_barrier() { + assert!(barrier(vec![define_property( + Expr::LocalGet(REQUIRE_ID), + Expr::String("cache".to_string()), + )])); + } + + /// A computed key could be `"__esModule"` at runtime, but it could be + /// anything else too; the predicate demands a literal. + #[test] + fn a_computed_key_on_the_exports_binding_still_arms_the_barrier() { + assert!(barrier(vec![define_property( + Expr::LocalGet(EXPORTS_ID), + Expr::LocalGet(55), + )])); + } + + /// A user binding that happens to be named `exports`, initialized by + /// something outside the scaffolding whitelist, is never exempt. + /// + /// `new` is the soundness hinge today: such a binding IS a rule-1 + /// `Ptr` candidate. `Call` guards the forward direction — #7034 §4's + /// return-shape facts already make a call to a proven function a provenance + /// seed, so a blacklist of `Expr::New` would silently widen this exemption + /// the day that seed is wired into `find_new_candidates`. + #[test] + fn an_exports_binding_outside_the_init_whitelist_is_not_exempt() { + let inits = [ + anon_shape("__AnonShape_user", Vec::new()), + Expr::Call { + callee: Box::new(Expr::LocalGet(77)), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + }, + ]; + for init in inits { + let mut m = perry_hir::Module::new("m.ts"); + m.init.push(let_stmt(EXPORTS_ID, "exports", init.clone())); + m.init.push(define_property( + Expr::LocalGet(EXPORTS_ID), + Expr::String(EXPORTS_KEY.to_string()), + )); + assert!( + collect_module_dispatch_facts(&m).has_shape_barrier_sites(), + "expected a barrier for an `exports` bound to {init:?}" + ); + } + } + + /// A LATER binding of the same id outside the whitelist disqualifies the + /// scaffolding binding too — `var` redeclaration reuses the `LocalId`. + #[test] + fn a_disqualifying_rebinding_of_the_exports_id_removes_the_exemption() { + let mut extra = scaffolding_sites(); + extra.push(let_stmt( + EXPORTS_ID, + "exports", + anon_shape("__AnonShape_user", Vec::new()), + )); + assert!(barrier(extra)); + } + + /// Untouched barrier families: the exemption is scoped to + /// `ObjectDefineProperty`, and only to two targets. + #[test] + fn the_other_barrier_families_are_untouched() { + let others = [ + Expr::Delete(Box::new(Expr::LocalGet(EXPORTS_ID))), + Expr::ObjectSetPrototypeOf(Box::new(Expr::LocalGet(EXPORTS_ID)), Box::new(Expr::Null)), + Expr::ObjectDefineProperties( + Box::new(Expr::LocalGet(EXPORTS_ID)), + Box::new(descriptor()), + ), + Expr::ReflectSet { + target: Box::new(Expr::LocalGet(EXPORTS_ID)), + key: Box::new(Expr::String(EXPORTS_KEY.to_string())), + value: Box::new(Expr::Bool(true)), + receiver: Box::new(Expr::LocalGet(EXPORTS_ID)), + }, + ]; + for other in others { + let mut sites = scaffolding_sites(); + sites.push(Stmt::Expr(other.clone())); + assert!(barrier(sites), "expected a barrier for {other:?}"); + } + } + + // ---- end-to-end: the exemption actually recovers a promotion ---- + + fn point_class() -> Class { + Class { + id: 0, + name: "Point".to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields: ["x", "y"] + .iter() + .map(|n| ClassField { + name: n.to_string(), + key_expr: None, + ty: Type::Number, + init: None, + is_private: false, + is_readonly: false, + decorators: Vec::new(), + }) + .collect(), + constructor: None, + methods: Vec::new(), + getters: Vec::new(), + setters: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + computed_members: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + } + } + + /// `const p = new Point(); p.x = 1; return p.x;` — a textbook rule-1..4 + /// promotion, so the ONLY thing that can deny it is the rule-5 kill. + fn promotable_body() -> Vec { + vec![ + let_stmt(POINT_ID, "p", anon_shape("Point", Vec::new())), + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::LocalGet(POINT_ID)), + property: "x".to_string(), + value: Box::new(Expr::Number(1.0)), + }), + Stmt::Return(Some(Expr::PropertyGet { + byte_offset: 0, + object: Box::new(Expr::LocalGet(POINT_ID)), + property: "x".to_string(), + })), + ] + } + + fn compute_fn() -> Function { + Function { + id: 17, + name: "compute".to_string(), + type_params: Vec::new(), + params: Vec::new(), + return_type: Type::Number, + body: promotable_body(), + is_async: false, + is_generator: false, + is_strict: true, + is_exported: true, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + } + } + + fn promotes_with(extra: Vec) -> bool { + let mut module = cjs_module(extra); + module.classes.push(point_class()); + module.functions.push(compute_fn()); + let facts = collect_module_dispatch_facts(&module); + let point = point_class(); + let classes = HashMap::from([("Point".to_string(), &point)]); + !collect_shape_proven_ptr_locals( + &promotable_body(), + &HashSet::new(), + &HashMap::new(), + &classes, + &facts, + &HashSet::new(), + ) + .is_empty() + } + + /// Red before #7139, green after: the CommonJS scaffolding no longer + /// denies an eligible local elsewhere in the module. + #[test] + fn an_eligible_local_promotes_in_a_module_carrying_only_scaffolding_sites() { + assert!(promotes_with(scaffolding_sites())); + } + + /// Sabotage in the other direction: one genuine barrier anywhere in the + /// module still denies that same local, so the test above is not asserting + /// a promotion that would happen regardless. + #[test] + fn the_same_local_is_denied_when_a_real_barrier_is_present() { + let mut sites = scaffolding_sites(); + sites.push(Stmt::Expr(Expr::Delete(Box::new(Expr::LocalGet( + EXPORTS_ID, + ))))); + assert!(!promotes_with(sites)); + } +} diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index 6ca59f5339..5ee230d7cd 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/crates/perry-codegen/src/collectors/mod.rs @@ -5,6 +5,7 @@ //! v0.5.1019 to satisfy the file-size CI gate. mod.rs is a re-export //! hub — public-API shape (`crate::collectors::*`) is preserved. +mod cjs_scaffolding; mod clamp_detect; mod class_accessors; mod closures; diff --git a/crates/perry-codegen/src/collectors/ptr_shape.rs b/crates/perry-codegen/src/collectors/ptr_shape.rs index 598eca0592..34802a1b49 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape.rs @@ -70,6 +70,18 @@ //! Perry never executes a runtime code string — see //! `perry-hir/src/eval_classifier.rs`.) //! +//! **One exemption** (#7139, `collectors/cjs_scaffolding.rs`): the two +//! `defineProperty` sites every `cjs_wrap`-compiled CommonJS module +//! carries — Perry's own preamble `Object.defineProperty(require, 'name', +//! …)` and the transpiler's `Object.defineProperty(exports, "__esModule", +//! …)` — target module scaffolding whose every binding initializer is a +//! field read, a function value, or nothing, so rule 1 can never seed it +//! and rule 2's containment keeps every promoted object out of it. That +//! initializer test is a whitelist, so widening rule 1's seed set (#7034 +//! §4's return-shape calls) cannot silently widen the exemption. Nothing +//! else is exempt: any other target, any other key, a computed key, or any +//! other barrier family still arms the kill. +//! //! ## Numeric-proven fields //! //! For the READ side to keep today's `JsNumber`/`NativeRep::F64` semantics on a diff --git a/crates/perry-codegen/src/collectors/scalar_method_dispatch.rs b/crates/perry-codegen/src/collectors/scalar_method_dispatch.rs index 72846fba94..231e621853 100644 --- a/crates/perry-codegen/src/collectors/scalar_method_dispatch.rs +++ b/crates/perry-codegen/src/collectors/scalar_method_dispatch.rs @@ -38,6 +38,8 @@ use std::collections::{HashMap, HashSet}; use perry_hir::{Class, Expr, Module, Stmt}; +use super::cjs_scaffolding::CjsScaffolding; + /// Everything in a module that can change what `C.prototype.` resolves to. #[derive(Debug, Clone)] pub struct ModuleDispatchFacts { @@ -188,13 +190,18 @@ pub fn collect_module_dispatch_facts(hir: &Module) -> ModuleDispatchFacts { return_shape_functions: HashMap::new(), }; - note_stmts(&hir.init, &mut facts); + // #7139: resolve the CommonJS wrap's `exports` / `require` scaffolding + // bindings first — the barrier classifier below consults them to skip the + // two `defineProperty` sites every `cjs_wrap`-compiled module contains. + let cjs = super::cjs_scaffolding::collect(hir); + + note_stmts(&hir.init, &mut facts, &cjs); for function in &hir.functions { - note_stmts(&function.body, &mut facts); + note_stmts(&function.body, &mut facts, &cjs); } for class in &hir.classes { if let Some(ctor) = &class.constructor { - note_stmts(&ctor.body, &mut facts); + note_stmts(&ctor.body, &mut facts, &cjs); } for method in class .methods @@ -204,18 +211,18 @@ pub fn collect_module_dispatch_facts(hir: &Module) -> ModuleDispatchFacts { .chain(class.setters.iter().map(|(_, f)| f)) .chain(class.computed_members.iter().map(|m| &m.function)) { - note_stmts(&method.body, &mut facts); + note_stmts(&method.body, &mut facts, &cjs); } for field in class.fields.iter().chain(class.static_fields.iter()) { if let Some(init) = &field.init { - note_expr_tree(init, &mut facts); + note_expr_tree(init, &mut facts, &cjs); } if let Some(key) = &field.key_expr { - note_expr_tree(key, &mut facts); + note_expr_tree(key, &mut facts, &cjs); } } for member in &class.computed_members { - note_expr_tree(&member.key_expr, &mut facts); + note_expr_tree(&member.key_expr, &mut facts, &cjs); } } @@ -231,34 +238,33 @@ pub fn collect_module_dispatch_facts(hir: &Module) -> ModuleDispatchFacts { facts } -fn note_stmts(stmts: &[Stmt], facts: &mut ModuleDispatchFacts) { - for_each_expr_in_stmts(stmts, &mut |expr| { - note_prototype_effect(expr, facts); - if super::ptr_shape::expr_is_shape_barrier(expr) { - facts.shape_barrier_sites = true; - } - if super::ptr_numarray::expr_is_numarray_prototype_index_barrier(expr) { - facts.numarray_prototype_index_barriers = true; - } - if super::proven_this::expr_is_freeze_barrier(expr) { - facts.freeze_barrier_sites = true; - } - }); +fn note_stmts(stmts: &[Stmt], facts: &mut ModuleDispatchFacts, cjs: &CjsScaffolding) { + for_each_expr_in_stmts(stmts, &mut |expr| note_expr(expr, facts, cjs)); } -fn note_expr_tree(expr: &Expr, facts: &mut ModuleDispatchFacts) { - for_each_expr(expr, &mut |node| { - note_prototype_effect(node, facts); - if super::ptr_shape::expr_is_shape_barrier(node) { - facts.shape_barrier_sites = true; - } - if super::ptr_numarray::expr_is_numarray_prototype_index_barrier(node) { - facts.numarray_prototype_index_barriers = true; - } - if super::proven_this::expr_is_freeze_barrier(node) { - facts.freeze_barrier_sites = true; - } - }); +fn note_expr_tree(expr: &Expr, facts: &mut ModuleDispatchFacts, cjs: &CjsScaffolding) { + for_each_expr(expr, &mut |node| note_expr(node, facts, cjs)); +} + +/// Classify one already-visited expression node. +fn note_expr(expr: &Expr, facts: &mut ModuleDispatchFacts, cjs: &CjsScaffolding) { + note_prototype_effect(expr, facts); + // #7139: the CommonJS wrap's own `defineProperty(require, 'name', …)` + // preamble and the transpiled-CJS `defineProperty(exports, "__esModule", + // …)` marker target module scaffolding that can never be a `Ptr` + // local, so they do not arm the rule-5 module-wide kill. Every other + // barrier family and every other target still does. See + // `collectors/cjs_scaffolding.rs` for the predicate and its soundness + // argument. + if super::ptr_shape::expr_is_shape_barrier(expr) && !cjs.exempts_shape_barrier(expr) { + facts.shape_barrier_sites = true; + } + if super::ptr_numarray::expr_is_numarray_prototype_index_barrier(expr) { + facts.numarray_prototype_index_barriers = true; + } + if super::proven_this::expr_is_freeze_barrier(expr) { + facts.freeze_barrier_sites = true; + } } /// Record what a single expression node does to some class's prototype. @@ -428,7 +434,7 @@ pub fn mark_unstable_scalar_method_receivers( // not descend into closure bodies (they are `Vec`), so both are wired up // here. -fn for_each_expr(expr: &Expr, f: &mut dyn FnMut(&Expr)) { +pub(super) fn for_each_expr(expr: &Expr, f: &mut dyn FnMut(&Expr)) { f(expr); perry_hir::walker::walk_expr_children(expr, &mut |child| for_each_expr(child, f)); if let Expr::Closure { body, .. } = expr { diff --git a/crates/perry-codegen/src/lib.rs b/crates/perry-codegen/src/lib.rs index 32d5cb5a9d..56d5a4b1f5 100644 --- a/crates/perry-codegen/src/lib.rs +++ b/crates/perry-codegen/src/lib.rs @@ -106,3 +106,20 @@ pub fn iter_native_method_signatures() -> impl Iterator }, ) } + +/// #7139 template-change canary: does `hir` carry a `Ptr` §5.2 module +/// barrier (`collectors::ModuleDispatchFacts::shape_barrier_sites`)? +/// +/// Public **solely** so the `perry` crate's `cjs_wrap` tests can assert that +/// the CommonJS preamble they emit is still recognised as module scaffolding +/// by `collectors::cjs_scaffolding` — the recogniser is here, the template is +/// there, and only `perry` can see both. That coupling is otherwise silent: +/// a template edit would not break anything, it would just quietly re-arm the +/// barrier for 100 % of CommonJS modules and evaporate the #7139 win with no +/// symptom. Same shape as [`iter_native_method_signatures`], which exists for +/// `perry-api-manifest`'s consistency test. +/// +/// Not part of any codegen contract; nothing in the compile pipeline calls it. +pub fn module_has_ptr_shape_barrier(hir: &perry_hir::Module) -> bool { + collectors::collect_module_dispatch_facts(hir).has_shape_barrier_sites() +} diff --git a/crates/perry/src/commands/compile/cjs_wrap/mod.rs b/crates/perry/src/commands/compile/cjs_wrap/mod.rs index cc1721e7a4..d5539409ad 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/mod.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/mod.rs @@ -44,6 +44,8 @@ mod wrap; #[cfg(test)] mod issue_6585_tests; +#[cfg(test)] +mod preamble_canary_tests; // Cross-sibling helpers — siblings reach for these via `use super::*;`. use detect::is_js_reserved_word; diff --git a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs new file mode 100644 index 0000000000..10b29e894b --- /dev/null +++ b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs @@ -0,0 +1,102 @@ +//! #7139 template-change canary: the CommonJS preamble this module emits must +//! stay recognisable to `perry-codegen`'s `collectors::cjs_scaffolding`. +//! +//! `Ptr` rule 5 disables all shape promotion in a module containing any +//! `defineProperty`-family site. Two sites are exempted as module scaffolding: +//! the transpiler's `Object.defineProperty(exports, "__esModule", …)` and +//! **this file's own** `Object.defineProperty(require, 'name', …)` preamble +//! (`wrap.rs`, the `cjs_preamble` literal). The second one is emitted into +//! every wrapped module, so before #7139 it armed the barrier in 100 % of the +//! CommonJS dependency graph. +//! +//! The recogniser matches on binding name, initializer shape and property key. +//! Nothing links it to the template: rename the `require` local, change the +//! key, or bind it through anything other than a function declaration, and the +//! exemption silently stops applying. Nothing breaks, no test fails, and the +//! entire #7139 win evaporates with no symptom — the "gate that cannot fail" +//! shape CLAUDE.md documents. +//! +//! This canary closes that. It runs the real template through the real +//! recogniser: wrap → parse → lower → `module_has_ptr_shape_barrier`. + +use std::path::Path; + +use super::wrap::wrap_commonjs_for_target; + +/// A minimal transpiled-CJS module: the `__esModule` marker, one class, one +/// function. Deliberately free of every barrier family, so the ONLY thing that +/// can arm the flag is the wrap's own preamble. +const CJS_FIXTURE: &str = r#""use strict"; +Object.defineProperty(exports, "__esModule", { value: true }); +exports.compute = void 0; +class Point { + constructor(x, y) { this.x = x; this.y = y; } + sum() { return this.x + this.y; } +} +function compute(n) { + const p = new Point(n, n + 1); + return p.sum(); +} +exports.compute = compute; +"#; + +fn wrap_and_lower(body: &str) -> perry_hir::Module { + let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); + let wrapped = wrap_commonjs_for_target(body, path, None); + let ast = perry_parser::parse_typescript(&wrapped, "index.js") + .expect("the wrap template must produce parseable ESM"); + perry_hir::lower_module(&ast, "dep", &path.to_string_lossy()) + .expect("the wrap template must produce lowerable HIR") +} + +/// The whole point. If this goes red, the CommonJS preamble in `wrap.rs` no +/// longer matches what `perry-codegen/src/collectors/cjs_scaffolding.rs` +/// recognises — fix one or the other, do not delete this test. +#[test] +fn cjs_preamble_does_not_arm_the_ptr_shape_module_barrier() { + let path = Path::new("/tmp/perry-canary/node_modules/dep/index.js"); + let wrapped = wrap_commonjs_for_target(CJS_FIXTURE, path, None); + + // Anti-vacuity, and the more precise failure of the two: assert the + // preamble still HAS the site the recogniser is written for. Without this + // the test would pass trivially the day the template stops emitting it, + // leaving `cjs_scaffolding.rs`'s `require` / `"name"` arm as dead code + // nobody notices. + assert!( + wrapped.contains("defineProperty(require,"), + "the CJS preamble no longer emits `Object.defineProperty(require, 'name', …)`.\n\ + Either it was renamed — in which case `REQUIRE_BINDING` / `REQUIRE_KEY` in \ + perry-codegen/src/collectors/cjs_scaffolding.rs must follow, or the rule-5 \ + barrier re-arms for every CommonJS module — or it was removed, in which case \ + that arm of the recogniser is now dead code and should go." + ); + assert!( + wrapped.contains(r#"defineProperty(exports, "__esModule""#), + "the wrap no longer passes the transpiler's `__esModule` marker through verbatim; \ + `EXPORTS_BINDING` / `EXPORTS_KEY` in cjs_scaffolding.rs may need to follow" + ); + + let hir = wrap_and_lower(CJS_FIXTURE); + assert!( + !perry_codegen::module_has_ptr_shape_barrier(&hir), + "the CommonJS scaffolding re-armed the Ptr rule-5 module barrier.\n\ + Every `Ptr` promotion in every CommonJS module is now disabled \ + (#7139). Compare the `exports` / `require` bindings the wrap emits against \ + the predicate in perry-codegen/src/collectors/cjs_scaffolding.rs." + ); +} + +/// Positive control: the same wrap → parse → lower → collect chain DOES report +/// a barrier when the body contains a real one. Without this, an empty or +/// failed lowering would make the canary above pass for the wrong reason. +#[test] +fn the_canary_chain_still_reports_a_genuine_barrier() { + let with_barrier = format!("{CJS_FIXTURE}\nconst o = {{ k: 1 }};\ndelete o.k;\n"); + let hir = wrap_and_lower(&with_barrier); + assert!( + perry_codegen::module_has_ptr_shape_barrier(&hir), + "a `delete` in the module body must still arm the barrier — if it does not, \ + the canary above is passing vacuously (parse/lower produced nothing, or the \ + exemption is far wider than #7139 intended)" + ); +}