Skip to content

Harden C# type-name shadow resolution - #3869

Merged
richlander merged 47 commits into
mainfrom
fix/3800-type-name-shadowing
Aug 12, 2026
Merged

Harden C# type-name shadow resolution#3869
richlander merged 47 commits into
mainfrom
fix/3800-type-name-shadowing

Conversation

@richlander

Copy link
Copy Markdown
Owner

Closes #3800.

C# declaration output now plans type-name collisions against the complete selected output unit, including caller imports, namespace visibility, primary constructors, nested declarations, generic parameters, attributes, and delegates. Ambiguous or shadowed identities remain qualified, and qualified fallbacks receive global:: only when ordinary qualification would rebind.

The public policy API and default remain unchanged. Safe same-namespace, sibling-type, unrelated-namespace-child, contextual-import, and cross-declaring-namespace shortening remain enabled; no aliases are synthesized.

Validation:

  • dotnet run --project src/ILInspector.CSharp.Tests -c Release — 512 passed
  • dotnet build dotnet-inspect.slnx -c Release
  • dotnet run --project src/ILInspector.Decompiler.Tests -c Release --no-build -- -class ILInspector.Decompiler.Tests.ReturnToSenderPrototypeTests — 197 passed

richlander and others added 5 commits August 6, 2026 23:18
Plan namespace and lexical shadowing across complete output units, preserve qualified identity when imports are unsafe, and emit global aliases when ordinary qualification would rebind.

Add formatter, type-printer, nested-scope, primary-constructor, attribute, delegate, and compile-back regressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Fold delegate planning into the existing declaration entry point and keep planner context as fields so the pinned RenderMemberUnit token remains stable. Update the unaffected downstream corpus token for the intentional helper-method additions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Review round at 22ec7f9a6 found three blockers:

  • GPT-5.6 Sol: leading keyword namespace roots did not match escaped replacement keys; dotted attribute enum values could be prefix-shortened.
  • Claude Opus 5: the same keyword-root mismatch, plus delegate declarations had regressed hostile-name containment.

Resolved in 29a83b460: replacement plans now carry separate qualified and shortened spellings, preserve qualified dotted attribute values while still applying global::/keyword escaping, match raw and escaped keyword-root spellings without double rewriting, escape emitted namespace/import syntax, and restore contained delegate/type names while preserving nested-name separators. Added close-negative and policy coverage. Local Release build and the focused C#, ReturnToSender, and authored-corpus gates pass. A new fixed-head review round will run after current-head CI is green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Review round at 29a83b460 converged on one blocker from both GPT-5.6 Sol and Claude Opus 5: a type declared in the global namespace was not treated as an ancestor of namespaced output units, so its simple name could capture a qualified namespace root without triggering global::.

Resolved in 09c6e0ae2 by treating the global namespace as an ancestor of every non-global namespace and adding the missing cross-namespace regression. Local Release build, 527 C# tests, ReturnToSender, and both authored-corpus gates pass. A new fixed-head review round will run after current-head CI is green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
…adowing

# Conflicts:
#	tools/DecompilerHarness/corpus/two-row-authored-corpus.jsonl
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Round 5 findings reconciled at 8e2dd48fe (with current-main integration c63d7a90b and corpus identity follow-up 5dbe27cf0):

  • GPT-5.6 Sol — explicit-interface qualifiers bypass planning: fixed. Interface qualifiers now participate as strong, qualification-only references, preserving the existing fully-qualified ReturnToSender contract while adding global::/keyword correction when lookup requires it.
  • GPT-5.6 Sol — delegate return attributes dropped: fixed. Return attributes are emitted and planned on delegate declarations.
  • GPT-5.6 Sol — hidden custom attributes generated diagnostics: not fixed as proposed. Reproduction showed hidden attributes remain necessary binding evidence: an omitted App.Foo attribute can prove that Foo.Bar would bind through App.Foo. The planner therefore keeps hidden attribute evidence while emitting no hidden attribute text; regression coverage pins the required global:: result.
  • GPT-5.6 Sol — global namespace/using root conflicts undiagnosed: fixed. Global type conflicts with emitted namespace or using roots now produce deduplicated diagnostics.
  • GPT-5.6 Sol — arbitrary attribute casts such as (External.Target)~0: dismissed as outside the producer contract. Metadata enum constants are emitted as numeric casts, which remain covered. Treating arbitrary parenthesized expressions as types would invent provenance.
  • Claude Opus 5 — sibling-member reference evidence was per-member: fixed. Complete-unit referenced type evidence now reaches every emitted member plan.
  • Claude Opus 5 — ancestor namespace type evidence omitted: fixed. Ancestor-or-self namespace references now shadow conflicting global roots.

