Skip to content

fix(extraction): index TypeScript interface members (#1638) - #1686

Open
maxmilian wants to merge 1 commit into
colbymchenry:mainfrom
maxmilian:fix/1638-ts-interface-members
Open

fix(extraction): index TypeScript interface members (#1638)#1686
maxmilian wants to merge 1 commit into
colbymchenry:mainfrom
maxmilian:fix/1638-ts-interface-members

Conversation

@maxmilian

Copy link
Copy Markdown
Contributor

Closes #1638.

tree-sitter-typescript spells interface members with their own node types — method_signature and property_signature — distinct from the class-member types the TS extractor listed, so an interface's members never entered the graph. Java and C# were never affected: their grammars reuse method_declaration, which is already in their methodTypes.

The cost lands on any codebase whose platform API is a .d.ts interface. With no declaration node for a member, every call site through that API has nothing to attach an edge to.

The fix is two lines in typescript.ts. The walker already treats an interface as a class-like parent (isInsideClassLikeNode lists 'interface'), so members attach with no traversal change. Precedent: extractTsTypeAliasMembers already makes type X = { foo(): T } members first-class (#359) — interfaces were the inconsistent gap.

Three consequences, each handled and tested:

  1. A bodiless signature must not take extractMethod's "no class-like parent → free function" fallback. Outside an interface such a node appears only in a type literal, whose members TypeScript type alias members not used in method-call resolution → false cross-module calls edges via path-proximity #359 already extracts — without a guard, type Handle = { stop(): void } gains a phantom top-level function stop.
  2. The property_signature / method_signature branch that hung type annotations off the enclosing interface is now unreachable (the new branches carry the same guard and claim those types first) and is removed. The references edges survive and now anchor on the member, so Api::fetch → PageId says which member wants the type where Api → PageId only said the file did.
  3. CG-28 — see below.

CG-28: a proposal, not an assertion

getAmbientDeclarationPathsAmong reads "every declared symbol is type-level", which a pure-interface .d.ts stops satisfying the moment its members are indexed.

Condition 4 turned out to be the dangerous one, not condition 2. Condition 2 breaks loudly. Condition 4 — "nothing else in the index points at it" — breaks silently and backwards: once members exist, call sites through a platform API finally have a signature to land on, so an ambient shim loses its damping precisely because the API it declares is widely used.

This PR makes an interface-owned member transparent to conditions 2, 3 and 4 — the treatment parameter already gets ("structural bookkeeping, neither qualifies nor disqualifies"), via one shared IS_INTERFACE_MEMBER(alias) fragment seeking idx_edges_target_kind. The reasoning is that this reproduces the graph the rule was measured on: before this change those nodes did not exist. The interface itself is untouched, so a depended-on types.ts still fails condition 4 and stays out of the flag.

I cannot verify that from outside. The 0–4% flag rate is a corpus measurement, and whether folding members in keeps it there needs a run against the corpus the comment cites — Kotlin sealed classes, Rust mod.rs re-exports, django locale tables. Please treat this half as a proposal and re-measure.

One test still fails, and I would rather flag it than paper over it

__tests__/explore-declaration-only.test.ts > the gate — a prose flow query > does not let it outrank the implementation files.

Detection is correct: the shim is still flagged ambient and still damped at 0.5. Magnitude is not. The fixture's shim holds 143 nodes where it held 29, and the extra bodiless names flood FTS, shifting the RWR restart vector — implementation files' graph mass halves (0.307 → 0.137) while the shim's holds steady, and it takes rank 1.

I fixed what I could argue from the code's own stated intent, both in tools.ts:

  • RELEVANCE_KIND_WEIGHT rates method at 1.0 as "a callable", and its own doc puts "a member of a type" at ~0.5. A method_signature is the latter. This restores the raw score almost exactly (53 → 52.5) and cuts the rendered envelope from 6044 to 1933 characters.
  • The named-seed tier means "the agent asked for the symbol defined here". Prose words like body, stream and metadata are member names in any platform .d.ts and were reaching that tier through coNamedInFile, sorting above the CG-28 penalty. Interface signatures now seed (RWR unchanged) but never tier.

Closing the rest means either retuning AMBIENT_DECLARATION_RANK_PENALTY — documented as "softer than GENERATED on purpose" — or changing what FTS returns. Both are calibration decisions that belong to whoever holds the corpus, so I stopped there rather than adjusting the assertion. Happy to take direction.

Tests

Baseline on b9ca4b7: 3950 pass / 84 fail (all 84 are CLI/MCP/spawn suites needing a built dist/, none touching extraction or queries). After: 3952 / 85 — failure-list diff is exactly the one entry above, with none disappearing.

Added:

  • interface members enter the graph (the issue's repro), and attach to the interface by a contains edge rather than merely existing;
  • no phantom top-level function from a type-literal signature;
  • a guardrail that a pure-interface .d.ts is still CG-28-damped, pinned from both ends — it asserts members are indexed as well as the flag, so it cannot pass vacuously on an index where extraction silently reverted.

Adjusted, in each case re-keyed on source rather than name:

  • extraction.test.ts [TypeScript] String literal type arguments in generic tuple elements are not indexed as symbols #634 excluded nodes reached by a contains edge from an interface; a node minted from Pick<User,'id'> or a tuple has no declaring interface, so the guard is intact.
  • object-literal-methods.test.ts — its Zustand fixture declares both interface Store { fetchUser(): … } and an action of that name, so find(n => n.name === 'fetchUser') started hitting the signature.
  • explore-declaration-only.test.ts's two fixture-shape assertions describe extraction output and must track it; they now accept interface-owned members via an isTypeLevel helper. A function or class creeping into that fixture still fails, and the gate assertions themselves are untouched.

tree-sitter-typescript spells interface members with their own node types,
`method_signature` and `property_signature`, distinct from the class-member
types the TS extractor listed — so an interface's members never entered the
graph. Java and C# were never affected: their grammars reuse
`method_declaration` for interface methods, which was already in methodTypes.

The cost lands on any codebase whose platform API is a `.d.ts` interface.
With no declaration node for a member, every call site through that API has
nothing to attach an edge to, so the calls are invisible to callers/impact.

Adds `method_signature` to methodTypes and `property_signature` as the TS
`propertyTypes`. The walker already treats an interface as a class-like
parent, so members attach to their interface with no traversal change.

Three consequences handled here:

- A bodiless signature must not take extractMethod's "no class-like parent,
  so treat it as a free function" fallback. Outside an interface it appears
  only in a type literal, whose members extractTypeAlias already extracts
  (colbymchenry#359) — without the guard `type Handle = { stop(): void }` gains a phantom
  top-level `function stop` beside the real `Handle::stop`.

- The `property_signature`/`method_signature` branch that hung type
  annotations off the enclosing interface is now unreachable and removed. The
  `references` edges survive via extractMethod/extractProperty and now hang
  off the member, a more precise anchor.

- CG-28's ambient-declaration rule reads "every declared symbol is
  type-level", which a pure-interface `.d.ts` stops satisfying the moment its
  members are indexed. An interface-owned member is now transparent to all
  four conditions, so the rule keeps measuring what it was measured on.
@danusha2345

Copy link
Copy Markdown
Contributor

Read through this while assembling a local integration of the open fixes. The two-line extractor change and the SIGNATURE_METHOD_NODE_TYPES guard look right, and anchoring the references on the member is a real improvement. Two things kept me from taking it into the local build as-is:

  1. The still-failing explore-declaration-only case: the 29 → 143 node jump on the fixture shim is the same thing that will happen to every real .d.ts in a project, so the ranking shift is the production effect, not a fixture artefact. A member weight of 0.5 restores the score but not the rank, as you note.
  2. The CG-28 IS_INTERFACE_MEMBER transparency is reasoned well, but it changes a corpus-measured rule; it wants the re-measure you ask for before it ships.

Might be worth splitting: the extractor + parity tests as one PR (unambiguous win, easy to review), and the ranking / CG-28 half as a follow-up with the corpus numbers.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Thanks for reading it that carefully, @danusha2345 — and I agree with your first point without reservation. The 29 → 143 jump is the production effect, not a fixture artefact: any real .d.ts gains a node per member, so the FTS dilution follows every project that has one. I would rather that sat in the PR body as a known cost than be discovered by someone downstream.

On splitting, I tried the shape you describe and it does not produce the clean first half either of us would want. The measurements:

  • Extractor + guard + test parity, without the CG-28 change: 7 new failures, six of them in explore-declaration-only. The moment interface members are indexed, a pure-interface .d.ts stops satisfying condition 2 ("every declared symbol is type-level"), so the ambient penalty stops applying. That is not a ranking nicety — it is the damping rule silently switching off, and it is worse than the ranking shift.
  • Extractor + guard + tests + IS_INTERFACE_MEMBER transparency (this PR): 1 new failure, the ranking one.

So the CG-28 half is not a follow-up that can wait — it is what keeps the first half from regressing the rule outright. And the failing test lands in whichever PR carries the extractor change, because the extra nodes are what shifts the RWR restart vector. A split moves the red from one PR to another rather than isolating it.

What is separable is the two tools.ts mitigations (the method_signature relevance weight, and signatures seeding without earning the named-FIRST tier). Those are ranking-only, and I would happily lift them out if @colbymchenry would rather review them apart from the extraction change.

The re-measure you and I both want is the same one: the 0–4% flag rate against the corpus the comment names. I cannot run that from outside, but if it would help your local integration, I can produce before/after node-count and flag-rate numbers on whatever repos you point me at.

@danusha2345

Copy link
Copy Markdown
Contributor

Took you up on the offer, but ran it here instead so the numbers come from repos you cannot see. Method: this PR merged onto current main (+ the open fixes I already carry), then for each repo a fresh index -f with the baseline build and with the PR build, and getAmbientDeclarationPathsAmong(<every indexed file>) as the flag-rate probe. The middle row is your "extractor without the CG-28 half" experiment — the PR's index queried by the baseline's SQL.

repo files nodes before → after interface members flagged before extractor-only (old rule) this PR
codegraph itself (TS, 6 .d.ts) 786 17,611 → 22,046 (+25%) 21 → 4,344 1 (0.13%) 0 1 (0.13%)
Android app: Kotlin + Go + TS (Wails) 274 5,022 → 5,022 43 → 43 0 0 0
Betaflight fork (C, 4k files) 4,055 88,135 → 88,135 0 6 (0.15%) 6 6
small TS/Dart app 51 958 → 958 0 0 0 0

So, on this sample:

  • The split really does not work — your "7 failures" is visible as a number: with the members indexed but the old rule, the only genuinely ambient shim in the TS repo (platform-shims.d.ts, the fixture) loses its flag. With the IS_INTERFACE_MEMBER transparency it keeps it, and nothing else gains one. Flag rate is unchanged on all four repos, which is the re-measure this half needed. Withdrawn: I'd take the PR as one piece.
  • Kotlin/Java/Go are untouched: their interface members were already nodes (43 here), and making them transparent flips no file. Same for C, which has no interface kind at all.
  • The cost is where you said: on an interface-heavy TS repo the graph grows a quarter (4.3k bodiless members). That is the FTS dilution behind the one remaining red test, and it will be felt in codegraph_explore on any repo whose platform API is a big .d.ts — worth stating in the PR body as the known trade-off, together with the two tools.ts mitigations that soften it.

Happy to re-run on other shapes if the maintainer names a corpus.

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

Labels

None yet

Projects

None yet

2 participants