diff --git a/changelog.d/9766-transitive-class-inlining.md b/changelog.d/9766-transitive-class-inlining.md new file mode 100644 index 0000000000..a276e18eec --- /dev/null +++ b/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). diff --git a/crates/perry-transform/src/inline/cross_module.rs b/crates/perry-transform/src/inline/cross_module.rs index 2027d24b3f..f3c5d865fd 100644 --- a/crates/perry-transform/src/inline/cross_module.rs +++ b/crates/perry-transform/src/inline/cross_module.rs @@ -155,12 +155,13 @@ pub fn gather_cross_module_functions(module: &Module) -> HashMap = module + let mut source_class_names: HashSet = 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 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()` 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 { - let mut set = HashSet::new(); + let mut set: HashSet = 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 { 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) -> bool { - fn check_expr(expr: &Expr, set: &HashSet) -> 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 + '_ { + 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) -> 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) -> 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) -> 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) -> bool { + let mut referenced = false; + walk_stmts(stmts, &mut |expr| { + referenced |= expr_references_class_in_set(expr, set); + }); + referenced } diff --git a/crates/perry-transform/src/inline/mod.rs b/crates/perry-transform/src/inline/mod.rs index 7f675217f7..c496965cda 100644 --- a/crates/perry-transform/src/inline/mod.rs +++ b/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"); diff --git a/test-files/fixtures/issue_9023/bag.ts b/test-files/fixtures/issue_9023/bag.ts new file mode 100644 index 0000000000..737a82acac --- /dev/null +++ b/test-files/fixtures/issue_9023/bag.ts @@ -0,0 +1,12 @@ +export class Bag { + private values = new Map>(); + add(key: K, value: V) { + let group = this.values.get(key); + if (!group) { group = new Set(); 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(); } +} diff --git a/test-files/fixtures/issue_9023/defaults.ts b/test-files/fixtures/issue_9023/defaults.ts new file mode 100644 index 0000000000..78b752bad7 --- /dev/null +++ b/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(); } +} diff --git a/test-files/fixtures/issue_9023/helpers.ts b/test-files/fixtures/issue_9023/helpers.ts new file mode 100644 index 0000000000..0eb3739cf2 --- /dev/null +++ b/test-files/fixtures/issue_9023/helpers.ts @@ -0,0 +1,9 @@ +import { Bag as ImportedBag } from "./bag.ts"; +export function makeBag() { return new ImportedBag(); } +export function getBag(store: Map>, key: number) { + return store.get(key) ?? new ImportedBag(); +} +export function track(store: Map>, key: number, a: number, b: number) { + if (!store.has(key)) store.set(key, new ImportedBag()); + store.get(key)!.add(a, b); +} diff --git a/test-files/fixtures/issue_9023/token.ts b/test-files/fixtures/issue_9023/token.ts new file mode 100644 index 0000000000..4750eca7b2 --- /dev/null +++ b/test-files/fixtures/issue_9023/token.ts @@ -0,0 +1,4 @@ +export default class Token { + label = "source"; + read() { return this.label; } +} diff --git a/test-files/test_parity_9023_transitive_class_collections.ts b/test-files/test_parity_9023_transitive_class_collections.ts new file mode 100644 index 0000000000..180fda6b6e --- /dev/null +++ b/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); diff --git a/test-files/test_parity_9023_transitive_class_defaults.ts b/test-files/test_parity_9023_transitive_class_defaults.ts new file mode 100644 index 0000000000..189322ba1c --- /dev/null +++ b/test-files/test_parity_9023_transitive_class_defaults.ts @@ -0,0 +1,13 @@ +import { readDefault, readThunk, Builder } from "./fixtures/issue_9023/defaults.ts"; + +// The helper's class name must not bind to an unrelated class in the consumer. +class ImportedToken { + read() { return "consumer"; } +} +console.log("consumer", new ImportedToken().read()); +console.log("default", readDefault()); +console.log("explicit", readDefault(new ImportedToken())); +console.log("thunk", readThunk()); +const builder = new Builder(); +console.log("method", builder.make().read()); +console.log("method default", builder.read());