fix(parser): preserve full C# nested type identity - #937
Conversation
code-review-graph reviewOverall risk: 0.40 (MEDIUM) — 34 changed function(s)/class(es), 0 affected flow(s), 16 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~239,855 fewer tokens (~98%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
Method nodes inside a nested class were qualified with only the immediate enclosing class, so two nested classes sharing a name collapsed their methods into one node: Details.QueryHandler.Handle and Edit.QueryHandler.Handle both became QueryHandler.Handle, silently losing one method with no uncertainty caveat. Only one level of nesting was ever retained. Accumulate the containing-type path when recursing into a class body, as Julia already did, and emit CONTAINS edges against the full path. The resolver has to move with the parser rather than in a separate change: parent_name is now a dotted path, so the old receiver lookup -- which reduced a scope to its last segment -- would miss every nested type and stop resolving C# receiver calls entirely. Receivers now resolve in C# lookup order, with every exact candidate exhausted before any suffix heuristic: 1. lexical - the receiver under each enclosing type, innermost first 2. exact - the receiver itself, then progressively shorter paths 3. suffix - the suffix map, matching a containing-type tail Ordering is load-bearing twice over. Inside Outer.Consumer a receiver D.E means Outer.D.E even when a top-level D.E exists. And because C# namespaces are not part of parent_name -- they live in csharp_namespaces_by_file -- a namespaced receiver App.Report.ExportHandler only matches exactly once shortened to Report.ExportHandler; letting a suffix hit land first would bind an unrelated nested type and, being unique, bypass the namespace disambiguator. Persistent graphs rebuild once through the existing version gate shared with C++, so stored identities pick up the new format. Fixes tirth8205#934
5588cd3 to
829ef29
Compare
The suffix map let a bare receiver select a nested type that is not lexically visible. A bare QueryHandler does not name Details.QueryHandler in C# -- that has to be qualified -- but the map held QueryHandler -> Details.QueryHandler, so a lone indexed candidate took the single_match path and rewrote the call with no visibility check at all. The real type is often in a dependency that was never indexed, which is exactly when the map is most confident and most wrong. Leaving such a receiver unresolved is the correct answer. Removing the fallback turns out to cost nothing: the lexical phase covers receivers bound through an enclosing type, and the exact phase, which shortens progressively, covers namespace-qualified receivers whose namespace is absent from parent_name. The whole suite passes without it. Renames test_longer_suffix_match_beats_shorter_exact_type, which described the removed mechanism rather than what it asserts.
|
The concern is real — I reproduced it before changing anything. Your exact case ( So I took the fix you suggested and dropped the containing-type suffix fallback entirely. It turned out to cost nothing. Your read was right that the valid cases are already covered:
So the separate namespace-metadata validation you offered as a fallback plan isn't needed; the exact phase already gets there. The whole suite passes with the map removed (2994 passed, 9 skipped, 2 xpassed), and the resolver is now two phases that both match a containing-type path exactly, with no arbitrary suffix matching left in it. Added Also renamed Pushed as 0e9b891. |
Phase 2 shortened a dotted receiver and treated each form as an exact lookup, so App.Report.ExportHandler fell back to Report.ExportHandler and matched a type sitting in namespace Other. One candidate takes the single_match path, so csharp_disambiguate never ran and nothing downstream caught it. C# resolves the leading identifier to a namespace or type and binds each later component relative to it; it never discards App and restarts at Report. Shortening is legitimate only because namespaces are absent from parent_name, so a shortened form now has to prove the part it dropped really is a namespace declared by the candidate's defining file, using the csharp_namespaces metadata the resolver already builds. A genuine App.Report.ExportHandler whose file declares namespace App still resolves; the mismatched one stays unresolved, which is right when the requested type may simply not be indexed. This also removes the last way a leading type name could be dropped: shortening Details.QueryHandler to QueryHandler now fails the check, since Details is a type rather than a namespace.
|
Confirmed before changing anything — your case resolved to Fixed in d62623b, and it's the check you proposed in your previous round. I said then that the exact phase made namespace validation unnecessary — that was wrong, and this is the case that shows it. Shortening is only legitimate in the first place because namespaces are absent from
Unresolved is the right answer there, since the requested type may simply not be indexed. One useful side effect: this closes the last route by which a leading type name could be dropped. Shortening Added 2995 passed, 9 skipped, 2 xpassed; ruff and mypy clean. |
…space The namespace evidence is per file, and a C# file may declare several namespaces; the parser records all of them. Membership alone therefore proved only that the dropped name appeared somewhere in the candidate's file, not that it enclosed the candidate. A file declaring both App and Other let App.Ledger.AuditHandler bind an Other.Ledger.AuditHandler, which as the sole candidate took the single_match path unchecked. Require the candidate's file to declare exactly the dropped namespace. Genuine matches in multi-namespace files are now left unresolved, which is the right trade for a resolver that must not fabricate edges. Resolving those properly needs per-node namespace information rather than per-file, which is a separate change to what the parser records.
|
Confirmed — the file-level set really does leak. Your case resolved to Took the conservative option, in b9c93f3: if csharp_namespaces_by_file.get(file, set()) != {dropped_namespace}:
rejectSo a shortened form now requires the candidate's file to declare exactly the dropped namespace. Genuine matches in multi-namespace files are left unresolved, which as you say is the right trade for a resolver that must not fabricate edges. I deliberately did not take the per-node namespace option here. It's the better architecture and I agree it's where this should end up, but it changes what the parser records per node, which is well outside a node-identity bug fix — this PR has already been split once for scope. Worth its own issue; happy to file it if you want, and I'd rather it be discussed on its own merits than smuggled in here. Added The invariant across the last three rounds is now a single sentence: the only thing a receiver may drop is an unambiguously verified namespace prefix. Wrong namespace, one-of-several namespaces, and a leading type name are all rejected by the same check. Also corrected the stale PR description — thanks for catching it. It no longer describes a third suffix phase, and now documents the multi-namespace limitation and its rationale. 2996 passed, 9 skipped, 2 xpassed; ruff and mypy clean. |
An exact containing-type match is not an exact type match. Namespaces are
absent from parent_name, so a type declared
namespace Other { class App { class Report { class ExportHandler } } }
is keyed as App.Report.ExportHandler and matches a receiver that means an
entirely different App.Report.ExportHandler, typically one from a referenced
assembly that was never indexed. Namespace validation only ran when the
receiver itself carried a namespace prefix, so this path went unchecked and,
as the sole candidate, took single_match.
When the receiver names no namespace, require the candidate's namespace to be
reachable from the call site: same file, the global namespace, a using
directive, or a namespace the caller declares. The evidence all already exists
in imports_by_file and csharp_namespaces_by_file.
This is not a regression from the branch -- main resolves the same call to the
same wrong node, and does so on the bare name ExportHandler rather than
requiring the full containing path to coincide, so the window was strictly
wider before. It is a long-standing consequence of namespaces being outside the
C# identity, now closed for the cases file-level evidence can decide.
|
Fixed in 2fb7b6d, and your test case is in. One correction to the framing, though: this is not a regression from this branch. I ran your exact case against
That said, you're right that it's real and that the PR asserted an invariant it didn't hold, so I've closed it. When a receiver names no namespace, the candidate's namespace must now be reachable from the call site: same file, global namespace, a Added On the architecture: I still think per-node namespaces belong in their own change rather than here. This PR fixes a node-identity bug and has already been split once for scope; folding a parser-level identity change into it would repeat exactly the mistake that split corrected. I'm not disputing that it's the right end state — file-level evidence remains genuinely unable to decide the multi-namespace case, and that limitation is documented in the PR description. I'd rather that land as its own reviewable change than as a fifth follow-up commit here. 2997 passed, 9 skipped, 2 xpassed; ruff and mypy clean. |
…eivers too" This reverts 2fb7b6d, which regressed a case main handles correctly. The predicate treated the caller's own file as the only source of visibility evidence. A global using in a different file applies project-wide, so a valid call resolved on main and became unresolved here: GlobalUsings.cs global using Other; Indexed.cs namespace Other; class Vault { ... } Consumer.cs Vault.Archive.StoreHandler handler; It was also wrong in the other direction, and still is after the revert: same file does not mean same namespace, and a using is scoped to its namespace body rather than the whole file, so file-level evidence over-permits as readily as it under-permits. Trading a false negative for a partial fix to a false positive is not a good trade, and both directions have the same cause -- the namespace is not in the identity, so no amount of file-level inference can settle it. Reverting restores parity with main: the wrong-namespace mislink remains, but it is pre-existing, main hits it on the bare name, and this branch already narrows it by requiring the full containing-type path to coincide. Keeps the case as an xfail so the limitation is recorded rather than forgotten, and adds a global-using test so any future visibility work has to preserve it.
|
You're right on all three, and the third one is decisive — so I've reverted 2fb7b6d rather than patch it. I measured each case against
The global-using row is the one that matters: my predicate broke a call After the revert the branch is at parity with Two tests to keep it honest:
On the underlying point, I now agree with you without reservation: file-level namespace evidence cannot establish this invariant in either direction, and per-node namespaces are the actual fix. What I'd push back on is doing it in this PR. It is a parser-level change to the C# identity, it needs its own rebuild-gate consideration, and this PR is a node-identity bug fix that has already been split once for scope. Four rounds here have each closed one instance of the same root cause; the fifth should close the cause. I'd rather open that as its own issue referencing this thread than grow this one again. Happy to file it now if you want, and to hold #937 until it lands if you'd prefer they ship together. 2997 passed, 9 skipped, 1 xfailed, 2 xpassed; ruff and mypy clean. |
|
Holding this PR pending #946. Filed the root cause as its own issue: C# namespaces are absent from the graph identity, so namespace-sensitive resolution is undecidable from the file-level evidence available. It collects all five failing cases with their To be clear about what this PR does and doesn't do in the meantime: it fixes #934, and it is at parity with Marking it draft. Once #946 lands, the Thanks for the persistence across these rounds — the same-file and |
2 participants
Summary
Method nodes inside a nested class were qualified with only the immediate enclosing
class, so two nested classes sharing a name collapsed their methods into a single
node —
Details.QueryHandler.HandleandEdit.QueryHandler.Handleboth becameQueryHandler.Handle, silently losing one method with no uncertainty caveat. Onlyone level of nesting was ever retained.
CONTAINSedges against the full pathFixes #934
Why the resolver moves with the parser
These are one change, not two.
parent_nameis now a dotted path, and the oldreceiver lookup reduced a scope to its last segment (
scope.rsplit(".", 1)[-1]).Shipping the parser fix alone would miss every nested type and stop resolving C#
receiver calls entirely.
Resolution model
A receiver is matched against a containing-type path exactly, in two phases:
(
Outer.Consumer.D.E, thenOuter.D.E)There is deliberately no suffix fallback. Selecting a nested type from a bare
receiver would bind names C# cannot bind: a bare
QueryHandlerdoes not nameDetails.QueryHandler, which has to be qualified. A lone indexed candidate wouldotherwise be rewritten as a single match with no visibility check at all, and the
real type is often in a dependency that was never indexed.
Phase 1 matters because inside
Outer.Consumera receiverD.EdenotesOuter.D.Eeven when a top-level
D.Eexists.Phase 2 exists only because namespaces are absent from
parent_name— they live incsharp_namespaces_by_file— soApp.Report.ExportHandleris stored asReport.ExportHandler. The only thing a receiver may drop is a verified namespaceprefix. A shortened form must prove the part it dropped is the namespace declared
by the candidate's defining file:
App→ resolvesOther→ unresolvedAppandOther→ unresolved, because file-level evidencecannot show which namespace encloses the candidate
Details.QueryHandler→QueryHandler) → unresolved,since
Detailsis not a namespaceThe multi-namespace case is conservative on purpose: it leaves genuine matches
unresolved rather than fabricating an edge. Resolving them properly needs per-node
namespace information rather than per-file, which is a follow-up.
Gated to C#.
_SCOPED_LANGUAGESisphp,rust,csharp; PHP and Rust are untouched.Scope
This PR is the identity fix and nothing else. Short-name addressing of a dotted
nested path (
children_of Details.QueryHandler) was split out to #942 — it carried aschema migration, an index, and Java-FQN precedence questions unrelated to this bug.
Nested types are addressable here by qualified name, which is what demonstrates the fix.
Safety
Testing
uv run pytest tests/ --tb=short -q(2996 passed, 9 skipped, 2 xpassed)CONTAINSparents forDetails.QueryHandlervsEdit.QueryHandlerA.B.C.M)Shadower.D.Ebeats top-levelD.Eone of several namespaces in the file, or a type rather than a namespace
uv run ruff check code_review_graph/uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional