Skip to content

fix(extraction): index const-bound functions inside a body as symbols - #1679

Open
danusha2345 wants to merge 1 commit into
colbymchenry:mainfrom
danusha2345:fix/1669-declarator-bound-nested-functions
Open

fix(extraction): index const-bound functions inside a body as symbols#1679
danusha2345 wants to merge 1 commit into
colbymchenry:mainfrom
danusha2345:fix/1669-declarator-bound-nested-functions

Conversation

@danusha2345

Copy link
Copy Markdown
Contributor

Fixes #1669.

Problem

const handleClear = () => {…} inside a component — every React handler written without useCallback — was never a symbol. The body walker names nested function declarations and hook-bound arrows (const x = useCallback(…)), but a plain declarator-bound arrow or function expression fell through, so the handler was absent from callers / callees / impact ("Symbol not found", which reads exactly like "no callers") and its calls attributed to the component. The same declaration at module scope already names a function.

Change

  • Body walker (wasm) and kernel: an anonymous arrow_function / function_expression that is the whole value of a variable_declarator with a plain identifier name is extracted as a function, contained by the enclosing one, with its own calls. extractFunction already resolves the name from the declarator. A destructuring binding, an inline JSX arrow and a non-function value stay as they were. JS family only.
  • torture.tsx gains the shape so kernel-tsjs-parity pins both arms.

Graph shape

A navigation such a handler makes is now the handler's own navigates edge, and the handler is a hop in the Screens via chain — the shape a useCallback handler already has (see the Next.js test's navs(sym('handleSubmit')) and push.via). The react-router and expo-router expectations are updated to that convention; the screen still reaches the handler through its onSubmit={…} reference and the links resolve as before, with the trigger now visible in Steps.

Verification

  • New __tests__/nested-declarator-functions.test.ts: arrow, function expression and let-bound arrow become Widget::name functions with their own calls and containment; a value, a destructuring and an inline arrow do not; Python is untouched.
  • npm run build:kernel && npm run build, full suite with CODEGRAPH_KERNEL_EXPECT=1: 237 files, 4233 passed, 9 skipped.

Re-index after upgrading to pick up the new symbols.

🤖 Generated with Claude Code

…colbymchenry#1669)

`const handleClear = () => {…}` inside a component — every React handler
that skips useCallback — was never a symbol: the body walker only named
nested function declarations and hook-bound arrows, so the handler was
absent from callers/impact ("Symbol not found", indistinguishable from
"no callers") and its calls attributed to the component. Bind the arrow
or function expression to its declarator the way module scope already
does, in both the wasm walker and the kernel.

A navigation such a handler makes is now the handler's own edge and a hop
in the Screens `via` chain — the shape a useCallback handler already has —
so the react-router and expo-router expectations follow that convention.
@bompus

bompus commented Sep 5, 2026

Copy link
Copy Markdown

Verified on a real TypeScript repo (Chrome MV3 extension, 582 files, TS/JS/Vue/markdown, Windows 11, tree-sitter wasm walker, kernel off). Branch: this PR merged onto current main (b9ca4b7) plus our fork's markdown/literal extras; control build indexed the same tree without the PR.

Merges clean. The six closures we had listed as missing (findAndAssign, assignFirstMatch, finish, collect, clear, tick) all appear as function nodes under their parent (+154 nodes, +699 edges). PR test files: 100/100.

Two things the new nodes attract that were not there before:

  1. Fuzzy call edges from unrelated files onto generically named closures: total resolvedBy: fuzzy edges 89 → 226. The new ones concentrate on readSeedFailureState::text (72 edges), buildRows::push (41), captureListeners::event (10). Every .text(...) or .push(...) call in the repo now has a fuzzy candidate. Worth scoping closure nodes out of the fuzzy resolver, or restricting them to same-file callers.
  2. Self-call edges 31 → 42. The eleven new ones are all closures calling themselves (walk, walkBag, tick, wrapElement), which is real recursion, so no action needed there.

@danusha2345

Copy link
Copy Markdown
Contributor Author

Thanks — the fuzzy finding is real and #1709 is the right fix for it, so I took #1709 into my integration build ahead of this one as you suggested. Measured on two real repos with both PRs (and #1511) in, kernel on, same trees re-indexed before/after #1709: resolvedBy: fuzzy call edges 337 → 2 on this repo's own tree (787 files) and 142 → 0 on a Kotlin+Go+TS app (274 files); the new closure nodes stay, nothing else moves, full suite 253 files / 4351 passed. The self-call edges you list are genuine recursion here too. I would keep this PR as is and let #1709 land first.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown

@danusha2345 before you sequence this behind #1709 — I ran your metric on a third repo and it moves the other way there.

Details and full tables on #1709: #1709 (comment)

Short version. Same vite checkout indexed at #1709's merge-base and at its tip, counting every edge by resolvedBy:

  • fuzzy: 13 → 60 (+47)
  • every other resolver: 0 delta

Cross-tabbed against the edge-set diff: 12 edges lost (all fuzzy, all correct removals of unreachable nested functions) and 59 gained (all fuzzy — 52 calls, 7 imports), nearly all pointing node:path's resolve, imported on line 1 of a vite playground config, at pluginContainer.ts:resolve.

I had assumed your metric simply couldn't see this failure mode. It can — that's why I measured before replying. On your two trees the filter only removes bad guesses (337 → 2, 142 → 0); on vite it leaves exactly one surviving candidate and the matcher commits to it. vite has ~12 symbols named resolve, most nested, which is the shape that triggers it.

So your measurement stands for your trees and #1709's removal half is doing real work. My concern is only with landing it first: the underlying bug is one layer up — when filtering leaves a single survivor, we commit without checking the call site's binding is internal — and until that's handled, #1709 is not a strict improvement on every tree.

Nothing here affects this PR's own change. I'd just rather the ordering decision be made with the vite number in view than have it re-raised as a blocker on you later.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown

Closing the loop I opened here. I raised the vite counter-example as a reason not to sequence this behind #1709; the underlying bug is now fixed in #1713, and I no longer think the sequencing concern should hold anything up.

#1713 is standalone — one commit off main, not stacked on #1709 — and on vite it removes 4 wrong edges and adds none, with every other resolver at 0 delta. With it in place, #1709 + #1713 is +3 fuzzy on vite rather than the +47 I reported for #1709 alone.

So the objection I raised against ordering is spent. Nothing here needs to wait on me.

Full tables and the two failure modes that remain: #1709 and #1713.

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.

Function-valued consts inside a function body are never indexed (React handlers invisible to callers/impact)

2 participants