feat(csharp): C# MethodExtractor config#582
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 4753 tests passed 46 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
…ram 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
…tation - Add test for ref parameter modifier (was only testing out) - Document that protected internal / private protected resolve to first modifier
…ate 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
C# MethodExtractor Coverage ReportComplete C# Method-Like Construct Coverage
Parameter Handling
Visibility
Intentionally Out of Scope
Test Coverage62 tests total (37 existing Java/Kotlin + 25 new C#) — all passing. Files Changed
|
Pipeline Integration VerificationTraced the end-to-end integration from language provider through parse-worker to confirm the C# method extractor is fully wired into the ingestion pipeline. Chain of executionVerification points
ConclusionNo additional wiring needed. The existing parse-worker infrastructure is generic — any language provider with a |
|
@claude ultra-think while identifying C# 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 39s —— View job C# MethodExtractor — Senior Compiler Front-End ReviewTasks
OverviewSolid, well-structured implementation that follows the established JVM pattern faithfully. The config-driven factory approach is a good architectural choice. Below is a deep-dive from a compiler front-end / static-analysis perspective, organized by severity. Verified Previous CommentsPipeline integration trace (from
Coverage table (from Bugs / Correctness Issues1. Primary Constructors (C# 12) — Silently Missed
The tree-sitter query file already captures this at ...but the method extractor config has no corresponding handler. The coverage table in the PR comment claims "Constructors (instance + static)" are supported — primary constructors are a counterexample. 2. Destructor Name Ambiguity
const dtor = result!.methods.find((m) => m.name === 'Resource');From a static analysis perspective, a destructor and a constructor are semantically distinct and should be distinguishable by name. The de-facto convention is to either prefix with 3.
|
| Scenario | Covered | Notes |
|---|---|---|
Primary constructor class Point(int x, int y) |
❌ | Silently not extracted |
| Destructor name disambiguation | Tested but extracts ambiguous name | |
Expression-bodied method (=> expr) |
❌ | No test at all |
virtual method |
❌ | Not modeled |
override method |
❌ | Not modeled |
Generic method T GetValue<T>() |
❌ | Name strips type params |
partial method (declaration vs impl) |
❌ | Behavior undefined |
async method |
❌ | Not modeled |
| Nested class — outer vs inner attribution | ❌ | Worth explicit test |
params grammar version change resilience |
❌ | Fragile assumption |
Summary
The core implementation is correct and well-tested for the declared scope. The JVM pattern was followed faithfully, the wiring is verified, and the compound visibility handling is a thoughtful addition. The main actionable items are:
- Fix:
collectModifierTextsduplication → move tohelpers.ts - Fix: Destructor name should be
~Resource, notResource - Document/test: Primary constructor gap (C# 12)
- Document:
paramsgrammar version dependency - Document:
virtual,override,async,partial, generic type params as known limitations in a code comment
The coverage table in the PR body is aspirational in a few areas (primary constructors, some modifier semantics) — adjusting it to reflect the actual coverage would keep the documentation honest.
· Branch
…ields 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.
|
@claude ultra-think while identifying C# 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 56s —— View job C# MethodExtractor — Senior Compiler Front-End ReviewTasks
Context: Previous Review vs. Current StateThe previous Claude review (in the comments) identified several issues. I read the current source after all 5 commits. Here is the verified status of each finding, plus new findings from this deep-dive. ✅ All Previous Findings Addressed
The response rate on the previous review is excellent — every actionable item was acted on. New Findings1. Bug: Primary Constructor Annotations Are Contaminated by Class-Level Attributes
[Serializable]
public class Point(int x, int y) { }
// → primary constructor gets annotations: ['@Serializable']
// but @Serializable belongs to the class, not the constructorIn C#, there is no syntax to place an attribute directly on a primary constructor (unlike parameters, which use The primary constructor tests at lines 1052–1093 don't assert on 2. Consistency Gap:
|
| Scenario | Status | Notes |
|---|---|---|
Primary constructor class Point(int x, int y) |
✅ Extracted | Via extractPrimaryConstructor |
| Primary constructor annotations | Class-level attributes bleed into ctor annotations | |
Destructor ~Resource naming |
✅ Fixed | ~ prefix in extractName |
virtual / override / async |
✅ Modeled | Optional fields on MethodInfo |
Expression-bodied => expr |
✅ Covered | Same AST node type, works naturally |
collectModifierTexts deduplication |
✅ Fixed | In helpers.ts |
record struct (C# 10) |
❌ Missing | Not in queries, extractors, or CLASS_CONTAINER_TYPES |
partial method declaration |
isAbstract: false, no test documents this |
|
Generic method T GetValue<T>() |
Type params stripped, no comment | |
params grammar version resilience |
Version not pinned in comment |
Summary
The implementation is solid. The previous review's six actionable items were all addressed cleanly. The three items that warrant action now:
- Fix (low-effort): Empty
annotations: []for primary constructors — class attributes should not bleed onto the constructor - Fix or document (medium):
record_struct_declaration— either add it to the extractor chain and tree-sitter queries, or add a// record struct not yet supportedcomment intypeDeclarationNodescross-referencingexport-detection.ts:76 - Comment (low-effort): Pin grammar version in the
paramscomment block, and add skipped/documented tests forpartialand generic type params
· Branch
- 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
|
@claude ultra-think while identifying C# 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 3m 45s —— View job C# MethodExtractor — Round 3 Deep ReviewTasks
Verified Status of Round 2 Findings
Finding 1 —
|
| Scenario | Status | Notes |
|---|---|---|
| Primary constructor (C# 12) — class/record | ✅ Fixed | extractPrimaryConstructor |
| Primary constructor annotations | ✅ Fixed | Hardcoded [] |
Destructor ~Resource naming |
✅ Fixed | ~ prefix in extractName |
| virtual / override / async | ✅ Fixed | Optional fields on MethodInfo |
Expression-bodied => expr |
✅ Covered | Same AST node type |
collectModifierTexts deduplication |
✅ Fixed | In helpers.ts |
params grammar version comment |
✅ Fixed | "verified against ^0.23.1" |
sealed override compound test |
isFinal tested; isOverride not verified in same case |
|
record_struct_declaration extractors |
✅ Fixed | In method/field configs and CLASS_CONTAINER_TYPES |
record_struct_declaration query |
❌ Missing | No tree-sitter capture pattern — dead code |
record_struct_declaration label mapping |
❌ Missing | Falls to 'Class' in CONTAINER_TYPE_TO_LABEL |
record_struct_declaration tests |
❌ Missing | No isTypeDeclaration, no method, no primary ctor test |
record_class_declaration (pre-existing) |
❌ Undocumented | In export-detection only; rest of pipeline blind to it |
partial method declaration |
isAbstract: false, no test documents this |
|
Generic method T GetValue<T>() |
Type params stripped, no comment |
Summary
The implementation is solid and the three actionable items from round 2 have been addressed cleanly. The main remaining gap is record_struct_declaration — the extractor configs were updated, but without a tree-sitter query to capture record struct as a definition node, the fix is effectively unreachable in production. The label mapping omission compounds this.
Prioritized action items:
- Must fix: Add
(record_struct_declaration ...)capture pattern totree-sitter-queries.ts+ addrecord_struct_declaration → 'Struct'toCONTAINER_TYPE_TO_LABEL - Should fix:
sealed overridetest should assert bothisFinalandisOverride - Nice to have: Documented skip tests for
partialand generic type params; comment onrecord_class_declarationgap
· Branch
…rride 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
|
@claude ultra-think while identifying C# 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 9s —— View job C# MethodExtractor — Round 4 Deep ReviewTasks
Verified Status of Round 3 Findings
Finding 1 — Label Mismatch:
|
| Scenario | Status | Notes |
|---|---|---|
| Primary constructors (class/record) | ✅ | extractPrimaryConstructor at csharp.ts:267 |
| Primary constructor annotations | ✅ | Hardcoded [] at csharp.ts:307 |
Destructor ~Resource naming |
✅ | ~ prefix at csharp.ts:185–187 |
virtual / override / async |
✅ | Optional fields in method-types.ts |
Expression-bodied => expr |
✅ | Same AST node |
collectModifierTexts deduplication |
✅ | In helpers.ts:157 |
params grammar version comment |
✅ | "verified against ^0.23.1" at csharp.ts:30 |
sealed override test completeness |
✅ | Both isFinal and isOverride asserted at line 745–746 |
record_struct_declaration extractor configs |
✅ | In typeDeclarationNodes and CLASS_CONTAINER_TYPES |
record_struct_declaration tree-sitter query |
✅ | tree-sitter-queries.ts:561, 579 |
record_struct_declaration — label mismatch |
❌ Bug | @definition.record → 'Record', but CONTAINER_TYPE_TO_LABEL → 'Struct'; HAS_METHOD edges broken |
record_struct_declaration tests |
❌ | No isTypeDeclaration, method, or primary ctor test |
record_class_declaration |
In export-detection.ts:77 only; no real tree-sitter node |
|
partial method declaration |
isAbstract: false, no documented test |
|
| Generic method type params stripped | No comment in extractName |
Summary
One new bug to fix before merge:
- Change
@definition.record→@definition.structintree-sitter-queries.ts:561forrecord_struct_declaration— the current mismatch withCONTAINER_TYPE_TO_LABELsilently breaks HAS_METHOD edge creation for allrecord structtypes.
Two quick additions:
2. Add three record struct tests (would have caught bug #1)
3. Add documented skip tests or comments for partial and generic type params
The rest of the implementation is solid and all six items from the Round 1 review plus the three from Round 2 have been cleanly addressed.
· Branch
…nted 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>)
|
@claude ultra-think while identifying C# 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 24s —— View job C# MethodExtractor — Round 5 Deep ReviewTasks
Round 4 Findings — Verified Status
All five actionable items from Round 4 are addressed. The sole carry-over is the Finding 1 —
|
| Field | Value |
|---|---|
name |
'Foo' |
isStatic |
true |
returnType |
null |
parameters |
[] |
visibility |
'private' (default — static ctors have no visibility modifier) |
An instance constructor public Foo(int x) produces name: 'Foo' with isStatic: false. Both live in the same methods array distinguished only by isStatic and line. This is acceptable for call-graph analysis (static constructors are rarely called explicitly), but there is no test documenting this behavior. If a future refactor changed the naming convention for static constructors (e.g. prefixing with static), the breakage would be silent.
A one-line documented test would anchor the behavior:
it('static constructor is extracted as isStatic: true, same name as class', () => {
const tree = parseCSharp(`
public class Config { static Config() { } }
`);
const result = extractor.extract(tree.rootNode.child(0)!, csharpCtx);
const ctor = result!.methods.find(m => m.name === 'Config');
expect(ctor!.isStatic).toBe(true);
expect(ctor!.parameters).toHaveLength(0);
});Finding 3 — partial Method Pair Produces Two Entries (Undocumented)
The "documented limitations" test at method-extraction.test.ts:1186 tests a declaration-only partial void OnChanged() correctly. However, when both the declaration and the implementation coexist in the same declaration_list:
public partial class Foo {
partial void OnChanged(); // ← declaration
partial void OnChanged() { } // ← implementation
}Both method_declaration nodes are traversed in extractMethodsFromBody. The result would be two MethodInfo entries both named OnChanged at different source lines. For call-graph purposes this is harmless (the method cache uses name:line), but for consumers that enumerate methods by name, the duplication is unexpected.
A test documenting that the pair produces two entries — or a comment in the existing test — would prevent confusion:
// note: if both declaration and implementation of a partial method appear in
// the same declaration_list, two MethodInfo entries are produced (one per node).
// Deduplication is the caller's responsibility.Finding 4 — record_class_declaration Dead Code (No Comment, Carry-Over)
export-detection.ts:77 contains 'record_class_declaration' in the type detection set. This appears nowhere else in the codebase (not in queries, not in extractors, not in CLASS_CONTAINER_TYPES). In tree-sitter-c-sharp, both record Foo {} and record class Foo {} produce a record_declaration AST node — the optional class keyword is absorbed by the grammar. The record_class_declaration entry in export-detection.ts never matches a real tree-sitter node and is effectively dead code.
A single comment would prevent future engineers from trying to "fix" this by adding the missing support:
// 'record_class_declaration' is listed defensively. In tree-sitter-c-sharp, the
// optional 'class' keyword in 'record class Foo {}' is absorbed into a regular
// 'record_declaration' node — no separate 'record_class_declaration' node is
// emitted by the grammar in practice.
'record_class_declaration',Architectural Observations (Persistent)
These were raised in prior reviews and remain unaddressed. Both are low-severity, but documenting them in code comments would benefit future language authors:
STATIC_OWNER_TYPES JVM contamination (generic.ts:20): The constant new Set(['companion_object', 'object_declaration']) is evaluated on every buildMethod call for all language configs, including C#. These types never appear in C# ASTs, so there is no runtime impact. A comment would clarify intent:
// JVM-specific: companion_object / object_declaration members (Kotlin) are static by convention.
// Other language configs never produce these owner node types.
const STATIC_OWNER_TYPES = new Set(['companion_object', 'object_declaration']);MethodVisibility = FieldVisibility (method-types.ts:8): The alias allows 'fileprivate' (Swift), 'open' (Swift), and 'package' (Java) to be valid MethodVisibility values in the C# extractor's type signature. The C# config never produces these values, so there is no runtime impact. A narrower per-language visibility type (type CSharpMethodVisibility = 'public' | 'private' | 'protected' | 'internal' | 'protected internal' | 'private protected') would make the type system enforce C#-validity, but this is a larger refactor given the shared type alias pattern.
Coverage Table — Final State
| Scenario | Status | Notes |
|---|---|---|
| Regular methods (all modifiers) | ✅ | Covered |
| Constructors (instance) | ✅ | Covered |
| Static constructors | Extracts correctly but no test; name collision with instance ctor documented only by isStatic |
|
Destructors (~Resource) |
✅ | ~ prefix in extractName |
| Operator overloads | ✅ | Covered |
| Conversion operators | ✅ | Covered |
| Expression-bodied members | ✅ | Same AST node |
| Abstract / sealed / static / virtual / override / async | ✅ | All modeled and tested |
| Primary constructors (C# 12) | ✅ | extractPrimaryConstructor |
| Primary constructor annotations | ✅ | Hardcoded [] |
| Generic methods (type params stripped) | ✅ Documented | Test at line 1203 |
partial method (declaration-only) |
✅ Documented | Test at line 1186 |
partial method (declaration + impl pair) |
Two entries produced, undocumented | |
params variadic |
✅ | Grammar version pinned in comment |
out / ref / in parameters |
✅ | Covered |
| Optional parameters | ✅ | Covered |
| Attribute annotations | [return: Attr] and [method: Attr] targets bleed into method annotations |
|
| Compound visibility | ✅ | protected internal, private protected |
record_struct_declaration |
✅ | Query, extractors, label mapping, tests all fixed |
record_class_declaration |
No comment explaining it never matches a real node |
Summary
After four rounds of feedback, the implementation is in excellent shape. The response rate on each review has been exemplary — all six actionable items from Round 1, all three from Round 2, and all five from Round 4 have been addressed.
One genuine bug remains:
- Fix:
extractCSharpAnnotationsincorrectly collects[return: Attr]-targeted attributes as method annotations — filterattribute_listnodes with anattribute_target_specifierchild
Two low-effort additions would complete the coverage documentation:
2. Test: Static constructor behavior (name collision with instance ctor, isStatic: true)
3. Comment: partial method declaration+implementation pair produces two entries; record_class_declaration is dead code in export-detection.ts
· Branch
…r 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.
…ic 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)
|
@claude ultra-think while identifying C# 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.. You must additionally verify what version of csharp tree-sitter we use and verify when you claim a new language features must be covered. |
|
Claude finished @magyargergo's task in 7m 10s —— View job C# MethodExtractor — Round 6 Deep ReviewTasks
Grammar Version BaselineInstalled version: Round 5 Findings — Verified Status
All six actionable items from Round 5 have been addressed. The response rate across all six rounds has been exemplary. Grammar Verification: The
|
| Scenario | Status | Notes |
|---|---|---|
| Regular methods (all modifiers) | ✅ | |
| Constructors (instance) | ✅ | |
| Static constructors | ✅ | Documented test: same name, isStatic: true |
Destructors (~Resource) |
✅ | ~ prefix in extractName |
| Operator overloads | ✅ | |
| Conversion operators | ✅ | |
Expression-bodied => expr |
✅ | Same AST node |
| abstract / sealed / static / virtual / override / async | ✅ | All modeled and tested |
isPartial |
✅ | Added in fb1e007; 3 tests |
| Primary constructors (C# 12 class/record) | ✅ | extractPrimaryConstructor |
| Primary constructor annotations | ✅ | Hardcoded [] |
| Generic methods (type params stripped) | ✅ Documented | Test at line 1267 |
partial method declaration-only |
✅ Documented | isAbstract: false documented at line 1226 |
partial declaration + impl pair |
✅ Documented | Two-entry behaviour documented at line 1244 |
params variadic |
✅ | Grammar quirk documented |
out / ref / in parameters |
✅ | Prefixed type strings |
this parameter (extension methods) |
❌ Missing | this modifier silently dropped; type becomes bare type string |
Targeted attribute lists [return: Attr] |
✅ Fixed | Filtered in extractCSharpAnnotations |
| Compound visibility | ✅ | protected internal, private protected |
record struct via record_declaration |
✅ Correct | Grammar-verified; no separate node type in ^0.23.1 |
record struct test title accuracy |
Title says record_struct_declaration; grammar emits record_declaration |
|
record_class_declaration dead code |
✅ Documented | Comment in export-detection.ts:76–78 |
params semver precision |
"verified against ^0.23.1" should state exact resolved version |
Summary
After six rounds of review, the implementation is in excellent shape. All five items from Round 5 have been addressed, and the record_struct_declaration story was correctly resolved with empirical grammar verification rather than assumption.
Actionable before merge:
- Fix (medium) — Handle
thisparameter modifier for extension methods (csharp.ts:110); add test - Comment (trivial) — Update
record structtest title at line 1183 to reflect actual grammar behaviour
Nice to have:
3. Pin exact tree-sitter-c-sharp version in the params comment block
· Branch
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
C# Method Extraction Review
Overall this is solid work — comprehensive coverage of C# constructs with good test coverage. A few items to consider:
1. Language-specific pattern matching bug
extractParametersFromList() has a subtle issue with the params variadic handling. The current loop can skip valid parameters when it advances i incorrectly after finding a params argument:
if (nameText) {
params.push({...});
}
i++; // This increments but the outer while also increments
continue;Consider restructuring to avoid double-incrementing i, or use a for-loop with manual control.
2. Missing validation for empty param names
In extractCSharpParameters(), if nameNode exists but has empty text, you still push a parameter. Consider validating nameNode.text.trim() is non-empty.
3. Test gap: null/undefined handling
The primary constructor tests don't cover edge cases where parameter_list exists but contains no valid parameters, or where parameters have malformed syntax. Worth adding a test for:
public class Bad(int, string); // Invalid C# but parser might produce nodes4. Visibility precedence in primary constructors
In extractPrimaryConstructor(), the compound visibility logic is duplicated from extractVisibility(). Consider extracting this to a shared helper to avoid drift.
5. Return type for conversion operators
The test extracts implicit conversion operator asserts conv!.parameters but doesn't verify returnType. Consider adding that assertion.
Minor nits
- The comment about
tree-sitter-c-sharpgrammar version inexport-detection.tsis helpful — consider adding similar version-gating comments tocsharp.tsfor the primary constructor handling - Several tests use
!.non-null assertions afterexpect().toBeDefined()— fine for tests, but the production code should have runtime guards
Security/Reliability
No concerns. The code doesn't execute user input, only parses it. The AST traversal is bounded by node count.
Verdict: Approve with minor suggestions. The architecture is clean and the test coverage is thorough.
… 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
|
@claude ultra-think and review this pull request as a senior context engineer, senior compiler front-end engineer, and expert in static analysis tools. Perform a deep, evidence-based review of the implementation, not just the PR description. Inspect the full diff, surrounding files, linked issue/context, and all prior review comments, then reason carefully about whether this change actually satisfies the requested behavior and fits the existing architecture. Review priorities:
You must investigate specifically:
Be especially strict about URL parsing edge cases:
Also verify whether the code is overfitting to one GitHub URL format instead of implementing a durable parsing strategy. In addition:
Important:
You must additionally verify what version of the C# tree-sitter grammar this repository currently uses, and if you mention any missing C# language feature coverage you must verify that claim against the actual grammar version in use before raising it. Do not speculate about unsupported C# constructs without checking the exact tree-sitter-c-sharp version and the repository’s current C# ingestion/parsing pipeline. Output format: VerdictState whether this is:
Confirmed findingsFor each finding use:
Architectural assessmentBriefly assess whether the solution fits the existing design and whether there is a cleaner or more durable approach. Previous comment verificationList prior reviewer requests and say whether each one is:
Missing testsList only the highest-value missing tests. Final review commentWrite one concise, paste-ready GitHub review comment in a strong senior-engineer tone that focuses on correctness, architecture, and follow-up actions. |
|
Claude finished @magyargergo's task in 5m 5s —— View job C# MethodExtractor — Round 7 Deep ReviewTasks
Grammar Version Baseline
Round 6 Findings — Verified Status
All three Round 6 actionable items are addressed or acknowledged. The semver nit is cosmetic. VerdictMergeable as is. All confirmed bugs across six rounds have been fixed. No new blockers or majors found below. The remaining findings are documentation nits or long-standing architectural observations that predate this PR. Confirmed Findings1. Severity: nit
2. Severity: nit
xkonjin Reviewer — Verification
Persistent Architectural Observations (Pre-existing, No Action Required for This PR)
Missing Tests (Highest Value)
Final Review Comment
· Branch |
…e 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
2 participants
Summary
Closes #573. Adds C# method extraction config following the established JVM pattern from PR #576.
gitnexus/src/core/ingestion/method-extractors/configs/csharp.ts— C#MethodExtractionConfigwith detection for:isAbstract:abstractmodifier or interface body method without bodyisFinal:sealedmodifier (C# terminology)isStatic:staticmodifierparamsarrays →isVariadic: trueout/refparameter modifiers → type prefixed (e.g.,"out int")isOptional: true[Attribute]syntax →annotationsarray (prefixed with@)public,private,protected,internal(default:private)languages/csharp.ts— wirescreateMethodExtractor(csharpMethodConfig)into the C# language providerDepends on
PR #576 (merged)
Test plan
tsc --noEmit)