#1691: Further improvements for decompiling the new VS 2019.3 string concatenation IL pattern

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

@@ -21,13 +21,30 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			}
 		}
 
+		private struct S
+		{
+			readonly int i;
+
+			public S(int i)
+			{
+				Console.WriteLine("  new C(" + i + ")");
+				this.i = i;
+			}
+
+			public override string ToString()
+			{
+				Console.WriteLine("  S(" + i + ").ToString()");
+				return i.ToString();
+			}
+		}
+
 		static string Space()
 		{
 			Console.WriteLine("  Space()");
 			return " ";
 		}
 
-		static void Main()
+		static void TestClass()
 		{
 			Console.WriteLine("string + C:");
 			Console.WriteLine(Space() + new C(1));
@@ -53,5 +70,38 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			Console.WriteLine("string.Concat(C, string) + C:");
 			Console.WriteLine(string.Concat(new C(30), Space()) + new C(31));
 		}
+
+		static void TestStruct()
+		{
+			Console.WriteLine("string + S:");
+			Console.WriteLine(Space() + new S(1));
+
+			Console.WriteLine("S + string:");
+			Console.WriteLine(new S(2) + Space());
+
+			Console.WriteLine("S + string + S:");
+			Console.WriteLine(new S(3) + Space() + new S(4));
+
+			Console.WriteLine("string + S + S:");
+			Console.WriteLine(Space() + new S(5) + new S(6));
+
+			Console.WriteLine("string.Concat(S, string, S):");
+			Console.WriteLine(string.Concat(new S(10), Space(), new S(11)));
+
+			Console.WriteLine("string.Concat(string.Concat(S, string), S):");
+			Console.WriteLine(string.Concat(string.Concat(new S(15), Space()), new S(16)));
+
+			Console.WriteLine("string.Concat(S, string.Concat(string, S)):");
+			Console.WriteLine(string.Concat(new S(20), string.Concat(Space(), new S(21))));
+
+			Console.WriteLine("string.Concat(S, string) + S:");
+			Console.WriteLine(string.Concat(new S(30), Space()) + new S(31));
+		}
+
+		static void Main()
+		{
+			TestClass();
+			TestStruct();
+		}
 	}
 }

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

@@ -4562,9 +4562,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 			// same temporary. In order to inline the generated value-type temporary, we
 			// need to split it, even though it has the address taken for the ToString() call.
 			if (flag) {
-				strings[1] += chars[i].ToString();
+				strings[1] += chars[i];
 			} else {
-				strings[0] += chars[i].ToString();
+				strings[0] += chars[i];
 			}
 		}
 #endif

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

