#1691: Avoid replacing string.Concat() with operator+ when the evaluation order depends on the compiler version.

ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs

@@ -280,6 +280,12 @@ namespace ICSharpCode.Decompiler.Tests
 			RunCS(options: options);
 		}
 
+		[Test]
+		public void StringConcat([ValueSource("defaultOptions")] CompilerOptions options)
+		{
+			RunCS(options: options);
+		}
+
 		[Test]
 		public void MiniJSON([ValueSource("defaultOptions")] CompilerOptions options)
 		{

ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj

@@ -79,6 +79,7 @@
 
   <ItemGroup>
     <Compile Include="DisassemblerPrettyTestRunner.cs" />
+    <Compile Include="TestCases\Correctness\StringConcat.cs" />
     <Compile Include="TestCases\ILPretty\ConstantBlobs.cs" />
     <Compile Include="TestCases\Pretty\OutVariables.cs" />
     <Compile Include="TestCases\Pretty\CustomTaskType.cs" />

ICSharpCode.Decompiler.Tests/TestCases/Correctness/StringConcat.cs

@@ -0,0 +1,57 @@
+using System;
+
+namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
+{
+	class StringConcat
+	{
+		private class C
+		{
+			readonly int i;
+
+			public C(int i)
+			{
+				Console.WriteLine("  new C(" + i + ")");
+				this.i = i;
+			}
+
+			public override string ToString()
+			{
+				Console.WriteLine("  C(" + i + ").ToString()");
+				return i.ToString();
+			}
+		}
+
+		static string Space()
+		{
+			Console.WriteLine("  Space()");
+			return " ";
+		}
+
+		static void Main()
+		{
+			Console.WriteLine("string + C:");
+			Console.WriteLine(Space() + new C(1));
+
+			Console.WriteLine("C + string:");
+			Console.WriteLine(new C(2) + Space());
+
+			Console.WriteLine("C + string + C:");
+			Console.WriteLine(new C(3) + Space() + new C(4));
+
+			Console.WriteLine("string + C + C:");
+			Console.WriteLine(Space() + new C(5) + new C(6));
+
+			Console.WriteLine("string.Concat(C, string, C):");
+			Console.WriteLine(string.Concat(new C(10), Space(), new C(11)));
+
+			Console.WriteLine("string.Concat(string.Concat(C, string), C):");
+			Console.WriteLine(string.Concat(string.Concat(new C(15), Space()), new C(16)));
+
+			Console.WriteLine("string.Concat(C, string.Concat(string, C)):");
+			Console.WriteLine(string.Concat(new C(20), string.Concat(Space(), new C(21))));
+
+			Console.WriteLine("string.Concat(C, string) + C:");
+			Console.WriteLine(string.Concat(new C(30), Space()) + new C(31));
+		}
+	}
+}

ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs

@@ -23,7 +23,9 @@ using System.Reflection;
 using System.Text;
 using ICSharpCode.Decompiler.CSharp.Syntax;
 using ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching;
+using ICSharpCode.Decompiler.Semantics;
 using ICSharpCode.Decompiler.TypeSystem;
+using ICSharpCode.Decompiler.Util;
 
 namespace ICSharpCode.Decompiler.CSharp.Transforms
 {
@@ -56,7 +58,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			var arguments = invocationExpression.Arguments.ToArray();
 
 			// Reduce "String.Concat(a, b)" to "a + b"
-			if (method.Name == "Concat" && method.DeclaringType.FullName == "System.String" && CheckArgumentsForStringConcat(arguments)) {
+			if (IsStringConcat(method) && CheckArgumentsForStringConcat(arguments)) {
 				invocationExpression.Arguments.Clear(); // detach arguments from invocationExpression
 				Expression expr = arguments[0];
 				for (int i = 1; i < arguments.Length; i++) {
@@ -174,9 +176,77 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			if (arguments.Length < 2)
 				return false;
 
-			return !arguments.Any(arg => arg is NamedArgumentExpression) &&
-				(arguments[0].GetResolveResult().Type.IsKnownType(KnownTypeCode.String) ||
-				arguments[1].GetResolveResult().Type.IsKnownType(KnownTypeCode.String));
+			if (arguments.Any(arg => arg is NamedArgumentExpression))
+				return false;
+
+			// The evaluation order when the object.ToString() calls happen is a mess:
+			// The C# spec says the evaluation for order for each individual string + should be:
+			//   * evaluate left argument
+			//   * evaluate right argument
+			//   * call ToString() on object argument
+			// What actually happens pre-VS2019.3:
+			//   * evaluate all arguments in chain of + operators from left to right
+			//   * call ToString() on all object arguments from left to right
+			// What happens in VS2019.3:
+			//   * for each argument in chain of + operators fom left to right:
+			//       * evaluate argument
+			//       * call ToString() on object argument
+			// See https://github.com/dotnet/roslyn/issues/38641 for details.
+			// To ensure the decompiled code's behavior matches the original IL behavior,
+			// no matter which compiler is used to recompile it, we require that all
+			// implicit ToString() calls except for the last are free of side effects.
+			foreach (var arg in arguments.SkipLast(1)) {
+				if (!ToStringIsKnownEffectFree(arg.GetResolveResult().Type)) {
+					return false;
+				}
+			}
+			foreach (var arg in arguments) {
+				if (arg.GetResolveResult() is InvocationResolveResult rr && IsStringConcat(rr.Member)) {
+					// Roslyn + mcs also flatten nested string.Concat() invocations within a operator+ use,
+					// which causes it to use the incorrect evaluation order despite the code using an
+					// explicit string.Concat() call.
+					// This problem is avoided if the outer call remains string.Concat() as well.
+					return false;
+				}
+			}
+
+			// One of the first two arguments must be string, otherwise the + operator
+			// won't resolve to a string concatenation.
+			return arguments[0].GetResolveResult().Type.IsKnownType(KnownTypeCode.String)
+				|| arguments[1].GetResolveResult().Type.IsKnownType(KnownTypeCode.String);
+		}
+
+		private bool IsStringConcat(IParameterizedMember member)
+		{
+			return member is IMethod method
+				&& method.Name == "Concat"
+				&& method.DeclaringType.IsKnownType(KnownTypeCode.String);
+		}
+
+		static bool ToStringIsKnownEffectFree(IType type)
+		{
+			type = NullableType.GetUnderlyingType(type);
+			switch (type.GetDefinition()?.KnownTypeCode) {
+				case KnownTypeCode.Boolean:
+				case KnownTypeCode.Char:
+				case KnownTypeCode.SByte:
+				case KnownTypeCode.Byte:
+				case KnownTypeCode.Int16:
+				case KnownTypeCode.UInt16:
+				case KnownTypeCode.Int32:
+				case KnownTypeCode.UInt32:
+				case KnownTypeCode.Int64:
+				case KnownTypeCode.UInt64:
+				case KnownTypeCode.Single:
+				case KnownTypeCode.Double:
+				case KnownTypeCode.Decimal:
+				case KnownTypeCode.IntPtr:
+				case KnownTypeCode.UIntPtr:
+				case KnownTypeCode.String:
+					return true;
+				default:
+					return false;
+			}
 		}
 
 		static BinaryOperatorType? GetBinaryOperatorTypeFromMetadataName(string name)