Browse Source

Fix #3518 by replacing `FixLoneIsInst` with an inlining restriction.

This way we avoid having to extract later, as we will never inline if the `isinst` argument if this could result in it being unrepresentable in C#.
This commit also refactors inlining restrictions to avoid requiring special cases in ILInlining itself.

But when making this change, I discovered that this broke our pattern-matching tests, and that the weird IL with double `isinst` is indeed generated by the C# compiler for `if (genericParam is StringComparison.Ordinal)` style code. So instead we also allow `isinst` with a `box(expr-without-side-effects)` argument to be represented with the `expr is T ? (T)expr : null` emulation.
null-coalescing-assignment
Daniel Grunwald 4 months ago
parent
commit
dd4bf7d8a4
  1. 1
      ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs
  2. 12
      ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs
  3. 2
      ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj
  4. 9
      ICSharpCode.Decompiler/IL/ILReader.cs
  5. 16
      ICSharpCode.Decompiler/IL/Instructions/Block.cs
  6. 19
      ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs
  7. 62
      ICSharpCode.Decompiler/IL/Instructions/IsInst.cs
  8. 45
      ICSharpCode.Decompiler/IL/Instructions/LdFlda.cs
  9. 69
      ICSharpCode.Decompiler/IL/Transforms/FixLoneIsInst.cs
  10. 61
      ICSharpCode.Decompiler/IL/Transforms/ILInlining.cs

1
ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs

@ -158,7 +158,6 @@ namespace ICSharpCode.Decompiler.CSharp @@ -158,7 +158,6 @@ namespace ICSharpCode.Decompiler.CSharp
},
new ProxyCallReplacer(),
new FixRemainingIncrements(),
new FixLoneIsInst(),
new CopyPropagation(),
new DelegateConstruction(),
new LocalFunctionDecompiler(),

12
ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs

@ -398,10 +398,12 @@ namespace ICSharpCode.Decompiler.CSharp @@ -398,10 +398,12 @@ namespace ICSharpCode.Decompiler.CSharp
// unbox.any T(isinst T(expr)) ==> "expr as T" for nullable value types and class-constrained generic types
// comp(isinst T(expr) != null) ==> "expr is T"
// on block level (StatementBuilder.VisitIsInst) => "expr is T"
if (SemanticHelper.IsPure(inst.Argument.Flags))
if (SemanticHelper.IsPure(inst.Argument.Flags) || (inst.Argument is Box box && SemanticHelper.IsPure(box.Argument.Flags)))
{
// We can emulate isinst using
// expr is T ? expr : null
// (doubling the boxing side-effect is harmless because the "expr is T" part won't observe object identity,
// and we need to support this because Roslyn pattern matching sometimes generates such code.)
return new ConditionalExpression(
new IsExpression(arg, ConvertType(inst.Type)).WithILInstruction(inst),
arg.Expression.Clone(),
@ -3185,16 +3187,16 @@ namespace ICSharpCode.Decompiler.CSharp @@ -3185,16 +3187,16 @@ namespace ICSharpCode.Decompiler.CSharp
return input.ConvertTo(targetType, this);
}
internal static bool IsUnboxAnyWithIsInst(UnboxAny unboxAny, IsInst isInst)
internal static bool IsUnboxAnyWithIsInst(UnboxAny unboxAny, IType isInstType)
{
return unboxAny.Type.Equals(isInst.Type)
&& (unboxAny.Type.IsKnownType(KnownTypeCode.NullableOfT) || isInst.Type.IsReferenceType == true);
return unboxAny.Type.Equals(isInstType)
&& (unboxAny.Type.IsKnownType(KnownTypeCode.NullableOfT) || isInstType.IsReferenceType == true);
}
protected internal override TranslatedExpression VisitUnboxAny(UnboxAny inst, TranslationContext context)
{
TranslatedExpression arg;
if (inst.Argument is IsInst isInst && IsUnboxAnyWithIsInst(inst, isInst))
if (inst.Argument is IsInst isInst && IsUnboxAnyWithIsInst(inst, isInst.Type))
{
// unbox.any T(isinst T(expr)) ==> expr as T
// This is used for generic types and nullable value types

2
ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj

@ -136,7 +136,6 @@ @@ -136,7 +136,6 @@
<Compile Include="Humanizer\Vocabulary.cs" />
<Compile Include="IL\ControlFlow\RemoveRedundantReturn.cs" />
<Compile Include="IL\Transforms\DeconstructionTransform.cs" />
<Compile Include="IL\Transforms\FixLoneIsInst.cs" />
<Compile Include="IL\Instructions\DeconstructInstruction.cs" />
<Compile Include="IL\Instructions\DeconstructResultInstruction.cs" />
<Compile Include="IL\Instructions\MatchInstruction.cs" />
@ -413,6 +412,7 @@ @@ -413,6 +412,7 @@
<Compile Include="IL\Instructions\LdFlda.cs" />
<Compile Include="IL\Instructions\NullableInstructions.cs" />
<Compile Include="IL\Instructions\StLoc.cs" />
<Compile Include="IL\Instructions\IsInst.cs" />
<Compile Include="DebugInfo\SequencePoint.cs" />
<Compile Include="IL\Instructions\CallIndirect.cs" />
<Compile Include="IL\Instructions\DefaultValue.cs" />

9
ICSharpCode.Decompiler/IL/ILReader.cs

@ -1138,7 +1138,14 @@ namespace ICSharpCode.Decompiler.IL @@ -1138,7 +1138,14 @@ namespace ICSharpCode.Decompiler.IL
case ILOpCode.Initobj:
return InitObj(PopStObjTarget(), ReadAndDecodeTypeReference());
case ILOpCode.Isinst:
return Push(new IsInst(Pop(StackType.O), ReadAndDecodeTypeReference()));
{
var type = ReadAndDecodeTypeReference();
if (type.IsReferenceType != true)
{
FlushExpressionStack(); // value-type isinst has inlining restrictions
}
return Push(new IsInst(Pop(StackType.O), type));
}
case ILOpCode.Ldelem:
return LdElem(ReadAndDecodeTypeReference());
case ILOpCode.Ldelem_i1:

16
ICSharpCode.Decompiler/IL/Instructions/Block.cs

@ -217,6 +217,22 @@ namespace ICSharpCode.Decompiler.IL @@ -217,6 +217,22 @@ namespace ICSharpCode.Decompiler.IL
}
}
internal override bool CanInlineIntoSlot(int childIndex, ILInstruction expressionBeingMoved)
{
switch (Kind)
{
case BlockKind.ControlFlow when Parent is BlockContainer:
case BlockKind.ArrayInitializer:
case BlockKind.CollectionInitializer:
case BlockKind.ObjectInitializer:
case BlockKind.CallInlineAssign:
// Allow inlining into the first instruction of the block
return childIndex == 0;
default:
return false;
}
}
/// <summary>
/// Gets the name of this block.
/// </summary>

19
ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs

@ -941,13 +941,28 @@ namespace ICSharpCode.Decompiler.IL @@ -941,13 +941,28 @@ namespace ICSharpCode.Decompiler.IL
}
/// <summary>
/// Gets whether the specified instruction may be inlined into the specified slot.
/// Note: this does not check whether reordering with the previous slots is valid; only wheter the target slot supports inlining at all!
/// Gets whether the expressionBeingMoved, which previously executes prior to `this`,
/// may be moved into a descendant of the specified slot.
/// (i.e. this controls whether FindLoadInNext may descent into the specified slot)
///
/// Note: this does not check whether reordering with the previous slots is valid; only whether the target slot supports inlining at all!
/// </summary>
internal virtual bool CanInlineIntoSlot(int childIndex, ILInstruction expressionBeingMoved)
{
return GetChildSlot(childIndex).CanInlineInto;
}
/// <summary>
/// Some slots in the ILAst have restrictions on which instructions can appear in them.
/// Used to suppress inlining if the new child does not satisfy the restrictions.
/// Unlike `CanInlineIntoSlot`, this is not about descendants of the slot, only about
/// whether SetChild(childIndex, newChild) is valid.
/// (i.e. this controls whether FindLoadInNext may return the specified slot as a final result)
/// </summary>
internal virtual bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild)
{
return true;
}
}
public interface IInstructionWithTypeOperand

62
ICSharpCode.Decompiler/IL/Instructions/IsInst.cs

