Skip to content

fix(cs-lsp): bind same-name C# types by declared namespace (#2120) - #2347

Open
DeusData wants to merge 1 commit into
mainfrom
fix/issue-2120
Open

DeusData wants to merge 1 commit into
mainfrom
fix/issue-2120

Conversation

@DeusData

Copy link
Copy Markdown
Owner

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.


Release note: existing indexes need one from-scratch reindex (delete_project + index) to pick this up; a full-mode reindex of unchanged files reuses the old edges.

Fixes #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>
@ersintarhan

Copy link
Copy Markdown

Tested this branch on the real repository behind #2120 (~53k nodes post-index). Verified against source using directives:

  • Every same-name family variant now receives its own real callers (e.g. UserClaimPrincipleFactory BackOffice/Platform/Partner = 1/0/1; previously the Platform variant absorbed the production calls).
  • The mixed plain-using + using-alias file (IdentityContractTests.cs) splits correctly: plain calls → Partner variant, alias calls → BackOffice variant. On 0.10.8 that file produced phantom/no edges.

One build note, probably unrelated to this PR but worth a look: building b220ecec from source on Arch (current glibc/gcc) segfaults at startup — backtrace dies in mi_free called from newlocale during libstdc++ static init, i.e. the MI_MALLOC_OVERRIDE=1 global interposition. Rebuilt with MIMALLOC_OVERRIDE_DEFINE= GLOBAL_OVERRIDE_DEFINE= and everything works (that's the binary these results come from). The official v0.11.0 release binary runs fine on the same machine.

@ersintarhan

Copy link
Copy Markdown

Follow-up from me: after the spot-checks above, I ran a full sweep over all 2,735 CALLS edges into same-name class families in the same repo (~2.1k classes, 137 families), adjudicating each edge against the caller's actual usings, enclosing namespaces, _Imports.razor, global usings and qualified references.

The reported modes are fixed — 1,843 edges visibility-verified correct (incl. the alias/enclosing-namespace cases), and ~700 receiver-typed instance calls (result.ToException() on an inferred DataResult receiver, etc.) bind correctly by unique method name.

But the sweep found ~125 residual mis-bound edges in three patterns, each verified against source:

1. Nested-type constructor on a duplicated command name (~90 edges; PlayerCommand 27, Result 21, PartnerCommand 19, UserCommand 15, CurrencyListCommand 2 …)

// ns CmdA.Shared.Cqrs.Partner          // ns CmdB.Shared.Cqrs.Domain — same repo, same shape
public class PartnerCommand {            public class DomainCommand {
    public class Result { … }               public class Result { public class ResultModel { … } }
}                                       }

// caller imports ONLY CmdA.Shared.Cqrs.Partner:
return new PartnerCommand.Result(false, "x", null);   // edge → CmdB…DomainCommand.Result.ResultModel

Worst case is cross-project: a BackOffice handler with using <BackOffice>.Shared.Cqrs.Shared; binds new CurrencyListCommand.Result(...) to the Partner project's variant.

2. Chained-constructor receiver dropped (19 edges)

return await new GrainOtp.Handler(dataSource, sender).Handle(cmd);
// file imports only the GrainOtp namespace; edge → GrainLayouts.Handler.Handle

15 of the 19 all pin to the same unrelated variant, 4 to a second one — same "registry order wins" signature as the original bug, so it looks like the explicit new X(...) receiver chain is discarded and short-name Handle resolution takes over across a 100+-variant Handler family.

3. Blazor code-behind [Inject] service (3 edges)
A layout's code-behind has using <BackOfficeWeb>.Services; [Inject] AppUpdateService AppUpdate; — three lifecycle-method edges bind to the other web project's AppUpdateService variant.

Patterns 1–2 look like the remaining paths where the new namespace-aware steps don't kick in (nested-type qualification and constructor-chained receivers). Happy to package a minimal multi-file repro if useful — the three patterns reproduce 100% deterministically on this repo.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants