Skip to content

fix(js): capture function-valued object properties like shorthand methods - #2947

Open
rajanpanth wants to merge 2 commits into
Graphify-Labs:v8from
rajanpanth:fix/object-property-function-values
Open

rajanpanth wants to merge 2 commits into
Graphify-Labs:v8from
rajanpanth:fix/object-property-function-values

Conversation

@rajanpanth

Copy link
Copy Markdown
Contributor

Two spellings of the same exported API produce different graphs:

module.exports = { m(){ return 1; } };        // m() captured
module.exports = { m: () => 1 };              // nothing
module.exports = { m: function(){ return 1; } }; // nothing

The shorthand parses as a method_definition, which the generic function branch captures. The arrow and function-expression forms parse as pair nodes, which nothing handled, so those symbols vanished. Same class of gap as the factory object API fix (#2745): common style, silently missing surface. { handler: async () => {...} } route tables and options objects are the everyday casualties.

Fix

A pair whose value is a function type is intercepted at the same walk position as the function branch and mirrors it exactly: same naming, same class-vs-file scoping, same callable_def_nids and local_bound_names registration, same function_bodies tracking so calls made inside the property resolve. Only plain identifier keys are named; computed and string keys stay out of the graph as before.

Because it sits in the same walk with the same guards, the scoping envelope is byte-for-byte the shorthand envelope:

Case Shorthand today Arrow before Arrow after
module.exports = { m } m() nothing m()
register({ m }) m() nothing m()
const api = { m } only api only api only api (unchanged)

Verification

  • 7 regression tests added (parity for all spellings, call-argument objects, body-call resolution, the unchanged const scoping baseline, computed/string keys still skipped). With the engine change reverted, 6 of 7 fail.
  • calls edges from inside an arrow property resolve: module.exports = { run: () => helper() } emits run() -> helper().
  • Full suite: 4650 passed. The failures on my machine (127) are identical with and without this change, all environmental Windows CLI test issues, none in extraction.

@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 2 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Captures function-valued object-literal properties ({ m: () => {} } and { m: function(){} }) in the JS/TS _extract_generic walk, mirroring the existing method_definition shorthand branch — same node/edge shape, scoping, and body tracking so calls inside the property resolve. Only plain property_identifier keys are handled; computed and string keys remain skipped. Adds tests/test_js_object_property_functions.py covering spelling parity, body-call resolution, and the const-object scoping baseline.

Worth a look

  • New pair-based method branch changes graph output for const-bound object literalsgraphify/extractors/engine.py:4038 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Parenthesized function-valued object properties are skippedgraphify/extractors/engine.py:4044 · 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 — 623 functions depend on the 212 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _extract_generic() — 18 callers, 24 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: extract_js() — 82 callers, 3 callees
  • new: extract_objc() — 27 callers, 9 callees
  • new: extract_julia() — 16 callers, 7 callees
  • new: extract_cpp() — 27 callers, 3 callees
  • new: extract_vue() — 10 callers, 6 callees
  • new: walk() — 1 callers, 56 callees
  • …and 8 more — each is listed as a finding

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

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: 563 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 16 more finding(s) on lines outside this diff (see the check run).

@rajanpanth
rajanpanth force-pushed the fix/object-property-function-values branch from d14928b to 50cc537 Compare August 22, 2026 08:29
@rajanpanth

Copy link
Copy Markdown
Contributor Author

Both advisories addressed.

Parenthesized values: real gap, fixed in 50cc537. { m: (() => 1) } wraps the function in parenthesized_expression nodes, which the type check skipped. The branch now unwraps those layers first, so single and doubled parens are captured, while a parenthesized non-function value ({ m: (fn()) }) stays out of the graph. Test added for both sides. 8 passing.

Const-bound object literals: no output change there, and the test suite locks it. const api = { m: () => 1 } emits only the api node, matching the shorthand baseline exactly, because lexical declarations are handled before the generic walk descends. The pair branch only fires where method_definition already fired, which is the parity this PR is about. test_const_object_scoping_baseline_unchanged asserts shorthand and arrow spellings produce identical labels for the const case.

@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.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Adds a pair-node branch to _extract_generic in graphify/extractors/engine.py so JS/TS function-valued object properties ({ m: () => {} }, { m: function(){} }, including parenthesized forms) emit the same node/edge shape and body tracking as shorthand method_definition members. Restricts capture to plain identifier keys with the existing #1899 normalize guard, leaving computed/string keys and const-object scoping unchanged. Adds tests/test_js_object_property_functions.py covering the three spellings, mixed objects, call-argument parity, in-body call resolution, and the skip/scoping baselines.

Worth a look

  • Pair-function branch ignores parent scoping context, emitting members for nested/const object literalsgraphify/extractors/engine.py:4059 · Escalate · high
    • 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 — 624 functions depend on the 213 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _extract_generic() — 18 callers, 24 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: extract_js() — 82 callers, 3 callees
  • new: extract_objc() — 27 callers, 9 callees
  • new: extract_julia() — 16 callers, 7 callees
  • new: extract_cpp() — 27 callers, 3 callees
  • new: extract_vue() — 10 callers, 6 callees
  • new: walk() — 1 callers, 56 callees
  • …and 8 more — each is listed as a finding

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

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: 564 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 16 more finding(s) on lines outside this diff (see the check run).

…hods

The shorthand { m(){} } is a method_definition and lands in the generic
function branch, but { m: () => {} } and { m: function(){} } parse as
pair nodes and were skipped, so the arrow and function-expression
spellings of the same API surface vanished from the graph. The pair
branch mirrors the function branch exactly: same walk position, same
scoping, same body tracking, so scoping baselines are unchanged.
Review advisory: { m: (() => {}) } wraps the function in a
parenthesized_expression, which the pair branch skipped.
@rajanpanth
rajanpanth force-pushed the fix/object-property-function-values branch from 50cc537 to 7ecd140 Compare September 18, 2026 08:30
@rajanpanth

Copy link
Copy Markdown
Contributor Author

Rebased onto current v8 to clear the conflict.

The conflict was worth a note: v8 has since added a Ruby singleton_class branch at the same position in walk() where this PR adds the JS pair branch. They are independent guards, so I kept both, Ruby first and then the JS one, rather than letting either win.

Verification after the rebase:

  • tests/test_js_object_property_functions.py: 8 passed.
  • The Ruby work that conflicted still passes: pytest -k "ruby or singleton" gives 89 passed, 4 skipped, so keeping both branches did not disturb the new singleton handling.
  • Mutation check: with graphify/extractors/engine.py reverted to v8 and the tests kept, 6 of the 8 fail, including test_mixed_spellings_all_captured, test_parenthesized_function_values_captured and test_property_body_calls_resolve. Restored, back to 8 passing.

Diff is unchanged in substance from the reviewed version: +41 in engine.py and the new test file.

@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.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Captures function-valued object-literal properties in the JS/TS extractor so { m: () => {} } and { m: function(){} } now emit the same method/contains node, body tracking, and local scoping as the shorthand { m(){} } did. Unwraps parenthesized_expression wrappers around the value, but only for plain property_identifier keys that survive normalize_id — computed keys, string keys, and non-function values stay out of the graph as before.

Worth a look

  • Named function-expression property symbols are no longer emittedgraphify/extractors/engine.py:4901 · 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 — 684 functions depend on the 240 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _extract_generic() — 18 callers, 29 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() — 17 callers, 7 callees
  • new: extract_cpp() — 29 callers, 3 callees
  • new: extract_vue() — 10 callers, 7 callees
  • new: walk() — 1 callers, 62 callees
  • …and 9 more — each is listed as a finding

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

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: 624 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

23 of 287 test file(s) selected (8%) 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
  • tests/test_import_extension_resolution.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_js_object_property_functions.py — impact, changed-test
  • 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_self_type.py — impact
  • tests/test_swift_computed_properties.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.

Formal verification

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: no capturable inputs from the test suite; property tier: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 17 more finding(s) on lines outside this diff (see the check run).

@rajanpanth

Copy link
Copy Markdown
Contributor Author

On the advisory finding, "Named function-expression property symbols are no longer emitted" at engine.py:4901: I checked it and I do not think it holds. The review flagged it as agreed but not verified, so here is the verification.

I ran the same six shapes through extract_js on v8 and on this branch:

source v8 this PR
module.exports = { named: function namedInner(){} } [] ['named()']
module.exports = { anon: function(){} } [] ['anon()']
module.exports = { outer: { named: function namedInner(){} } } [] ['named()']
module.exports = { named: function namedInner(){ return helper(); } } ['helper()'] ['helper()', 'named()']
const api = { named: function namedInner(){} } ['api'] ['api']
const f = function namedInner(){} ['f()'] ['f()']

No row loses a symbol. The first four gain one, and the last two are unchanged.

The reason the finding does not apply: on v8 a named function-expression property emitted nothing at all, so namedInner was not being dropped by this change, it was never there. The walk did not reach into pair values, which is the bug this PR fixes.

The last row is the one that matters for the concern behind the finding. A named function expression outside an object literal is not a pair, so it never enters the new branch and still emits f() exactly as before.

On which name gets used: the new branch emits the property key, so { named: function namedInner(){} } becomes named() rather than namedInner(). That is deliberate and it is the point of the PR. named is what callers see on the API surface, namedInner is only bindable inside the function body for self-reference, and the shorthand spelling { named(){} } has always emitted named(). Emitting the inner name instead would break the parity these tests exist to lock.

Happy to add the named-function-expression case to the test file as an explicit guard if you would like that pinned down.

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.

1 participant