Fix #3850: recover ReadOnlySpan<T> array literals from the legacy lazy cache#3851
Fix #3850: recover ReadOnlySpan<T> array literals from the legacy lazy cache#3851sailro wants to merge 3 commits into
Conversation
… legacy lazy cache
Roslyn caches a ReadOnlySpan<T> created from an array literal in a
<PrivateImplementationDetails> field on target frameworks without
RuntimeHelpers.CreateSpan (e.g. .NET Framework / netstandard2.0 + System.Memory):
object obj = <PrivateImplementationDetails>.cache;
if (obj == null) {
obj = new char[] { '\r', '\n' };
<PrivateImplementationDetails>.cache = (char[])obj;
}
... new ReadOnlySpan<char>((char[])obj) ...
The decompiled output referenced the compiler-synthesized
<PrivateImplementationDetails> type, whose escaped name is not expressible in C#
and is never declared, so the output failed to recompile (CS0400).
The modern RuntimeHelpers.CreateSpan form was already handled
(TransformRuntimeHelpersCreateSpanInitialization); this adds the analogous
handling for the legacy lazy-cache form, mirroring CachedDelegateInitialization
(which collapses the same lazy-static-field cache for anonymous-method delegates).
Once the cache is collapsed, the existing array-initializer transforms recover
the array literal, so the <PrivateImplementationDetails> reference disappears.
Test: ILPretty/CachedReadOnlySpanInitialization.
There was a problem hiding this comment.
Pull request overview
This PR addresses a decompilation correctness issue on frameworks without RuntimeHelpers.CreateSpan by collapsing Roslyn’s legacy lazy static cache pattern used for ReadOnlySpan<T> array literals, preventing unexpressible/undeclared <PrivateImplementationDetails> references in the generated C#.
Changes:
- Add
CachedReadOnlySpanInitializationblock transform to collapse the legacy lazy-cache initialization pattern. - Register the new transform immediately after
CachedDelegateInitializationin the IL transform pipeline. - Add an ILPretty regression test (
CachedReadOnlySpanInitialization) with expected C# output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ICSharpCode.Decompiler/IL/Transforms/CachedReadOnlySpanInitialization.cs | New block transform that recognizes and collapses the legacy lazy cache pattern for ReadOnlySpan<T> array literals. |
| ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs | Inserts the new transform after CachedDelegateInitialization in the per-block transform pipeline. |
| ICSharpCode.Decompiler.Tests/TestCases/ILPretty/CachedReadOnlySpanInitialization.il | New ILPretty input reproducing the legacy cached-array pattern (using <PrivateImplementationDetails>). |
| ICSharpCode.Decompiler.Tests/TestCases/ILPretty/CachedReadOnlySpanInitialization.cs | Expected decompiled C# output fixture for the new ILPretty test. |
| ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs | Adds a new NUnit test entry to run the new ILPretty case. |
| ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj | Includes the new .il test input in the project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| @@ -0,0 +1,126 @@ | |||
| // Copyright (c) 2024 ICSharpCode | |||
There was a problem hiding this comment.
| // Copyright (c) 2024 ICSharpCode | |
| // Copyright (c) 2026 Sebastien Lebreton |
| static bool MatchIsNull(ILInstruction condition, ILVariable v) | ||
| { | ||
| // comp(ldloc V == ldnull) | ||
| if (condition.MatchCompEquals(out var left, out var right) && right.MatchLdNull() && left.MatchLdLoc(v)) |
There was a problem hiding this comment.
the catch-all form to solve this would be a while (MatchLogicNot) loop that unpacks the negation. This is a proven pattern used throughout the codebase. After that use MatchCompEqualsNull to catch left == null and null == right at the same time.
| { | ||
| if (!context.Settings.ArrayInitializers) | ||
| return; | ||
| for (int i = context.IndexOfFirstAlreadyTransformedInstruction - 1; i >= 1; i--) |
There was a problem hiding this comment.
i >= 1 made me raise an eyebrow until I read the pattern further down... might be worth a comment.
| static bool DoTransform(Block block, int i, IfInstruction inst, BlockTransformContext context) | ||
| { | ||
| // The if must be a simple `if (...) { ... }` (no else) with a two-instruction body. | ||
| if (!inst.FalseInst.MatchNop() || inst.TrueInst is not Block trueBlock || trueBlock.Instructions.Count != 2) |
There was a problem hiding this comment.
nit: could use property patterns to make it shorter
| return false; | ||
| // storeBeforeIf: stloc V(ldobj(ldsflda cacheField)), cacheField a compiler-generated static field. | ||
| if (block.Instructions[i - 1] is not StLoc storeBeforeIf || storeBeforeIf.Value is not LdObj ldobj | ||
| || !ldobj.Target.MatchLdsFlda(out var cacheField) || !cacheField.IsCompilerGeneratedOrIsInCompilerGeneratedClass()) |
There was a problem hiding this comment.
should probably be split into multiple sequential if statements, like we do in other transforms, for readability
| } | ||
| // V is assigned exactly twice (before-if load + in-if init) and read exactly three times | ||
| // (null-check condition + cache write-back + one real downstream usage), with no address-of. | ||
| if (v.StoreCount != 2 || v.LoadCount != 3 || v.AddressCount != 0) |
There was a problem hiding this comment.
this check could be moved up, so we don't spend time on matching the full pattern, if this simple check fails at the end.
| } | ||
|
|
||
| [Test] | ||
| public async Task CachedReadOnlySpanInitialization() |
There was a problem hiding this comment.
is there a reason why this is not a normal C# Pretty test? ILPretty tests have the disadvantage that they don't detect changes in Roslyn's pattern detection, when we update or add a new compiler version.
| @@ -0,0 +1,55 @@ | |||
| .assembly extern mscorlib | |||
| { | |||
There was a problem hiding this comment.
How did you generate this IL? It looks like it was generated by an LLM. Please use a C# program as starting point and "convert" it to IL using ildasm, ilspycmd or https://lab.razor.fyi. This way we ensure we actually test against the IL shape Roslyn emits (or emitted at a certain point in time) and the produced IL is generated by a decade old battle-tested tool instead of a "potentially hallucinating DWIM slot machine".
Fixes #3850.
On target frameworks without
RuntimeHelpers.CreateSpan(.NET Framework, or netstandard2.0 + System.Memory), Roslyn caches the backing array of aReadOnlySpan<T>built from an array literal in a lazy<PrivateImplementationDetails>static field:The decompiled output referenced the compiler-synthesized
<PrivateImplementationDetails>type, which is never declared and whose name is not expressible in C#, so the output failed to recompile (CS0400 in whole-project output).Fix
A new
CachedReadOnlySpanInitializationblock transform collapses the lazy cache, mirroringCachedDelegateInitialization(which collapses the same lazy-static-field cache for anonymous-method delegates) and running right after it. Once the cache is collapsed, the existing array-initializer transforms recover the array literal, so the<PrivateImplementationDetails>reference disappears - the same result the modernRuntimeHelpers.CreateSpanpath (net7+) already produces.Before (does not recompile):
After:
The transform only fires on a compiler-generated static cache field with this exact lazy-init shape, so it does not touch the existing
<PrivateImplementationDetails>switch-on-string cache (covered by the existingCS1xSwitch_Debugtest) or user code.Test
ILPretty/CachedReadOnlySpanInitializationexercises the legacy lazy-cache IL and checks the recovered output.Real-world occurrence: Markdig
CodeInlineParser(net462).