fix(transform): preserve transitive class dependencies when inlining

changelog.d/9766-transitive-class-inlining.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Keep cross-module helpers and methods that depend on imported classes in their source module, preserving constructors, methods, and iterators. Fixes the three failing codehz/ecs comprehensive performance tests (#9023).

crates/perry-transform/src/inline/cross_module.rs

@@ -155,12 +155,13 @@ pub fn gather_cross_module_functions(module: &Module) -> HashMap<String, Functio
     // the source module's classes. Anon-shape classes are the one safe
     // exception: their names are content-addressed and the existing anon-class
     // propagation pass installs their definitions in the destination.
-    let source_class_names: HashSet<String> = module
+    let mut source_class_names: HashSet<String> = module
         .classes
         .iter()
         .flat_map(|class| std::iter::once(class.name.clone()).chain(class.aliases.iter().cloned()))
         .filter(|name| !name.starts_with("__AnonShape_"))
         .collect();
+    source_class_names.extend(imported_binding_names(module));
 
     let mut out = HashMap::new();
     for (exported_name, root_id) in &module.exported_functions {
@@ -193,7 +194,7 @@ pub fn gather_cross_module_functions(module: &Module) -> HashMap<String, Functio
             stmt_count = stmt_count.saturating_add(recursive_stmt_count(&function.body));
             if stmt_count > MAX_CROSS_MODULE_FUNCTION_STMTS
                 || !function_shell_is_cross_module_safe(function, &allowed_ids, &mut extern_names)
-                || body_references_class_in_set(&function.body, &source_class_names)
+                || function_references_class_in_set(function, &source_class_names)
             {
                 safe = false;
                 break;
@@ -1016,7 +1017,7 @@ pub fn gather_cross_module_methods(module: &Module) -> HashMap<(String, String),
             if !is_cross_module_safe(&method.body) {
                 continue;
             }
-            if body_references_class_in_set(&method.body, &nonexported) {
+            if function_references_class_in_set(method, &nonexported) {
                 continue;
             }
             out.insert(
@@ -1095,7 +1096,7 @@ pub fn gather_cross_module_methods_with_extern_imports(
             // so the source module's codegen — which DOES have the class
             // metadata — emits the correct inline-alloc with the right
             // class_id.
-            if body_references_class_in_set(&method.body, &nonexported) {
+            if function_references_class_in_set(method, &nonexported) {
                 continue;
             }
             extern_names.sort();
@@ -1242,13 +1243,13 @@ pub fn is_cross_module_safe_with_externs(body: &[Stmt], extern_names: &mut Vec<S
 /// `Expr::ClassRef` / `Expr::StaticFieldGet` / etc. that names one of these
 /// classes will lose its class metadata at codegen time. Refs #486.
 ///
-/// The `__AnonShape_*` content-addressed shapes are deliberately INCLUDED in
-/// the set despite never being marked `is_exported` — but the inliner already
-/// propagates them via `extra_anon_classes` so the destination module
-/// synthesizes the same definition. We exclude them here so methods that
-/// `new __AnonShape_<hash>()` keep their inlinability.
+/// Imported bindings are also source-local dependencies (#9023): exporting a
+/// method does not export the classes that its module imports.
+///
+/// The `__AnonShape_*` content-addressed shapes are excluded: the inliner
+/// propagates their definitions via `extra_anon_classes`.
 pub fn collect_nonexported_class_names(module: &Module) -> HashSet<String> {
-    let mut set = HashSet::new();
+    let mut set: HashSet<String> = imported_binding_names(module).collect();
     for c in &module.classes {
         if c.is_exported {
             // Refs #486: even for an EXPORTED class, the inner self-binding
@@ -1278,98 +1279,86 @@ pub fn collect_nonexported_class_names(module: &Module) -> HashSet<String> {
     set
 }
 
-/// Returns true iff `stmts` references any class whose name is in `set`.
-/// Walks every Expr variant that carries a `class_name` string. Used by
-/// the cross-module method gathering passes to reject candidates whose
-/// body would dangle (or worse: silently fall to a class_id=0 placeholder)
-/// after being copied into a destination module.
-pub fn body_references_class_in_set(stmts: &[Stmt], set: &HashSet<String>) -> bool {
-    fn check_expr(expr: &Expr, set: &HashSet<String>) -> bool {
-        match expr {
-            Expr::New { class_name, .. }
-            | Expr::ClassRef(class_name)
-            | Expr::StaticFieldGet { class_name, .. }
-            | Expr::StaticFieldSet { class_name, .. }
-            | Expr::ClassStaticSymbolSet { class_name, .. }
-            | Expr::RegisterClassParentDynamic { class_name, .. }
-            | Expr::RegisterClassStaticSymbol { class_name, .. }
-            | Expr::StaticMethodCall { class_name, .. }
-                if set.contains(class_name) =>
+/// Imported class bindings belong to the source module just like local class
+/// names. Copying `new ImportedBag()` does not copy its import or class metadata
+/// (#9023). Include every import binding: the class-reference check below only
+/// consults this set for class-bearing expressions, so ordinary imported calls
+/// still use the existing extern-import localization path.
+fn imported_binding_names(module: &Module) -> impl Iterator<Item = String> + '_ {
+    module.imports.iter().flat_map(|import| {
+        import.specifiers.iter().map(|specifier| match specifier {
+            ImportSpecifier::Named { local, .. }
+            | ImportSpecifier::Default { local }
+            | ImportSpecifier::Namespace { local } => local.clone(),
+        })
+    })
+}
+
+fn function_references_class_in_set(function: &Function, set: &HashSet<String>) -> bool {
+    body_references_class_in_set(&function.body, set)
+        || function.params.iter().any(|param| {
+            param
+                .default
+                .as_ref()
+                .is_some_and(|default| expr_references_class_in_set(default, set))
+        })
+}
+
+fn expr_references_class_in_set(expr: &Expr, set: &HashSet<String>) -> bool {
+    let contains = |name: &str| {
+        set.contains(name)
+            || name
+                .split_once('.')
+                .is_some_and(|(namespace, _)| set.contains(namespace))
+    };
+    match expr {
+        Expr::New { class_name, .. }
+        | Expr::ClassRef(class_name)
+        | Expr::StaticFieldGet { class_name, .. }
+        | Expr::StaticFieldSet { class_name, .. }
+        | Expr::ClassStaticSymbolSet { class_name, .. }
+        | Expr::RegisterClassParentDynamic { class_name, .. }
+        | Expr::RegisterClassStaticSymbol { class_name, .. }
+        | Expr::StaticMethodCall { class_name, .. }
+            if contains(class_name) =>
+        {
+            return true;
+        }
+        Expr::ClassExprFresh { template, .. } if contains(template) => {
+            return true;
+        }
+        Expr::Closure { params, body, .. } => {
+            if body_references_class_in_set(body, set)
+                || params.iter().any(|param| {
+                    param
+                        .default
+                        .as_ref()
+                        .is_some_and(|default| expr_references_class_in_set(default, set))
+                })
             {
                 return true;
             }
-            Expr::ClassExprFresh { template, .. } if set.contains(template) => {
-                return true;
-            }
-            _ => {}
         }
-        let mut hit = false;
-        walk_expr_children(expr, &mut |child| {
-            if check_expr(child, set) {
-                hit = true;
-            }
-        });
-        hit
+        _ => {}
     }
-    fn check_stmt(s: &Stmt, set: &HashSet<String>) -> bool {
-        match s {
-            Stmt::Let { init, .. } => init.as_ref().is_some_and(|e| check_expr(e, set)),
-            Stmt::Expr(e) | Stmt::Throw(e) | Stmt::Return(Some(e)) => check_expr(e, set),
-            Stmt::Return(None) | Stmt::Break | Stmt::Continue => false,
-            Stmt::LabeledBreak(_) | Stmt::LabeledContinue(_) => false,
-            Stmt::If {
-                condition,
-                then_branch,
-                else_branch,
-            } => {
-                check_expr(condition, set)
-                    || then_branch.iter().any(|s| check_stmt(s, set))
-                    || else_branch
-                        .as_ref()
-                        .is_some_and(|eb| eb.iter().any(|s| check_stmt(s, set)))
-            }
-            Stmt::While { condition, body } | Stmt::DoWhile { body, condition } => {
-                check_expr(condition, set) || body.iter().any(|s| check_stmt(s, set))
-            }
-            Stmt::For {
-                init,
-                condition,
-                update,
-                body,
-            } => {
-                init.as_ref().is_some_and(|s| check_stmt(s, set))
-                    || condition.as_ref().is_some_and(|e| check_expr(e, set))
-                    || update.as_ref().is_some_and(|e| check_expr(e, set))
-                    || body.iter().any(|s| check_stmt(s, set))
-            }
-            Stmt::Switch {
-                discriminant,
-                cases,
-            } => {
-                check_expr(discriminant, set)
-                    || cases.iter().any(|c| {
-                        c.test.as_ref().is_some_and(|e| check_expr(e, set))
-                            || c.body.iter().any(|s| check_stmt(s, set))
-                    })
-            }
-            Stmt::Try {
-                body,
-                catch,
-                finally,
-            } => {
-                body.iter().any(|s| check_stmt(s, set))
-                    || catch
-                        .as_ref()
-                        .is_some_and(|c| c.body.iter().any(|s| check_stmt(s, set)))
-                    || finally
-                        .as_ref()
-                        .is_some_and(|f| f.iter().any(|s| check_stmt(s, set)))
-            }
-            Stmt::Labeled { body, .. } => check_stmt(body.as_ref(), set),
-            Stmt::PreallocateBoxes(_) | Stmt::PreallocateTdzBoxes(_) | Stmt::ReleaseBoxes(_) => {
-                false
-            }
+    let mut hit = false;
+    walk_expr_children(expr, &mut |child| {
+        if expr_references_class_in_set(child, set) {
+            hit = true;
         }
-    }
-    stmts.iter().any(|s| check_stmt(s, set))
+    });
+    hit
+}
+
+/// Returns true iff `stmts` references any class whose name is in `set`.
+/// Walks every Expr variant that carries a `class_name` string. Used by
+/// the cross-module method gathering passes to reject candidates whose
+/// body would dangle (or worse: silently fall to a class_id=0 placeholder)
+/// after being copied into a destination module.
+pub fn body_references_class_in_set(stmts: &[Stmt], set: &HashSet<String>) -> bool {
+    let mut referenced = false;
+    walk_stmts(stmts, &mut |expr| {
+        referenced |= expr_references_class_in_set(expr, set);
+    });
+    referenced
 }

crates/perry-transform/src/inline/mod.rs

@@ -1328,6 +1328,96 @@ mod tests {
         );
     }
 
+    #[test]
+    fn cross_module_imported_class_dependencies_stay_in_the_source_module() {
+        for (specifier, class_name) in [
+            (
+                ImportSpecifier::Named {
+                    imported: "Bag".into(),
+                    local: "ImportedBag".into(),
+                },
+                "ImportedBag",
+            ),
+            (
+                ImportSpecifier::Default {
+                    local: "ImportedBag".into(),
+                },
+                "ImportedBag",
+            ),
+            (
+                ImportSpecifier::Namespace {
+                    local: "bags".into(),
+                },
+                "bags.Bag",
+            ),
+        ] {
+            let mut source = Module::new("/src/helpers.ts");
+            source.imports.push(perry_hir::Import {
+                source: "./bag".into(),
+                specifiers: vec![specifier],
+                is_native: false,
+                module_kind: ModuleKind::NativeCompiled,
+                resolved_path: Some("/src/bag.ts".into()),
+                type_only: false,
+                runtime_erased: false,
+                is_dynamic: false,
+                is_dynamic_target: false,
+                is_deferred_require: false,
+                is_adopted_require: false,
+            });
+            let mut factory = function(1, vec![anon_new(class_name)]);
+            factory.name = "make".into();
+            factory.is_exported = true;
+            source.functions.push(factory.clone());
+            source.exported_functions.push(("make".into(), 1));
+
+            // There is no class declaration in this module: the old local-only
+            // dependency census admitted the imported constructor.
+            assert!(
+                gather_cross_module_functions(&source).is_empty(),
+                "{class_name}"
+            );
+
+            let mut builder = anon_class(2, "Builder");
+            builder.is_exported = true;
+            builder.methods.push(factory);
+            source.classes.push(builder);
+            assert!(
+                gather_cross_module_methods(&source).is_empty(),
+                "{class_name}"
+            );
+            assert!(
+                gather_cross_module_methods_with_extern_imports(&source).is_empty(),
+                "{class_name}"
+            );
+
+            let Stmt::Expr(constructor) = anon_new(class_name) else {
+                unreachable!()
+            };
+            source.functions[0].body = vec![Stmt::Return(Some(Expr::LocalGet(1)))];
+            source.functions[0].params.push(Param {
+                id: 1,
+                name: "value".into(),
+                ty: Type::Any,
+                default: Some(constructor),
+                decorators: Vec::new(),
+                is_rest: false,
+                arguments_object: None,
+            });
+            assert!(
+                gather_cross_module_functions(&source).is_empty(),
+                "default {class_name}"
+            );
+
+            // Importing a class must not disable unrelated helper inlining.
+            source.functions[0].params[0].default = Some(Expr::Integer(7));
+            assert!(
+                gather_cross_module_functions(&source).contains_key("make"),
+                "independent {class_name}"
+            );
+        }
+    }
+
     #[test]
     fn cross_module_free_function_with_module_local_is_rejected() {
         let mut source = Module::new("/src/constants.ts");

test-files/fixtures/issue_9023/bag.ts

@@ -0,0 +1,12 @@
+export class Bag<K, V> {
+ private values = new Map<K, Set<V>>();
+ add(key: K, value: V) {
+  let group = this.values.get(key);
+  if (!group) { group = new Set<V>(); this.values.set(key, group); }
+  group.add(value);
+ }
+ *entries(): IterableIterator<[K, V]> {
+  for (const [key, group] of this.values) for (const value of group) yield [key, value];
+ }
+ [Symbol.iterator]() { return this.entries(); }
+}

test-files/fixtures/issue_9023/defaults.ts

@@ -0,0 +1,8 @@
+import ImportedToken from "./token.ts";
+
+export function readDefault(value = new ImportedToken()) { return value.read(); }
+export function readThunk(make = () => new ImportedToken()) { return make().read(); }
+export class Builder {
+ make() { return new ImportedToken(); }
+ read(value = new ImportedToken()) { return value.read(); }
+}

test-files/fixtures/issue_9023/helpers.ts

@@ -0,0 +1,9 @@
+import { Bag as ImportedBag } from "./bag.ts";
+export function makeBag() { return new ImportedBag<number, number>(); }
+export function getBag(store: Map<number, ImportedBag<number, number>>, key: number) {
+ return store.get(key) ?? new ImportedBag<number, number>();
+}
+export function track(store: Map<number, ImportedBag<number, number>>, key: number, a: number, b: number) {
+ if (!store.has(key)) store.set(key, new ImportedBag<number, number>());
+ store.get(key)!.add(a, b);
+}

test-files/fixtures/issue_9023/token.ts

@@ -0,0 +1,4 @@
+export default class Token {
+ label = "source";
+ read() { return this.label; }
+}

test-files/test_parity_9023_transitive_class_collections.ts

@@ -0,0 +1,13 @@
+// Import only helpers: their inlined bodies must not lose the transitive class.
+import { makeBag, getBag, track } from "./fixtures/issue_9023/helpers.ts";
+const empty = getBag(new Map(), 1);
+let count = 0;
+for (const pair of empty) count++;
+console.log("empty", count);
+const bag = makeBag();
+bag.add(1, 2);
+bag.add(1, 3);
+for (const [key, value] of bag) console.log("pair", key, value);
+const store = new Map();
+track(store, 9, 4, 5);
+for (const [key, value] of getBag(store, 9)) console.log("tracked", key, value);