fix(extraction): index TypeScript interface members (#1638) - #1686
fix(extraction): index TypeScript interface members (#1638)#1686maxmilian wants to merge 4 commits 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. |
… path
The TS half of this branch indexes property_signature / method_signature,
but typescript and tsx are both in DEFAULT_ROUTED, so on any install
carrying a codegraph-kernel.node the Rust walker replaces extraction and
interface members stay unindexed. is_method_type matched only
method_definition and TS public_field_definition, with no signature node
type anywhere on this side.
Four edits, mirroring the TS extractor one for one:
- method_signature joins is_method_type (typescriptExtractor.methodTypes).
- New is_property_type for property_signature (propertyTypes). It carries
no value, so it is always a property and never reaches
classify_ts_class_member.
- New is_signature_method_type, guarding the method branch with
`&& (!is_signature_method_type(kind) || self.inside_class_like())`. This
mirrors SIGNATURE_METHOD_NODE_TYPES: inside_class_like already treats an
interface as class-like, so without the guard a bare
`type Handle = { stop(): void }` takes extract_method's "no class-like
parent, so treat it as a free function" fallback and the file gains a
phantom top-level `function stop` beside the real Handle::stop.
- The branch matching property_signature and method_signature together,
which hung their type annotations off the enclosing interface, becomes
the property branch. The references edges survive — extract_method and
extract_property each call extract_type_annotations — and now anchor on
the member: Api::fetch -> PageId instead of Api -> PageId.
extract_property reads the `type` field only for real field definitions
and otherwise takes the generic child scan, which is the wasm behaviour
including its quirk of repeating the member name rather than naming the
type (colbymchenry#808 fixed the field case only). Parity is the contract, so
correcting that has to move both sides in one commit; raised on the PR.
Verified with scripts/kernel-parity.mjs over src, __tests__ and ui
(626 files, wasm totals 16,308 nodes / 17,353 edges / 100,384 refs):
before 452/626 byte-parity, 169 files with diffs
(2,386 property and 1,174 method nodes missing in kernel,
3,560 contains edges, ~3k references on each side)
after 619/626 byte-parity, 2 files with diffs
Both remaining files are Dart fixtures that diverge identically before
this change; no TS or TSX file diverges.
…t its name twice
`interface Stats { counts: Record<string, number> }` extracted `counts`
with signature "counts counts".
extractProperty reads the explicit `type` field only for
public_field_definition / field_definition and otherwise takes a generic
named-child scan (colbymchenry#808, aimed at fields whose other children are the name
and an initializer VALUE). A property_signature missed that test, so it
took the scan — and the scan's exclusion list covers `identifier` but not
the `property_identifier` an interface member is named with, so it stopped
on the name node and the type annotation was never read.
colbymchenry#808 targeted field definitions carrying initializer values; interface
members could not reach this code path when it was written, so this is a
gap rather than a decision.
Fixed on both paths in one commit: the kernel and wasm extractors have to
agree or scripts/kernel-parity.mjs fails, and the previous commit's port
had mirrored the quirk deliberately for that reason. The test is named
explicitly rather than folded into the field test, so no other language's
property_declaration moves off the generic scan.
The whole affected set is nodes colbymchenry#1638 introduces — before it no node
existed for a property_signature on either path — so no signature that
ships today changes.
Verified, both paths, on `interface Stats { counts: Record<string, number>;
label: string; fetch(id: string): Promise<void> }`:
wasm counts => "Record<string, number> counts"
kernel counts => "Record<string, number> counts"
scripts/kernel-parity.mjs over src, __tests__ and ui holds at 619/626
byte-parity, the same 2 pre-existing Dart fixtures.
|
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. |
CG-28: on a prose flow query, a declaration-only file outranks the implementation it declares. The damage enters through the RWR restart vector, not through connectivity. `contains` is not a RANK_EDGE, so a declared member is near-isolated and carries almost no walk mass of its own — but the restart vector is uniform over seeds, so since colbymchenry#1638 a platform `.d.ts` contributes one seed per member, and those member names (`body`, `stream`, `metadata`) are exactly what a prose flow query matches. Every such seed divides the restart mass the implementation files are competing for. That is what halves an implementation file's graph mass while the shim's holds steady. Filter the damped files out of the seed set only. They stay candidates, stay reachable, and keep their `score` contribution; this changes where the walk starts and nothing else. The predicate is `isDampedDeclaration` rather than a bare ambient test because it already exempts a file whose declared type the query named — so the counter-case holds: on a query about the declared type the shim still ranks first, at mass 1.0. Fixture (ambient-decls-ts), flow query: storage/metadata.ts 0.137461 -> 0.504025 rank 2 -> 1 storage/stream.ts 0.058601 -> 0.214871 rank 4 -> 2 platform-shims.d.ts 0.184398 -> 0.009459 rank 1 -> 3, still named __tests__/explore-declaration-only.test.ts: 12 passed, 0 failed. Full suite against this base: 31 failed -> 30 failed, and the CG-28 gate is the only difference in the failing set. Verified on vitejs/vite (1,719 files, 13,793 nodes, 32,822 edges), one index shared across arms so ranking is the only variable: zero changed rows against the unpatched base on four prose flow queries, and the type counter-case keeps types/hmrPayload.d.ts at rank 1 (mass 0.185539). The change is inert where it is not needed.
|
The do-not-merge condition is lifted — both extraction paths are now covered, and this branch is ready to review. @bompus wrote the two halves I could not: the Rust kernel mirror (with the I re-ran everything myself on the merged branch rather than carrying his numbers over — macOS arm64, Kernel parity ( No TS or TSX file diverges. The two that do are the Dart fixtures Full suite ( The 4 are exactly those pre-existing Dart parity assertions. The CG-28 gate — the one test I flagged as still failing when I opened this — Two notes on the review surface, so nobody re-derives them:
The one thing still outside what I can verify is unchanged from the original body: whether folding interface members into the CG-28 conditions keeps the 0–4% ambient flag rate. That needs the corpus the comment cites, and remains a proposal for whoever holds it. Thanks @bompus — the parity harness call, the port, and finding that it was the restart vector rather than connectivity were all yours. |
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.Both paths: the Rust kernel mirrors the TS extractor
typescriptandtsxare both inDEFAULT_ROUTED(src/extraction/kernel/index.ts:37), so wherever acodegraph-kernel.nodeis present the Rust walker replaces extraction outright — andis_method_type(codegraph-kernel/src/tsjs/mod.rs:56) matched onlymethod_definitionand TSpublic_field_definition..github/workflows/release.yml:33makes the kernel the release's headline as of 1.5.0, so a TS-only fix would have been inert on a released install. Nor would the existing guards have caught 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.@bompus raised this in review and then wrote the port (maxmilian#1), which is merged here. It mirrors the TS side one for one:
method_signaturejoinsis_method_type; a newis_property_typecoversproperty_signature; a newis_signature_method_typeguards the method branch with&& (!is_signature_method_type(kind) || self.inside_class_like()), the Rust counterpart ofSIGNATURE_METHOD_NODE_TYPESand just as load-bearing (inside_class_like()already treatsinterfaceas class-like, so without it a baretype Handle = { stop(): void }gains the same phantom top-levelfunction stop); and the combinedproperty_signature | method_signaturebranch is replaced by the property branch, so thereferencesedges anchor on the member instead of the enclosing interface.Acceptance is
scripts/kernel-parity.mjs, the tool that would have caught the gap. Measured on this merged branch (darwin-arm64, kernel built fromscripts/build-kernel.sh, run oversrc __tests__ ui— 626 files, wasm totals 16,308 nodes / 17,353 edges / 100,388 refs):No TS or TSX file diverges. The two files that do are Dart fixtures (
torture.dart,TortureCtors.dart), and they are pre-existing:__tests__/kernel-dart-parity.test.tsfails on the same four assertions with a kernel built from948e455(this branch before the port), and nothing in the merged diff touches Dart —git diff 948e455..HEADiscodegraph-kernel/src/tsjs/{extractors,mod}.rs,src/extraction/tree-sitter.ts,src/mcp/tools.ts.For scale, @bompus's control run — this branch's TS change with an unported kernel — sits at 452/626 with 169 files diverging: 2,386
propertyand 1,174methodnodes missing in the kernel, 3,560containsedges, and ~3kreferencesanchoring differently. That is what a released install would have lost.CG-28: what I proposed, and how the ranking half got closed
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 flag-rate half 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 that half as a proposal and re-measure.The ranking half is now closed. Detection was never the problem — the shim stayed flagged and damped at 0.5 — magnitude was: the fixture's shim holds 143 nodes where it held 29, and the extra bodiless names shifted the RWR restart vector, halving implementation files' graph mass (0.307 → 0.137) while the shim's held steady. I had mitigated
RELEVANCE_KIND_WEIGHT(method_signaturerated as "a member of a type" at ~0.5, restoring the raw score 53 → 52.5 and cutting the rendered envelope from 6,044 to 1,933 characters) and the named-seed tier, but neither movesgraphScore, which is what orders the list.@bompus found the actual mechanism and fixed it in one place (maxmilian#2, merged here): the restart vector is uniform over seeds, so every seed divides the mass implementation files compete for, and
containsis not aRANK_EDGE— a declared member carries almost no walk mass of its own, so occupying a seed is the whole of its effect. The fix filters already-damped declaration files out of the seed set only; they stay candidates, stay reachable, keep theirscorecontribution. The predicate isisDampedDeclaration, which already exempts a file whose declared type the query named, so a query genuinely about the declared type keeps its seeds and the shim still ranks 1 at mass 1.0. There is a fallback to the unfiltered seeds when every seed is damped, so the walk never loses its restart vector.On the fixture's flow query:
src/storage/metadata.ts0.137461 → 0.504025 (rank 2 → 1),src/storage/stream.ts0.058601 → 0.214871 (4 → 2),types/platform-shims.d.ts0.184398 → 0.009459 (1 → 3, still named). He also ran it against vitejs/vite (1,719 files, 13,793 nodes, 32,822 edges): four prose flow queries produced 0 changed rows, and the type counter-case keptpackages/vite/types/hmrPayload.d.tsat rank 1, identical patched and unpatched — the change is inert where it isn't needed.Tests
Measured on this merged branch, macOS arm64, with
dist/and the UI built and a host kernel staged (npm run build && scripts/build-kernel.sh),npx vitest run:The 4 failures are the pre-existing Dart parity ones described above (
kernel-dart-parity.test.ts, identical on a948e455kernel).__tests__/explore-declaration-only.test.tsis 12 passed / 0 failed — the CG-28 gate that this PR previously left failing.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.Also folded in:
signature: "counts counts"on interface propertiesFound by @bompus while porting, and fixed on both paths in one commit.
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 namesproperty_signatureexplicitly rather than folding it into the field test, so no other language'sproperty_declarationscan moves.Credit: the Rust kernel port, the
signaturefix on both halves, and the CG-28 restart-vector fix are @bompus's work (maxmilian#1 and #2), merged into this branch with his commits and authorship intact.