@ -0,0 +1,62 @@ @@ -0,0 +1,62 @@
#nullable enable
// Copyright (c) 2025 Daniel Grunwald
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System.Diagnostics;
using ICSharpCode.Decompiler.CSharp;
namespace ICSharpCode.Decompiler.IL;
partial class IsInst
{
internal override bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild)
{
Debug.Assert(childIndex == 0);
Debug.Assert(base.SatisfiesSlotRestrictionForInlining(childIndex, newChild));
if (this.Type.IsReferenceType == true)
{
return true; // reference-type isinst is always supported
}
if (SemanticHelper.IsPure(newChild.Flags))
{
return true; // emulated via "expr is T ? (T)expr : null"
}
else if (newChild is Box box && SemanticHelper.IsPure(box.Argument.Flags))
{
// Also emulated via "expr is T ? (T)expr : null".
// This duplicates the boxing side-effect, but that's harmless as one of the boxes is only
// used in the `expr is T` type test where the object identity can never be observed.
// This appears as part of C# pattern matching, inlining early makes those code patterns easier to detect.
return true;
}
if (this.Parent is UnboxAny unboxAny && ExpressionBuilder.IsUnboxAnyWithIsInst(unboxAny, this.Type))
{
return true; // supported pattern "expr as T?"
}
if (this.Parent != null && (this.Parent.MatchCompEqualsNull(out _) || this.Parent.MatchCompNotEqualsNull(out _)))
{
return true; // supported pattern "expr is T"
}
if (this.Parent is Block { Kind: BlockKind.ControlFlow })
{
return true; // supported via StatementBuilder.VisitIsInst
}
return false;
}
}

45
ICSharpCode.Decompiler/IL/Instructions/LdFlda.cs

@ -48,28 +48,35 @@ namespace ICSharpCode.Decompiler.IL @@ -48,28 +48,35 @@ namespace ICSharpCode.Decompiler.IL
public sealed partial class StObj
{
/// <summary>
/// For a store to a field or array element, C# will only throw NullReferenceException/IndexOfBoundsException
/// after the value-to-be-stored has been computed.
/// This means a LdFlda/LdElema used as target for StObj must have DelayExceptions==true to allow a translation to C#
/// without changing the program semantics. See https://github.com/icsharpcode/ILSpy/issues/2050
/// </summary>
public bool CanInlineIntoTargetSlot(ILInstruction inst)
internal override bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild)
{
switch (inst.OpCode)
if (childIndex == 0) // target slot
{
case OpCode.LdElema:
case OpCode.LdFlda:
Debug.Assert(inst.HasDirectFlag(InstructionFlags.MayThrow));
// If the ldelema/ldflda may throw a non-delayed exception, inlining will cause it
// to turn into a delayed exception after the translation to C#.
// This is only valid if the value computation doesn't involve any side effects.
return SemanticHelper.IsPure(this.Value.Flags);
// Note that after inlining such a ldelema/ldflda, the normal inlining rules will
// prevent us from inlining an effectful instruction into the value slot.
default:
return true;
Debug.Assert(GetChildSlot(childIndex) == TargetSlot);
// For a store to a field or array element, C# will only throw NullReferenceException/IndexOfBoundsException
// after the value-to-be-stored has been computed.
// This means a LdFlda/LdElema used as target for StObj must have DelayExceptions==true to allow a translation to C#
// without changing the program semantics. See https://github.com/icsharpcode/ILSpy/issues/2050
switch (newChild.OpCode)
{
case OpCode.LdElema:
case OpCode.LdFlda:
{
Debug.Assert(newChild.HasDirectFlag(InstructionFlags.MayThrow));
// If the ldelema/ldflda may throw a non-delayed exception, inlining will cause it
// to turn into a delayed exception after the translation to C#.
// This is only valid if the value computation doesn't involve any side effects.
if (!SemanticHelper.IsPure(this.Value.Flags))
{
return false;
}
// Note that after inlining such a ldelema/ldflda, the normal inlining rules will
// prevent us from inlining an effectful instruction into the value slot.
break;
}
}
}
return base.SatisfiesSlotRestrictionForInlining(childIndex, newChild);
}
/// <summary>

69
ICSharpCode.Decompiler/IL/Transforms/FixLoneIsInst.cs

