fix(js): emit one node per exported destructured name (#2604) - #3552
ayushcodes10 wants to merge 6 commits into
Conversation
export const { auth, handlers, signIn, signOut } = NextAuth(config)
is NextAuth version five's own documented boilerplate, and
createNavigation from next intl follows the exact same shape. The
extractor collapsed the whole pattern into one combined node, id
joining every destructured name and label the literal pattern
syntax, so a single name import elsewhere could never find a node
named just auth and the edge, and any call resolved through it,
dangled. Each shorthand and renamed property in the pattern now
becomes its own node sharing the statement's line, exactly what the
export would have produced had it been written as N separate
statements. A renamed property exports under its key, since that is
what an importer binds by, never its local alias. A rest element or
a default valued entry does not correspond to one discrete exported
name and is skipped rather than guessed at.
The split only applies to an actually exported statement. An
unexported destructure of the identical shape, const doWork equals
require of lib being the common one, is a local import binding, not
a module export, and giving it a bare named node in the importing
file would collide with the real definition it only imports, turning
a clean resolution into a false ambiguous name. Caught this against
the existing suite before it ever reached a commit. Fixes Graphify-Labs#2604's
class 2.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Four destructured names on one export statement must each get their own node, and the old combined pattern syntax label must not survive anywhere in the output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
A single name import must resolve to its own split node, and a call made through it must resolve too, not just the import edge on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
A renamed destructured property must export under its property key, never under the local alias it is bound to, since that alias is not what an importer would ever bind by. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
A rest element does not correspond to one discrete exported name, so it must be skipped rather than mangled into a bogus node, and its sibling names must still extract cleanly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Pins the exact interaction the fix commit already describes: a CommonJS const equals require of a module destructure must still resolve to exactly one EXTRACTED calls edge, not zero, confirming the export only gate keeps it out of the per name split entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
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 5 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Emits one node per exported name for an exported object-destructure like export const { auth, handlers } = NextAuth(config) in _js_extra_walk, each on the statement's line, so a single-name import (import { auth }) resolves to its own node instead of dangling against the old combined _auth_handlers node. Renamed properties (handlers: h) export under their key, while rest/default-valued patterns are skipped rather than mangled. Gates the split on is_exported so an un-exported const { doWork } = require(...) stays a local import binding and doesn't collide with the real definition it imports.
Worth a look
- const_found may be undefined in new object_pattern branch —
graphify/extractors/engine.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Aliased destructuring exports the local binding, not the object key —
graphify/extractors/engine.py:2599· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Default-valued destructuring export is skipped entirely —
graphify/extractors/engine.py:2601· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Aliased destructuring exports the property key instead of the declared binding —
graphify/extractors/engine.py:2602· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Default and rest destructuring bindings are skipped as exports —
graphify/extractors/engine.py:2606· 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 — 667 functions depend on the 223 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_extract_generic()— 18 callers, 26 callees - new:
extract_js()— 85 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, 59 callees - …and 8 more — each is listed as a finding
Verification — 667 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: 607 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
23 of 276 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_destructured_export.py— impact, changed-testtests/test_js_exported_scalar_bindings.py— impacttests/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 \_js\_extra\_walk.
The verifier did not have enough to check \_js\_extra\_walk, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
· 16 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.62 (now on PyPI: |
Summary
Fixes #2604's "Class 2" (52/1657, 3%, of the reported dangling-endpoint edges — the smaller, genuinely-fixable half; the larger "Class 1" is a health-check reporting-classification request, not a bug, and is left out of scope here).
export const { auth, handlers, signIn, signOut } = NextAuth(config)is NextAuth v5's own documented boilerplate, and next-intl'screateNavigation()follows the exact same shape. The extractor collapsed the whole destructuring pattern into one combined node — id joining every destructured name (stem_auth_handlers_signin_signout), label the literal pattern syntax ("{ auth, handlers, signIn, signOut }") — so a single-name import elsewhere (import { auth } from './auth') could never find a node named justauth, and the import edge (plus any call resolved through it) dangled.Each shorthand and renamed property in the pattern now emits its own node, sharing the statement's line — exactly what the export would have produced had it been written as N separate
export const auth = ...statements. A renamed property (handlers: h) exports under its key (handlers), since that's what an importer binds by, never the local alias. A rest element (...rest) or a default-valued entry doesn't correspond to one discrete exported name and is skipped rather than guessed at.Caught and fixed a real regression during testing, not after: the split is gated to only fire on an actually-exported statement. An un-exported destructure of the identical shape —
const { doWork } = require('./lib'), the ordinary CommonJS import pattern — is a local import binding, not a module export. My first pass didn't have this gate, and running the existing test suite immediately caught it breakingtest_cross_file_call_promoted_to_extracted_with_import_evidence: giving the import binding a bare-named node in the importing file made it collide with the real definition it merely imports, turning a clean single-candidate cross-file resolution into a false "ambiguous name" and silently losing the realcallsedge. Fixed before ever landing a broken commit — see the commit history for how this played out.Test plan
tests/test_js_destructured_export.py, 5 tests: the core per-name split; cross-file import + call resolution through a split node; renamed-property key-not-alias naming; rest-pattern skip without breaking siblings; and a dedicated regression guard reproducing the exact require()-collision interaction the fix had to be gated against.test_extract.pysuite (416 tests) — all pass, no regressions, including the specific test my first ungated attempt broke.python3 -m pytest -q— 5491 passed, only the pre-existing unrelated failures (test_ollama_retry_cap.pymissingopenaiin this env, one flaky timing assertion intest_ts_import_type_arguments.py).python3 -m tools.skillgen --check— OK.🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh