fix(cs-lsp): resolve nested types through type qualifiers, bind global-ns types by ersintarhan · Pull Request #2401 · DeusData/codebase-memory-mcp

ersintarhan · 2026-09-28T16:45:28Z

Follow-up to #2347 (issue #2120), from the full-repo sweep reported in #2347 (comment).

Disclosure: this PR was written by an AI agent (Claude, in a pi coding session) acting for @ersintarhan, who reviewed it and is accountable for the sign-off. Every claim below is checkable: the failing test, the commands, the numbers.

What still mis-bound after #2347

Sweeping all 2,735 CALLS edges into same-name C# class families on the (private) repository behind #2120, adjudicated against the callers' source usings, left ~125 mis-bound edges in three shapes. Two of them are type-name-resolution gaps that this PR fixes; the third family turned out to live in other layers (see "Out of scope").

1. Qualified name whose qualifier is a TYPE, not a namespace

// ns A.Cqrs                              // ns B.Cqrs — same repo
public class Command {                    public class OtherCommand {
    public class Result { ... }               public class Result { ... }
}                                        }

using A.Cqrs;
return new Command.Result(false, "x");   // fell to short-name fallback → B variant
return new Grain.Handler(dep).Handle();  // same: receiver type unresolved

cs_resolve_by_declared_namespace treated the qualifier only as a namespace (cs_visible_namespaces builds ns.Qualifier candidates), and nested types carry no namespace_qn, so the scan had no candidate at all. Now, when the namespace scan misses, the qualifier is resolved as a type (recursively — one segment shorter, so A.B.C works too) and the nested member binds through the registry as parentQn.Simple.

2. Top-level type in the global namespace

A class declared with no namespace (e.g. a service consumed via [Inject]) got namespace_qn = NULL from cs_top_level_namespace — invisible to step 8b, even though C# resolves it from anywhere. Top-level C# types now register namespace_qn = "", and cs_visible_namespaces appends the global namespace last, per C# lookup order (NULL stays reserved for nested types).

Test

parallel_csharp_nested_type_and_global_namespace_binding (tests/test_parallel.c) covers the nested-type constructor + member call on its result, the chained-constructor receiver, and a global-namespace variant competing with an imported one — in the sequential pipeline and in the parallel pipeline with the production Tier-2 C# registry. Decoys are registered first, so without the fix the registry-order behavior fails the test. Verified on this machine:

with the fix stashed: nested_code=0/1 chained=0/1 global_ns=0/0 → FAIL
with the fix: parallel suite 76/76 PASS
full suite: 8176 passed, 1 failed, 8 skipped — the failure is tests/test_cli.c:2080, a CLI/environment test (the cli suite does not complete in this environment without this patch either; the diff touches only cs_lsp.c and test_parallel.c)