A follow-up audit caught and corrected two overreaches before settlement: hidden attributes remain analysis evidence, and explicit-interface qualifiers remain non-shortenable. Exact-head local evidence after integrating current main: Release solution build; C# suite 388/388; ReturnToSender 197/197; authored corpus gates 255/255; full decompiler suite 4867/4867. Formal round 6 will run only after current-head CI is green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Round-six reconciliation at settled head 155218b3f:

  • Constructed base/interface types127a76428 now extracts the constructed root and each generic argument independently, preserving nested paths such as N.Outer<T>.Middle.Inner<U>.
  • Unit-wide attribute evidence — primary-constructor, assembly, module, hidden, delegate, and synthesized System.Obsolete references now participate in the same binding context and global-root diagnostics.
  • Explicit-interface provenance — qualification-only interface qualifiers remain qualified when the same identity also appears in a shortenable role.
  • Delegate policy — signature types shorten normally; parameter/return attribute types remain qualification-only; dual-provenance attribute identities no longer erase genuine signature evidence.
  • Overlapping replacements — rewriting now performs one longest-match pass over the original text and retains identity entries, so a kept-qualified longer name cannot be rewritten by a shorter prefix.
  • Binding-evidence audit follow-ups — qualification-only references still contribute when their namespace is visible or imported, and a referenced type full name cannot be emitted as a namespace import.
  • Compatibility follow-up155218b3f preserves historical bare [Obsolete] output when safe while still emitting System.Obsolete/global::System.Obsolete for sibling and global-root conflicts.

Validation at this head: Release solution build; 402 C# tests; 3,128 CLI tests; 557 ReturnToSender/authored-corpus focused tests; 4,867 full decompiler tests. Authored-corpus identity was remeasured (CSharpFormatter.FormatMember = 0x0600013A). Exact-head CI run 31233612074 is green after rerunning one unrelated Windows package-acquisition timeout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Round-seven reconciliation, now settled at exact head 6810f08f0:

  • Primary-constructor attribute rebinding — fixed in 9e7675026. Declared attribute names no longer derive unit imports, standalone shortening checks the implicit Attribute suffix, and contextual collision analysis only considers namespaces that can actually be unqualified. [External.Marker] therefore cannot rebind to a visible MarkerAttribute, while safe standalone [Marker] shortening remains supported.
  • Generic global type false conflict — fixed in 9e7675026. Only arity-zero global declarations block namespace roots; Foo<T> no longer reports a conflict with namespace Foo.Bar.
  • Hidden-attribute emitted-reference diagnostic — fixed in 9e7675026. Global-root diagnostics are now attached to replacements and materialize only when the conflicting qualified spelling is actually rendered. Hidden attributes remain binding evidence without claiming omitted text was emitted.
  • Assembly/module attributes before using directives — dismissed as pre-existing. A clean Roslyn reproduction produced byte-for-byte identical invalid ordering at origin/main (de56a0e11) and the reviewed head. This branch did not create or worsen that behavior, so it is outside PR Harden C# type-name shadow resolution #3869.
  • Claude Opus 5 seat — clean. Its differential real-assembly/Roslyn checks found no new binding failures and confirmed the one-pass replacement, generic parsing, explicit-interface, delegate, attribute, and global-root behavior.

After the findings, current main was integrated in merge commits c301e8fc7 and 6810f08f0. Validation at final head: Release solution build; 406 C# tests; 3,186 CLI tests; 4,870 full decompiler tests; corpus identity remains CSharpFormatter.FormatMember = 0x0600013A. Exact-head CI run 31272746033 is green, and the PR is mergeable.

Preserve binding across imported nested type paths, unit-wide attribute suffix lookup, raw string literals, and global nested declarations.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Round eight reconciliation

Round eight began against 6810f08f0, but main moved while the reviewers were reading. Its results therefore cannot satisfy the fixed-head review gate. I reproduced and addressed its attributable findings, then merged current main and validated the settled head 5e0952382.

Findings resolved

  • Imported nested attribute-value paths: retain a relative path only when exactly one imported namespace supplies the declared nested path; ambiguous providers no longer authorize the relative spelling.
  • Unit-wide attribute lookup: import safety now considers attribute names symmetrically, including implicit Attribute suffix lookup and declarations in the namespace being rendered.
  • Raw string preservation: the type-name rewriter now skips raw, interpolated, verbatim, and nested literals through the shared literal scanner.
  • Global nested declarations: exact declared nested paths no longer receive a false unresolvable namespace-root diagnostic.

The fixes are in 92dd76337; 5e0952382 merges current main without conflict.

Quick-read disposition

A subsequent non-formal MAI-Code quick-read reported that a dotted attribute value should itself synthesize a using for a matching declared nested type. I reproduced the output but dismissed the requested behavior: dotted attribute values are intentionally qualification-only evidence and cannot prove namespace-versus-nested-type provenance. A negative regression now verifies that attribute-value text alone does not drive an import; an independently established safe import still preserves the declared nested path.

Current-head evidence

  • Release solution build: clean
  • C# planner suite: 414 passed
  • CLI suite: 3,197 passed, 4 platform skips
  • Decompiler suite: 4,875 passed, including ReturnToSender, authored-corpus integrity, and fidelity gates
  • Authored corpus: RenderMemberUnit remains 0x06000092; CSharpFormatter.FormatMember was remeasured and updated to 0x0600013d
  • Exact-head CI: run 31299087052, passed for 5e09523822c74d8ca9d7f891d85ed887307814ab

Round eight is reconciled but not clean at the current fixed head. Per the six-round limit, a ninth formal round requires fresh user approval.

Only exempt a declared simple name when it identifies one exact top-level declaration, preserving qualification across sibling namespaces, generic arity, and nested paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Round nine reconciliation

Round nine reviewed exact base 945e7502f through exact head c1b9f1699 after current main was integrated, the PR was mergeable, and exact-head CI was green.

Claude Opus 5 finding — fixed

Claude found a high-severity silent rebinding case in the declared-type import exception. Given declarations for App.Marker and N.Sub.Marker, a reference to N.Sub.Marker could emit using N.Sub; plus bare Marker; inside namespace App, that field silently bound to App.Marker. The exception used separate flat bags of simple and full names, so “some declaration has this simple name” and “this full name is declared” could be satisfied by different declarations.

I reproduced the semantic mismatch on the untouched review head and confirmed the base kept N.Sub.Marker qualified. Commit 38a09b7eb now permits the exception only when the simple name maps to one exact top-level declaration identity. Declaration paths normalize generic arity, and nested declarations remain non-importable so they cannot authorize illegal using directives for containing types. Regressions cover the root collision, generic declaration, and nested declaration cases.

GPT-5.6 Sol finding — dismissed as pre-existing

GPT reported that CSharpTypeNamePolicy.Qualified emits synthesized [Obsolete] without using System;. The output is real, but exact base and exact head both emit the same [Obsolete]; Claude independently reached the same base comparison. It is therefore not attributable to this PR and was not changed here.

A non-formal MAI-Code quick-read of the follow-up found no additional defect.

