Skip to content

Split StackType.O into StackType.Obj+StackType.VT - #4091

Open
dgrunwald wants to merge 30 commits into
masterfrom
split-obj-vt
Open

Split StackType.O into StackType.Obj+StackType.VT#4091
dgrunwald wants to merge 30 commits into
masterfrom
split-obj-vt

Conversation

@dgrunwald

Copy link
Copy Markdown
Member

Object references and value types are quite different; so let's try keeping them in separate StackTypes throughout the whole compiler.
We have many assertions based on stack type comparisons, so the main effect of this change is to strengthen our invariants.

dgrunwald and others added 22 commits September 5, 2026 22:11
Compute the underlying result type instead of storing it explicitly.
…able-type-replacement.

With the recent improvements, value type variables are guaranteed to already have the correct type in the ILAst, so we no longer need these hacks.
System.Linq.Expressions resolves the user-defined operator behind a binary
factory by metadata name (Expression.Add looks up op_Addition), and the
checked factories reuse the unchecked names: AddChecked also looks up
op_Addition, never op_CheckedAddition. Recording that name at the call site
keeps the mapping next to the factory it belongs to.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
ConvertBind is the only expression-tree converter whose IType nobody reads:
ConvertMemberInit, its sole caller, takes Item1 and discards the rest, and
the member type is recoverable from the Call/StObj it builds anyway.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
Every converter handed back a (Func<ILInstruction>, IType) pair, and the
IType was consumed by matchers that ran before the builder. Now that the
ILAst instructions carry their own types, InferType() on the built operand
answers the same questions, so the type-dependent decisions move into the
builders and the pair collapses to the builder alone. Builders that could
already fail (ConvertArrayIndex) set the precedent for returning null from
inside one; ConvertInstruction propagates that.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
ConvertConstant is its only caller, and with the result type gone the
out parameter that reconstructed it - a switch over LdNull/LdStr/Ldc* -
has no consumer. What remains is a match condition: the two-argument
Expression.Constant overload must pass its type as typeof(T), while the
one-argument overload legacy csc emits for display-class instances has
nothing to check.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
Every converter now states the Expression.* call it matches and the ILAst it
produces, and the argument-count switches label the factory overload each case
stands for. The shapes were read off ILAst dumps of compiled expression trees
rather than from the factory signatures; two branches are documented as
unreachable, since no arithmetic or logical factory declares the four-argument
(left, right, liftToNull, method) overload their case matches.

Also drops the result-type local left in ConvertField, which BuildField
re-derives from the field and the type hint.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
Compiling the same C# twice, once as Expression<Func<...>> and once as a plain
Func<...>, and diffing the two ILFunction bodies exposes where the conversion
reconstructs something the IL reader would never build. Three such cases:

The sign is part of the opcode only for the checked add/sub/mul and for
div/rem/shr; ILReader leaves it at Sign.None elsewhere, while the conversion
took it from the operand type unconditionally.

Expression.MemberInit is an object initializer, not a collection initializer.

Expression.Convert's three-argument overload carries the user-defined
conversion operator - which is how the decimal conversions are encoded - and
that argument was read by nothing, so every such conversion collapsed into an
opaque cast that dropped the method. Emitting the call matches what the
transform already does for decimal arithmetic.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
A conversion from a value type to a reference type produced an opaque cast, so
no box instruction appeared anywhere in the converted tree, while the same C#
compiled as a plain lambda yields box T. The operand type is what box takes,
and it is available from the converted operand.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
An expression tree leaves the boxing of a value-type receiver implicit: it
carries no Convert node for it, because the boxing follows from the method
being declared on a reference type. Enum.HasFlag invoked on an enum value is
the common case, and it reached CallInstruction with an I4 'this' argument
where Obj was expected, which aborts a debug build outright.

Deciding by the target's own type also retires the StackType.VT arm, which
ExpectedTypeForThisPointer never returns. Since box records the type of what
it boxes, an expression-tree cast to that same type in front of it is dropped.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
An unconstrained type parameter has no known IsReferenceType, and since the
stack types were split it takes StackType.VT, so a call on it reached
CallInstruction with a VT 'this' argument where Obj was expected. It might be
a value type at runtime, so it needs the same box a known value type gets.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
A conversion of a small integer type to Int32 returns its operand unchanged,
because such values already occupy an I4 stack slot. The two operands of
`(short a, int b) => a + b` are therefore Int16 and Int32, and requiring them
to be equal rejected the conversion; the whole expression tree was then left
untransformed, or worse, aborted the enclosing method. What
BinaryNumericInstruction requires of its operands is a common stack type.

TryConvertExpressionTree also has to cope with a builder that fails, rather
than dereferencing the lambda it did not get.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The Span<T>/ReadOnlySpan<T> initializer patterns replaced their call with an
array initializer block, so an array stood where a span was expected: the
enclosing leave and any call taking the result saw StackType.Obj against the
StackType.VT the span type demands. The conversion the C# compiler applies is
the implicit operator, and it has to wrap the block rather than sit inside it,
because an ArrayInitializer block must keep ldloc as its final instruction.

