diff --git a/changelog.d/10630-fn-identity-own-module.md b/changelog.d/10630-fn-identity-own-module.md new file mode 100644 index 0000000000..27f7e52f13 --- /dev/null +++ b/changelog.d/10630-fn-identity-own-module.md @@ -0,0 +1,37 @@ +### Fixed + +- **A function referenced inside its own module is a different object from + the same function imported elsewhere (#10554).** `function f(){}; export + function isSame(x){ return x === f; }; export { f };` gave `isSame(f)` + `false` for an importer's own `f` — identity checks, registries/caches + keyed by function, `removeEventListener`/`off(fn)`, and memoization all + silently took the wrong branch. + + Root cause: the cross-module *function* inliner in + `perry-transform`'s `inline/cross_module.rs` harvests an exported + function's whole value-dependency graph — every function it transitively + references, including by value (`x === f`), not just as a call target — + and clones the entire graph into every importing module under fresh + `__perry_xmod_inline__` symbols. When the referenced function + (`f`) is *also* independently exported, it got cloned alongside the + candidate instead of resolved through its own canonical wrapper. Every + function value materializes into a heap closure keyed by its wrapper + *symbol* (`js_closure_alloc_singleton`), so the clone's `f` and the + canonical `f` every importer resolves through produced two distinct + closures — an in-module identity check comparing them disagreed with + every importer's own view. + + Fix: `gather_cross_module_functions` now refuses a candidate whose + dependency graph would need to bundle a *separately exported* sibling + function referenced by value — it falls back to an ordinary cross-module + call instead, which resolves through the shared canonical wrapper. + Self-recursion is unaffected. The directly-affected shape actually gets + **faster**, not slower: the unsound inline was paying for an extra + closure materialization on every call. + + Validation: new `test_gap_10554_fn_identity_own_module` (function + declaration, function expression, arrow-in-const, named export, a barrel + re-export, a default export referencing an exported sibling, `Set` + membership, both identity directions) fails on the baseline and matches + Node on the fix; the existing `test_gap_10434`/export/import/cross-module/ + inline/module gap-test families (16 tests) are unaffected. diff --git a/crates/perry-transform/src/inline/cross_module.rs b/crates/perry-transform/src/inline/cross_module.rs index f3c5d865fd..ebb9b4fef9 100644 --- a/crates/perry-transform/src/inline/cross_module.rs +++ b/crates/perry-transform/src/inline/cross_module.rs @@ -163,6 +163,25 @@ pub fn gather_cross_module_functions(module: &Module) -> HashMap_` wrapper symbol, a different + // `js_closure_alloc_singleton` key than the sibling's own canonical + // `__perry_wrap_perry_fn___`, so `x === exportedSibling` + // inside the inlined body silently disagrees with every importer's view + // of `exportedSibling`. `exported_ids` gates `collect_function_graph`'s + // dependency walk below: a graph that would need to bundle a + // separately-exported function is refused entirely (no candidate), + // falling back to the ordinary cross-module call, which shares the + // source module's own canonical wrapper. + let exported_ids: HashSet = module + .exported_functions + .iter() + .map(|(_, id)| *id) + .collect(); + let mut out = HashMap::new(); for (exported_name, root_id) in &module.exported_functions { let mut visiting = HashSet::new(); @@ -174,6 +193,7 @@ pub fn gather_cross_module_functions(module: &Module) -> HashMap, visited: &mut HashSet, out: &mut Vec, + exported_ids: &HashSet, ) -> bool { if visited.contains(&id) { return true; @@ -268,7 +289,15 @@ fn collect_function_graph( refs.sort_unstable(); refs.dedup(); for dependency in refs { - if !collect_function_graph(dependency, functions, visiting, visited, out) { + // #10554: a dependency pulled in only because the body reads it as a + // VALUE (`Expr::FuncRef`) -- not merely calls it -- must not be + // bundled as a private clone when it is ALSO independently exported. + // `dependency != id` lets a function's own export status not block + // its (already-permitted) self-recursion. + if dependency != id && exported_ids.contains(&dependency) { + return false; + } + if !collect_function_graph(dependency, functions, visiting, visited, out, exported_ids) { return false; } if visited.len() > MAX_CROSS_MODULE_FUNCTION_GRAPH { diff --git a/test-files/_helpers/fn_identity_10554/lib.ts b/test-files/_helpers/fn_identity_10554/lib.ts new file mode 100644 index 0000000000..9f5012ed7c --- /dev/null +++ b/test-files/_helpers/fn_identity_10554/lib.ts @@ -0,0 +1,37 @@ +// Shared helpers for #10554: a function referenced inside its own module +// must be the SAME object an importer sees. +export function fnDecl() { + return 1; +} +export const fnExpr = function fnExprNamed() { + return 2; +}; +export const arrowFn = () => 3; + +// Value comparisons made FROM WITHIN this module -- the defect's exact +// shape: `isSame*` is itself exported, so it is a candidate for +// cross-module inlining, and its body's reference to the sibling export is +// a plain in-module reference, not an import. +export function isSameDecl(x: unknown) { + return x === fnDecl; +} +export function isSameExpr(x: unknown) { + return x === fnExpr; +} +export function isSameArrow(x: unknown) { + return x === arrowFn; +} +export function bothSame(a: unknown, b: unknown) { + return a === b; +} +export function makeSet() { + return new Set([fnDecl, fnExpr, arrowFn]); +} + +// A default export that ALSO references an exported sibling by value -- +// #10548 fixed the export-ROW identity for `export default F`; this checks +// the (distinct) cross-module-inliner defect #10554 fixes doesn't resurface +// under a default export. +export default function useDecl(x: unknown) { + return x === fnDecl; +} diff --git a/test-files/_helpers/fn_identity_10554/reexport.ts b/test-files/_helpers/fn_identity_10554/reexport.ts new file mode 100644 index 0000000000..b283403569 --- /dev/null +++ b/test-files/_helpers/fn_identity_10554/reexport.ts @@ -0,0 +1,13 @@ +// Barrel re-export -- a THIRD view of the same bindings, once removed from +// the declaring module. +export { + fnDecl, + fnExpr, + arrowFn, + isSameDecl, + isSameExpr, + isSameArrow, + bothSame, + makeSet, +} from "./lib.ts"; +export { default as useDeclDefault } from "./lib.ts"; diff --git a/test-files/test_gap_10554_fn_identity_own_module.ts b/test-files/test_gap_10554_fn_identity_own_module.ts new file mode 100644 index 0000000000..00c70bc13e --- /dev/null +++ b/test-files/test_gap_10554_fn_identity_own_module.ts @@ -0,0 +1,50 @@ +// #10554: a function referenced inside its own module is a different +// object from the same function imported elsewhere. +import useDecl, { + fnDecl, + fnExpr, + arrowFn, + isSameDecl, + isSameExpr, + isSameArrow, + bothSame, + makeSet, +} from "./_helpers/fn_identity_10554/lib.ts"; +import * as ns from "./_helpers/fn_identity_10554/lib.ts"; +import { + fnDecl as reFnDecl, + isSameDecl as reIsSameDecl, + useDeclDefault, +} from "./_helpers/fn_identity_10554/reexport.ts"; + +// 1. function declaration, function expression, arrow: in-module identity +// checked from a call made through the IMPORTED value. +console.log("decl:", isSameDecl(fnDecl)); +console.log("expr:", isSameExpr(fnExpr)); +console.log("arrow:", isSameArrow(arrowFn)); + +// 2. Both directions: importer's namespace view vs named-import view vs +// in-module (through the exported checker functions). +console.log("ns decl:", isSameDecl(ns.fnDecl), ns.fnDecl === fnDecl, fnDecl === ns.fnDecl); +console.log("ns expr:", isSameExpr(ns.fnExpr), ns.fnExpr === fnExpr); +console.log("ns arrow:", isSameArrow(ns.arrowFn), ns.arrowFn === arrowFn); + +// 3. Re-export (barrel): a third view, once removed. +console.log("reexport decl:", reIsSameDecl(reFnDecl), reFnDecl === fnDecl, isSameDecl(reFnDecl)); + +// 4. default export whose body ALSO references an exported sibling by +// value (distinct from #10434/#10548's export-row identity; exercises the +// cross-module-inliner defect instead). +console.log("default:", useDecl(fnDecl), useDeclDefault(fnDecl), useDecl === useDeclDefault); + +// 5. bothSame -- direct pass-through, no local materialization inside the +// callee (a control: unaffected by this defect, should always have passed). +console.log("bothSame decl:", bothSame(fnDecl, fnDecl), bothSame(fnDecl, ns.fnDecl)); +console.log("bothSame cross:", bothSame(fnDecl, fnExpr)); + +// 6. Set membership -- identity through collection storage/lookup, built +// FROM WITHIN the module (the same defect shape via a different value +// consumer than `===`). +const set = makeSet(); +console.log("set has:", set.has(fnDecl), set.has(fnExpr), set.has(arrowFn)); +console.log("set has via ns:", set.has(ns.fnDecl));