feat(java,kotlin): MethodExtractor abstraction with per-language configs#576
Conversation
…igs (#569) Add structured method metadata extraction mirroring the FieldExtractor pattern. Enables isAbstract/isFinal detection, structured ParameterInfo, annotations, and visibility for Java and Kotlin methods. New files: method-types.ts, method-extractors/generic.ts, configs/jvm.ts Modified: NodeProperties, LanguageProviderConfig, parse-worker, language providers Tests: 23 cases covering classes, interfaces, abstract classes, enums, overloads
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude ultra-think while identifying kotlin and java languages gaps. Perfom this analysis on this pull request and verify those findings. Act as a senior compiler font-end engineer and expert in static analysis tools and review this change from that perpective and review the architectural fit. Also check out previous comments and reason with their requests. |
|
Claude finished @magyargergo's task in 4m 52s —— View job Deep Analysis: Java/Kotlin Language Gaps & Compiler Front-End ReviewTasks
Overall AssessmentThe config+factory approach is sound and the FieldExtractor parallel is well-executed. The core plumbing (cache keying, fallback chain, 🔴 Critical Gaps — Silent Data Loss1. Java:
|
| # | Gap | Language | Severity | Fix Complexity |
|---|---|---|---|---|
| 1 | annotation_type_element_declaration missing |
Java | 🔴 Silent data loss | Low |
| 2 | compact_constructor_declaration missing |
Java | 🔴 Silent data loss | Low |
| 3 | companion_object not a type container |
Kotlin | 🔴 Silent data loss | Medium |
| 4 | vararg always isVariadic: false |
Kotlin | 🔴 Wrong data | Low |
| 5 | Enum constant body methods not extracted | Java | 🔴 Silent data loss | Medium |
| 6 | isFinal semantics inverted |
Kotlin | 🟡 Misleading | Medium (schema) |
| 7 | ownerFqn is not FQN |
Both | 🟡 Misleading name | Low (rename) |
| 8 | Missing throws clause |
Java | 🟡 Schema gap | Medium |
| 9 | Missing suspend modifier |
Kotlin | 🟡 Critical for coroutines | Medium |
| 10 | annotations missing from ParsedSymbol |
Both | 🔵 Arch gap | Low |
| 11 | Primitive vararg type loss | Java | 🔵 Minor inaccuracy | Low |
Items 1-4 are straightforward fixes that could land in this PR. Items 3 and 5-9 may warrant a follow-up PR (in line with the RFC #568 series).
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 4706 tests passed 46 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
…nstructors, varargs - Add annotation_type_element_declaration to Java methodNodeTypes - Add compact_constructor_declaration to Java methodNodeTypes - Add primitive types (integral_type, floating_point_type, boolean_type) to Java vararg type extraction - Implement Kotlin vararg detection via parameter_modifiers - Add tests: annotation elements, compact constructors, primitive varargs, Kotlin vararg
|
@claude ultra-think while identifying kotlin and java languages gaps. Perfom this analysis on this pull request and verify those findings. Act as a senior compiler font-end engineer and expert in static analysis tools and review this change from that perpective and review the architectural fit. Also check out previous comments and reason with their requests. |
|
Claude finished @magyargergo's task in 4m 39s —— View job Deep Analysis: Java/Kotlin Language Gaps & Compiler Front-End ReviewTasks
Correction of Previous AnalysisThe prior review was generated against an earlier revision of the branch. All four of its 🔴 "critical silent data loss" items and one "minor inaccuracy" are already implemented and tested in the current code. This needs to be stated clearly before discussing what's actually missing.
The remaining findings from the prior comment (companion_object, isFinal semantics, ownerFqn naming, throws, suspend, annotations/ParsedSymbol, findBodies brittleness) are valid and confirmed below. Additional gaps not in the prior review are also identified. Overall AssessmentThe design is solid. The What follows is the verified gap list from this review. 🔴 Confirmed Real Gaps1. Kotlin
|
| # | Gap | Language | Severity | Already in previous review? |
|---|---|---|---|---|
| 1 | companion_object not in typeDeclarationNodes + ownerFqn extraction fails for it |
Kotlin | 🔴 Data loss | ✓ partially (missed ownerFqn sub-issue) |
| 2 | Enum constant body methods not extracted | Java | 🔴 Data loss (edge case) | ✓ |
| 3 | isFinal semantics (explicit vs default-closed) |
Kotlin | 🟡 Misleading | ✓ |
| 4 | ownerFqn is simple name, not FQN |
Both | 🟡 Misleading contract | ✓ |
| 5 | suspend modifier not captured |
Kotlin | 🟡 Call-graph accuracy | ✓ |
| 6 | throws clause not captured |
Java | 🟡 Schema gap | ✓ |
| 7 | annotations not in ParsedSymbol |
Both | 🔵 Arch asymmetry | ✓ |
| 8 | findBodies fallback masks config typos |
Both | 🔵 Future fragility | ✓ |
| 9 | hasKeyword redundant alongside hasModifier |
Java | 🔵 Dead code / clarity | ✓ (called redundancy) |
| 10 | FUNCTION_NODE_TYPES missing compact ctor + annotation element |
Java | 🔵 Enclosing-function gap | ✗ New |
| 11 | object_declaration functions never isStatic: true |
Kotlin | 🟡 JVM semantic gap | ✗ New |
| 12 | companion_object ownerFqn extraction sub-issue (simple_identifier vs type_identifier) |
Kotlin | 🔴 Would break fix for #1 | ✗ New |
Items 1 (with sub-issue 12) and 2 are the only outstanding 🔴 gaps. The rest are reasonable follow-ups in the RFC series. The code quality, test coverage for the happy paths, and architectural fit are all good. Ready to merge once the companion_object issue has a plan (even if the fix lands in a follow-up PR with a tracking issue).
- Add companion_object to Kotlin typeDeclarationNodes with unnamed
companion fallback ("Companion")
- Recurse into enum constant anonymous class bodies for method extraction
- Fix Kotlin isFinal: closed by default (final unless open/abstract/override)
- Add domain invariant: isAbstract forces isFinal=false
- Mark companion_object/object_declaration members as isStatic=true
- Rename ownerFqn → ownerName (not an FQN, just simple class name)
- Add compact_constructor_declaration + annotation_type_element_declaration
to FUNCTION_NODE_TYPES in ast-helpers.ts
- Remove hasKeyword redundancy in Java isStatic/isFinal/isAbstract
- Add simple_identifier fallback for named companion ownerName resolution
- Tests: companion objects, enum constant bodies, isFinal semantics,
domain invariant (34 total, up from 27)
|
@claude ultra-think while identifying kotlin and java languages gaps. Perfom this analysis on this pull request and verify those findings. Act as a senior compiler font-end engineer and expert in static analysis tools and review this change from that perpective and review the architectural fit. Also check out previous comments and reason with their requests. |
|
Claude finished @magyargergo's task in 5m 45s —— View job Third-Pass Deep Analysis: Java/Kotlin Language Gaps (Compiler Front-End Perspective)
|
| Prior Finding | Review 1 Said | Review 2 Said | Actual Status in Current Code |
|---|---|---|---|
companion_object absent from typeDeclarationNodes |
🔴 critical | 🔴 confirmed | FIXED — jvm.ts:257 |
| Enum constant body methods not extracted | 🔴 critical | 🔴 confirmed | FIXED — generic.ts:119-126 recurses into enum_constant → class_body |
vararg always isVariadic: false |
🔴 critical | corrected (false alarm) | IMPLEMENTED — jvm.ts:172-183 via parameter_modifiers sibling scan |
Primitive vararg type loss (int...) |
🔴 critical | corrected (false alarm) | IMPLEMENTED — jvm.ts:82-84 handles integral_type, floating_point_type, boolean_type |
annotation_type_element_declaration missing |
🔴 critical | corrected (false alarm) | IMPLEMENTED — jvm.ts:115, ast-helpers.ts:69 |
compact_constructor_declaration missing |
🔴 critical | corrected (false alarm) | IMPLEMENTED — jvm.ts:114, ast-helpers.ts:68 |
isFinal Kotlin semantics inverted |
🟡 semantic | 🟡 confirmed gap | FIXED — jvm.ts:306-311 returns true by default, false for open/abstract/override |
ownerFqn misleading name |
🟡 semantic | 🟡 confirmed | FIXED — field is ownerName in method-types.ts:36 and generic.ts:66 |
object_declaration never isStatic |
🟡 (new) | 🟡 confirmed | FIXED — generic.ts:20, 145 STATIC_OWNER_TYPES set |
companion_object ownerFqn extraction sub-issue |
🔴 (new) | 🔴 confirmed | FIXED — generic.ts:41-57 handles unnamed→"Companion", named→simple_identifier |
hasKeyword redundancy |
🔵 | 🔵 confirmed | FIXED — not present in current jvm.ts |
FUNCTION_NODE_TYPES missing compact ctor + annotation element |
🔵 (new) | 🔵 confirmed | FIXED — ast-helpers.ts:68-69 |
Both prior reviews were incorrect about the current state of the branch. The code is substantially more complete than either review acknowledged.
What's Genuinely Still Missing (Verified Against Current Code)
🟡 Gap 1 — Kotlin extension functions: no receiverType in schema
This is the only gap both prior reviews missed entirely, and it's the most significant from a compiler front-end perspective.
In tree-sitter-kotlin, fun String.format(template: String): String parses as function_declaration with a receiver type node (user_type: "String") before the function name identifier. The current extractName scans for the first simple_identifier — which correctly skips the user_type and returns "format". Name extraction is correct.
However, MethodInfo has no receiverType field. The fact that this method is bound to String is completely lost. This matters architecturally because:
- Extension function dispatch is static (resolved at compile time), unlike virtual dispatch.
str.format(...)in bytecode becomesExtensionKt.format(str, ...)— not a method onString. - Two methods with the same name are not the same:
fun Foo.process()andfun Bar.process()are entirely different call targets. - Any future "find callers of X" analysis will conflate extension functions on different receivers if
receiverTypeis absent.
No test covers extension functions. This is a schema gap worth a tracking issue before the CSV schema hardens.
Current behavior: fun String.repeat(n: Int) → { name: "repeat", receiverType: undefined, ... } — the extension receiver is silently discarded.
🟡 Gap 2 — Java compact_constructor_declaration yields parameters: []
jvm.ts:48-99 — extractJavaParameters calls node.childForFieldName('parameters') at line 50. Compact constructors have no explicit parameter list in the AST — the parameters are the enclosing record's components. This call returns null, so parameters: [] is emitted.
public record Point(int x, int y) {
public Point { // compact_constructor_declaration
Objects.requireNonNull(...);
// implicitly has parameters x:int, y:int — but MethodInfo.parameters = []
}
}Fixing this properly requires accessing the parent record_declaration's formal_parameters, which means the factory would need a parent-node traversal path — a non-trivial change. The correct interim behaviour is to document this in a comment at the compact_constructor_declaration case. The current code is silently wrong for anyone who calls method.parameters.length on a compact constructor.
🟡 Gap 3 — isAbstract/isFinal/annotations absent from ParsedSymbol
parse-worker.ts:111-125 and 1827-1841 — Verified. The ParsedSymbol interface does not include isAbstract, isFinal, or annotations. Lines 1827-1841 push to result.symbols but these three fields are intentionally omitted:
result.symbols.push({
// isAbstract, isFinal, annotations — not here
visibility, isStatic, isReadonly, // ← field metadata IS included
parameterCount, returnType, ...
});These fields ARE on result.nodes (lines 1810-1812). The asymmetry is deliberate per the PR description ("not yet persisted to LadybugDB"), but it means call resolution in the symbol table has no visibility into method modifier context. @Transactional, @Cacheable, @Override, abstract — all invisible at the symbol level. Worth a deliberate decision before the symbol table is extended.
🔵 Gap 4 — findBodies fallback silently validates config typos
generic.ts:85-88 — If bodyNodeTypes has a typo, the fallback path at line 85 uses whatever childForFieldName('body') returns, giving correct results by accident. Any new language config author gets false confidence that their config is correct. A DEV assertion or warn log on the fallback path would catch this.
🔵 Gap 5 — Java annotated_type parameter type accuracy
For void foo(@NonNull String name), tree-sitter-java wraps the type in annotated_type. childForFieldName('type') returns the annotated_type node. extractSimpleTypeName likely can't resolve this, falling back to annotated_type.text?.trim() which produces "@NonNull String" rather than "String". Low impact today but a consistency gap for annotation-heavy Spring/Jakarta codebases.
JVM Bytecode Schema Completeness Audit
Compared against JVM .class method flags and Kotlin @Metadata compiler data:
| Modifier | Java | Kotlin | In Schema | Priority |
|---|---|---|---|---|
public/private/protected |
✅ captured | ✅ captured (internal too) |
✅ | — |
static |
✅ | ✅ (via STATIC_OWNER_TYPES) | ✅ | — |
final |
✅ | ✅ (correct default semantics) | ✅ | — |
abstract |
✅ | ✅ (including interface inference) | ✅ | — |
synchronized |
ACC_SYNCHRONIZED | n/a (@Synchronized annotation) |
❌ | Follow-up |
native |
ACC_NATIVE | external keyword |
❌ | Follow-up |
| throws clause | checked exceptions | n/a | ❌ | Follow-up |
suspend |
n/a | Kotlin coroutine marker | ❌ | Follow-up |
operator |
n/a | Kotlin operator overloading | ❌ | Follow-up |
inline |
n/a | Kotlin call-site inlining | ❌ | Follow-up |
tailrec |
n/a | Kotlin TCO | ❌ | Low priority |
override |
@Override annotation captured |
modifier (→ isFinal:false) |
partial | Follow-up |
| extension receiver | n/a | fun T.foo() receiver type |
❌ | Track now |
| type parameters | <T> on method |
<T : Bound> |
❌ | Follow-up |
Items marked "Track now" should have a tracking issue before the schema is written to disk.
What's Well-Designed (No Corrections Needed)
isFinalKotlin semantics (jvm.ts:306-311): The current implementation is correct. Regular Kotlin functions returnisFinal: trueby default;open,abstract, andoverrideall correctly returnfalse. Theif (isAbstract) isFinal = falsedomain invariant ingeneric.ts:142closes the only remaining corner case. Prior reviews calling this "inverted" were wrong.STATIC_OWNER_TYPESingeneric.ts:20: Correctly marks bothcompanion_objectandobject_declarationmembers asisStatic: truevia the owner-node check at line 145, independent of the language config'sisStaticcallback.- Unnamed companion object name resolution (
generic.ts:55-57): The"Companion"fallback matches Kotlin's JVM naming convention exactly. - Two-param
isAbstract(node, ownerNode): Correct design for interface implicit abstraction. Avoids threading external context. name:linecache key: Correct overload-safe design. No cross-file contamination possible.extractAnnotationsoptional: Forces intentional opt-in. Java and Kotlin both have it; the factory handlesundefinedcleanly with?? [].enrichedByMethodExtractorflag: Clean fallback path in parse-worker. No double AST walks.
Test Coverage Gaps (New)
The existing 23 tests cover all happy paths well. Gaps that would be caught by new tests:
| Missing Test | What It Would Reveal |
|---|---|
Kotlin extension function (fun String.foo()) |
No receiverType in schema (gap #1) |
| Java record compact constructor — parameter count | parameters: [] incorrect (gap #2) |
object_declaration isStatic: true |
Would confirm STATIC_OWNER_TYPES works correctly (positive confirmation) |
Java @NonNull String parameter type |
annotated_type text fallback behavior (gap #5) |
Kotlin override fun → isFinal: false |
Confirms override changes finality |
Summary
Both previous reviews were reviewing stale code and incorrectly identified multiple items as missing that are already correctly implemented. The current code on this branch is in substantially better shape than either prior review indicated.
Genuine remaining items:
| # | Gap | Severity | Action |
|---|---|---|---|
| 1 | Kotlin extension function receiverType absent from schema |
🟡 | Open tracking issue before schema hardens |
| 2 | compact_constructor_declaration yields parameters: [] |
🟡 | Comment in code + tracking issue |
| 3 | isAbstract/isFinal/annotations absent from ParsedSymbol |
🟡 | Deliberate decision point |
| 4 | findBodies fallback masks config typos |
🔵 | Add dev-mode assertion |
| 5 | annotated_type parameter type accuracy |
🔵 | Low impact, track for follow-up |
This PR is ready to merge. Items 1-2 warrant tracking issues (not blocking). Items 3-5 are appropriate for the follow-up RFC series as noted in the PR description.
…dSymbol fields - Add receiverType to MethodInfo for Kotlin extension functions (fun String.format() → receiverType: "String") - Extract compact constructor parameters from parent record_declaration - Add isAbstract/isFinal/annotations to ParsedSymbol for symbol table parity - Add dev-mode warning in findBodies fallback to catch config typos - Gap #5 (annotated_type) verified as non-issue: @nonnull String param correctly resolves to type_identifier "String" - Tests: extension functions, compact ctor params, receiverType null check (36 total, up from 34)
|
@claude ultra-think while identifying kotlin and java languages gaps. Perfom this analysis on this pull request and verify those findings. Act as a senior compiler font-end engineer and expert in static analysis tools and review this change from that perpective and review the architectural fit. Also check out previous comments and reason with their requests. |
|
Claude finished @magyargergo's task in 5m 43s —— View job Fourth-Pass Deep Analysis: Java/Kotlin Language Gaps (Ground-Truth)
|
| Prior Finding | Review 1 | Review 2 | Review 3 | Actual Status |
|---|---|---|---|---|
annotation_type_element_declaration missing |
🔴 critical | corrected (false) | corrected (false) | Present — jvm.ts:122, test at line 314 |
compact_constructor_declaration missing |
🔴 critical | corrected (false) | corrected (false) | Present — jvm.ts:121, parent-param traversal jvm.ts:52-57, test at line 332 |
Kotlin vararg always isVariadic: false |
🔴 critical | corrected (false) | corrected (false) | Present — parameter_modifiers scan jvm.ts:183-188, test at line 436 |
Primitive vararg type loss (int...) |
🔴 critical | corrected (false) | corrected (false) | Present — integral_type/floating_point_type/boolean_type at jvm.ts:89-91, test at line 353 |
| Enum constant body methods not extracted | 🔴 critical | 🔴 confirmed | corrected (false) | Present — generic.ts:125-133, test at line 292 |
companion_object absent from typeDeclarationNodes |
🔴 critical | 🔴 confirmed | corrected (false) | Present — jvm.ts:264 |
isFinal Kotlin semantics inverted |
🟡 | 🟡 confirmed | corrected (false) | Correct — jvm.ts:313-318: returns true by default, false for open/abstract/override |
ownerFqn misleading name |
🟡 | 🟡 confirmed | corrected (false) | Fixed — field is ownerName in method-types.ts:37 |
| Unnamed companion object ownerName | — | — | 🔴 new | Present — generic.ts:55-57 returns "Companion" |
companion_object ownerName: simple_identifier lookup |
— | — | 🔴 new | Present — generic.ts:46-50 scans for type_identifier|simple_identifier |
hasKeyword redundancy |
🔵 | 🔵 | corrected (false) | Eliminated — jvm.ts:144-160 uses hasModifier only |
FUNCTION_NODE_TYPES missing new node types |
— | 🔵 new | corrected (false) | Present — ast-helpers.ts:68-69 |
annotations missing from ParsedSymbol |
🔵 | 🔵 confirmed | 🔵 confirmed | FALSE — ParsedSymbol has isAbstract/isFinal/annotations at parse-worker.ts:125-127, propagated at lines 1844-1846 |
STATIC_OWNER_TYPES for companion/object |
— | 🟡 new | corrected (false) | Present — generic.ts:20, 152 |
receiverType for extension functions |
— | — | 🟡 new | Present — method-types.ts:19, jvm.ts:339-351, test at line 453 |
findBodies fallback masks typos |
🔵 | 🔵 | 🔵 confirmed | Addressed — dev-mode console.warn at generic.ts:88-91 |
Genuine Remaining Gaps (New Findings From This Pass)
🔴 Gap 1 — final override isFinal logic bug
jvm.ts:313-318
isFinal(node) {
if (hasModifier(node, 'modifiers', 'open')) return false;
if (hasModifier(node, 'modifiers', 'abstract')) return false;
if (hasModifier(node, 'modifiers', 'override')) return false; // ← bug
return true;
}override check runs before any final check. final override fun bar() is valid Kotlin — the method overrides and then seals the override chain. The current code returns isFinal: false for it because the override guard fires unconditionally. The correct priority order is: explicit final wins regardless of override.
isFinal(node) {
if (hasModifier(node, 'modifiers', 'final')) return true; // explicit final wins
if (hasModifier(node, 'modifiers', 'open')) return false;
if (hasModifier(node, 'modifiers', 'abstract')) return false;
if (hasModifier(node, 'modifiers', 'override')) return false;
return true;
}final override is uncommon but architecturally important for sealing inheritance hierarchies (e.g. framework hooks). No test covers this case. Fix this →
🟡 Gap 2 — Kotlin secondary_constructor not in methodNodeTypes
jvm.ts:265
methodNodeTypes: ['function_declaration'],secondary_constructor is a distinct node type in tree-sitter-kotlin (fwcd). It is not function_declaration. Kotlin classes with explicit constructor overloads silently lose them:
class User(val name: String) {
constructor(name: String, age: Int) : this(name) { ... } // NEVER EXTRACTED
constructor() : this("Guest") { } // NEVER EXTRACTED
}The body is class_body, already in bodyNodeTypes. The fix is adding 'secondary_constructor' to methodNodeTypes and handling its name extraction (secondary constructors don't have a name field — the name should be the enclosing class name, same as Java constructor extraction). This is a silent data-loss gap for DI-heavy Kotlin codebases where secondary constructors are common. Fix this →
🔵 Gap 3 — Unnamed companion object methods get HAS_METHOD edges from outer class
Integration: ast-helpers.ts:264-277 × parse-worker.ts:1826-1827
findEnclosingClassId (for HAS_METHOD edges) walks up the AST looking for CLASS_CONTAINER_TYPES. For unnamed companion_object, it finds the node but then can't resolve a name (childForFieldName('name') → null, no type_identifier child). So it returns null and continues up to the outer class_declaration. Result: fun create(): Foo inside an unnamed companion gets a HAS_METHOD edge from the outer class (e.g., UserService), not from a companion node.
This is separate from MethodExtractor (which correctly extracts methods and sets isStatic: true). The isStatic: true flag plus the outer-class ownership is arguably the right JVM-semantic representation of unnamed companion methods — but it is inconsistent with named companions, which get their own node. Worth documenting as intentional or fixing. No test covers this behavior path.
🔵 Gap 4 — annotated_type Java parameter type loss
jvm.ts:65-70
const typeNode = param.childForFieldName('type');
// typeNode for `@NonNull String name` is `annotated_type` in tree-sitter-java
// extractSimpleTypeName(annotated_type) → likely falls back to annotated_type.text = "@NonNull String"For Spring/Jakarta codebases with pervasive @NonNull/@Valid annotations on parameters, this produces type strings like "@NonNull String" instead of "String". Impact is highest on type-based disambiguation (call resolution using parameterTypes). Third review noted this as gap #5. Fixable by unwrapping annotated_type before calling extractSimpleTypeName. Fix this →
Verified Status of Previous Review Items
annotations in ParsedSymbol — All Three Reviews Were Wrong
Review 1 identified this as a gap. Reviews 2 and 3 confirmed it. All three were incorrect.
ParsedSymbol does include isAbstract?, isFinal?, annotations? at parse-worker.ts:125-127. They are propagated to result.symbols at parse-worker.ts:1844-1846. This was addressed before the first review ran.
Companion Object Integration Path — Works End-to-End
Previous reviews raised uncertainty about whether unnamed companion objects work through the parse-worker. The actual integration path for all companion objects (named and unnamed):
function_declarationinside any companion object body is captured by the generic function query (tree-sitter-queries.ts:863-864)- Parse-worker calls
findEnclosingClassNode(definitionNode)which walks up tocompanion_object(inCLASS_CONTAINER_TYPESatast-helpers.ts:127) getMethodInfo(companion_object, ...)callsprovider.methodExtractor.extract(companion_object, ...)- MethodExtractor finds
companion_objectintypeDeclarationNodes✅, extracts functions, setsisStatic: trueviaSTATIC_OWNER_TYPES
The named-only companion query (tree-sitter-queries.ts:858-860) controls whether the companion gets its own graph node — it doesn't affect method extraction, which happens via the function query path above.
isFinal Kotlin Semantics — Third Review Was Correct, Others Wrong
The third review correctly identified this as already fixed. The current implementation (jvm.ts:313-318) is correct for the documented behavior: returns true by default, false for open/abstract/override. The first review calling this "inverted" was analyzing stale code. The only remaining issue is the final override edge case (#1 above).
Architecture Fit — Compiler Front-End Perspective
Strong Design Choices
Config + factory (no class hierarchy): Exactly right. The JVM config would be identical whether structured as a base class or a config object. The factory removes the protected abstract boilerplate pattern that plagues base-class hierarchies.
Two-param isAbstract(node, ownerNode): The cleanest possible signature for the "implicit abstraction" pattern. Java interface methods, Kotlin interface methods, annotation type elements — all require owner context, and both get it without threading a stateful context object.
name:line cache key: The composite key is the only correct key for Java/Kotlin. Name-only keys would silently merge overloads. startIndex on the map-of-maps (keyed per class node) prevents cross-file contamination. The methodInfoCache.clear() at parse-worker.ts:330 correctly scopes it to per-file lifetime.
STATIC_OWNER_TYPES in generic.ts, not in language configs: JVM semantics (companion_object → static) shouldn't be each config's responsibility. Centralizing in the factory is the right layer.
extractReceiverType?: (node) => string | undefined: Optional in the config interface, meaning non-Kotlin languages never pay for it. The ?? null at generic.ts:156 correctly normalizes to null for the MethodInfo.receiverType field.
if (isAbstract) isFinal = false invariant at generic.ts:149: Domain invariant enforced at the factory level, not in each config. No config can accidentally produce isAbstract: true, isFinal: true regardless of implementation. This is exactly how a compiler IR would model it.
One Design Note
The bodyNodeTypes fallback path (generic.ts:85-94) correctly adds a dev-mode console.warn. For a future production scenario, this should be a structured log rather than console.warn. Not a bug, but noted for when the logging infrastructure is standardized.
Verified Priority Table
| # | Gap | Language | Severity | In Prior Reviews? |
|---|---|---|---|---|
| 1 | final override fun → isFinal: false (should be true) |
Kotlin | 🔴 Logic bug | ✗ New |
| 2 | secondary_constructor not in methodNodeTypes |
Kotlin | 🟡 Silent data loss | ✗ New |
| 3 | Unnamed companion HAS_METHOD edges go to outer class | Kotlin | 🔵 Arch inconsistency | Partially (reviews discussed companion but missed this sub-issue) |
| 4 | annotated_type parameter type includes annotation text |
Java | 🔵 Data quality | ✓ Review 3 gap #5 |
| 5 | suspend modifier not captured |
Kotlin | 🟡 Schema gap | ✓ All reviews |
| 6 | throws clause not captured |
Java | 🟡 Schema gap | ✓ All reviews |
| 7 | operator/inline/tailrec modifiers |
Kotlin | 🔵 Schema gap | ✓ Review 3 |
Items 1 and 2 are the only new actionable findings. Item 1 is a correctness bug; item 2 is silent data loss. Items 3-7 are appropriate follow-ups in the RFC series as the PR description notes.
This PR is ready to merge after addressing gap #1 (trivial one-line fix) or tracking it as a known issue. Gap #2 (secondary constructors) is a clear follow-up scope for the RFC series.
|
We'll come back to it! Tracking it here as a known gap. |
Add C# method extraction config mirroring the JVM pattern from PR #576. Wire csharpMethodConfig into the C# language provider and add 18 tests covering classes, interfaces, abstract classes, structs, records, constructors, params/out/ref/optional parameters, sealed methods, attributes, and visibility modifiers.
* feat(csharp): add C# MethodExtractor config (#573) Add C# method extraction config mirroring the JVM pattern from PR #576. Wire csharpMethodConfig into the C# language provider and add 18 tests covering classes, interfaces, abstract classes, structs, records, constructors, params/out/ref/optional parameters, sealed methods, attributes, and visibility modifiers. * fix(csharp): add destructor, operator, conversion operator, and in-param support - Add destructor_declaration, operator_declaration, and conversion_operator_declaration to methodNodeTypes - Custom extractName for operators (e.g., "operator +", "implicit operator double") - Fix extractReturnType for operator declarations (use type field, not returns) - Add in modifier to parameter extraction (alongside out/ref) - Add 4 new tests: destructor, operator+, implicit conversion, in parameter * fix(csharp): add ref param test and document compound visibility limitation - Add test for ref parameter modifier (was only testing out) - Document that protected internal / private protected resolve to first modifier * feat(csharp): support compound visibilities (protected internal, private protected) - Add 'protected internal' and 'private protected' to FieldVisibility union - Detect compound modifiers in both C# method and field extractors via collectModifierTexts helper scanning adjacent modifier nodes - Add 2 tests for compound visibility detection * feat(csharp): primary constructors, virtual/override/async, primary fields Address all known limitations from review: - Primary constructor support (C# 12): add extractPrimaryConstructor to MethodExtractionConfig and extractPrimaryFields to FieldExtractionConfig. Record params become public readonly properties; class params become private captured fields. - Add isVirtual, isOverride, isAsync optional fields to MethodInfo, MethodExtractionConfig, NodeProperties, and parse-worker propagation. - Detect virtual/override/async modifiers in C# method config. - Move collectModifierTexts to shared helpers.ts (deduplicate). - Fix destructor name to ~ClassName (disambiguates from constructor). - Add expression-bodied method test. - 118 tests total across method + field extraction suites, all passing. * fix(csharp): review round 2 — annotations, record_struct, grammar pin - Fix primary constructor annotations: use [] instead of extracting class-level attributes (C# has no syntax for ctor-specific attributes) - Add record_struct_declaration to typeDeclarationNodes in both method and field extractors, CLASS_CONTAINER_TYPES, and isRecord visibility check - Pin tree-sitter-c-sharp version (^0.23.1) in params comment * fix(csharp): complete record_struct query + label mapping, sealed override test - Add record_struct_declaration capture patterns to tree-sitter-queries.ts (type definition + primary constructor) - Add record_struct_declaration → 'Struct' in CONTAINER_TYPE_TO_LABEL - Assert isOverride: true alongside isFinal in sealed override test * fix(csharp): record_struct label mismatch, add record struct + documented limitation tests - Fix record_struct_declaration query tag: @Definition.struct (not @definition.record) to match CONTAINER_TYPE_TO_LABEL and prevent broken HAS_METHOD edges - Add 3 record struct tests: isTypeDeclaration, method extraction, primary constructor - Add documented limitation tests: partial method (isAbstract: false), generic type parameter stripping (name excludes <T>) * fix(csharp): remove record_struct_declaration — not a real tree-sitter node type tree-sitter-c-sharp 0.23.1 parses 'record struct' as record_declaration (absorbs the 'struct' keyword as an unnamed child token). The non-existent record_struct_declaration in queries caused TSQueryErrorNodeType, breaking ALL C# file processing. Remove from: tree-sitter-queries.ts, typeDeclarationNodes in both extractors, CLASS_CONTAINER_TYPES, and CONTAINER_TYPE_TO_LABEL. Record struct types are already handled via record_declaration. * feat(csharp): add isPartial support, filter targeted attributes, static ctor test - Add isPartial optional field to MethodInfo, MethodExtractionConfig, NodeProperties, and parse-worker propagation pipeline - Detect partial modifier in C# config — marks both declaration-only and implemented partial methods - Filter targeted attribute lists (e.g. [return: MarshalAs(...)]) in extractCSharpAnnotations — only untargeted attributes collected - Add static constructor test (isStatic: true, same name as class) - Add 3 partial method tests: declaration-only, with body, coexisting pair - Document record_struct/record_class as defensive dead code in export-detection.ts (grammar absorbs keywords into record_declaration) * fix(csharp): this param for extension methods, dedup visibility, test fixes - Handle this modifier on extension method parameters (type prefixed as 'this string', consistent with out/ref/in handling) - Deduplicate visibility logic in extractPrimaryConstructor — reuse csharpMethodConfig.extractVisibility instead of inline compound check - Fix record struct test title to reflect actual grammar behavior - Add conversion operator returnType assertion - Add extension method this parameter test * fix(csharp): primary constructor line points to param list, empty name guard - Use paramList.startPosition instead of ownerNode.startPosition for primary constructor line number (avoids methodInfoCache key collision) - Guard against empty param names from tree-sitter error recovery nodes
* main: (114 commits) feat(csharp): C# MethodExtractor config (abhigyanpatwari#582) docs: add gitnexus-shared build step before gitnexus-web (abhigyanpatwari#585) chore: add enterprise offering section to README, ignore local_docs/ (abhigyanpatwari#579) fix(eval): exclude litellm 1.82.7 and 1.82.8 due to compatibility issues (abhigyanpatwari#580) feat(java,kotlin): MethodExtractor abstraction with per-language configs (abhigyanpatwari#576) feat: added skip-agents-md cli flag (abhigyanpatwari#517) feat(wiki): Azure OpenAI support for wiki command (abhigyanpatwari#562) refactor: reduce explicit any types (abhigyanpatwari#566) feat(java): method references, worker overload disambiguation, interface dispatch (abhigyanpatwari#540) feat: configure eslint with unused import removal (abhigyanpatwari#564) feat: configure prettier with pre-commit hook (abhigyanpatwari#563) feat: unify web and cli ingestion pipeline (abhigyanpatwari#536) fix/opencode mcp gitnexus timeout (abhigyanpatwari#363) chore: bump version to 1.4.10, update CHANGELOG fix: resolve tree-sitter peer dependency conflicts (abhigyanpatwari#538) chore: bump version to 1.4.9, add CHANGELOG.md refactor: Phase 8 & 9 — Field Types and Return-Type Binding (abhigyanpatwari#494) feat: add COBOL language support with regex extraction pipeline (abhigyanpatwari#498) fix: close remaining Dart language support gaps (abhigyanpatwari#524) refactor: split global BUILT_IN_NAMES into per-language provider fields (abhigyanpatwari#523) ... # Conflicts: # gitnexus/src/core/wiki/llm-client.ts
* feat(csharp): add C# MethodExtractor config (abhigyanpatwari#573) Add C# method extraction config mirroring the JVM pattern from PR abhigyanpatwari#576. Wire csharpMethodConfig into the C# language provider and add 18 tests covering classes, interfaces, abstract classes, structs, records, constructors, params/out/ref/optional parameters, sealed methods, attributes, and visibility modifiers. * fix(csharp): add destructor, operator, conversion operator, and in-param support - Add destructor_declaration, operator_declaration, and conversion_operator_declaration to methodNodeTypes - Custom extractName for operators (e.g., "operator +", "implicit operator double") - Fix extractReturnType for operator declarations (use type field, not returns) - Add in modifier to parameter extraction (alongside out/ref) - Add 4 new tests: destructor, operator+, implicit conversion, in parameter * fix(csharp): add ref param test and document compound visibility limitation - Add test for ref parameter modifier (was only testing out) - Document that protected internal / private protected resolve to first modifier * feat(csharp): support compound visibilities (protected internal, private protected) - Add 'protected internal' and 'private protected' to FieldVisibility union - Detect compound modifiers in both C# method and field extractors via collectModifierTexts helper scanning adjacent modifier nodes - Add 2 tests for compound visibility detection * feat(csharp): primary constructors, virtual/override/async, primary fields Address all known limitations from review: - Primary constructor support (C# 12): add extractPrimaryConstructor to MethodExtractionConfig and extractPrimaryFields to FieldExtractionConfig. Record params become public readonly properties; class params become private captured fields. - Add isVirtual, isOverride, isAsync optional fields to MethodInfo, MethodExtractionConfig, NodeProperties, and parse-worker propagation. - Detect virtual/override/async modifiers in C# method config. - Move collectModifierTexts to shared helpers.ts (deduplicate). - Fix destructor name to ~ClassName (disambiguates from constructor). - Add expression-bodied method test. - 118 tests total across method + field extraction suites, all passing. * fix(csharp): review round 2 — annotations, record_struct, grammar pin - Fix primary constructor annotations: use [] instead of extracting class-level attributes (C# has no syntax for ctor-specific attributes) - Add record_struct_declaration to typeDeclarationNodes in both method and field extractors, CLASS_CONTAINER_TYPES, and isRecord visibility check - Pin tree-sitter-c-sharp version (^0.23.1) in params comment * fix(csharp): complete record_struct query + label mapping, sealed override test - Add record_struct_declaration capture patterns to tree-sitter-queries.ts (type definition + primary constructor) - Add record_struct_declaration → 'Struct' in CONTAINER_TYPE_TO_LABEL - Assert isOverride: true alongside isFinal in sealed override test * fix(csharp): record_struct label mismatch, add record struct + documented limitation tests - Fix record_struct_declaration query tag: @Definition.struct (not @definition.record) to match CONTAINER_TYPE_TO_LABEL and prevent broken HAS_METHOD edges - Add 3 record struct tests: isTypeDeclaration, method extraction, primary constructor - Add documented limitation tests: partial method (isAbstract: false), generic type parameter stripping (name excludes <T>) * fix(csharp): remove record_struct_declaration — not a real tree-sitter node type tree-sitter-c-sharp 0.23.1 parses 'record struct' as record_declaration (absorbs the 'struct' keyword as an unnamed child token). The non-existent record_struct_declaration in queries caused TSQueryErrorNodeType, breaking ALL C# file processing. Remove from: tree-sitter-queries.ts, typeDeclarationNodes in both extractors, CLASS_CONTAINER_TYPES, and CONTAINER_TYPE_TO_LABEL. Record struct types are already handled via record_declaration. * feat(csharp): add isPartial support, filter targeted attributes, static ctor test - Add isPartial optional field to MethodInfo, MethodExtractionConfig, NodeProperties, and parse-worker propagation pipeline - Detect partial modifier in C# config — marks both declaration-only and implemented partial methods - Filter targeted attribute lists (e.g. [return: MarshalAs(...)]) in extractCSharpAnnotations — only untargeted attributes collected - Add static constructor test (isStatic: true, same name as class) - Add 3 partial method tests: declaration-only, with body, coexisting pair - Document record_struct/record_class as defensive dead code in export-detection.ts (grammar absorbs keywords into record_declaration) * fix(csharp): this param for extension methods, dedup visibility, test fixes - Handle this modifier on extension method parameters (type prefixed as 'this string', consistent with out/ref/in handling) - Deduplicate visibility logic in extractPrimaryConstructor — reuse csharpMethodConfig.extractVisibility instead of inline compound check - Fix record struct test title to reflect actual grammar behavior - Add conversion operator returnType assertion - Add extension method this parameter test * fix(csharp): primary constructor line points to param list, empty name guard - Use paramList.startPosition instead of ownerNode.startPosition for primary constructor line number (avoids methodInfoCache key collision) - Guard against empty param names from tree-sitter error recovery nodes

Summary
Closes #569 (PR 1 of RFC #568)
createMethodExtractor()driven byMethodExtractionConfigisAbstract/isFinaldetection, structuredParameterInfo, annotations, visibilitymethodInfoCache(keyed byname:line, not name alone)isAbstract,isFinal,annotationsadded toNodePropertiesin gitnexus-sharedNew Files (3)
method-types.tsMethodInfo,ParameterInfo,MethodExtractionConfig,MethodExtractor,ExtractedMethodsmethod-extractors/generic.tscreateMethodExtractor()factorymethod-extractors/configs/jvm.tsModified Files (5)
gitnexus-shared/src/graph/types.tsisAbstract?,isFinal?,annotations?on NodePropertieslanguage-provider.tsmethodExtractor?: MethodExtractoron LanguageProviderConfigworkers/parse-worker.tsmethodInfoCache,getMethodInfo(), Method/Function enrichment with MethodExtractor-first fallbacklanguages/java.tsmethodExtractorlanguages/kotlin.tsmethodExtractorKey Design Decisions
BaseMethodExtractorclass (11/12 field extractors use the factory, base class is dead weight)isAbstract(node, ownerNode)— two-param signature needed for interface method detection (Java bodyless methods, Kotlin interface keyword child)Map<string, MethodInfo>keyed byname:linecomposite, not name alone, to support Java/Kotlin method overloadingextractMethodSignature(), avoiding double AST walksMethodExtractorContextomitstypeEnv/symbolTable(methods don't need type resolution)Schema Note
isAbstract,isFinal,annotationsare populated in the in-memory graph but not yet persisted to LadybugDB — the METHOD_SCHEMA and CSV generator need column additions in a follow-up PR.Test Plan
tsc --noEmitcleanPost-Deploy Monitoring & Validation
No additional operational monitoring required: this is a build-time ingestion enrichment with no runtime API surface change. Validate by running
gitnexus analyzeon a Java repo and checking Method node properties viamcp__gitnexus__cypher.