Repository navigation
fix(csharp): blank preprocessor directives and #if branches that derail parsing - #5003
rohit-jsfreaky wants to merge 1 commit into
Conversation
…derail parsing tree-sitter-c-sharp parses `#if`/`#elif`/`#else`/`#endif` with every branch in place. Its error recovery around a directive can close the enclosing class early, so every later member became a class-less node: it lost its `Type.` owner, its field/receiver typing and same-class resolution, and its calls were lost or wrong (e.g. `Writer.Flush -> Buffer.Append` was missing). Before parsing, extract_csharp now blanks every directive line (including #region, #pragma, #nullable and #define). The first branch of each #if group is always kept. A later #elif/#else branch is kept as well, so members that exist only there stay in the graph (Graphify-Labs#2631). It is blanked only when keeping it would derail the parse: - its `{}` or `()` do not net to zero on their own (`class A : B {` #else `class A {`), or - it declares a member (name + arity) that a kept branch of the same group already declares (`void Write(Span<char>)` #else `void Write(string)` sharing one body). Nested groups are decided first. The rewrite keeps the byte length the same (removed bytes become spaces, CR/LF kept), so every remaining byte keeps its line and column. A `#` line inside a comment or a multi-line string is not treated as a directive. Measured against the Roslyn answer key on Newtonsoft.Json, Dapper and AutoMapper: 5544/7349 links (75.4%, was 70.8%), and 264 wrong `calls` edges (was 288). No kind loses recall except `ctor` (-4): five `new JValue(...)` calls inside JValue. Those methods now correctly belong to JValue, and the graph keeps one edge per node pair, so the JValue -> method edge replaces the calls edge. Keeping only the first branch scores 5545 but drops #else-only members. Bumps the AST cache schema to 38 so cached C# extractions re-run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @rohit-jsfreaky. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 not verified).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).
Not verified on this run: extract\_csharp (vacuous: never exercised).
Graphify review — findings
Fixes C# extraction losing class membership around preprocessor directives: directives are now blanked before parsing, and an #if branch that would derail tree-sitter-c-sharp is blanked too, with byte length preserved so lines and columns stay put. Previously the grammar's error recovery could close the enclosing class early, leaving every later member class-less with its calls dropped. The branch-selection logic in _blank_csharp_preprocessor is where fragment #else branches (base lists, split headers) can survive next to the #if branch, and _csharp_token_end has to scan interpolated strings to find token boundaries. Bumps _AST_CACHE_SCHEMA to 38 so cached C# extractions re-run, and adds C# member-call tests covering receiver typing (using-directive disambiguation, out declarations, primary-constructor parameters, local shadowing) and nested split-signature preprocessor headers.
Worth a look
- Fragment #else branches (base lists, split headers) are kept alongside #if branch, derailing the parse —
graphify/extract.py:3922· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Deep nested interpolated strings can crash C# extraction —
graphify/extract.py· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 4699 functions depend on the 545 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_csharp()— 20 callers, 3 callees
Verification — 4699 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 4012 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
325 of 518 test file(s) selected (63%) via static blast radius.
tests/test_4874_jvm_resolution.py— impacttests/test_affected_overrides.py— impacttests/test_alias_folding.py— impacttests/test_analyze.py— impacttests/test_astro_extraction.py— impacttests/test_astro_import_ids.py— impacttests/test_binary_media_words.py— impacttests/test_blade_extractor.py— impacttests/test_build.py— impacttests/test_build_located_semantic_identity.py— impacttests/test_build_merge_dedup_scope.py— impacttests/test_build_merge_ghost_fold_4955.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_builtin_global_type_refs.py— impacttests/test_c_include_resolution.py— impacttests/test_cache.py— impacttests/test_cache_long_paths.py— impacttests/test_cache_stale_import_target.py— impacttests/test_cache_windows_case_4925.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_case_only_decl_ids_4904.py— impacttests/test_case_sensitive_resolution.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_cjs_module_extension.py— impacttests/test_cli_export.py— impacttests/test_cluster.py— impacttests/test_cobol_extractor.py— impacttests/test_code_only_scope_4923.py— impacttests/test_confidence.py— impacttests/test_confidence_score_output.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cpp_local_callable_shadowing.py— impacttests/test_cpp_member_call_by_receiver_type.py— impacttests/test_cpp_method_declarations.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cpp_receiver_parameters.py— impacttests/test_cpp_split_header_owner.py— impacttests/test_cpp_template_type_params.py— impacttests/test_cpp_type_alias.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_cross_language_call_resolution.py— impacttests/test_cross_repo_external_call_guards.py— impacttests/test_cross_repo_member_calls.py— impacttests/test_csharp_call_site_generic_args.py— impacttests/test_csharp_delegate_declarations.py— impacttests/test_csharp_enum_members.py— impacttests/test_csharp_field_generic_args.py— impact- … and 275 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Docs that may be stale (advisory)
CHANGELOG.md§ 0.9.14 (2026-07-12) (lines 1325-1344): references changed symbolsearlyCHANGELOG.md§ 0.6.7 (2026-05-02) (lines 2238-2247): references changed symbolsearlydocs/superpowers/plans/2026-05-04-incremental-updates-dedup.md§ Task 4: Incremental updates — semantic cache + manifest in __main__.py (lines 603-911): references changed symbolsearlygraphify/skill-agents.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 631-740): references changed symbolskeepgraphify/skill-amp.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 631-740): references changed symbolskeepgraphify/skill-claw.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 634-743): references changed symbolskeepgraphify/skill-codex.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 635-744): references changed symbolskeepgraphify/skill-copilot.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 634-743): references changed symbolskeepgraphify/skill-droid.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 631-740): references changed symbolskeepgraphify/skill-kilo.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 634-743): references changed symbolskeep
…and 10 more.
Formal verification
Could not verify: Could not verify extract\_csharp.
The verifier did not have enough to check extract\_csharp, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly NameError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below.
| return bytes(out) | ||
|
|
||
|
|
||
| def extract_csharp(path: Path) -> dict: |
There was a problem hiding this comment.
extract_csharp()
20 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
I checked both bot findings:
|
Closes #5002
What does this PR do?
tree-sitter-c-sharp parses
#if/#elif/#else/#endifwith every branch in place. Around a directive its error recovery can close the enclosing class early, so every later member loses its class (repro in the issue).Before parsing,
extract_csharpnow rewrites the source:#if,#else,#region,#pragma,#nullable,#define, ...).#ifgroup is always kept.#elif/#elsebranch is also kept, so members that exist only there stay in the graph (C#: members inside#if/#elseblocks produce no nodes, and calls to them produce no edges #2631, its test intests/test_dotnet.pypasses unchanged). It is blanked only when keeping it would derail the parse:{}or()do not net to zero on their own (class A : B {/#else/class A {), orvoid Write(Span<char> s)/#else/void Write(string s)sharing one body).Nested groups are decided first. Blanked bytes become spaces and CR/LF are kept, so every byte keeps its line and column. A
#line inside a comment or a multi-line string is not treated as a directive.AST cache schema 37 -> 38 because C# output changes. (#5001 also bumps it to 38. Whichever lands second takes 39.)
Diff:
graphify/extract.py+277 / -3, 5 new tests intests/test_csharp_member_calls.py.Numbers
callslinks that Roslyn resolves on Newtonsoft.Json, Dapper and AutoMapper, and how many graphify finds:callsedgesPer project: Newtonsoft.Json 72.8% -> 79.0%, Dapper 81.2% -> 81.7%, AutoMapper unchanged (55.3%).
The
ctordrop (-4) comes from fivenew JValue(...)calls inside the partial classJValue. Those methods now correctly belong toJValue, and the graph keeps one edge per node pair, so theJValue -> methodedge takes the place of thecallsedge.I also tried keeping only the first branch. It scores one link more (5545) but drops
#else-only members, which breaks #2631.Limitations
bool Ignoredthen=> false;/#else/{ get; }in AutoMapper. They cost no Roslyn links here.#elsebranch that the net8.0 build does not compile now show up as edges. That is expected when both branches are kept.Type of change
Verification & Invariants
Invariant: the rewrite never moves a byte. It only turns directive lines and derailing branches into spaces, so every node keeps its real
source_location. Members in any balanced, non-duplicate branch stay in the graph.How was this tested?
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. (Not needed: no skill fragments changed.)🤖 Generated with Claude Code