Without the operator the conversion cannot be expressed at all, so the
original call is left untransformed instead.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The Span<T>/ReadOnlySpan<T> initializer patterns replaced their call with an
array initializer block, so an array stood where a span was expected: the
enclosing leave and any call taking the result saw StackType.Obj against the
StackType.VT the span type demands. The block now ends in the implicit
conversion the C# compiler applies, which is the one shape besides a bare
ldloc that an array initializer may take; the expression builder keeps the
conversion out of the output but not out of the expression's type.

Naming that operator wants a method looked up by signature rather than by a
predicate over the type's members, so MetadataModule grows a ResolveMethod
overload for it, sharing its signature matching with the metadata path.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The block addresses its allocation through the pointer localloc returns, and
only its result is a Span<T>. Retyping the initializer variable to the span
made every element store ask for a pointer it no longer had, which was papered
over with a conv from the span; once Obj and VT became distinct stack types
that conv had no conversion kind left and the whole method failed to decompile.

The span constructor becomes the block's final instruction instead, so the
element stores keep the pointer they were written against and the block still
evaluates to the span.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
An array initializer standing in for a Span<T>/ReadOnlySpan<T> reaches the
call builder as an implicit span conversion over the array creation, a shape
the params expansion did not know, so a params span argument came out as the
array the compiler had built rather than as the argument list that was
written.

