From a1f9a6b6f555c54179eb747136463281e83368bf Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Tue, 30 Jun 2026 20:54:36 +0200 Subject: [PATCH] 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 --- .../Pretty/ConstructorInitializers.cs | 164 +++++++++++++++++ .../IL/Transforms/NullCoalescingTransform.cs | 167 +++++++++++++++++- 2 files changed, 327 insertions(+), 4 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs index 7f671a7eb..eebe03476 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs +++ b/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 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 public class PrimaryCtorClassThisChain(Guid id) { diff --git a/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs index b88984f34..7c9bdd9dd 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs @@ -36,10 +36,11 @@ namespace ICSharpCode.Decompiler.IL.Transforms { public void Run(Block block, int pos, StatementTransformContext context) { - if (!TransformRefTypes(block, pos, context)) - { - TransformThrowExpressionValueTypes(block, pos, context); - } + if (TransformRefTypes(block, pos, context)) + return; + if (TransformHoistedConstructorArgumentNullGuard(block, pos, context)) + return; + TransformThrowExpressionValueTypes(block, pos, context); } /// @@ -107,6 +108,164 @@ namespace ICSharpCode.Decompiler.IL.Transforms return false; } + /// + /// When an argument of a chained constructor call contains a throwing null-check + /// (e.g. arg ?? throw ...) and arg is evaluated more than once, the C# + /// compiler hoists the null-check in front of the chained call: + /// + /// if (comp.o(ldloc param == ldnull)) throw(...) + /// call Base..ctor(..., ldlen(ldloc param), ..., ldloc param, ...) + /// + /// The guard then blocks TransformFieldAndConstructorInitializers from lifting the chained + /// call into a : this(...) / : base(...) clause. Replace the guard with + /// + /// stloc temp(if.notnull(ldloc param, throw(...))) + /// + /// redirecting the first following use of the parameter (the position the check was hoisted + /// from) to temp, 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 (base/this..ctor calls) are identified by IL offset via + /// ; value types chain via + /// this = new TSelf(...), i.e. stobj(ldthis, newobj TSelf(...)), which + /// does not report, so the stobj + /// shape is matched directly. + /// + 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().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() + .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; + } + + /// + /// 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. + /// + 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; + } + + /// + /// Determines whether the guard at precedes the constructor's chained + /// this/base call, i.e. belongs to the hoisted argument evaluation of the constructor + /// initializer. 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). + /// + 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; + } + + /// + /// True if is a value-type chained constructor call + /// this = new TSelf(...), i.e. stobj(ldthis, newobj TSelf(...)) where TSelf + /// is the constructor's declaring type. + /// + 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); + } + + /// + /// 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 this = new TSelf(...) call. + /// + 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(); + } + /// /// stloc v(value) /// if (logic.not(call get_HasValue(ldloca v))) throw(...)