Fixed-head evidence for 38a09b7eb

  • Release solution build: clean
  • C# planner suite: 417 passed
  • CLI suite: 3,208 passed, 4 platform skips
  • ReturnToSender/compile-back gate: 599 passed
  • Full decompiler suite: 4,889 passed, including authored corpus and fidelity
  • Exact-head CI: run 31317866789, passed

Round nine produced and resolved one blocking finding, so its reviewed head moved and the round is not clean at the current SHA. A tenth formal round requires fresh user approval.

@richlander

Copy link
Copy Markdown
Owner Author

Round-10 carry-forward update: integrated exact main tip eb20768cbed49d907acee004b40922a6f876a082 in merge commit $(git rev-parse HEAD) after confirming the range had no files or behavior overlapping the C# type planner. The user approved carrying the clean round-10 reviews forward. Release solution build, 417 planner tests, and 3,208 CLI tests pass at this merge head. Current-head CI remains required before readiness.

@richlander

Copy link
Copy Markdown
Owner Author

Correction to the immediately preceding carry-forward note: the merge head is c04d4e86b446615ba3f0b0bd5b1845d7f9002b39.

@richlander

Copy link
Copy Markdown
Owner Author

Round-10 carry-forward update: integrated exact main tip f77a127d8a7c2878cd77ada59d6c26f3ecc5b786 in merge commit 0a5966973da660ee97ada0b947b451754f87d24a after confirming zero overlap with the C# type planner and no relevant behavioral dependency. The user approved carrying the clean round-10 reviews forward. Release solution build, 417 planner tests, and 3,208 CLI tests pass at this head. Current-head CI remains required before readiness.

@richlander

Copy link
Copy Markdown
Owner Author

Round-10 carry-forward update: integrated exact main tip 8e6161e85d0004afd0aba398daf1b4909cd8300f in merge commit d269cd40623cdb9b75c33ddc73c4c418c56f9839 after confirming zero overlap and no relevant dependency on C# type planning. The user approved carrying the clean round-10 reviews forward. Release solution build, 417 planner tests, and 3,208 CLI tests pass at this head. Current-head CI remains required before readiness.

Resolve the C# printer and ReturnToSender conflicts by retaining complete-unit binding-safe type planning while carrying the product-owned frozen body replacement range through rendered fragments and compile-back diagnostics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

main moved to 383df6e0bdde36b6641c5c593002c6ef776e4ab7 with #3998, which directly overlapped this PR's CSharpTypePrinter, planner tests, and ReturnToSenderTypePlanner. GitHub reported the PR conflicting and scheduled no CI for d269cd406.

Resolved in merge commit 9e499e9d111a8e984458e9d38280324163834f29: complete-unit binding-safe type planning and its diagnostics remain intact, while the new product-owned CSharpSourceArtifact replacement range is carried through namespace/nested/member/accessor RenderedFragment composition and returned intact by ReturnToSender. This preserves the new frozen-shell harness contract without reverting the planner hardening.

Validation at the merge head: Release solution build; 423/423 C# planner/artifact tests; 4,939/4,939 decompiler tests; and 3,319 passed CLI tests with 4 platform skips. Because this was an overlapping conflict resolution, round 10 cannot carry across it; a fresh round 11 is required after current-head CI is green.

@richlander

Copy link
Copy Markdown
Owner Author

Integrated current main tip ee2050be03f323c9e1a19cd9ae6edde9c7537fb2 before round 11. This range overlapped the new frozen-source artifact and ReturnToSender tests but merged without conflict; it preserves raw authored-body bytes during replacement and adds authored-control attribution. The binding-safe planner composition from 9e499e9d1 remains intact.

Validation at the new head: Release solution build; 424/424 C# planner/artifact tests; and 207/207 focused ReturnToSender prototype tests. Current-head CI remains required before round 11 can begin.

@richlander

Copy link
Copy Markdown
Owner Author

Adversarial review round 11 — clean at 13c2725f1

