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.

Author: dgrunwald

ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs

@@ -158,7 +158,6 @@ public static List<IILTransform> GetILTransforms()
 				},
 				new ProxyCallReplacer(),
 				new FixRemainingIncrements(),
-				new FixLoneIsInst(),
 				new CopyPropagation(),
 				new DelegateConstruction(),
 				new LocalFunctionDecompiler(),

ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs

@@ -398,10 +398,12 @@ protected internal override TranslatedExpression VisitIsInst(IsInst inst, Transl
 				//  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 @@ TranslatedExpression ConvertArrayIndex(TranslatedExpression input, StackType sta
 			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

ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj

@@ -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 @@
     <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" />

ICSharpCode.Decompiler/IL/ILReader.cs

@@ -1138,7 +1138,14 @@ DecodedInstruction DecodeInstruction()
 				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:

ICSharpCode.Decompiler/IL/Instructions/Block.cs

@@ -217,6 +217,22 @@ public override StackType ResultType {
 			}
 		}
 
+		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>

ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs

@@ -941,13 +941,28 @@ internal virtual bool PrepareExtract(int childIndex, Transforms.ExtractionContex
 		}
 
 		/// <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

ICSharpCode.Decompiler/IL/Instructions/IsInst.cs

@@ -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;
+	}
+}

ICSharpCode.Decompiler/IL/Instructions/LdFlda.cs

@@ -48,28 +48,35 @@ internal override void CheckInvariant(ILPhase phase)
 
 	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>

ICSharpCode.Decompiler/IL/Transforms/FixLoneIsInst.cs

@@ -821,46 +821,27 @@ internal static FindResult FindLoadInNext(ILInstruction expr, ILVariable v, ILIn
 				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 @@ internal static FindResult FindLoadInNext(ILInstruction expr, ILVariable v, ILIn
 				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>