Browse Source

Fix #3824: fold a hoisted argument null-guard at the ILAst level

When a chained constructor-call argument contains a throwing null-check
(e.g. `value?.Length ?? throw ...`) and the argument is evaluated more
than once, the compiler hoists the null-check in front of the chained
call. The hoisted `if (value == null) throw ...;` then became the first
body statement, so MoveConstructorInitializer could not recognize the
chained call and left it as an illegal in-body `base..ctor(...)` /
`this..ctor(...)` (a parse error).

Fix it in the ILAst, where the `?? throw` shape already lives, rather
than re-deriving it on the C# AST: NullCoalescingTransform folds a guard
that directly precedes the chained call back into the first use of the
parameter as `if.notnull(ldloc param, throw)`. Nothing in a constructor
body can legally run before the chained call, so a statement preceding it
is necessarily compiler-hoisted; matching is by ILVariable identity, not
parameter name. The guard disappears before the AST transforms run, so
they need no change.

Reference types chain via a base/this..ctor CallInstruction; value types
chain via `this = new TSelf(...)`, i.e. stobj(ldthis, newobj TSelf(...)),
which ChainedConstructorCallILOffset does not report -- so the value-type
chain (including the case where this is spilled to a stack slot because
the guard sits between its load and the call) is matched directly.

Assisted-by: Claude:claude-opus-4-8:Claude Code
pull/3863/head
Siegfried Pammer 1 week ago committed by Siegfried Pammer
parent
commit
a1f9a6b6f5
  1. 164
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs
  2. 167
      ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs

164
ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs

@ -232,6 +232,170 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
} }
} }
#if CS70
public class NullCheckedArgumentBase
{
public NullCheckedArgumentBase(int a, int b, int c, string s)
{
}
}
public class NullCheckedArgumentChain : NullCheckedArgumentBase
{
public string Value;
// 'value?.Length ?? throw ...' combined with a later reuse of 'value' makes the compiler
// hoist the null-check in front of the chained constructor call. The decompiler must fold
// that guard back into the first argument as 'value ?? throw ...' rather than emit an
// illegal in-body 'base..ctor(...)' call.
public NullCheckedArgumentChain(string value)
#if EXPECTED_OUTPUT
: base(0, (value ?? throw new ArgumentNullException("value")).Length, value.Length, "value")
#else
: base(0, value?.Length ?? throw new ArgumentNullException("value"), value.Length, "value")
#endif
{
Value = value;
}
}
public class NullCheckedArgumentThisChain
{
public string Value;
public NullCheckedArgumentThisChain(int a, int b, int c, string s)
{
}
public NullCheckedArgumentThisChain(string value)
#if EXPECTED_OUTPUT
: this(0, (value ?? throw new ArgumentNullException("value")).Length, value.Length, "value")
#else
: this(0, value?.Length ?? throw new ArgumentNullException("value"), value.Length, "value")
#endif
{
Value = value;
}
}
public struct NullCheckedStructThisChain
{
public int Leet;
// Value types chain via 'this = new TSelf(...)'; ChainedConstructorCallILOffset only
// reports reference-type chained calls, so the hoisted guard fold must locate the struct
// this(...) call itself, otherwise the guard survives and defeats the initializer.
public NullCheckedStructThisChain(string value)
#if EXPECTED_OUTPUT
: this(0, (value ?? throw new ArgumentNullException("value")).Length, value.Length, "value")
#else
: this(0, value?.Length ?? throw new ArgumentNullException("value"), value.Length, "value")
#endif
{
Leet += value.Length;
}
public NullCheckedStructThisChain(int a, int b, int c, string s)
{
Leet = a + b + c;
}
}
public struct NullCheckedStructTwoArguments
{
public int Leet;
// Two reused parameters before a value-type this(...) chain produce two stacked hoisted
// guards; both must be folded.
public NullCheckedStructTwoArguments(string a, string b)
#if EXPECTED_OUTPUT
: this((a ?? throw new ArgumentNullException("a")).Length, (b ?? throw new ArgumentNullException("b")).Length, b.Length, a)
#else
: this(a?.Length ?? throw new ArgumentNullException("a"), b?.Length ?? throw new ArgumentNullException("b"), b.Length, a)
#endif
{
Leet += a.Length;
}
public NullCheckedStructTwoArguments(int a, int b, int c, string s)
{
Leet = a + b + c;
}
}
public class NullCheckedTwoArguments : NullCheckedArgumentBase
{
// Two reused parameters produce two stacked hoisted guards before the chained call;
// each must be folded into the first argument that uses it.
public NullCheckedTwoArguments(string a, string b)
#if EXPECTED_OUTPUT
: base((a ?? throw new ArgumentNullException("a")).Length, (b ?? throw new ArgumentNullException("b")).Length, a.Length, b)
#else
: base(a?.Length ?? throw new ArgumentNullException("a"), b?.Length ?? throw new ArgumentNullException("b"), a.Length, b)
#endif
{
}
}
public class NullCheckedNullableArgument : NullCheckedArgumentBase
{
// A nullable value type argument with 'value ?? throw ...' and a later reuse: here the
// compiler emits its own Nullable<T> copy plus a HasValue check in the middle of the
// argument evaluation (instead of hoisting a comparison against null), which the
// value-type throw-expression fold reassembles.
public NullCheckedNullableArgument(int? value)
: base(value ?? throw new ArgumentNullException("value"), value.Value, 0, "value")
{
}
}
public struct NullCheckedNullableStructThisChain
{
public int Leet;
public NullCheckedNullableStructThisChain(int? value)
: this(value ?? throw new ArgumentNullException("value"), value.Value, 0, "value")
{
Leet += value.Value;
}
public NullCheckedNullableStructThisChain(int a, int b, int c, string s)
{
Leet = a + b + c;
}
}
public class NullCheckedFirstArgument : NullCheckedArgumentBase
{
// The guarded parameter is used by the very first argument, so the fold targets argument
// index 0.
public NullCheckedFirstArgument(string value)
#if EXPECTED_OUTPUT
: base((value ?? throw new ArgumentNullException("value")).Length, value.Length, 0, "value")
#else
: base(value?.Length ?? throw new ArgumentNullException("value"), value.Length, 0, "value")
#endif
{
}
}
public class NotHoistedBodyGuard
{
public string Value;
// An ordinary argument-validation guard runs after the (implicit) base call, so it must
// stay an in-body statement and must not be folded into an initializer.
public NotHoistedBodyGuard(string value)
{
if (value == null)
{
throw new ArgumentNullException("value");
}
Value = value;
}
}
#endif
#if CS100 #if CS100
public class PrimaryCtorClassThisChain(Guid id) public class PrimaryCtorClassThisChain(Guid id)
{ {

167
ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs

@ -36,10 +36,11 @@ namespace ICSharpCode.Decompiler.IL.Transforms
{ {
public void Run(Block block, int pos, StatementTransformContext context) public void Run(Block block, int pos, StatementTransformContext context)
{ {
if (!TransformRefTypes(block, pos, context)) if (TransformRefTypes(block, pos, context))
{ return;
TransformThrowExpressionValueTypes(block, pos, context); if (TransformHoistedConstructorArgumentNullGuard(block, pos, context))
} return;
TransformThrowExpressionValueTypes(block, pos, context);
} }
/// <summary> /// <summary>
@ -107,6 +108,164 @@ namespace ICSharpCode.Decompiler.IL.Transforms
return false; return false;
} }
/// <summary>
/// When an argument of a chained constructor call contains a throwing null-check
/// (e.g. <c>arg ?? throw ...</c>) and <c>arg</c> is evaluated more than once, the C#
/// compiler hoists the null-check in front of the chained call:
/// <code>
/// if (comp.o(ldloc param == ldnull)) throw(...)
/// call Base..ctor(..., ldlen(ldloc param), ..., ldloc param, ...)
/// </code>
/// The guard then blocks TransformFieldAndConstructorInitializers from lifting the chained
/// call into a <c>: this(...)</c> / <c>: base(...)</c> clause. Replace the guard with
/// <code>
/// stloc temp(if.notnull(ldloc param, throw(...)))
/// </code>
/// redirecting the first following use of the parameter (the position the check was hoisted
/// from) to <c>temp</c>, and leave moving the coalescing expression into the chained call to
/// ILInlining, which does so only when that preserves the order of evaluation.
/// Reference-type chains (<c>base/this..ctor</c> calls) are identified by IL offset via
/// <see cref="ILInlining.IsInConstructorInitializer"/>; value types chain via
/// <c>this = new TSelf(...)</c>, i.e. <c>stobj(ldthis, newobj TSelf(...))</c>, which
/// <see cref="ILFunction.ChainedConstructorCallILOffset"/> does not report, so the stobj
/// shape is matched directly.
/// </summary>
bool TransformHoistedConstructorArgumentNullGuard(Block block, int pos, StatementTransformContext context)
{
// A throw-expression is the only way to express the folded form.
if (!context.Settings.ThrowExpressions)
return false;
var function = block.Ancestors.OfType<ILFunction>().FirstOrDefault();
if (function?.Method is not { IsConstructor: true, IsStatic: false })
return false;
// Match `if (comp(ldloc param == ldnull)) throw(...)` with no else branch.
var guard = block.Instructions[pos];
if (!IsArgumentNullGuard(guard, out var paramLoad, out var throwInst))
return false;
if (!GuardPrecedesChainedConstructorCall(block, pos, function, out int searchEndPos))
return false;
// Redirect the first following use of the parameter, i.e. the position where the
// null-check sat before the compiler hoisted it.
LdLoc firstUse = null;
for (int i = pos + 1; i <= searchEndPos && firstUse == null; i++)
{
firstUse = block.Instructions[i].Descendants.OfType<LdLoc>()
.FirstOrDefault(ld => ld.Variable == paramLoad.Variable);
}
if (firstUse == null)
return false; // parameter not used up to the chained call -> cannot fold; leave guard in place
context.Step($"NullCoalescingTransform: fold hoisted null-guard of '{paramLoad.Variable.Name}' into argument of chained constructor call", guard);
var temp = function.RegisterVariable(VariableKind.StackSlot, paramLoad.Variable.Type);
firstUse.Variable = temp;
throwInst.resultType = StackType.O;
var stloc = new StLoc(temp, new NullCoalescingInstruction(NullCoalescingKind.Ref, paramLoad, throwInst));
stloc.AddILRange(guard);
block.Instructions[pos] = stloc;
context.EndStep(stloc);
ILInlining.InlineOneIfPossible(block, pos, InliningOptions.None, context);
return true;
}
/// <summary>
/// Matches a hoisted argument null-guard `if (comp(ldloc param == ldnull)) throw(...)`
/// (no else branch). Only parameters qualify: nothing else is in scope before the
/// constructor initializer.
/// </summary>
static bool IsArgumentNullGuard(ILInstruction inst, out LdLoc paramLoad, out Throw throwInst)
{
paramLoad = null;
throwInst = null;
if (!inst.MatchIfInstruction(out var condition, out var trueInst))
return false;
if (!(Block.Unwrap(trueInst) is Throw t))
return false;
if (!(condition.MatchCompEquals(out var left, out var right) && right.MatchLdNull() && left is LdLoc load))
return false;
if (load.Variable.Kind != VariableKind.Parameter)
return false;
paramLoad = load;
throwInst = t;
return true;
}
/// <summary>
/// Determines whether the guard at <paramref name="pos"/> precedes the constructor's chained
/// this/base call, i.e. belongs to the hoisted argument evaluation of the constructor
/// initializer. <paramref name="searchEndPos"/> is the last statement index that may contain
/// the parameter use to redirect (the statement containing the chained call, if it is in
/// this block).
/// </summary>
static bool GuardPrecedesChainedConstructorCall(Block block, int pos, ILFunction function, out int searchEndPos)
{
searchEndPos = -1;
if (ILInlining.IsInConstructorInitializer(function, block.Instructions[pos]))
{
// Reference-type chain: everything before ChainedConstructorCallILOffset is the
// initializer's argument evaluation. Search up to and including the first statement
// that reaches past that offset (the statement containing the chained call).
int ctorCallStart = function.ChainedConstructorCallILOffset;
for (int i = pos + 1; i < block.Instructions.Count; i++)
{
searchEndPos = i;
if (block.Instructions[i].EndILOffset > ctorCallStart)
break;
}
return searchEndPos > pos;
}
// Value-type chain: `this = new TSelf(...)` is not reported by
// ChainedConstructorCallILOffset, so match the stobj shape directly. Only further
// hoisted guards may sit between this guard and the chained call; anything else means
// the stobj is a plain body statement rather than a chain.
for (int i = pos + 1; i < block.Instructions.Count; i++)
{
var inst = block.Instructions[i];
if (IsValueTypeChainedConstructorCall(inst, function))
{
searchEndPos = i;
return true;
}
if (!IsArgumentNullGuard(inst, out _, out _))
return false;
}
return false;
}
/// <summary>
/// True if <paramref name="inst"/> is a value-type chained constructor call
/// <c>this = new TSelf(...)</c>, i.e. <c>stobj(ldthis, newobj TSelf(...))</c> where TSelf
/// is the constructor's declaring type.
/// </summary>
static bool IsValueTypeChainedConstructorCall(ILInstruction inst, ILFunction function)
{
return inst is StObj { Value: NewObj { Method.IsConstructor: true } newObj } stobj
&& newObj.Method.DeclaringType.IsReferenceType == false
&& newObj.Method.DeclaringTypeDefinition == function.Method.DeclaringTypeDefinition
&& MatchLdThisOrStackSlotCopy(stobj.Target);
}
/// <summary>
/// Matches a load of the this-pointer, either directly or via a single-definition stack slot
/// that copies it. The compiler spills this to such a slot when a hoisted guard sits between
/// the this-load and the chained <c>this = new TSelf(...)</c> call.
/// </summary>
static bool MatchLdThisOrStackSlotCopy(ILInstruction inst)
{
if (inst.MatchLdThis())
return true;
return inst.MatchLdLoc(out var v)
&& v.Kind == VariableKind.StackSlot
&& v.IsSingleDefinition
&& v.StoreInstructions.Count == 1
&& v.StoreInstructions[0] is StLoc { Value: { } storeValue }
&& storeValue.MatchLdThis();
}
/// <summary> /// <summary>
/// stloc v(value) /// stloc v(value)
/// if (logic.not(call get_HasValue(ldloca v))) throw(...) /// if (logic.not(call get_HasValue(ldloca v))) throw(...)

Loading…
Cancel
Save