Reviewed exact range ee2050be03f323c9e1a19cd9ae6edde9c7537fb2..13c2725f11d22bda5283da5136d8f9579c6c51fc in separate fixed-head worktrees with GPT-5.6 Sol and Claude Opus 5. Both reviewers found no significant defects.

The round focused on the post-round-10 integration between complete-unit binding-safe name planning and product-owned frozen source artifacts. Reviewers verified replacement-range propagation through namespaces, multiple units, nested types, constructors, methods, property/event accessors, attributes, and prologues; exact non-target-byte preservation with authored CRLF/literal content; recursive planner context and accessor-attribute formatting; ReturnToSender's direct use of SourceArtifact without shell recomposition; and planner diagnostic replacement rather than duplication on full-body rerenders.

Additional evidence included 19 Roslyn symbol-identity scenarios, seven typed range/AST probes, 5,462 randomized differential units versus the exact base, 424/424 C# planner/artifact tests, 207/207 focused ReturnToSender tests, 281/281 authored-control/corpus checks, and corpus token verification. The fuzz run reduced silent misbinds from 29 at base to 3 at head; all three remaining cases were byte-identical pre-existing behavior.

Exact-head ci-required passed in run 31451943409. Round 11 is clean at the pushed head 13c2725f11d22bda5283da5136d8f9579c6c51fc.

richlander and others added 14 commits August 11, 2026 12:22
Preserve complete-unit binding-safe planning and frozen source artifacts while adopting collision-safe immutable sorted using sets.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Integrate the repository-wide .NET 11 Preview 6 CI pin after the Preview 7 channel rollover invalidated existing union patterns.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Integrate annotated source node-kind and printer range-map changes after validating the type planner and compile-back source artifact boundary.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Treat unqualified type-position names as collision evidence, preserve ambiguous parenthesized attribute values, and recognize declared paths visible through ancestor namespaces.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Avoid shortening a type declared in another namespace when any second namespace is imported, since namespace-only using inputs cannot prove that the simple name remains unambiguous.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Preserve qualification for delegate and primary-constructor references when a second configured or derived namespace can make their newly shortened names ambiguous.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Keep delegate constraints and primary-constructor types qualified unless configured, declared, or existing unit evidence establishes their dotted prefix as a namespace.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3787daf2-0222-4f9a-ab24-90df5356dccc
@richlander

Copy link
Copy Markdown
Owner Author

Review reconciliation for the final fixed-head round:

  • Reviewed head: dabc0142f3c2def51dbca434180888b6eed61301 on main 84c1c172677830f8b27353df139b4fe6a1211147.
  • Claude Opus 5 returned clean after compiler-backed and semantic fuzzing.
  • GPT-5.6 Sol's remaining synthesized-Obsolete concern was reproduced against exact base and head. Both policies generated byte-identical source on base and head and silently selected the sibling ObsoleteAttribute; this is a real pre-existing planner limitation, not a regression from this PR. The earlier attributable signature/import findings are fixed in the reviewed head.
  • Main through 923cc84e548583cd4df646ac2ad3c45b2d4d7931 was integrated as 62b4c809f20801d17e8fcbc2f9d2dc1d1abbecee. The intervening changes did not touch or alter the C# planner contract, and the user explicitly approved carrying the clean reviews forward.
  • Exact-head validation at 62b4c809f: Release solution build; C# planner 437/437; full decompiler 5,031/5,031; full IL round-trip 23/23; CLI 3,444/3,444 with four environment skips; ci-required passed in run 31620616235.
  • Main subsequently advanced to b921298360364de5ace8ec152e1f1ce5c3c9dbbf with call-cycle findings, workflow corpus docs, and flags-enum fixtures. It has no file or behavior overlap with this PR. The user explicitly chose to carry the clean reviews and green current-head CI forward without another integration.

Ready to merge

@richlander
richlander merged commit 2b7a2b8 into main Aug 12, 2026
10 checks passed
@richlander
richlander deleted the fix/3800-type-name-shadowing branch August 12, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden C# type-name shortening for lexical shadowing

1 participant