diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs index 31837e1ec..094bd0731 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstructorInitializers.cs @@ -307,18 +307,26 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty { 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) + // Value types chain via 'this = new TSelf(...)', an ordinary body statement, so the + // hoisted argument null-guard may legally precede it: the guard is deliberately not + // folded back and the chain is not lifted into a this(...) initializer. #if EXPECTED_OUTPUT - : this(0, (value ?? throw new ArgumentNullException("value")).Length, value.Length, "value") + public NullCheckedStructThisChain(string value) + { + if (value == null) + { + throw new ArgumentNullException("value"); + } + this = new NullCheckedStructThisChain(0, value.Length, value.Length, "value"); + Leet += value.Length; + } #else + public NullCheckedStructThisChain(string value) : this(0, value?.Length ?? throw new ArgumentNullException("value"), value.Length, "value") -#endif { Leet += value.Length; } +#endif public NullCheckedStructThisChain(int a, int b, int c, string s) { @@ -331,16 +339,29 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty 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) + // guards; both stay in the body (see NullCheckedStructThisChain). #if EXPECTED_OUTPUT - : this((a ?? throw new ArgumentNullException("a")).Length, (b ?? throw new ArgumentNullException("b")).Length, b.Length, a) + public NullCheckedStructTwoArguments(string a, string b) + { + if (a == null) + { + throw new ArgumentNullException("a"); + } + int length = a.Length; + if (b == null) + { + throw new ArgumentNullException("b"); + } + this = new NullCheckedStructTwoArguments(length, b.Length, b.Length, a); + Leet += a.Length; + } #else + public NullCheckedStructTwoArguments(string a, string b) : this(a?.Length ?? throw new ArgumentNullException("a"), b?.Length ?? throw new ArgumentNullException("b"), b.Length, a) -#endif { Leet += a.Length; } +#endif public NullCheckedStructTwoArguments(int a, int b, int c, string s) { diff --git a/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs index 7c9bdd9dd..b8ad1e251 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/NullCoalescingTransform.cs @@ -124,11 +124,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms /// 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. + /// Only reference-type chains (base/this..ctor calls, identified by IL offset via + /// ) need this: value types chain via + /// this = new TSelf(...), which is an ordinary body statement, so a preceding + /// guard statement is legal C# and can stay. /// bool TransformHoistedConstructorArgumentNullGuard(Block block, int pos, StatementTransformContext context) { @@ -145,26 +144,37 @@ namespace ICSharpCode.Decompiler.IL.Transforms if (!IsArgumentNullGuard(guard, out var paramLoad, out var throwInst)) return false; - if (!GuardPrecedesChainedConstructorCall(block, pos, function, out int searchEndPos)) + // Only a guard sitting in the initializer's argument evaluation, i.e. before the + // chained constructor call, is necessarily compiler-hoisted. + if (!ILInlining.IsInConstructorInitializer(function, guard)) 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++) + // The compiler hoists the guard directly in front of the statement containing the + // parameter use it was lifted out of, so that use must be order-safely reachable + // within the immediately following statement. + var paramLoadParent = paramLoad.Parent; + var paramLoadChildIndex = paramLoad.ChildIndex; + var throwInstParent = throwInst.Parent; + var throwInstChildIndex = throwInst.ChildIndex; + var expressionWithThrow = new NullCoalescingInstruction(NullCoalescingKind.Ref, paramLoad, throwInst); + var result = ILInlining.FindLoadInNext(block.Instructions[pos + 1], paramLoad.Variable, expressionWithThrow, + InliningOptions.None); + if (result.Type != ILInlining.FindResultType.Found || result.LoadInst is not LdLoc firstUse) { - firstUse = block.Instructions[i].Descendants.OfType() - .FirstOrDefault(ld => ld.Variable == paramLoad.Variable); + // reset the primary positions (see remarks on ILInstruction.Parent) + var paramLoadSiblings = paramLoadParent.Children; + paramLoadSiblings[paramLoadChildIndex] = paramLoad; + var throwInstSiblings = throwInstParent.Children; + throwInstSiblings[throwInstChildIndex] = throwInst; + return false; } - 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)); + var stloc = new StLoc(temp, expressionWithThrow); stloc.AddILRange(guard); block.Instructions[pos] = stloc; context.EndStep(stloc); @@ -194,78 +204,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms 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(...)