Skip to content

fix(js): extract methods on exported objects to resolve CommonJS calls (#3778) - #4121

Closed
harshaygadekar wants to merge 1 commit into
Graphify-Labs:v8from
harshaygadekar:fix/js-commonjs-call-edges-3778
Closed

harshaygadekar wants to merge 1 commit into
Graphify-Labs:v8from
harshaygadekar:fix/js-commonjs-call-edges-3778

Conversation

@harshaygadekar

Copy link
Copy Markdown
Contributor

Summary

Fixes #3778.

In Express-style CommonJS architectures (and similar Node.js packages), core methods are assigned to module-level objects that are subsequently exported:

// response.js
var normalizeType = require('./utils').normalizeType;
var res = Object.create(http.ServerResponse.prototype);

res.format = function(obj) {
    this.set('Content-Type', normalizeType(key).value);
};

module.exports = res;

Previously, _js_extra_walk only captured exports.x = fn and <Class>.prototype.x = fn. Top-level member assignments on objects (res.format = fn) were skipped to prevent phantom god-nodes (#1077). Consequently:

  1. res.format never received a node.
  2. Its AST body was never appended to function_bodies.
  3. walk_calls never walked the method body.
  4. Calls made inside those methods (such as normalizeType(...)) were never extracted, resulting in 0 resolved call edges despite valid destructured require statements.

Fix

  1. Detect Exported Objects: Added _js_find_exported_objects(program_node, source) to identify top-level identifiers exported via CommonJS or ESM (module.exports = res, exports = module.exports = ..., exports.response = res, module.exports = { res }, etc.).
  2. Method Extraction on Exported Objects: Handled kind == "object" in _js_extra_walk when owner_name is exported. We emit the owner node, method node .{member}(), method edge, and append the method body to function_bodies so walk_calls scans all call expressions inside.
  3. Preserve Guard Extraction noise: scope-blind identifier collision + over-eager markdown fragment nodes #1077: Arbitrary local objects that are not exported (const obj = {}; obj.whatever = () => 1;) continue to be skipped, keeping the phantom god-node guard intact.

Verification

  • Reproduction: Tested with minimal reproduction script mirroring Express response.js calling utils.normalizeType. Confirmed method node and cross-file calls edge are emitted.
  • Test Suite:
    • Added unit test test_extract_js_exported_object_member_assignment_and_calls in tests/test_extract.py.
    • All 54 JavaScript extract tests pass cleanly (uv run pytest tests/test_extract.py tests/test_languages.py -k "js").
    • Phantom god-node guard test test_extract_js_arbitrary_member_assignment_not_captured passes.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

Thanks for the pull request, @harshaygadekar. A maintainer will review it soon.

Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions.

A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.


Graphify review — findings

Captures function assignments onto members of exported JS objects (e.g. res.format = function… after module.exports = res) as methods, so the owner object gets a node with a contains edge and calls inside the body resolve. _js_find_exported_objects decides what counts as exported by scanning top-level ESM export statements and CommonJS module.exports/exports assignments, including chained assignments and object literals. Assignments to non-exported or non-top-level objects stay uncaptured, as before.

Worth a look

  • ESM exported declarations are not detected — graphify/extractors/engine.py:2759 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1238 functions depend on the 754 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _extract_generic() — 18 callers, 31 callees
  • new: extract_js() — 87 callers, 4 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: extract_objc() — 27 callers, 9 callees
  • new: extract_julia() — 19 callers, 7 callees
  • new: extract_svelte() — 14 callers, 7 callees
  • new: extract_cpp() — 32 callers, 3 callees
  • new: extract_vue() — 10 callers, 7 callees
  • …and 10 more — each is listed as a finding

Verification — 1238 functions in the blast radius were not formally verified this run (proofs are advisory here).

Health delta baseline: last indexed commit 35adf43, 1 commit(s) behind this PR's base.

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 1147 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

29 of 329 test file(s) selected (9%) via static blast radius.

  • tests/test_astro_extraction.py — impact
  • tests/test_build.py — impact
  • tests/test_cjs_module_extension.py — impact
  • tests/test_cpp_nested_and_cli.py — impact
  • tests/test_dotnet.py — impact
  • tests/test_extract.py — impact, changed-test
  • tests/test_extract_php_closures.py — impact
  • tests/test_import_extension_resolution.py — impact
  • tests/test_indirect_call_block_scoped_shadow.py — impact
  • tests/test_indirect_dispatch.py — impact
  • tests/test_indirect_dispatch_assign_return.py — impact
  • tests/test_indirect_dispatch_getattr.py — impact
  • tests/test_js_exported_scalar_bindings.py — impact
  • tests/test_languages.py — impact
  • tests/test_multilang.py — impact
  • tests/test_python_underscore_resolution.py — impact
  • tests/test_rationale.py — impact
  • tests/test_ruby_resolution.py — impact
  • tests/test_scala_context_bounds.py — impact
  • tests/test_scala_self_type.py — impact
  • tests/test_scala_top_level_binding.py — impact
  • tests/test_scala_type_definition.py — impact
  • tests/test_svelte_extraction.py — impact
  • tests/test_swift_computed_properties.py — impact
  • tests/test_swift_protocol_requirements.py — impact
  • tests/test_trailing_newline_not_a_syntax_error.py — impact
  • tests/test_ts_new_expression_calls.py — impact
  • tests/test_typescript_module_extensions.py — impact
  • tests/test_vue_extraction.py — impact

Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.

· 1 grounded finding(s) anchored inline below; 17 more finding(s) on lines outside this diff (see the check run).

Comment thread graphify/extractors/engine.py
@harshaygadekar
harshaygadekar force-pushed the fix/js-commonjs-call-edges-3778 branch from 693fe98 to 65f2da5 Compare October 5, 2026 17:23
@safishamsi

Copy link
Copy Markdown
Member

Landed in v0.9.77 via an authorship-preserving cherry-pick, so your commit is on v8 with you credited as the author. Closing as shipped — thanks @harshaygadekar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JavaScript: calls through a CommonJS destructured require are never resolved (express: 63 call edges from 11,733 call sites)

2 participants