fix(extraction): index TypeScript interface members (#1638) - #1686
fix(extraction): index TypeScript interface members (#1638)#1686maxmilian wants to merge 1 commit into
Conversation
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.
|
Read through this while assembling a local integration of the open fixes. The two-line extractor change and the
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. |
|
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 On splitting, I tried the shape you describe and it does not produce the clean first half either of us would want. The measurements:
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 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. |
|
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
So, on this sample:
Happy to re-run on other shapes if the maintainer names a corpus. |
|
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 Merges clean onto main. Index of our repo before/after:
PR test files on Node 22 (bundled runtime): 648 pass, 1 fail — One side effect worth a look before merge: |
|
Heads up on a gap this PR will hit in practice, and an offer of the missing half. The fix lands on the wasm path only. The changed files are all TS-side ( I ported the mirror while verifying a fork on the kernel path. Offering it here rather than as a competing PR — it is your change, and it wants to ride the same branch. The Rust side is four edits in
Happy to open it as a PR against One thing the port surfaced that is a question for the TS side. Routing interface members through interface Stats {
counts: Record<string, number>;
}the extracted That is what this PR ships today; I mirrored it in Rust deliberately, because kernel/wasm parity is the harder contract and my parity suite fails the moment the two disagree. But it looks unintended — reading |
|
Thanks — I verified both points against current On the kernel gap: conceded, and it is worse than "an install where a kernel is present." Your read of the loader guard is also right — Please open it as a PR against On The reason to fix it here rather than later: before this PR, no node existed for a So: yes to the Rust half of that too, in the same commit as the TS half. I will hold off touching the TS side of the signature fix until your PR is up, so the two land together instead of racing. |
|
The Rust port is up as requested: maxmilian#1, based on Putting the acceptance numbers here as well, since this is the PR @colbymchenry will be reviewing and the kernel gap is the thing that decides whether it does anything on a released install.
The first row is the scope limit measured rather than argued: 2,386 The two files still diverging are Dart fixtures ( One thing I could not run and would rather say than leave implied: the vitest suite. My host runs Bun and vitest's worker pool does not survive it on Windows, so the "648 pass, 1 fail" I reported earlier came from a Node 22 run I no longer have. The port itself is Rust-only, but the signature commit touches Also still open from my earlier comment, unrelated to the port: |
|
Correcting one thing I said above, before it gets read as a general claim about this PR. I reported that
The baseline is already 489 before this PR's members exist at all — on this repo most of those edges are not TS interface members (Rust traits and the like reach the same #1538 still zeroes them, and it still merges cleanly onto Also, on the kernel gap: running the full suite on both arms turned up a sharper piece of evidence than the |
|
Dug into the one remaining red test. One finding changes where the fix has to go, so putting it here rather than only on the branch. Rank is ordered by
The shim already loses on Both Where the members actually hurt: the restart vector, not connectivity. Dropping interface-owned signatures from the restart vector only — they stay candidates, stay reachable, keep their
The halving is undone and I also measured the "member of a type ~0.5" weighting applied to the per-file mass sum: it moves the shim only 0.191047 → 0.189274, because the members were never carrying the mass. Combined, the gap is 0.00027 and still inverted. The residual is no longer about members — it is the shim's 9 interface nodes against It is on maxmilian#2 as a draft against this branch, with the full numbers. Full suite: 94 failures before, 94 after, the one differing name being a Entirely yours to take, rewrite, or close — it is your ranking core, and the corpus re-measure is the part neither of us can do from outside. |
|
Update on the CG-28 red test: it is fixed. First, a correction to my last comment. I wrote that "rank is ordered by The load-bearing part survives: What moves it. A declaration-only file is not a place a walk starts.
The shim stays a candidate, stays reachable, keeps its The predicate is This touches neither @maxmilian it is yours to take or rewrite. With it, this branch has no failing test I can find. |
Closes #1638.
tree-sitter-typescript spells interface members with their own node types —
method_signatureandproperty_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 reusemethod_declaration, which is already in theirmethodTypes.The cost lands on any codebase whose platform API is a
.d.tsinterface. 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 (isInsideClassLikeNodelists'interface'), so members attach with no traversal change. Precedent:extractTsTypeAliasMembersalready makestype X = { foo(): T }members first-class (#359) — interfaces were the inconsistent gap.Three consequences, each handled and tested:
extractMethod's "no class-like parent → free function" fallback. Outside an interface such a node appears only in a type literal, whose members TypeScripttypealias members not used in method-call resolution → false cross-modulecallsedges via path-proximity #359 already extracts — without a guard,type Handle = { stop(): void }gains a phantom top-levelfunction stop.property_signature/method_signaturebranch 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. Thereferencesedges survive and now anchor on the member, soApi::fetch → PageIdsays which member wants the type whereApi → PageIdonly said the file did.CG-28: a proposal, not an assertion
getAmbientDeclarationPathsAmongreads "every declared symbol is type-level", which a pure-interface.d.tsstops 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
parameteralready gets ("structural bookkeeping, neither qualifies nor disqualifies"), via one sharedIS_INTERFACE_MEMBER(alias)fragment seekingidx_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-ontypes.tsstill 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.rsre-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_WEIGHTratesmethodat 1.0 as "a callable", and its own doc puts "a member of a type" at ~0.5. Amethod_signatureis the latter. This restores the raw score almost exactly (53 → 52.5) and cuts the rendered envelope from 6044 to 1933 characters.body,streamandmetadataare member names in any platform.d.tsand were reaching that tier throughcoNamedInFile, 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 builtdist/, none touching extraction or queries). After: 3952 / 85 — failure-list diff is exactly the one entry above, with none disappearing.Added:
containsedge rather than merely existing;.d.tsis 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 acontainsedge from an interface; a node minted fromPick<User,'id'>or a tuple has no declaring interface, so the guard is intact.object-literal-methods.test.ts— its Zustand fixture declares bothinterface Store { fetchUser(): … }and an action of that name, sofind(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 anisTypeLevelhelper. Afunctionorclasscreeping into that fixture still fails, and the gate assertions themselves are untouched.Raised by @bompus in review, verified against current
main, and it is the first thing a reviewer should know.The change here is entirely on the TypeScript side (
src/extraction/tree-sitter.ts,src/extraction/languages/typescript.ts). Buttypescriptandtsxare both inDEFAULT_ROUTED(src/extraction/kernel/index.ts:37), so when a kernel binary is present the Rust walker replaces extraction outright — andis_method_type(codegraph-kernel/src/tsjs/mod.rs:56) matches onlymethod_definitionand TSpublic_field_definition, with noproperty_signature/method_signatureanywhere.That is not an edge case.
.github/workflows/release.yml:33states that as of 1.5.0 the kernel is the release's headline, not an optional extra; the prebuild matrix is required and the binaries ship underrelease/kernel/. So the wasm path this PR fixes is the fallback, and on a released install the change is currently inert.Nor do the existing guards catch it: the loader compares node/edge tables plus an ABI version (
src/extraction/kernel/loader.ts:132), which a divergence in extraction logic passes silently.scripts/kernel-parity.mjsis the tool that would have caught it, and is the acceptance test the port should be judged on.@bompus is opening the Rust mirror as a PR against this branch (
fix/1638-ts-interface-members) so the two halves stay one reviewable unit. This PR should not be merged until that lands — merging the TS half alone would advertise a fix that a released install does not get.Also folded in:
signature: "counts counts"on interface propertiesFound by @bompus while porting.
isTsJsField(tree-sitter.ts:2039) gatesextractProperty's narrowing onpublic_field_definition/field_definition, so aproperty_signaturefalls through to the generic named-child scan — whose exclusion list coversidentifierbut notproperty_identifier. The scan therefore stops on the name node and the type annotation is never read, sointerface Stats { counts: Record<string, number> }yieldssignature: "counts counts".This is a gap rather than a decision: #808 targeted field definitions carrying initializer values, and interface members could not reach that code path when it was written.
It is fixed here rather than in a follow-up because of blast radius. Before this PR no node existed for a
property_signatureon either path, so reading itstypefield changes the signature of nothing that ships today — the whole affected set is nodes this PR introduces. Landing them with a known-wrong field and correcting it later is the worse trade. The fix is gated onproperty_signaturespecifically, so no other language'sproperty_declarationscan moves.