Skip to content

Share one escalation loop across the member-reference builders - #4144

Merged
siegfriedpammer merged 4 commits into
masterfrom
refactor/disambiguation-ladder
Sep 19, 2026
Merged

siegfriedpammer merged 4 commits into
masterfrom
refactor/disambiguation-ladder

Conversation

@siegfriedpammer

@siegfriedpammer siegfriedpammer commented Sep 17, 2026

Copy link
Copy Markdown
Member

Emitting a member reference starts from its shortest spelling, which in the output's name-lookup context may bind to a different member than the one the IL referenced. Six places re-resolved what they were about to write and, while it did not bind back, escalated to a more explicit spelling and tried again: field accesses, calls, constructor calls, accessors, and the two method group forms. Each kept its own retry state, and each held targetResolveResult in sync with requireTarget by hand.

Disambiguator now owns that loop. A For* factory per form of reference builds one, escalates and hands it back finished, naming the steps that form prefers and whether it is written with invocation syntax - the one thing an IMethod does not say, since a method group is not, a property access is not even though it is a call in IL, and a parameterized property's accessor is despite being an accessor.

Everything that answers "does this spelling still reach the member" moved with the loop, including the two checks asked standalone (TransformParamsArgument, CheckSimpleCall) and the decision to write type arguments up front for a method whose arguments cannot infer them, which belongs next to the step that takes that guess back. What is left in CallBuilder builds call syntax.

Commits

24489de5f Rename CallTransformation to ReferenceTransformation mechanical
a7f2a98f8 The shared loop and all six sites output-neutral
16196731d One rule for every qualifier changes output
8faa1848d Close the fixtures that hold an assembly open drive-by, tests only

Behaviour

The first two commits change no output. The third does, and is separable.

Whether a reference has to name its target was decided in three places - RequiresQualifier and a copy each in the accessor and call paths - and the copies disagreed twice.

AlwaysQualifyMemberReferences was consulted by every qualifier check except the method group one. That setting is on for the WinForms InitializeComponent method, which is also decompiled with implicit method group conversion off, so every event handler there came out as new EventHandler(Foo) while everything around it was qualified.

A base target was the other. Dropping base. leaves the reference dispatching virtually, which reaches the same member unless the member can be overridden and the IL did not dispatch virtually - so that decides it, not the kind of reference. The copies answered three ways: never for fields (right: a field access does not dispatch), always for accessors and method groups (keeps qualifiers that change nothing), and a virtual-only test for calls (too narrow - an abstract or overriding member dispatches virtually without carrying the keyword). All of them now ask IsOverridable.

Effect on the corpus below: 2132 of 37553 types. Within those, base. qualifiers go 16247 -> 1935 - mostly redundant ones dropped, and a handful restored where master wrongly dropped them (base.RetryForException, base.Dispose, base.Remove). base.NonVirtualMember loses a qualifier it never needed; base.AbstractMember and base.OverridingMember keep the one they do. Worth weighing: the unqualified form recompiles to callvirt where the original was call, which is behaviourally identical on a non-null this but not byte-identical IL.

Verification

  • ICSharpCode.Decompiler.Tests: 3697 tests, 0 failed. The qualifier cases that round-trip are in Pretty/QualifierTests (18 configs); the spellings that change are Ugly/BaseQualifier and Ugly/QualifiedMethodGroup (8 configs each).
  • TestTools/decompdiff against master over 167 assemblies, 37553 types, 4057601 lines: the refactor commit alone changes 0 types; the tip changes 2132, all of them the qualifier rule, with 0 new errors.
  • TestTools/nugetfuzz over the same 200 packages, 41108 types, 4970/4970 references resolved: no crashes, asserts or exceptions. Its findings are pre-existing name leaks, identical in count on both sides (2544).

Not covered: RoundtripAssembly is gated to Windows and did not run locally, so recompilation of the decompiled output is unverified here. The skipTargetCast escape - a protected member declared in a base type, where the qualifier has to be dropped again rather than cast - is the one escalation that is not monotone, and the corpus gives no evidence of reaching it.

Drive-by

A passing Windows run carries half a dozen TearDown failed ... because it is being used by another process. The cause is not a timing race: a PEFile keeps its assembly mapped until disposed, and three fixtures opened one and never did - DuplicateAssemblyReferenceTests both directly and for every reference its resolver returns, TypeForwarderResolutionTests for everything the resolver probes, and LocationsInAstTests for the module behind each decompiler. Their teardowns then delete the directory those files live in. They now dispose what they opened, the way WpfProjectExportTests already did.

Deleting is cleanup either way, so every custom teardown now goes through RepeatOnIOError rather than straight at the file system, and that helper no longer rethrows: its last of five attempts was deliberately unguarded, and four retries ten milliseconds apart is a short window for a handle a virus scanner still holds. Pre-existing on master, unrelated to the rest of this PR, and separable.

Follow-ups, not in this PR

The three ambiguity checks still differ in their tails; the method group one checks neither BestCandidateErrors nor IsAmbiguous where the other two check both, which is worth measuring on its own. params expansion is still decided outside the loop by a one-shot check, so the loop can detect that the expanded form is wrong but cannot collapse it back. And the escalation is still a greedy walk in a fixed order, so the spelling it settles on is not minimal - the TODO in Escalate is the older note asking for that.


Written by Claude Code (Opus 5) working from my direction, reviewed by me before opening.

@siegfriedpammer
siegfriedpammer force-pushed the refactor/disambiguation-ladder branch 8 times, most recently from a4b1372 to 1d1c3e1 Compare September 18, 2026 15:19
The enum names the ways a member reference can be made more explicit so that
it resolves back to the intended member. Field accesses and method groups
escalate the same way calls do, and are about to share this vocabulary, at
which point "Call" in the name would describe only one of its users.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
Emitting a member reference starts from its shortest spelling, which in the
output's name-lookup context may bind to a different member than the one the IL
referenced. Six places re-resolved what they were about to write and, while it
did not bind back, escalated to a more explicit spelling and tried again: field
accesses, calls, constructor calls, accessors, and the two method group forms.
Each kept its own retry state, and each held targetResolveResult in sync with
requireTarget by hand.

Disambiguator owns that loop. A For* factory per form of reference builds one,
escalates and hands it back finished, naming the steps that form prefers and
whether it is written with invocation syntax - the one thing an IMethod does
not say, since a method group is not, a property access is not even though it
is a call in IL, and a parameterized property's accessor is despite being an
accessor.

Everything that answers "does this spelling still reach the member" moves with
the loop, including the two checks that are asked standalone, and the decision
to write type arguments up front for a method whose arguments cannot infer
them, which belongs next to the step that takes that guess back. What is left
in CallBuilder builds call syntax. CheckSimpleCall now takes its compilation
from the resolver rather than the type system; they are the same one.

The escalations are already named by ReferenceTransformation, which is also the
record of what has been applied, so the allowed-set gating becomes one test and
clearing a bit to stop a step repeating becomes unnecessary: one use spends a
step, which is also what ends the loop. Spent is tracked apart from what is in
effect for the single escalation that is not monotone, where a protected member
of a base type cannot be reached through a cast target and the qualifier has to
be dropped again.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
Whether a member reference has to name its target was decided in three places:
RequiresQualifier, and a copy each in the accessor and call paths. They
disagreed in two ways that both look like oversights.

Only the method group path failed to consult AlwaysQualifyMemberReferences.
That setting is on for the WinForms InitializeComponent method, which is also
decompiled with implicit method group conversion off, so every event handler
there came out as new EventHandler(Foo) while everything around it was
qualified.

A base target was the other, and the copies answered it three ways: never for
fields, always for accessors and method groups, and a virtual-only test for
calls. Dropping "base." leaves the reference dispatching virtually, which
reaches the same member unless the member can be overridden and the IL did not
dispatch virtually - so IsOverridable decides it, not the kind of reference.
Never is right only because a field access does not dispatch; always keeps
qualifiers that change nothing; and virtual-only is too narrow, since an
abstract or overriding member is dispatched virtually without carrying the
keyword. That last one drops the qualifier on base.OverridingMember, which
reaches this type's member instead of the base one the IL named, and the
escalation cannot catch it: with nothing overridden here the shorter spelling
binds to the same member and looks correct.

QualifierTests already covered base references, but only where the calling type
shadows or overrides the member, so the qualifier was always required and
over-removal could not show up. It now also covers a member overridden further
up the hierarchy. The spellings that change instead of round-tripping are
Ugly test cases.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
A PEFile keeps its assembly mapped until it is disposed, and three fixtures
opened one and never did: DuplicateAssemblyReferenceTests both directly and
for every reference its resolver hands back, TypeForwarderResolutionTests for
everything the resolver probes, and LocationsInAstTests for the module behind
each decompiler. Their teardowns then deleted the directory those files live
in, which cannot work while the mapping is open, so a Windows run reported a
failed teardown for a run that otherwise passed. They now dispose what they
opened, the way WpfProjectExportTests already did.

Deleting is cleanup either way, so it goes through RepeatOnIOError rather than
straight at the file system, and that helper no longer rethrows: its last of
five attempts was deliberately unguarded, and four retries ten milliseconds
apart is a short window to wait for a handle that a virus scanner or the
indexer still holds. It now backs off to about a second and a half and, failing
that, reports and leaves the file behind, which costs nothing.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
@siegfriedpammer
siegfriedpammer force-pushed the refactor/disambiguation-ladder branch from 1d1c3e1 to 8faa184 Compare September 19, 2026 06:48
@siegfriedpammer
siegfriedpammer merged commit c7dcdef into master Sep 19, 2026
15 checks passed
@christophwille
christophwille deleted the refactor/disambiguation-ladder branch September 20, 2026 07:57
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.

1 participant