Numbers (private repo behind #2120, same sweep as the PR-2347 comment)

mis-bound edges into same-name families: 123 → 109; the 14 fixed are exactly the chained-constructor receivers (new Grain.Handler(dep).Handle(...)), consistent with the new test.
visibility-verified edges: 1843 → 1857.

Out of scope — the remaining 109, by layer (not type-name resolution)

~90 constructor edges (new Command.Result(...)): the cs_ctor row resolves correctly now (verified via debug logging) but loses the pass_calls carrier join, so the textual suffix-match fallback still creates the edge to the registry-first variant. Needs a matching-rule fix in the LSP-row ↔ carrier join.
a few X x = new(...) implicit (target-typed) constructions: cs_eval_object_creation_type returns unknown for implicit_object_creation_expression.
3 [Inject] field-type edges: the field type resolves in the registration context (cs_register_lsp_defs builds a CSLSPContext with no usings), so step 8b's visibility list is empty there.

Happy to take any of those on next if the approach here looks right.

Build note

Built from source on Arch (gcc/glibc current) with MIMALLOC_OVERRIDE_DEFINE= GLOBAL_OVERRIDE_DEFINE=; the default MI_MALLOC_OVERRIDE=1 build segfaults at startup on this machine regardless of this patch (see PR-2347 comment). Official release binaries are unaffected.

Signed-off-by: Ersin Tarhan <[email protected]> (DCO)

github-actions · 2026-09-28T16:46:53Z

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

It will not be closed for inactivity. No stale bot touches pull requests here.
It may still sit a while before a human reads it. That is on us, not on you.
Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
Get CI green, or say which failures you believe are pre-existing.
Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

Copilot

Copilot review overview

🟡 Changes recommended

Address the type-identity and using-scope binding risks, remove unconditional diagnostics, and strengthen the global-namespace test.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

Prevent unrelated types from being conflated · New
Remove unconditional constructor debug output · New
Add coverage for global versus imported type resolution · New

What changed in this PR

Improves C# cross-file resolution for nested qualified types and global-namespace types, with metadata propagation and regression coverage.

Changes:

Tracks declared namespaces through extraction, compaction, and registries.
Resolves nested types recursively and searches the global namespace.
Adds sequential and parallel pipeline tests.

File	Summary
`tests/test_parallel.c`	Adds C# binding regression tests; global-vs-imported coverage needs strengthening.
`src/pipeline/pipeline.c`	Includes namespace metadata in census accounting.
`src/pipeline/pass_lsp_cross.c`	Propagates namespace metadata; PR scope note needs correction.
`internal/cbm/result_compact.c`	Preserves namespace metadata during compaction.
`internal/cbm/lsp/type_registry.h`	Adds registry namespace metadata.
`internal/cbm/lsp/cs_lsp.c`	Implements namespace and nested-type resolution; contains debug output, lookup conflation, and using-scope concerns.
`internal/cbm/extract_defs.c`	Extracts declared C# namespaces.
`internal/cbm/cbm.h`	Adds definition metadata; global namespace invariant documentation needs correction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ersintarhan · 2026-09-28T17:05:23Z

+        if (!c->namespace_qn || !c->short_name || !c->qualified_name ||
+            strcmp(c->short_name, t->short_name) != 0 ||
+            strcmp(c->namespace_qn, t->namespace_qn) != 0 ||
+            strcmp(c->qualified_name, t->qualified_name) == 0)


Good point, but this lookup is part of the stacked base #2347 (now the PR base), not this diff — with the base retargeted it should appear there. Cross-assembly name conflation is real, though: on my repo two projects legitimately share namespace+name (same-name types in sibling projects are exactly what #2120 tripped over), and today the resolver's only signals are namespace + simple name. Registry-level provenance (project/assembly origin) would also fix the remaining wrong-variant picks I mapped in the PR description. Worth its own issue if you agree — I did not want to widen this PR further.

ersintarhan · 2026-09-28T17:05:25Z

+    if (ts_node_is_null(tnode)) {
+        fprintf(stderr, "DBG ctor: no type field\n");
        return;
+    }
    const CBMType *t = cs_parse_type_node(ctx, tnode);
    const char *tqn = NULL;
    if (t && t->kind == CBM_TYPE_NAMED)
        tqn = t->data.named.qualified_name;
    else if (t && t->kind == CBM_TYPE_TEMPLATE)
        tqn = t->data.template_type.template_name;
+    fprintf(stderr, "DBG ctor: tqn=%s kind=%d enclosing=%s\n", tqn ? tqn : "(null)",
+            t ? (int)t->kind : -1, ctx->enclosing_func_qn ? ctx->enclosing_func_qn : "(null)");


You're right — those were my temporary diagnostics that slipped into the commit; dropped in 874286a (good catch, exactly why the review exists).

ersintarhan · 2026-09-28T17:05:27Z

+    static const char other_update_src[] =
+        "namespace Acme.Other.Web.Services;\n"
+        "public class UpdateService {\n"
+        "    public string Check() => \"other\";\n"
+        "}\n";


Fair — the original test only proved global-ns visibility, not lookup ORDER against an imported candidate. 874286a adds ImportUser: same simple name resolvable via using Acme.Other.Web.Services; AND present in the global namespace; it asserts the imported variant wins and the global one is never hit, pinning the appended-last behavior of cs_visible_namesnamespaces. (also retargeted the PR base to fix/issue-2120 so the diff is just the two files of this change.)

…2120) Two projects declaring a class with the same simple name in namespaces that share a root (Contoso.Platform.Identity.UserIdentityInfo vs Contoso.Platform.BackOffice.Identity.UserIdentityInfo) had every CALLS edge attached to whichever variant registered first, ignoring the caller's `using` directive, enclosing namespace, fully-qualified name and using-alias. Root cause: C# graph QNs are path-derived, so cs_resolve_type_name's namespace-prefix and using-namespace lookups (steps 5 and 7) never hit a project type. Resolution fell through to the short-name fallback, whose prefix score compares the file namespace against the path QN and is 0 for every candidate, so registry order decided. Fix: - Record the declared namespace of each top-level C# type at extraction (CBMDefinition.decl_namespace, walked from the AST). The file-level namespace_name keeps only a file's FIRST namespace, which mislabels every type of a multi-namespace file such as a reference assembly. pxc_build_lsp_def carries it into the type's CBMLSPDef, and result_compact relocates it. - CBMRegisteredType.namespace_qn (C# only, top-level types only) is set by cs_register_lsp_defs, which both the Tier-2 shared registry and the per-file registry use, so sequential and parallel stay in lockstep. - New resolution step 8b binds a simple or qualified type name to the visible declaration in C# lookup order: enclosing namespaces innermost outward, then `using` namespaces (or the global namespace for a qualified name). A `using A = Ns.Type` alias binds the same way. - cs_lookup_method also searches the type's other same-namespace declarations (`partial` pieces, reference-assembly copies). These are separate registry entries but one C# type. Without this, choosing the namespace-correct piece that lacks the member would lose the edge to a name-only fallback. Proof on dotnet/runtime src/libraries (20,866 .cs files, same machine, both binaries deterministic across two runs): CALLS 649,226 -> 657,564; nodes unchanged (595,714). Most retargeted edges move from a target the caller cannot see to one it can see through an enclosing namespace or `using`. Many more move from non-method targets (enum members such as ExpressionType.Constant) to the real partial-class methods (Expression.Constant). The new edges are unqualified and typed calls into sibling `partial` pieces (ImmutableArray.Create, XmlReader.Create, AsnReader.ReadBoolean). Test: parallel::parallel_csharp_same_name_class_binds_by_namespace covers plain `using` in both directions, the enclosing namespace, a fully-qualified name, a multi-namespace file and a `partial` member, in the sequential pipeline and in the parallel pipeline with the production Tier-2 C# registry. The harness gains an opt-in flag for that registry, because cbm_pxc_run_one has no C# case. Signed-off-by: Martin Vogel <[email protected]>

