Fix #1552: user-defined and decimal increments

For user-defined increments, there were problems with Roslyn was optimizing out one of the stores.
The new transform FixRemainingIncrements now takes increments/decrements that were not detected by TransformAssignment and introduces a temporary variable that can be incremented.
This sometimes requires un-inlining via the new ILInstruction.Extract() operation.
Extract() is not supported in all possible contexts, so it is possible but unlikely that some op_Increment calls remain.

For decimals, the situation is different: legacy csc actually was optimizing "d + 1m" to "op_Increment(d)", so we can get rid of any left-over increments by undoing this optimization. This now happens in ReplaceMethodCallsWithOperators.

ICSharpCode.Decompiler.Tests/TestCases/Pretty/CompoundAssignmentTest.cs

@@ -4720,5 +4720,50 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 		{
 			return (*GetPointer())++;
 		}
+
+		public void Issue1552Pre(CustomStruct a, CustomStruct b)
+		{
+			CustomStruct customStruct = a + b;
+			Console.WriteLine(++customStruct);
+		}
+
+		public void Issue1552Stmt(CustomStruct a, CustomStruct b)
+		{
+			CustomStruct customStruct = a + b;
+			++customStruct;
+		}
+
+		public void Issue1552StmtUseLater(CustomStruct a, CustomStruct b)
+		{
+			CustomStruct lhs = a + b;
+			++lhs;
+			Console.WriteLine();
+			Console.WriteLine(lhs * b);
+		}
+
+		public void Issue1552Decimal(decimal a)
+		{
+			// Legacy csc compiles this using op_Increment,
+			// ensure we don't misdetect this as an invalid pre-increment "++(a * 10m)"
+			Console.WriteLine(a * 10m + 1m);
+		}
+
+#if !(ROSLYN && OPT)
+		// Roslyn opt no longer has a detectable post-increment pattern
+		// due to optimizing out some of the stores.
+		// Our emitted code is valid but has some additional temporaries.
+		public void Issue1552Post(CustomStruct a, CustomStruct b)
+		{
+			CustomStruct customStruct = a + b;
+			Console.WriteLine(customStruct++);
+		}
+
+		public void Issue1552StmtTwice(CustomStruct a, CustomStruct b)
+		{
+			CustomStruct customStruct = a + b;
+			++customStruct;
+			++customStruct;
+		}
+#endif
 	}
 }

ICSharpCode.Decompiler.Tests/TestCases/Pretty/ConstantsTests.cs

@@ -35,5 +35,20 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 		private void Test(bool expr)
 		{
 		}
+
+		private void Test(decimal expr)
+		{
+		}
+
+		public void Decimal()
+		{
+			// Roslyn and legacy csc both normalize the decimal constant references,
+			// but to a different representation (ctor call vs. field use)
+			Test(0m);
+			Test(1m);
+			Test(-1m);
+			Test(decimal.MinValue);
+			Test(decimal.MaxValue);
+		}
 	}
 }

ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs

@@ -155,6 +155,7 @@ namespace ICSharpCode.Decompiler.CSharp
 					}
 				},
 				new ProxyCallReplacer(),
+				new FixRemainingIncrements(),
 				new DelegateConstruction(),
 				new LocalFunctionDecompiler(),
 				new TransformDisplayClassUsage(),

ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs

@@ -122,6 +122,22 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			}
 			UnaryOperatorType? uop = GetUnaryOperatorTypeFromMetadataName(method.Name);
 			if (uop != null && arguments.Length == 1) {
+				if (uop == UnaryOperatorType.Increment || uop == UnaryOperatorType.Decrement) {
+					// `op_Increment(a)` is not equivalent to `++a`,
+					// because it doesn't assign the incremented value to a.
+					if (method.DeclaringType.IsKnownType(KnownTypeCode.Decimal)) {
+						// Legacy csc optimizes "d + 1m" to "op_Increment(d)",
+						// so reverse that optimization here:
+						invocationExpression.ReplaceWith(
+							new BinaryOperatorExpression(
+								arguments[0].Detach(),
+								(uop == UnaryOperatorType.Increment ? BinaryOperatorType.Add : BinaryOperatorType.Subtract),
+								new PrimitiveExpression(1m)
+							).CopyAnnotationsFrom(invocationExpression)
+						);
+					}
+					return;
+				}
 				arguments[0].Remove(); // detach argument
 				invocationExpression.ReplaceWith(
 					new UnaryOperatorExpression(uop.Value, arguments[0]).CopyAnnotationsFrom(invocationExpression)

ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj

@@ -227,6 +227,8 @@
     <Compile Include="CSharp\Resolver\NameLookupMode.cs" />
     <Compile Include="CSharp\Resolver\OverloadResolution.cs" />
     <Compile Include="CSharp\Resolver\OverloadResolutionErrors.cs" />
+    <Compile Include="IL\Transforms\FixRemainingIncrements.cs" />
+    <Compile Include="IL\Transforms\ILExtraction.cs" />
     <Compile Include="TypeSystem\Implementation\LocalFunctionMethod.cs" />
     <Compile Include="CSharp\Resolver\TypeInference.cs" />
     <Compile Include="CSharp\Transforms\CombineQueryExpressions.cs" />

ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs

@@ -19,6 +19,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Threading;
 using ICSharpCode.Decompiler.IL.Patterns;
 using ICSharpCode.Decompiler.TypeSystem;
 using ICSharpCode.Decompiler.Util;
@@ -758,6 +759,41 @@ namespace ICSharpCode.Decompiler.IL
 			}
 			return false;
 		}
