Skip to content

fix(kernel): mirror interface members (#1638) on the Rust path, and fix the member signature on both - #1

Merged
maxmilian merged 2 commits into
maxmilian:fix/1638-ts-interface-membersfrom
bompus:fix/1638-kernel-interface-members
Sep 7, 2026
Merged

fix(kernel): mirror interface members (#1638) on the Rust path, and fix the member signature on both#1
maxmilian merged 2 commits into
maxmilian:fix/1638-ts-interface-membersfrom
bompus:fix/1638-kernel-interface-members

Conversation

@bompus

@bompus bompus commented Sep 6, 2026

Copy link
Copy Markdown

The Rust half of colbymchenry#1686, offered against this branch as @maxmilian asked, so the two paths stay one reviewable unit. Two commits: the port, then the signature: "counts counts" fix on both sides at once.

The TS side of this branch indexes property_signature / method_signature, but typescript and tsx are both in DEFAULT_ROUTED (src/extraction/kernel/index.ts:37), so wherever a codegraph-kernel.node is present the Rust walker replaces extraction outright and interface members stay unindexed. is_method_type (codegraph-kernel/src/tsjs/mod.rs:56) matched only method_definition and TS public_field_definition.

1. The port — four edits

Each mirrors its TS counterpart one for one.

  1. method_signature joins is_method_type — mirrors typescriptExtractor.methodTypes.
  2. New is_property_type for property_signature — mirrors propertyTypes. It carries no value, so it is always a property and never goes through classify_ts_class_member.
  3. 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, and it has to exist: inside_class_like() (mod.rs:287) already treats 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 same failure the TS guard prevents.
  4. The combined property_signature | method_signature branch is replaced by the property branch. It used to hang both members' type annotations off the enclosing interface, the only anchor available while the members themselves went unextracted. 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.

Acceptance: scripts/kernel-parity.mjs

The gate you named. Run over src, __tests__ and ui — 626 files, wasm totals 16,308 nodes / 17,353 edges / 100,384 refs — with the kernel built from this branch versus from 948e455 (the TS change alone):

byte-parity files with diffs
this branch without the port (948e455) 452 / 626 169
with the port 619 / 626 2

The control run is the scope limit stated as numbers: 2,386 property and 1,174 method nodes missing in the kernel, 3,560 contains edges, and ~3k references on each side that anchor differently. That is the shape of what a released install currently loses.

Both files still diverging are Dart fixtures (torture.dart, TortureCtors.dart), and they diverge identically in the control run — pre-existing and unrelated. No TS or TSX file diverges.

2. signature: "counts counts" — fixed, both halves in one commit

You said this belongs in the same commit as its TS half rather than a follow-up, so b7cb38f carries both.

extractProperty reads the explicit type field only for public_field_definition / field_definition and otherwise takes the generic named-child scan. A property_signature missed that test, 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 never read the type annotation.

Both sides now name property_signature explicitly rather than folding it into the field test, so no other language's property_declaration moves off the generic scan. Verified on interface Stats { counts: Record<string, number>; label: string; fetch(id: string): Promise<void> }:

--- wasm                                        --- kernel
property counts => "Record<string, number> counts"   property counts => "Record<string, number> counts"
property label  => "string label"                    property label  => "string label"
method   fetch  => "(id: string): Promise<void>"     method   fetch  => "(id: string): Promise<void>"

Parity holds at 619/626 after this commit — the same two Dart fixtures, no new divergence.

As you noted, 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.

What I could not run

The vitest suite. My host runs Bun, and vitest's worker pool does not survive it on Windows; the earlier "648 pass, 1 fail" figure on colbymchenry#1686 came from a Node 22 run I no longer have. The first commit is Rust-only. The second touches tree-sitter.ts, and while the change is gated on a node type that only colbymchenry#1638 makes reachable, it is a TS file and I have not re-run the suite against it — worth confirming on your end alongside the parity sweep.

… 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.
@bompus bompus changed the title fix(kernel): mirror interface members (#1638) on the Rust path fix(kernel): mirror interface members (#1638) on the Rust path, and fix the member signature on both Sep 6, 2026
@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Ran the suite after all, so the "could not run vitest" caveat in the description is closed. Node 26.8.1 rather than the 22 I used before; both arms identical otherwise — kernel built from the checkout under test, tsc rebuilt, full vitest run.

test files tests
this branch alone (948e455) 12 failed / 219 passed 43 failed / 4,143 passed
with this PR (b7cb38f) 11 failed / 220 passed 36 failed / 4,150 passed

Diffing the failing test names rather than the counts, the port fixes 9 and adds 1.

Fixed — and the first one is the headline:

Added — the one you already documented:

  • explore-declaration-only.test.ts > does not let it outrank the implementation files. This is the known CG-28 ranking failure from your PR body, not a new one. It is absent from the control run only because without the port the kernel never indexes the members, so the fixture shim never reaches 143 nodes and the RWR restart vector never shifts. Putting the members on the kernel path is what makes the documented failure reachable there too.

Not a real failure: mcp-daemon.test.ts > concurrent launchers converge on a single daemon (lockfile race) appears in the branch run but passes 3/3 in isolation on both arms — a full-suite race on this host.

The remaining ~35 failures are identical across both arms and untouched by either side of this change: JVM FQN imports, PHP/C++ include resolution, the MCP initialize/roots/subproject suites, and the two Dart kernel-parity fixtures that also account for the 2 residual parity diffs.

@maxmilian

Copy link
Copy Markdown
Owner

Merged into fix/1638-ts-interface-members (fast-forward, both commits with your authorship intact) — GitHub closed this automatically. Upstream colbymchenry#1686 now carries it.

I re-ran the acceptance gate on the merged branch myself rather than quoting yours, and it lands exactly where you measured: node scripts/kernel-parity.mjs src __tests__ ui gives 619/626 byte-parity (2 with diffs, 5 deferred-to-wasm; wasm totals 16308 nodes / 17353 edges / 100388 refs), with no TS or TSX file diverging. I also checked the two Dart fixtures the hard way instead of taking "pre-existing" on trust — rebuilt the kernel with codegraph-kernel/src/tsjs/ reverted to 948e455 and kernel-dart-parity.test.ts fails the same four assertions with the same node-count deltas.

Full suite on the merged branch (dist/ and UI built, host kernel staged): 4228 passed / 4 failed / 11 skipped, the 4 being those Dart assertions. So the TS-side risk you flagged as unverified — tree-sitter.ts touched without a vitest run — is clear.

Thank you. The DEFAULT_ROUTED catch, naming kernel-parity.mjs as the gate, and the is_signature_method_type guard (which I'd have had to rediscover through a phantom function stop) were the difference between a fix and a fix that a released install actually gets. You're credited in the upstream PR body and in my comment there.

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.

2 participants