…l-ns types Follow-up to DeusData#2347 (issue DeusData#2120). Step 8b binds same-name TOP-LEVEL types by declared namespace, but two shapes still fell through to the short-name fallback, where the prefix score against a path-derived QN is 0 for every variant and registration order decided: 1. A qualified `A.B` whose qualifier A is a TYPE, not a namespace: `new Command.Result(...)` and `new Grain.Handler(dep).Handle(...)`. cs_visible_namespaces only tried A as a namespace, and nested types carry no namespace_qn, so the scan never had a candidate. Resolution now falls back to resolving the qualifier as a type (recursively, one segment shorter) and binding the nested member through the registry. 2. A top-level type with NO namespace declaration (global namespace), e.g. a service consumed via [Inject]: cs_top_level_namespace returned NULL for it, making it invisible to step 8b even though C# sees the global namespace from everywhere. Top-level C# types now register namespace_qn "" and cs_visible_namespaces appends the global namespace last, per C# lookup order. Test: parallel_csharp_nested_type_and_global_namespace_binding covers a nested-type constructor call and the member call on its result, a chained-constructor receiver `new Grain.Handler(dep).Handle()`, and a same-name type split across the global namespace and an imported namespace — in the sequential pipeline and in the parallel pipeline with the production Tier-2 C# registry. Decoy files are registered first, so the registry-order behavior fails the test without the fix (verified: nested_code=0/1 chained=0/1 global_ns=0/0 without it). Proof on the private repository behind DeusData#2120 (2,735 CALLS edges into same-name class families, adjudicated against source): mis-bound edges 123 -> 109; the 14 fixed are exactly the chained-constructor receivers. The remaining 109 route through other layers, not type-name resolution: ~90 constructor edges whose cs_ctor row loses the pass_calls carrier join to the textual suffix fallback, a few `X x = new(...)` target-typed implicit constructions, and 3 [Inject] field-type resolutions that run in the registration context without file usings. Full suite: 8176 passed, 1 failed, 8 skipped; the failure (tests/test_cli.c:2080) is a CLI/environment test unaffected by this diff (the cli suite also fails to complete in this environment without the patch). Signed-off-by: Ersin Tarhan <[email protected]>