+
+		/// <summary>
+		/// Extracts the this instruction.
+		///   The instruction is replaced with a load of a new temporary variable;
+		///   and the instruction is moved to a store to said variable at block-level.
+		///   Returns the new variable.
+		/// 
+		/// If extraction is not possible, the ILAst is left unmodified and the function returns null.
+		/// May return null if extraction is not possible.
+		/// </summary>
+		public ILVariable Extract()
+		{
+			return Transforms.ExtractionContext.Extract(this);
+		}
+
+		/// <summary>
+		/// Prepares "extracting" a descendant instruction out of this instruction.
+		/// This is the opposite of ILInlining. It may involve re-compiling high-level constructs into lower-level constructs.
+		/// </summary>
+		/// <returns>True if extraction is possible; false otherwise.</returns>
+		internal virtual bool PrepareExtract(int childIndex, Transforms.ExtractionContext ctx)
+		{
+			if (!GetChildSlot(childIndex).CanInlineInto) {
+				return false;
+			}
+			// Check whether re-ordering with predecessors is valid:
+			for (int i = childIndex - 1; i >= 0; --i) {
+				ILInstruction predecessor = GetChild(i);
+				if (!GetChildSlot(i).CanInlineInto) {
+					return false;
+				}
+				ctx.RegisterMoveIfNecessary(predecessor);
+			}
+			return true;
+		}
 	}
 	
 	public interface IInstructionWithTypeOperand

ICSharpCode.Decompiler/IL/Instructions/NullableInstructions.cs

@@ -18,6 +18,7 @@
 
 using System.Diagnostics;
 using System.Linq;
+using ICSharpCode.Decompiler.IL.Transforms;
 
 namespace ICSharpCode.Decompiler.IL
 {
@@ -125,5 +126,11 @@ namespace ICSharpCode.Decompiler.IL
 					return StackType.O;
 			}
 		}
+
+		internal override bool PrepareExtract(int childIndex, ExtractionContext ctx)
+		{
+			return base.PrepareExtract(childIndex, ctx)
+				&& (ctx.FlagsBeingMoved & InstructionFlags.MayUnwrapNull) == 0;
+		}
 	}
 }

ICSharpCode.Decompiler/IL/Transforms/ExpressionTransforms.cs

@@ -390,11 +390,37 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			return false;
 		}
 
+		bool TransformDecimalFieldToConstant(LdObj inst, out LdcDecimal result)
+		{
+			if (inst.MatchLdsFld(out var field) && field.DeclaringType.IsKnownType(KnownTypeCode.Decimal)) {
+				decimal? value = null;
+				if (field.Name == "One") {
+					value = decimal.One;
+				} else if (field.Name == "MinusOne") {
+					value = decimal.MinusOne;
+				} else if (field.Name == "Zero") {
+					value = decimal.Zero;
+				}
+				if (value != null) {
+					result = new LdcDecimal(value.Value).WithILRange(inst).WithILRange(inst.Target);
+					return true;
+				}
+			}
+			result = null;
+			return false;
+		}
+
 		protected internal override void VisitLdObj(LdObj inst)
 		{
 			base.VisitLdObj(inst);
 			EarlyExpressionTransforms.AddressOfLdLocToLdLoca(inst, context);
-			EarlyExpressionTransforms.LdObjToLdLoc(inst, context);
+			if (EarlyExpressionTransforms.LdObjToLdLoc(inst, context))
+				return;
+			if (TransformDecimalFieldToConstant(inst, out LdcDecimal decimalConstant)) {
+				context.Step("TransformDecimalFieldToConstant", inst);
+				inst.ReplaceWith(decimalConstant);
+				return;
+			}
 		}
 
 		protected internal override void VisitStObj(StObj inst)
@@ -407,6 +433,12 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			TransformAssignment.HandleCompoundAssign(inst, context);
 		}
 