@@ -401,7 +401,8 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 				Console.WriteLine("MoveNext");
 				if (enumerator.MoveNext()) {
 					object current = enumerator.Current;
-					Console.WriteLine("current: " + current);
+					Console.WriteLine("please don't inline 'current'");
+					Console.WriteLine(current);
 				}
 			} finally {
 				IDisposable disposable = enumerator as IDisposable;

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

@@ -366,7 +366,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 #if !ROSLYN
 		public static string SwitchOverBool(bool b)
 		{
-			Console.WriteLine("SwitchOverBool: " + b.ToString());
+			Console.WriteLine("SwitchOverBool: " + b);
 			switch (b) {
 				case true:
 					return bool.TrueString;

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

@@ -184,7 +184,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 		public static void Issue56(int i, out string str)
 		{
 			str = "qq";
-			str += i.ToString();
+			str += i;
 		}
 
 		public static void CopyAroundAndModifyField(S s)

ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs

@@ -1409,7 +1409,8 @@ namespace ICSharpCode.Decompiler.CSharp
 			if (UserDefinedCompoundAssign.IsStringConcat(inst.Method)) {
 				Debug.Assert(inst.Method.Parameters.Count == 2);
 				var value = Translate(inst.Value).ConvertTo(inst.Method.Parameters[1].Type, this, allowImplicitConversion: true);
-				return new AssignmentExpression(target, AssignmentOperatorType.Add, value)
+				var valueExpr = ReplaceMethodCallsWithOperators.RemoveRedundantToStringInConcat(value, inst.Method, isLastArgument: true).Detach();
+				return new AssignmentExpression(target, AssignmentOperatorType.Add, valueExpr)
 					.WithILInstruction(inst)
 					.WithRR(new OperatorResolveResult(inst.Method.ReturnType, ExpressionType.AddAssign, inst.Method, inst.IsLifted, new[] { target.ResolveResult, value.ResolveResult }));
 			} else if (inst.Method.Parameters.Count == 2) {

ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs

@@ -60,9 +60,9 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			// Reduce "String.Concat(a, b)" to "a + b"
 			if (IsStringConcat(method) && CheckArgumentsForStringConcat(arguments)) {
 				invocationExpression.Arguments.Clear(); // detach arguments from invocationExpression
-				Expression expr = RemoveRedundantToStringInConcat(arguments[0], method).Detach();
+				Expression expr = RemoveRedundantToStringInConcat(arguments[0], method, isLastArgument: false).Detach();
 				for (int i = 1; i < arguments.Length; i++) {
-					var arg = RemoveRedundantToStringInConcat(arguments[i], method).Detach();
+					var arg = RemoveRedundantToStringInConcat(arguments[i], method, isLastArgument: i == arguments.Length - 1).Detach();
 					expr = new BinaryOperatorExpression(expr, BinaryOperatorType.Add, arg);
 				}
 				expr.CopyAnnotationsFrom(invocationExpression);
@@ -223,19 +223,46 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 				&& method.Name == "Concat"
 				&& method.DeclaringType.IsKnownType(KnownTypeCode.String);
 		}
-		
-		static readonly InvocationExpression ToStringCallPattern = new InvocationExpression(new MemberReferenceExpression(new AnyNode("target"), "ToString"));
 
-		static Expression RemoveRedundantToStringInConcat(Expression expr, IMethod concatMethod)
+		static readonly Pattern ToStringCallPattern = new Choice {
+			// target.ToString()
+			new InvocationExpression(new MemberReferenceExpression(new AnyNode("target"), "ToString")),
+			// target?.ToString()
+			new UnaryOperatorExpression(
+				UnaryOperatorType.NullConditionalRewrap,
+				new InvocationExpression(
+					new MemberReferenceExpression(
+						new UnaryOperatorExpression(UnaryOperatorType.NullConditional, new AnyNode("target")),
+						"ToString"))
+			).WithName("nullConditional")
+		};
+
+		internal static Expression RemoveRedundantToStringInConcat(Expression expr, IMethod concatMethod, bool isLastArgument)
 		{
 			var m = ToStringCallPattern.Match(expr);
-			if (m.Success) {
-				var target = m.Get<Expression>("target").Single();
-				if (ToStringIsKnownEffectFree(target.GetResolveResult().Type) && concatMethod.Parameters.All(IsStringParameter)) {
-					return target;
-				}
+			if (!m.Success)
+				return expr;
+
+			if (!concatMethod.Parameters.All(IsStringParameter)) {
+				// If we're using a string.Concat() overload involving object parameters,
+				// string.Concat() itself already calls ToString() so the C# compiler shouldn't
+				// generate additional ToString() calls in this case.
+				return expr;
+			}
+			var target = m.Get<Expression>("target").Single();
+			var type = target.GetResolveResult().Type;
+			if (!(isLastArgument || ToStringIsKnownEffectFree(type))) {
+				// ToString() order of evaluation matters, see CheckArgumentsForStringConcat().
+				return expr;
 			}
-			return expr;
+			if (type.IsReferenceType != false && !m.Has("nullConditional")) {
+				// ToString() might throw NullReferenceException, but the builtin operator+ doesn't.
+				return expr;
+			}
+
+			// All checks succeeded, we can eliminate the ToString() call.
+			// The C# compiler will generate an equivalent call if the code is recompiled.
+			return target;
 
 			bool IsStringParameter(IParameter p)
 			{