Assisted-by: Claude:claude-opus-5[1m]:Claude Code
@dgrunwald dgrunwald changed the title [WIP] Split StackType.O into StackType.Obj+StackType.VT Split StackType.O into StackType.Obj+StackType.VT Sep 5, 2026
parameters = new List<IParameter>(arrayLength);
for (int i = 0; i < arrayLength; i++)
{
parameters.Add(new DefaultParameter(type3, string.Empty));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preexisting, but weird how the expected parameters are constructed based on the argument types.
Surely they should be based on the unexpanded parameter type instead?

Reviewed with Stampeded!

Comment thread ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs Outdated
Comment thread ICSharpCode.Decompiler/IL/Transforms/ExpressionTransforms.cs
Comment thread ICSharpCode.Decompiler/TypeSystem/MetadataModule.cs Outdated
siegfriedpammer and others added 5 commits September 6, 2026 10:51
MatchArrayInitializerFinal selects the operator by its return type, so
the block's result type is that return type. The declaring type only
happens to be the same one for the two array-to-span operators; Span<T>
also declares the conversion to ReadOnlySpan<T>.

Assisted-by: Claude:claude-opus-5:Claude Code
Resolving a method reference and resolving one by name and signature
differed in three ways that had no reason to differ: only the metadata
path found a static constructor, only it restricted the candidates to
the declared members, and only it matched a vararg signature against
its required parameters plus __arglist.

Assisted-by: Claude:claude-opus-5:Claude Code
The parameters standing in for the expanded arguments were built from
the element type of the array the compiler had built, not from the
element type the params collection declares. The two can only differ
where the collection is covariant in its element type, and no compiler
emits that shape today - it materializes the array into a local of the
target type first, which the pattern no longer matches - so this only
removes the dependency on that.

Where the params collection is one overload resolution cannot unpack,
the expanded form is now abandoned instead of being built from a type
that resolution would never have used.

Assisted-by: Claude:claude-opus-5:Claude Code
A hand-built Expression.Equal whose operands are a type parameter has no
lambda to decompile to: `v == other` is CS0019 for a type parameter, and
boxing both operands compiles but compares box identity where the tree
compares values once the parameter is a value type. The conversion now
declines, leaving the Expression calls that built the tree.

Found in EF Core's BoolToTwoValuesConverter<TProvider>.ToBool, which
decompiled to code that does not compile.

Assisted-by: Claude:claude-opus-5:Claude Code
The builder returned by ConvertLambda hands back null when a nested
conversion declines, and the result was cast and dereferenced before
anything checked it, so a tree the transform cannot handle took down the
whole method with a NullReferenceException instead of being left alone.

EF Core's StringCharConverter.ToChar is such a tree: the conditional
spills the Expression.Call arguments into stack slots, which
MatchGetMethodFromHandle does not see through.

Assisted-by: Claude:claude-opus-5:Claude Code
A constant narrower than its stack type builds as a plain ldc.i4, which
infers as int, so a conditional whose other branch really is a char saw
two different types and the whole tree was left as the Expression calls
that built it - EF Core's StringCharConverter.ToChar is one. Bool and
enum constants were already wrapped for this reason; the wrap now
applies wherever the built value does not infer as the declared type.

Assisted-by: Claude:claude-opus-5:Claude Code
Declining every type-parameter operand was too broad. The reason a type
parameter has no lambda spelling is that `v == other` is CS0019 and boxing
both operands compares box identity where the tree compares values - and both
only apply while the parameter may be a value type. A parameter constrained to
a reference type compares as a reference, which is what the tree asks for and
what `t == null` spells, so it converts like any other reference comparison.

IsReferenceType is the distinction the type system already makes here:
TypeUtils.GetStackType maps a type parameter to Obj or VT by the same
question. An unconstrained parameter answers null and keeps declining.

Assisted-by: Claude:claude-opus-5:Claude Code
@siegfriedpammer

Copy link
Copy Markdown
Member

decompdiff over the top-200 nuget.org corpus — master (dbf23c6) against this branch rebased onto that same commit, so the diff isolates the PR. 167 assemblies, 37,530 types.

Counters are flat and there are no new errors: 37,505 types byte-identical, 25 changed, 137 changed lines in total.

old new delta
lines 4,055,141 4,055,142 +1
goto statements 2,315 2,315 0
//IL_ warnings 24,191 24,191 0
generated-name leaks 2,544 2,544 0

One of the 25 changes is a behavioural difference.

Invented (byte) cast truncates Regex character masks

7 of the changed types, all System.Text.RegularExpressions.Generated.*, in AWSSDK.Core, Dapper (x2), FluentValidation, OpenTelemetry and Renci.SshNet (x2).

// master
if (((c = span[i]) < 0x80 ? (mask[c >> 4] & (1 << (c & 0xF)))
    : (CharInClass(c, "...") ? 1 : 0)) == 0)

// this branch
if (!((c = span[i]) < 0x80 ? ((byte)(mask[c >> 4] & (1 << (c & 0xF))) != 0)
    : CharInClass(c, "...")))

The bool-shaped conditional is an improvement and matches the shape the source generator actually emitted. The (byte) is not in the IL — there is no conv.u1 anywhere in the method:

IL_0053: ldstr "\0\0..."   // the mask table
IL_0058: ldloc.2
IL_0059: ldc.i4.4
IL_005a: shr               // c >> 4
IL_005b: call instance char String::get_Chars(int32)
IL_0060: ldc.i4.1
IL_0061: ldloc.2
IL_0062: ldc.i4.s 15
IL_0064: and               // c & 15
IL_0065: ldc.i4.s 31
IL_0067: and               // & 31
IL_0068: shl               // 1 << ((c & 15) & 31)  -> up to 32768
IL_0069: and               // mask[c >> 4] & bit
IL_006a: ldc.i4.0
IL_006b: cgt.un            // != 0

The shift count is (c & 15) & 31, so the masked value reaches 32768 and (byte) discards every bit from 8 up. With AWSSDK's mask[2] = U+7800, character '+' (0x2B) tests bit 11: master computes 0x800 != 0 and matches, this branch computes (byte)0x800 == 0 and does not. FluentValidation's mask holds U+FFFE and U+FFFF entries, so most of its character class is affected.

git bisect run over the branch points at 30552b0, Replace a couple of inst.ResultType -> IType conversions with InferType() calls — specifically one line in ExpressionBuilder.VisitIfInstruction, on the path that picks a target type for a conditional whose branches have no common type:

- targetType = FindType(inst.ResultType, context.TypeHint.GetSign());
+ targetType = inst.InferType(compilation);

The old form derived the type from the stack type, so an I4 conditional became int. InferType narrows from the operands instead: c & 15 has range 0–15 and fits a byte, but that inference must not survive shl, because 1 << n is not byte-ranged. Reverting just this line at the branch tip removes the cast.

One thing to note for a fix: the guard removed from VisitSwitchInstruction in the same commit would not have caught this either, since it compares GetStackType() and both byte and int are I4.

Repro:

ilspycmd ~/.cache/nugetfuzz/awssdk.core/4.0.102.4/lib/net8.0/AWSSDK.Core.dll \
    -t "System.Text.RegularExpressions.Generated.<RegexGenerator_g>F23AE...__RoleSessionNameRegex_0"

The other 18 changes

  • 10 types differ only in warning text: //IL_xxxx: Expected O, but got Unknown becomes Expected Obj, from the enum rename.
  • BoolToTwoValuesConverter<TProvider> now declines instead of emitting (TProvider v) => (object)v == (object)trueValue. Master compared box identity where the tree compares values, so this is a fix.
  • Logging.Logger, DebuggerDisplayFormatting: LogLevel[] readOnlySpan becomes ReadOnlySpan<LogLevel>.
  • StackExchange.Redis.Condition: RedisCommand num becomes redisCommand — inference now yields the enum rather than its underlying integer.
  • SharpCompress ZStandard: (nint)(...) becomes (nint)((IntPtr)(...)). Same type, so semantically identical, just slightly worse to read.

Corpus caveats: four assemblies are undecompilable on both sides for missing references (System.Web.Mvc, Microsoft.Azure.WebJobs.Host, MongoDB.Driver, MongoDB.Driver.Core) and twelve more decompile with unresolved references. Those are identical on both sides, so they are corpus gaps rather than PR effects.

Posted by an AI agent (Claude) on Siegfried's behalf.

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.

2 participants