+		protected internal override void VisitStLoc(StLoc inst)
+		{
+			base.VisitStLoc(inst);
+			TransformAssignment.HandleCompoundAssign(inst, context);
+		}
+
 		protected internal override void VisitIfInstruction(IfInstruction inst)
 		{
 			inst.TrueInst.AcceptVisitor(this);

ICSharpCode.Decompiler/IL/Transforms/FixRemainingIncrements.cs

@@ -0,0 +1,65 @@
+using System;
+using System.Linq;
+using System.Collections.Generic;
+using System.Text;
+using System.Diagnostics;
+using ICSharpCode.Decompiler.TypeSystem;
+
+namespace ICSharpCode.Decompiler.IL.Transforms
+{
+	public class FixRemainingIncrements : IILTransform
+	{
+		void IILTransform.Run(ILFunction function, ILTransformContext context)
+		{
+			var callsToFix = new List<Call>();
+			foreach (var call in function.Descendants.OfType<Call>()) {
+				if (!(call.Method.IsOperator && (call.Method.Name == "op_Increment" || call.Method.Name == "op_Decrement")))
+					continue;
+				if (call.Arguments.Count != 1)
+					continue;
+				if (call.Method.DeclaringType.IsKnownType(KnownTypeCode.Decimal)) {
+					// For decimal, legacy csc can optimize "d + 1m" to "op_Increment(d)".
+					// We can handle these calls in ReplaceMethodCallsWithOperators.
+					continue;
+				}
+				callsToFix.Add(call);
+			}
+			foreach (var call in callsToFix) {
+				// A user-defined increment/decrement that was not handled by TransformAssignment.
+				// This can happen because the variable-being-incremented was optimized out by Roslyn,
+				// e.g.
+				//   public void Issue1552Pre(UserType a, UserType b)
+				//   {
+				//      UserType num = a + b;
+				//      Console.WriteLine(++num);
+				//   }
+				// can end up being compiled to:
+				//      Console.WriteLine(UserType.op_Increment(a + b));
+				if (call.SlotInfo == StLoc.ValueSlot && call.Parent.SlotInfo == Block.InstructionSlot) {
+					var store = (StLoc)call.Parent;
+					var block = (Block)store.Parent;
+					context.Step($"Fix {call.Method.Name} call at 0x{call.StartILOffset:x4} using {store.Variable.Name}", call);
+					// stloc V(call op_Increment(...))
+					// ->
+					// stloc V(...)
+					// compound.assign op_Increment(V)
+					call.ReplaceWith(call.Arguments[0]);
+					block.Instructions.Insert(store.ChildIndex + 1,
+						new UserDefinedCompoundAssign(call.Method, CompoundEvalMode.EvaluatesToNewValue,
+						new LdLoca(store.Variable), CompoundTargetKind.Address, new LdcI4(1)).WithILRange(call));
+				} else {
+					context.Step($"Fix {call.Method.Name} call at 0x{call.StartILOffset:x4} using new local", call);
+					var newVariable = call.Arguments[0].Extract();
+					if (newVariable == null) {
+						Debug.Fail("Failed to extract argument of remaining increment/decrement");
+						continue;
+					}
+					newVariable.Type = call.GetParameter(0).Type;
+					Debug.Assert(call.Arguments[0].MatchLdLoc(newVariable));
+					call.ReplaceWith(new UserDefinedCompoundAssign(call.Method, CompoundEvalMode.EvaluatesToNewValue,
+						new LdLoca(newVariable), CompoundTargetKind.Address, new LdcI4(1)).WithILRange(call));
+				}
+			}
+		}
+	}
+}

ICSharpCode.Decompiler/IL/Transforms/ILExtraction.cs

@@ -0,0 +1,103 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
+using System.Threading;
+
+namespace ICSharpCode.Decompiler.IL.Transforms
+{
+	/// <summary>
+	/// Context object for the ILInstruction.Extract() operation.
+	/// </summary>
+	class ExtractionContext
+	{
+		/// <summary>
+		/// Nearest function, used for registering the new locals that are created by extraction.
+		/// </summary>
+		readonly ILFunction Function;
+
+		/// <summary>
+		/// Combined flags of all instructions being moved.
+		/// </summary>
+		internal InstructionFlags FlagsBeingMoved;
+
+		/// <summary>
+		/// List of actions to be executed when performing the extraction.
+		/// 
+		/// Each function in this list has the side-effect of replacing the instruction-to-be-moved
+		/// with a load of a fresh temporary variable; and returns the the store to the temporary variable,
+		/// which will be inserted at block-level.
+		/// </summary>
+		readonly List<Func<ILInstruction>> MoveActions = new List<Func<ILInstruction>>();
+
+		ExtractionContext(ILFunction function)
+		{
+			Debug.Assert(function != null);
+			this.Function = function;
+		}
+
+		internal void RegisterMove(ILInstruction predecessor)
+		{
+			FlagsBeingMoved |= predecessor.Flags;
+			MoveActions.Add(delegate {
+				var v = Function.RegisterVariable(VariableKind.StackSlot, predecessor.ResultType);
+				predecessor.ReplaceWith(new LdLoc(v));
+				return new StLoc(v, predecessor);
+			});
+		}
+
+		internal void RegisterMoveIfNecessary(ILInstruction predecessor)
+		{
+			if (!CanReorderWithInstructionsBeingMoved(predecessor)) {
+				RegisterMove(predecessor);
+			}
+		}
+
+		/// <summary>
+		/// Currently, <c>predecessor</c> is evaluated before the instructions being moved.
+		/// If this function returns true, <c>predecessor</c> can stay as-is, despite the move changing the evaluation order.
+		/// If this function returns false, <c>predecessor</c> will need to also move, to ensure the evaluation order stays unchanged.
+		/// </summary>
+		public bool CanReorderWithInstructionsBeingMoved(ILInstruction predecessor)
+		{
+			// We could track the instructions being moved and be smarter about unnecessary moves,
+			// but given the limited scenarios where extraction is used so far,
+			// this seems unnecessary.
+			return predecessor.Flags == InstructionFlags.None;
+		}
+
+		/// <summary>
+		/// Extracts the specified instruction:
+		///   The instruction is replaced with a load of a new temporary variable;
+		///   and the instruction is moved to a store to said variable at block-level.
+		/// 
+		/// May return null if extraction is not possible.
+		/// </summary>
+		public static ILVariable Extract(ILInstruction instToExtract)
+		{
+			var function = instToExtract.Ancestors.OfType<ILFunction>().First();
+			ExtractionContext ctx = new ExtractionContext(function);
+			ctx.FlagsBeingMoved = instToExtract.Flags;
+			ILInstruction inst = instToExtract;
+			while (inst != null) {
+				if (inst.Parent is Block block && block.Kind == BlockKind.ControlFlow) {
+					// We've reached the target block, and extraction is possible all the way.
+					int insertIndex = inst.ChildIndex;
+					// Move instToExtract itself:
+					var v = function.RegisterVariable(VariableKind.StackSlot, instToExtract.ResultType);
+					instToExtract.ReplaceWith(new LdLoc(v));
+					block.Instructions.Insert(insertIndex, new StLoc(v, instToExtract));
+					// Apply the other move actions:
+					foreach (var moveAction in ctx.MoveActions) {
+						block.Instructions.Insert(insertIndex, moveAction());
+					}
+					return v;
+				}
+				if (!inst.Parent.PrepareExtract(inst.ChildIndex, ctx))
+					return null;
+				inst = inst.Parent;
+			}
+			return null;
+		}
+	}
+}

ICSharpCode.Decompiler/IL/Transforms/TransformAssignment.cs

@@ -28,6 +28,11 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 	/// <summary>
 	/// Constructs compound assignments and inline assignments.
 	/// </summary>
+	/// <remarks>
+	/// This is a statement transform;
+	/// but some portions are executed as an expression transform instead
+	/// (with HandleCompoundAssign() as entry point)
+	/// </remarks>
 	public class TransformAssignment : IStatementTransform
 	{
 		StatementTransformContext context;
@@ -294,6 +299,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			}
 			ILInstruction newInst;
 			if (UnwrapSmallIntegerConv(setterValue, out var smallIntConv) is BinaryNumericInstruction binary) {
+				if (compoundStore is StLoc) {
+					// transform local variables only for user-defined operators
+					return false;
+				}
 				if (!IsMatchingCompoundLoad(binary.Left, compoundStore, out var target, out var targetKind, out var finalizeMatch, forbiddenVariable: storeInSetter?.Variable))
 					return false;
 				if (!ValidateCompoundAssign(binary, smallIntConv, targetType))
@@ -335,6 +344,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				newInst = new DynamicCompoundAssign(dynamicBinaryOp.Operation, dynamicBinaryOp.BinderFlags, target, dynamicBinaryOp.LeftArgumentInfo, dynamicBinaryOp.Right, dynamicBinaryOp.RightArgumentInfo, targetKind);
 			} else if (setterValue is Call concatCall && UserDefinedCompoundAssign.IsStringConcat(concatCall.Method)) {
 				// setterValue is a string.Concat() invocation
+				if (compoundStore is StLoc) {
+					// transform local variables only for user-defined operators
+					return false;
+				}
 				if (concatCall.Arguments.Count != 2)
 					return false; // for now we only support binary compound assignments
 				if (!targetType.IsKnownType(KnownTypeCode.String))