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 <ersintarhan@gmail.com> (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 <martin.vogel.tech@gmail.com>

…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 <ersintarhan@gmail.com>

…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 <ersintarhan@gmail.com>

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

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!

This branch has not been deployed

Reviewers

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants