fix(extractors): qualify object literals returned from factory functions - #2263
Conversation
…ons (#2033) extractObjectLiteralFunctions only fired for object literals assigned via a variable declarator (const x = {...}), so calls inside a closure property returned from a function body (return { prop: () => f() }) attributed to the enclosing factory itself rather than the property — misleading call-graph edges since the factory's own body never executes that call. Extend the mechanism to return_statement object literals, qualifying against the nearest enclosing named function (or ClassName.method for methods, or the variable a function/arrow is directly assigned to). Also add a self-typing return-type inference so const p = factory(); p.prop() resolves through the qualified definition, closing the loop with #2032's reachability-based dead code detection. Mirrored in both WASM (shared runCollectorWalk, used by both extraction paths) and native (match_js_node/match_js_type_map/store_return_type). docs check acknowledged: internal extractor/resolver fix, no new commands, languages, or architecture changes — README/CLAUDE.md/ROADMAP unaffected. Impact: 9 functions changed, 14 affected
Greptile SummaryThe PR extends JavaScript factory-return extraction while preventing the previously reported impossible call edges for async and generator wrappers.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Factory["Synchronous factory"] --> Returned["Returned object literal"]
Returned --> Qualified["Qualified property definitions"]
Factory --> SelfType["Factory self return type"]
SelfType --> Variable["Call-result variable type"]
Variable --> Call["Resolved property call edge"]
Async["Async or generator factory"] --> Wrapper["Promise or generator wrapper"]
Wrapper --> Skip["Skip self-type inference"]
Reviews (2): Last reviewed commit: "fix(extractors): skip return-type self-i..." | Re-trigger Greptile |
| const body = fnNode.childForFieldName('body'); | ||
| if (body) { | ||
| const inferred = findReturnNewExprType(body); | ||
| const inferred = findReturnNewExprType(body) ?? findReturnObjectLiteralSelfType(body, fnName); |
There was a problem hiding this comment.
Async and generator return types
When an async or generator factory directly returns an object literal with callable properties, this inference types the immediate call result as the object itself. Calls such as make().run() can then resolve to make.run even though the runtime result is a Promise or generator object, producing impossible call edges and incorrect reachability and dead-code results.
Knowledge Base Used:
There was a problem hiding this comment.
Fixed in b1fbfbd: added isAsyncFunctionNode/isGeneratorFunctionNode guards (mirrored in both TS and native) that skip the return-type self-inference — and the pre-existing return new Ctor() inference, which had the identical flaw — for async/generator factories. The qualified property definitions are still extracted regardless; only the self-typing that would let a caller resolve through the wrapper without await/iteration is skipped. Added regression tests on both engines (async factory, generator factory, and the pre-existing return-new-Constructor case).
Codegraph Impact Analysis11 functions changed → 16 callers affected across 3 files
|
…factories An async or generator function's runtime return value is a Promise/Generator wrapper around the returned expression, not the expression itself. Both the new #2033 self-type inference and the pre-existing `return new Ctor()` inference wrongly typed such factories as their own return shape, which would let `const p = asyncMakeThing(); p.method()` resolve without the required await/iteration. Gate both inferences on isAsyncFunctionNode/ isGeneratorFunctionNode in TS and native; the qualified property definitions themselves are still extracted regardless. docs check acknowledged: internal extractor/resolver fix, no docs impact. Impact: 3 functions changed, 10 affected
Summary
Closes #2033.
extractObjectLiteralFunctions(the mechanism that creates qualifiedvarName.propNamedefinitions so calls inside an object-literal property's closure attribute to the property, not the enclosing scope) only fired for object literals assigned via a variable declarator (const x = {...}). It never fired for object literals appearing in areturnstatement inside a function body — so calls inside those closures fell through to generic caller-attribution, which resolved to the enclosing factory function itself, even though the factory's own body never executes that call.Before:
computeDeltaCPM's caller showed asmakePartition. After:makePartition.deltaCPM.Changes
src/extractors/javascript.ts:findEnclosingFunctionQualifier/qualifierForFunctionScopeNodewalk up to the nearest enclosing function scope and derive its qualifier name (function declaration name,ClassName.method, or the variable a function/arrow is directly assigned to — anonymous non-assigned closures get no qualifier and fall back to prior behavior).handleReturnStmtObjectLiteralwires this intoextractObjectLiteralFunctions+handleObjectLiteralTypeMap, hooked intorunCollectorWalk'sreturn_statementcase — shared by both the walk and query extraction paths.storeReturnType: a function whose body directly returns an object literal with callable properties is typed as itself, soconst p = makePartition(42); p.deltaModularity(1)resolvesp's type and finds the qualified definition — this is what lets roles --role dead: non-transitive fan-in means a function called only by another dead function is never flagged dead #2032's reachability-based dead-code detection close the loop end-to-end (only the unused property becomes dead, not a used sibling property).crates/codegraph-core/src/extractors/javascript.rs: mirrored extraction (handle_return_stmt,handle_return_stmt_type_map,find_enclosing_function_qualifier,find_return_object_literal_self_type). Also fixed a same-file gap in Rust's Phase 8.2 inter-procedural return-type propagation (handle_var_declarator_type_map's call_expression branch only handled cross-file/imported callees; added the same-file identifier lookup TS already had) and reorderedJsExtractor::extract's walks soreturn_type_mapis fully populated beforematch_js_type_mapreads it — both engines now produce identical graphs for this shape.Verification
computeDeltaCPM's caller is nowmakePartition.deltaCPM(notmakePartition);p.deltaModularity(1)resolves tomakePartition.deltaModularity.codegraph roles --role deadnow correctly flagscomputeDeltaCPMas dead (its only caller,makePartition.deltaCPM, is itself unreachable) whilecomputeDeltaModularityis correctly NOT flagged (reachable viauseIt).npm test(4412 passed, 30 skipped, 2 todo),npm run lint,cargo test --lib/cargo test --release(742 passed),node scripts/parity-compare.mjs(34 languages) — no new divergences.tests/parsers/javascript.test.ts, a query/walk parity case intests/engines/query-walk-parity.test.ts, and an end-to-end WASM+native integration test intests/integration/issue-2033-factory-returned-closure-attribution.test.ts.Follow-ups filed (out of scope, discovered during validation)
codegraph buildnever setsisFullBuild: true(an emptyfile_hashestable is treated as incremental), so roles --role dead: non-transitive fan-in means a function called only by another dead function is never flagged dead #2032's reachability downgrade never runs until an explicit--no-incrementalrebuild.jelly-microfixture's cross-filesuper-dispatchfallback (confirmed present on unmodifiedorigin/main, unrelated to this change).Test plan
npm testnpm run lintcargo test --lib/cargo test --releasenode scripts/parity-compare.mjscodegraph diff-impact --staged -T