fix(js): capture function-valued object properties like shorthand methods - #2947
rajanpanth wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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 literals —
graphify/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 skipped —
graphify/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).
d14928b to
50cc537
Compare
|
Both advisories addressed. Parenthesized values: real gap, fixed in 50cc537. Const-bound object literals: no output change there, and the test suite locks it. |
There was a problem hiding this comment.
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 literals —
graphify/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.
50cc537 to
7ecd140
Compare
|
Rebased onto current The conflict was worth a note: Verification after the rebase:
Diff is unchanged in substance from the reviewed version: |
There was a problem hiding this comment.
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 emitted —
graphify/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— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_import_extension_resolution.py— impacttests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_js_object_property_functions.py— impact, changed-testtests/test_languages.py— impacttests/test_multilang.py— impacttests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_self_type.py— impacttests/test_swift_computed_properties.py— impacttests/test_trailing_newline_not_a_syntax_error.py— impacttests/test_ts_new_expression_calls.py— impacttests/test_typescript_module_extensions.py— impacttests/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).
|
On the advisory finding, "Named function-expression property symbols are no longer emitted" at I ran the same six shapes through
No row loses a symbol. The first four gain one, and the last two are unchanged. The reason the finding does not apply: on 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 On which name gets used: the new branch emits the property key, so Happy to add the named-function-expression case to the test file as an explicit guard if you would like that pinned down. |
Two spellings of the same exported API produce different graphs:
The shorthand parses as a
method_definition, which the generic function branch captures. The arrow and function-expression forms parse aspairnodes, 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
pairwhose 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, samecallable_def_nidsandlocal_bound_namesregistration, samefunction_bodiestracking 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:
module.exports = { m }m()m()register({ m })m()m()const api = { m }apiapiapi(unchanged)Verification
constscoping baseline, computed/string keys still skipped). With the engine change reverted, 6 of 7 fail.callsedges from inside an arrow property resolve:module.exports = { run: () => helper() }emitsrun() -> helper().