…al order - remove two temporary DBG fprintf lines left in cs_resolve_object_creation (they fired on every C# object creation) - add ImportUser to the nested/global test: same simple name resolvable through an imported namespace AND present in the global namespace must bind to the imported variant, proving cs_visible_namespaces appends the global namespace last rather than merging it as an equal parallel suite: 76/76 PASS Signed-off-by: Ersin Tarhan <[email protected]>

DeusData

Thank you, @ersintarhan, for following up on #2347 with such a careful sweep, and for disclosing exactly how this PR was produced. Both gaps you found are real. Qualified names whose qualifier is a type (new Command.Result(...)) and types declared outside any namespace currently fall to the short-name fallback, and your test pins both shapes well. We'd like to land this; here is what is needed first.

1. Retarget and rebase. The PR's base is fix/issue-2120, which has since been merged into main (as a510646), so the diff currently shows #2347's own files too. Please retarget the PR to main and rebase onto it, keeping only your two commits (cs_lsp.c and test_parallel.c merge onto current main without conflicts). Please also squash the intermediate commit that added the debug fprintf lines, so it doesn't end up in main's history.

2. Bound the recursion in cs_resolve_type_name. The new qualifier-as-type fallback calls cs_resolve_type_name(ctx, qualifier) again, and the existing alias step resolves an alias's target, which can be longer than the alias. With using X = X.Y; (legal C# when X is an external namespace), any use of X as a type goes alias -> declared-namespace scan -> X -> alias, and so on without end. Mutual aliases (using A = B.T; using B = A.U;) do the same. That's a stack overflow that takes down the indexer on valid input. A depth counter in CSLSPContext (or resolving the qualifier with the alias step switched off) would fix both. A small test with each alias shape would pin it.

3. Look up the global namespace in the C# spec's order. The PR checks the global namespace after the compilation-unit using directives, and the test asserts "imported beats global" for that case. In the C# lookup rules, members of the global namespace are found before the compilation-unit using directives; only using directives inside a namespace block come earlier than the enclosing namespaces. Please follow the spec order and adjust that part of the test.

4. Don't make namespace-less types into partial twins. Mapping a missing namespace from NULL to "" also feeds the partial-twin merge from #2347 (cs_namespace_twins / cs_lookup_method). Every same-named type declared outside a namespace anywhere in the repo now becomes a "partial" of every other. dotnet/runtime, for example, has 365 namespace-less Program classes and 209 Test classes. A partial class needs the same namespace and the same assembly, so please exclude namespace "" from twin matching.

5. A public before/after. Your numbers come from a private repository, which is completely understandable. Could you add a CALLS delta on dotnet/runtime (the corpus #2347 used): edges added and removed, plus a few sampled examples? It would also help to see that the extra resolution step doesn't change indexing time noticeably.

Once those are in we'll measure it on our C# corpora and come back quickly. Thanks again for the thorough work; the type-qualifier case in particular is a gap we hadn't pinned down.

DeusData · 2026-11-01T01:51:10Z

Hi @ersintarhan, thank you so much for this PR and for the careful analysis behind it. You found two real gaps in the #2120 work: nested types reached through a type qualifier, and types in the global namespace. Your fix for both is the core of what just landed in #2867 (6e0c59f), and your commits are credited there with a Co-authored-by trailer.

We carried the change over to a fresh branch instead of merging this one for practical reasons. This branch was based on fix/issue-2120, which has since merged as #2347, so it still carried a copy of that work, plus the temporary debug-print commit. We also added a few things on top:

A depth bound on the qualifier walk. An alias whose target names itself (using Loop = Loop.Inner;, or a mutual pair) made the resolution recurse until the stack overflowed.
C# spec lookup order for the global namespace. C# checks the members of the global namespace BEFORE the usings at compilation-unit level, so a type declared without a namespace hides one imported by a top-level using, while a using inside a namespace body still wins. That flips the ImportUser expectation in your test; we added a namespace-body case next to it.
Global-namespace types are not treated as partial pieces of one another, since across a repository the same name usually means a different type per project.
A type only counts as global when its file parsed cleanly. On dotnet/runtime, reference assemblies with ref partial struct members confused the parser and made thousands of types look global.

Your out-of-scope list (the constructor carrier join, target-typed new(), and [Inject] field types) is a very useful map for follow-ups. Thanks again for the thorough work and the clear write-up!

…l-ns types Follow-up to DeusData#2347 (issue DeusData#2120). Step 8b binds same-name TOP-LEVEL types by declared namespace, but two shapes still fell through to the short-name fallback, where the prefix score against a path-derived QN is 0 for every variant and registration order decided: 1. A qualified A.B whose qualifier A is a TYPE, not a namespace: new Command.Result(...) and new Grain.Handler(dep).Handle(...). cs_visible_namespaces only tried A as a namespace, and nested types carry no namespace_qn, so the scan never had a candidate. Resolution now falls back to resolving the qualifier as a type (recursively, one segment shorter) and binding the nested member through the registry. 2. A top-level type with no namespace declaration (global namespace): cs_top_level_namespace returned NULL for it, making it invisible to step 8b. Top-level C# types now register namespace_qn "" and cs_visible_namespaces appends the global namespace last. Test: parallel_csharp_nested_type_and_global_namespace_binding covers a nested-type constructor and the member call on its result, a chained constructor receiver, and a same-name type split across the global namespace and an imported namespace, in the sequential pipeline and in the parallel pipeline with the production Tier-2 C# registry. Decoys are registered first. Adapted to current main: the two constructor-edge checks gate the parallel path only, because the sequential constructor edge is still drawn by the textual constructor carrier, a layer this change does not touch. Distilled from DeusData#2401: the net change of commits d11be54 and 613060e. The temporary debug prints added in d11be54 and the stale copy of DeusData#2347 the branch carried (already on main as 50d19f8) are not taken. Co-authored-by: Ersin Tarhan <[email protected]> Signed-off-by: Martin Vogel <[email protected]>

ersintarhan requested a review from DeusData as a code owner September 28, 2026 16:45

Copilot AI lite review requested due to automatic review settings September 28, 2026 16:45

Copilot started reviewing on behalf of ersintarhan September 28, 2026 16:47

View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

ersintarhan changed the base branch from main to fix/issue-2120 September 28, 2026 17:04

DeusData force-pushed the fix/issue-2120 branch from b220ece to 95f0912 Compare September 30, 2026 22:34

DeusData and others added 3 commits October 2, 2026 00:23

Merge branch 'main' into fix/issue-2120

2986050

ersintarhan force-pushed the fix/cs-nested-type-qualifier-and-global-namespace branch from 874286a to 613060e Compare October 2, 2026 10:22

DeusData force-pushed the fix/issue-2120 branch from 2986050 to a510646 Compare October 4, 2026 16:48

DeusData mentioned this pull request Oct 9, 2026

C#: CALLS edge attaches to wrong same-name class variant (shared namespace root) — using-directive scoping ignored; trace_path inbound returns callers_total: 0 #2120

Closed

2 tasks done

DeusData requested changes Oct 17, 2026

View reviewed changes

DeusData mentioned this pull request Nov 1, 2026

fix(cs-lsp): resolve nested types through type qualifiers, bind global-namespace types #2867

Merged

DeusData closed this Nov 1, 2026

DeusData mentioned this pull request Nov 1, 2026

C# extraction and resolution gaps found while landing #2867 #2876

Open

Navigation Menu

Repository navigation

fix(cs-lsp): resolve nested types through type qualifiers, bind global-ns types - #2401

fix(cs-lsp): resolve nested types through type qualifiers, bind global-ns types#2401
ersintarhan wants to merge 4 commits into
DeusData:fix/issue-2120from
ersintarhan:fix/cs-nested-type-qualifier-and-global-namespace

ersintarhan commented Sep 28, 2026

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ersintarhan Sep 28, 2026

Uh oh!

ersintarhan Sep 28, 2026

Uh oh!

ersintarhan Sep 28, 2026

Uh oh!

DeusData left a comment

Uh oh!

DeusData commented Nov 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ersintarhan commented Sep 28, 2026

What still mis-bound after #2347

1. Qualified name whose qualifier is a TYPE, not a namespace

2. Top-level type in the global namespace

Test

Numbers (private repo behind #2120, same sweep as the PR-2347 comment)

Out of scope — the remaining 109, by layer (not type-name resolution)

Build note

Uh oh!

github-actions Bot commented Sep 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

ersintarhan Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

ersintarhan Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

ersintarhan Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

DeusData left a comment

Choose a reason for hiding this comment

Uh oh!

DeusData commented Nov 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants