Load_Dependencies_Resolves_References_And_Keeps_Them_In_The_List timed out on a loaded
CI agent. The idle predicate it waited on also covers the dispatcher queue, and
LoadDependenciesAsync ends with RefreshDecompiledView, so the test was waiting for a
decompilation to finish - work whose duration is a property of the machine, not of the
condition being asserted. Instrumenting the wait shows every assembly already loaded on
the first poll while dispatcher jobs stay queued for seconds, so the loads were never the
holdup.
Waiting for the list to show the resolved dependencies drops the dependency on machine
speed: under a deliberately shortened one-second deadline the previous wait failed every
run and this one passed every run.
Assisted-by: Claude:claude-opus-5:Claude Code
Obfuscators put arbitrary characters into BAML strings, and XML 1.0 has no
representation for most control characters - a numeric character reference is invalid
for them too. Writing such a document threw ArgumentException from XmlWriter, which
loses the resource on project export and shows an exception instead of the page in the
UI. The escapes are spelled the way the C# output spells them, so one convention covers
both languages. Namespace URIs have to be escaped where the XNamespace is created rather
than in the final pass: the URI is baked into every element name built from it, so
patching only the xmlns declaration would desync the two. Characters XML can carry stay
untouched, so ordinary documents decompile byte-identically.
Every BAML stream of an assembly lives in one .resources container, and the recovery
around resource writing sat outside the loop over its entries, so a single page that
could not be written discarded every other page sharing the container with it.
Assisted-by: Claude:claude-opus-5:Claude Code
An assembly can map one CLR namespace to several XML namespaces: PresentationFramework
maps its namespaces to both the winfx/2006 and the netfx/2007 presentation namespace. The
fallback used when the BAML xmlns records name no namespace for a type always preferred the
winfx/2006 one, so a document that binds the default prefix to netfx/2007 ended up with a
root start tag that declares one presentation namespace and needs the other for its own
name, which XmlWriter rejects. Only the root carries the xmlns declaration, which is why
skipping XClassRewritePass worked around it.
Assisted-by: Claude:claude-opus-5:Claude Code
GetPointerElementType existed because a pointer passing through a stack
slot could be typed IntPtr: ILReader replaced the slot type with
FindType(StackType) whenever the inferred type did not match the stack
type. With InferType() implemented on every ILInstruction that fallback
is gone (FlushExpressionStack now asserts the inferred type is
stack-accurate), so the target's inferred type is precise and the
definition chain no longer needs to be walked. Merged stack slots were
never recovered by the helper anyway (it required a single store).
Disabling the PointerType arm makes the uint*/byte* deconstruction
fixtures fail, so the sign-agnostic stobj.Type fallback remains guarded.
Assisted-by: Claude:claude-fable-5:Claude Code
Change InferType() to an abstract method and implement for every ILInstruction.
With this change, we now always have enough information to create a variable of an appropriate type to store the result of evaluating the instruction.
This previously was not the case for instructions producing "other value type", for which the stacktype-based fallback incorrectly produced `object`.
The naming rules have been warnings since they were introduced; they are
advisory conventions rather than build gates, and the noise they add to
every file listing outweighs their value.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The nugetfuzz package cache is usually where the corpus itself came from,
so it holds exactly the dependency closure a corpus assembly references;
probing it resolves references the machine-wide NuGet cache does not have.
Reference assemblies name GAC-only internals such as SMDiagnostics and
System.ServiceModel.Internals, which ship in no pack and no package.
Reporting them as missing suggested --refs could supply them, so the
staging walk now tracks whether a name was reached through a reference
pack and drops those from the report.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
C# 12 allows both on the explicitly typed parameter list of a lambda, and
nowhere else: an anonymous method cannot declare either, and neither can a
lambda whose parameter list is about to be dropped. Guarded by a setting so
the output stays valid for earlier language versions.
Only what the anonymous function's own metadata declares is written. A lambda
may state a default the delegate does not have, a different one, or none where
the delegate has one, and reflection over the lambda's method reports what the
lambda declared - so filling either in from the delegate's Invoke would make
the recompiled assembly describe itself differently from the original. Call
sites are unaffected either way, because they bind against the delegate, which
still declares both.
Roslyn writes ParamArrayAttribute on the anonymous function's own method only
from version 5 on; before that it stands on the delegate type alone, where it
is not the lambda's to restate, so the fixture guards those cases on ROSLYN5.
The correctness test reads the metadata back through reflection, which is the
only place the difference is observable.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The C# 10 grammar only allows attributes on a lambda or its parameters
when the parameter list is parenthesized, but LambdaNeedsParenthesis
predates attribute support and only considered the single parameter's
type and modifiers. An attributed lambda whose parameter type is erased
for being anonymous therefore printed as '[My] a => a.X', which does not
parse. Latent since attributed-lambda decompilation was added: every
other attributed lambda has explicitly typed parameters, which already
force the parenthesized form.
Assisted-by: Claude:claude-fable-5:Claude Code
A missing value-type definition makes ILReader insert a Ref-to-Unknown conversion before instance calls. Preserve the managed-reference receiver so C# output does not fall back to invalid ref casts or unsafe pointers.
Assisted-by: Codex:gpt-5.6-sol:Codex
Assigning through a ref-conditional, (cond ? ref a : ref b) = value, was
emitted without the parentheses, so it re-parsed as
cond ? ref a : (ref b = value) and failed to compile (CS8156 / CS0201).
The target was only parenthesized above assignment precedence, but a
conditional binds tighter than assignment, so the check let it through.
Require the assignment target to have precedence above the conditional
operator. Ordinary lvalues (locals, fields, indexers) are primary
expressions and are unaffected; the postfix ++ form already parenthesized
correctly via unary precedence. Covers plain and compound assignments alike.
Assisted-by: Claude:claude-opus-4-8:Claude Code
Invariants that involve types (stack type of a variable against its IType,
the element type of an array access, the operand types of a comparison)
need a type system to resolve them against, and the only correct one is the
type system the instruction tree was decoded with. Until now CheckInvariant
took only the phase, so such a check had no compilation to use:
DeconstructInstruction.CheckInvariant called IsAssignment with a null type
system, which only held up because the targets it sees are ldloc, whose
InferType never touches the compilation; a ldflda-wrapped or pointer target
would have failed inside the invariant instead of reporting a violation.
Every call site already has that type system in scope: the ILReader's
compilation, the ILTransformContext of the running transform, or the
decompiler's own IDecompilerTypeSystem. It is now passed explicitly and the
base implementation asserts it is present, so a future invariant can rely
on it without re-plumbing the callers.
Assisted-by: Claude:claude-fable-5:Claude Code
This way, we don't need the MapToMergedBounds logic to split the merged list back into lower/upper.
Also, this commit avoids the quadratic merge-everything-with-everything else -- instead we use a dictionary to compare only types that are equivalent to begin with.
This is the same approach as Roslyn MethodTypeInference.Fix/AddAllCandidates.
NullPropagationTransform only rewrites "x != null ? x.Chain : fallback"
into "x?.Chain ?? fallback" when the chain's inferred type is a
non-nullable value type, and InferType had no case for ldlen. Array length
therefore came back as UnknownType, so "arr?.Length ?? 0" was left as a
ternary.
The inferred type mirrors ExpressionBuilder.VisitLdLen, which decides
between Array.Length and Array.LongLength from the result type alone.
Found while investigating #3704, where the surviving ternary also keeps the
tested array in a stack slot and strands the typeof of a dynamic call's
static target. That issue is fixed separately in #4072, whose DynamicTests
cases pinned the ternary as expected output; those blocks round-trip
exactly now, so they are gone.
Also carries a review follow-up that missed #4072: the static-target test
in VisitDynamicInvokeMemberInstruction is a plain null check, the way
DynamicInvokeMemberInstruction itself tests the field, rather than a
pattern match binding a name it does not need.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
A local function nested in a lambda can capture closures at two depths:
csc emits it as an instance method on the enclosing method's display
class that takes the lambda's display class as a parameter. Combining
those two capture scopes with FindCommonAncestorInstruction picked the
enclosing method, moving the function out of the lambda that owns the
deeper closure; the variables captured there were then unreachable and
the display-class parameter survived into the output as an undeclared
identifier. Nested capture scopes resolve to the innermost instead,
which a local function can always see - it reaches the outer closure
through the display class it is declared on.
Assisted-by: Claude:claude-fable-5:Claude Code
Three places decided independently whether a backing field would still be
declared, and they disagreed. ReplaceBackingFieldUsage rewrote a constructor
store into a property assignment whenever the property looked collapsible,
without asking whether PatternStatementTransform would actually remove the
declaration; ConvertField printed the "field" keyword on the FieldKeyword
setting alone, ignoring the GetterOnlyAutomaticProperties veto that
MemberIsHidden applies to the same field.
Two consequences, both silent. A setter-less property under
GetterOnlyAutomaticProperties = false kept its declaration and got "field" in
the getter anyway, so the keyword bound to a second synthesized field and the
declared one went unwritten. A settable property under AutomaticProperties =
false had its initializing store turned into a property assignment that
TransformFieldAndConstructorInitializers could no longer lift, leaving the
constructor in the output with an unconverted base-constructor call.
BackingFieldWillBeRemoved is now the single verdict every branch consults, and
it mirrors the transform's own entry gate. A property that keeps explicit
accessors is no longer addressed by name: the store stays a field reference and
becomes the property initializer, which is what field-backed storage means. The
one exception is a setter-less property's constructor store, which C# allows to
be written as an assignment and which has no other expressible form.
IsBackingFieldOfAutomaticProperty now answers through TryGetBackingField instead
of its own name check, so the two directions of "is this field that property's
storage" cannot diverge on staticness or field type. ReplaceBackingFieldUsage
dispatches on the resolve result rather than the identifier's spelling; after
ConvertField the same field appears both as "field" and under its metadata name
carrying the same annotation, so matching the name was matching the wrong thing.
The keyword's own precondition moves into a CanUseFieldKeyword local function.
Seven clauses with comment blocks wedged between them had to be read as one
expression; as one early return per rule the list reads in order.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
ExpressionBuilder.ConvertField prints the C# 14 "field" keyword based on the
FieldKeyword setting alone, but PatternStatementTransform only entered the
property transform when AutomaticProperties was on. With FieldKeyword on and
AutomaticProperties off the backing-field declaration was therefore never
removed, and the output declared the backing field next to accessors already
written in terms of "field".
That output still compiles, which is what makes it dangerous: the keyword binds
to a second, freshly synthesized backing field while the declared one stays
unwritten, so the recompiled assembly has different storage than the input.
Only a CS0169 "field is never used" warning hints at it.
AutomaticProperties governs only whether trivial accessors collapse to
"get;"/"set;"; the declaration removal inside the transform is a separate step
that FieldKeyword alone is enough to justify. The entry gate now mirrors
CSharpDecompiler.MemberIsHidden, which already made the field's visibility
depend on either setting, with GetterOnlyAutomaticProperties vetoing the
getter-only case for both.
Assisted-by: Claude:claude-opus-5[1m]:Claude Code
ICSharpCode.Decompiler.Generators pins a stable Microsoft.CodeAnalysis.CSharp
(a source generator must not reference a Roslyn newer than the host compiler)
and ILSpy.AddIn.VS2022 pins a stable 4.0.1. Stable Roslyn lives only on
nuget.org, but the repo-root NuGet.config mapped Microsoft.CodeAnalysis.*
exclusively to the dotnet-tools feed, which carries only prerelease builds.
Both projects worked around that with a per-project NuGet.config. dotnet
restore honors it, since settings are computed per project directory, but
Visual Studio starts NuGet.config discovery at the solution directory and
never sees it: the first restore in VS on a machine with a cold NuGet cache
fails with NU1103 exactly as reported. Once any successful restore has put
the stable package into the global packages folder, source mapping is
satisfied from the cache and the problem never reappears on that machine,
which is why #3835 was closed as unreproducible.
NuGet consults every source that declares the longest pattern matching a
package id, so mapping Microsoft.CodeAnalysis.* to nuget.org as well as to
dotnet-tools makes both feeds available for the whole family: the stable
versions resolve from nuget.org and the prerelease $(RoslynVersion) from
dotnet-tools. Both per-project configs are then redundant and removed.
Verified with an empty NUGET_PACKAGES and the root config forced as the only
config (simulating VS's discovery): the old config reproduces the reported
NU1103; with the new config the generator, the decompiler tests (prerelease
Roslyn) and the VS add-in (stable 4.0.1 with its Workspaces dependencies)
all restore; restore.ps1 over ILSpy.sln leaves the lock files unchanged.
Assisted-by: Claude:claude-fable-5:Claude Code
The headless UI tests synchronized with the application by pumping a fixed
number of frames (39 loops of RunJobs/Delay across 19 files) and by pressing
at a point computed once from a control's bounds. Both encode how fast the
machine that wrote the test was: on the loaded Windows Debug CI agent the
frame count comes up short and the point goes stale, which is the recurring
timeout in the tree context-menu tests and the reason each such failure was
repaired one test at a time.
Waiters.WaitForIdleAsync replaces the frame loops. It observes the actual
precondition - no dispatcher job queued at Background priority or above, no
assembly still loading in the background sweep, a frame rendered - and
requires it on two consecutive polls so a thread-pool continuation about to
post back is caught as well.
Window.ClickAsync replaces element-targeted MouseDown/MouseUp pairs. It
re-resolves the target on every poll and presses only once the window's hit
test at the click point answers with that target, reporting the point and
what was hit instead on timeout. That diagnostic exposed one vacuous test:
User_Click_On_Visible_Row_Does_Not_Recentre_Viewport clicked the centre of a
row wider than the tree viewport, which lies under the decompiler text view,
so its assertion held without the row ever being clicked. It now clamps the
point to the viewport like the other tree-row clicks.
Clicks at text positions and press-only gutter clicks stay raw; they do not
target an element.
Assisted-by: Claude:claude-fable-5:Claude Code
Right_Clicking_A_Second_Row_Moves_The_Context_Highlight_To_It still timed
out on the Windows Debug CI job, now in the hit-test wait added for the
second right-click: for the full 60s no hit at the precomputed point matched
the captured row container. A light-dismiss overlay that survives one frame
cannot explain that many rendered frames; a container that is no longer the
one on screen can. The test only waits for three assemblies, so the rest of
the list keeps loading on the slow agent while the test runs, and every
insertion reshuffles the rows - re-realising containers and moving them -
after the row and point were captured.
The wait now resolves the row container and the click point on every poll
and matches the hit by node instead of by container identity, and a timeout
reports the point and what was hit instead so a further failure is
diagnosable from the log.
Assisted-by: Claude:claude-fable-5:Claude Code
Siegfried Pammer
dependabot[bot]
Siegfried Pammer
Daniel Grunwald
sonyps5201314