@ -1,69 +0,0 @@ @@ -1,69 +0,0 @@
// Copyright (c) 2020 Daniel Grunwald
//
// Permission is hereby granted, free of charge, to any person obtaining a copy of this
// software and associated documentation files (the "Software"), to deal in the Software
// without restriction, including without limitation the rights to use, copy, modify, merge,
// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
// to whom the Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in all copies or
// substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
using System.Collections.Generic;
using System.Linq;
using ICSharpCode.Decompiler.CSharp;
namespace ICSharpCode.Decompiler.IL.Transforms
{
/// <summary>
/// C# cannot represent `isinst T` directly for value-types.
/// This transform un-inlines the argument of `isinst` instructions that can't be directly translated to C#,
/// thus allowing the emulation via "expr is T ? (T)expr : null".
/// </summary>
public class FixLoneIsInst : IILTransform
{
void IILTransform.Run(ILFunction function, ILTransformContext context)
{
var instructionsToFix = new List<IsInst>();
foreach (var isInst in function.Descendants.OfType<IsInst>())
{
if (isInst.Type.IsReferenceType == true)
{
continue; // reference-type isinst is always supported
}
if (SemanticHelper.IsPure(isInst.Argument.Flags))
{
continue; // emulated via "expr is T ? (T)expr : null"
}
if (isInst.Parent is UnboxAny unboxAny && ExpressionBuilder.IsUnboxAnyWithIsInst(unboxAny, isInst))
{
continue; // supported pattern "expr as T?"
}
if (isInst.Parent.MatchCompEqualsNull(out _) || isInst.Parent.MatchCompNotEqualsNull(out _))
{
continue; // supported pattern "expr is T"
}
if (isInst.Parent is Block { Kind: BlockKind.ControlFlow })
{
continue; // supported via StatementBuilder.VisitIsInst
}
instructionsToFix.Add(isInst);
}
// Need to delay fixing until we're done with iteration, because Extract() modifies parents
foreach (var isInst in instructionsToFix)
{
// Use extraction to turn isInst.Argument into a pure instruction, thus making the emulation possible
context.Step("FixLoneIsInst", isInst);
isInst.Argument.Extract(context);
}
}
}
}

61
ICSharpCode.Decompiler/IL/Transforms/ILInlining.cs

@ -821,46 +821,27 @@ namespace ICSharpCode.Decompiler.IL.Transforms @@ -821,46 +821,27 @@ namespace ICSharpCode.Decompiler.IL.Transforms
return FindResult.Stop;
if (expr.MatchLdLoc(v) || expr.MatchLdLoca(v))
{
// Match found, we can inline
if (expr.SlotInfo == StObj.TargetSlot && !((StObj)expr.Parent).CanInlineIntoTargetSlot(expressionBeingMoved))
// Match found, we can inline unless there are slot restrictions.
if (expr.Parent.SatisfiesSlotRestrictionForInlining(expr.ChildIndex, expressionBeingMoved))
{
if ((options & InliningOptions.AllowChangingOrderOfEvaluationForExceptions) != 0)
{
// Intentionally change code semantics so that we can avoid a ref local
if (expressionBeingMoved is LdFlda ldflda)
ldflda.DelayExceptions = true;
else if (expressionBeingMoved is LdElema ldelema)
ldelema.DelayExceptions = true;
}
else
{
// special case: the StObj.TargetSlot does not accept some kinds of expressions
return FindResult.Stop;
}
return FindResult.Found(expr);
}
return FindResult.Found(expr);
}
else if (expr is Block block)
{
// Inlining into inline-blocks?
switch (block.Kind)
// We cannot inline because the targets slot restrictions are not satisfied
if ((options & InliningOptions.AllowChangingOrderOfEvaluationForExceptions) != 0 && expr.SlotInfo == StObj.TargetSlot)
{
case BlockKind.ControlFlow when block.Parent is BlockContainer:
case BlockKind.ArrayInitializer:
case BlockKind.CollectionInitializer:
case BlockKind.ObjectInitializer:
case BlockKind.CallInlineAssign:
// Allow inlining into the first instruction of the block
if (block.Instructions.Count == 0)
return FindResult.Stop;
return NoContinue(FindLoadInNext(block.Instructions[0], v, expressionBeingMoved, options));
// If FindLoadInNext() returns null, we still can't continue searching
// because we can't inline over the remainder of the block.
case BlockKind.CallWithNamedArgs:
return NamedArgumentTransform.CanExtendNamedArgument(block, v, expressionBeingMoved);
default:
return FindResult.Stop;
// Special case: inlining will change code semantics,
// but we accept that so that we can avoid a ref local.
if (expressionBeingMoved is LdFlda ldflda)
ldflda.DelayExceptions = true;
else if (expressionBeingMoved is LdElema ldelema)
ldelema.DelayExceptions = true;
return FindResult.Found(expr);
}
return FindResult.Stop;
}
else if (expr is Block { Kind: BlockKind.CallWithNamedArgs } block)
{
return NamedArgumentTransform.CanExtendNamedArgument(block, v, expressionBeingMoved);
}
else if (options.HasFlag(InliningOptions.FindDeconstruction) && expr is DeconstructInstruction di)
{
@ -886,14 +867,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms @@ -886,14 +867,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms
return FindResult.Stop; // abort, inlining not possible
}
private static FindResult NoContinue(FindResult findResult)
{
if (findResult.Type == FindResultType.Continue)
return FindResult.Stop;
else
return findResult;
}
/// <summary>
/// Determines whether it is safe to move 'expressionBeingMoved' past 'expr'
/// </summary>

Loading…
Cancel
Save