Avoid making explicit ToString() implicit when doing so changes the semantics for mutable value types.

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

@@ -6,7 +6,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 	{
 		private class C
 		{
-			readonly int i;
+			int i;
 
 			public C(int i)
 			{
@@ -17,13 +17,13 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			public override string ToString()
 			{
 				Console.WriteLine("  C(" + i + ").ToString()");
-				return i.ToString();
+				return (i++).ToString();
 			}
 		}
 
 		private struct S
 		{
-			readonly int i;
+			int i;
 
 			public S(int i)
 			{
@@ -34,7 +34,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			public override string ToString()
 			{
 				Console.WriteLine("  S(" + i + ").ToString()");
-				return i.ToString();
+				return (i++).ToString();
 			}
 		}
 
@@ -98,10 +98,20 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			Console.WriteLine(string.Concat(new S(30), Space()) + new S(31));
 		}
 
+		static void TestStructMutation()
+		{
+			Console.WriteLine("TestStructMutation:");
+			S s = new S(0);
+			Console.WriteLine(Space() + s);
+			Console.WriteLine(Space() + s.ToString());
+			Console.WriteLine(s);
+		}
+
 		static void Main()
 		{
 			TestClass();
 			TestStruct();
+			TestStructMutation();
 		}
 	}
 }

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

@@ -90,7 +90,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 		private void UsingStatementOnNullableStruct(UsingStruct? us)
 		{
 			using (us) {
-				Console.WriteLine("using-body: " + us);
+				Console.WriteLine("using-body: " + us.ToString());
 			}
 		}
 

ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs

@@ -233,14 +233,15 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 
 		static readonly Pattern ToStringCallPattern = new Choice {
 			// target.ToString()
-			new InvocationExpression(new MemberReferenceExpression(new AnyNode("target"), "ToString")),
+			new InvocationExpression(new MemberReferenceExpression(new AnyNode("target"), "ToString")).WithName("call"),
 			// target?.ToString()
 			new UnaryOperatorExpression(
 				UnaryOperatorType.NullConditionalRewrap,
 				new InvocationExpression(
 					new MemberReferenceExpression(
 						new UnaryOperatorExpression(UnaryOperatorType.NullConditional, new AnyNode("target")),
-						"ToString"))
+						"ToString")
+				).WithName("call")
 			).WithName("nullConditional")
 		};
 
@@ -256,6 +257,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 				// generate additional ToString() calls in this case.
 				return expr;
 			}
+			var toStringMethod = m.Get<Expression>("call").Single().GetSymbol() as IMethod;
 			var target = m.Get<Expression>("target").Single();
 			var type = target.GetResolveResult().Type;
 			if (!(isLastArgument || ToStringIsKnownEffectFree(type))) {
@@ -266,6 +268,13 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 				// ToString() might throw NullReferenceException, but the builtin operator+ doesn't.
 				return expr;
 			}
+			if (!ToStringIsKnownEffectFree(type) && toStringMethod != null && IL.Transforms.ILInlining.MethodRequiresCopyForReadonlyLValue(toStringMethod)) {
+				// ToString() on a struct may mutate the struct.
+				// For operator+ the C# compiler creates a temporary copy before implicitly calling ToString(),
+				// whereas an explicit ToString() call would mutate the original lvalue.
+				// So we can't remove the compiler-generated ToString() call in cases where this might make a difference.
+				return expr;
+			}
 
 			// All checks succeeded, we can eliminate the ToString() call.
 			// The C# compiler will generate an equivalent call if the code is recompiled.

ICSharpCode.Decompiler/IL/Transforms/ILInlining.cs

@@ -249,7 +249,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			// Thus, we have to ensure we're operating on an r-value.
 			// Additionally, we cannot inline in cases where the C# compiler prohibits the direct use
 			// of the rvalue (e.g. M(ref (MyStruct)obj); is invalid).
-			if (!IsUsedAsThisPointerInCall(loadInst))
+			if (!IsUsedAsThisPointerInCall(loadInst, out var method))
 				return false;
 			switch (ClassifyExpression(inlinedExpression)) {
 				case ExpressionClassification.RValue:
@@ -261,14 +261,30 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				case ExpressionClassification.ReadonlyLValue:
 					// For struct method calls on readonly lvalues, the C# compiler
 					// only generates a temporary if it isn't a "readonly struct"
-					return !(v.Type.GetDefinition()?.IsReadOnly == true);
+					return MethodRequiresCopyForReadonlyLValue(method);
 				default:
 					throw new InvalidOperationException("invalid expression classification");
 			}
 		}
 
+		internal static bool MethodRequiresCopyForReadonlyLValue(IMethod method)
+		{
+			var type = method.DeclaringType;
+			if (type.IsReferenceType == true)
+				return false; // reference types are never implicitly copied
+			if (type.GetDefinition()?.IsReadOnly == true)
+				return false; // readonly structs are never implicitly copied
+			return true;
+		}
+
 		internal static bool IsUsedAsThisPointerInCall(LdLoca ldloca)
 		{
+			return IsUsedAsThisPointerInCall(ldloca, out _);
+		}
+
+		static bool IsUsedAsThisPointerInCall(LdLoca ldloca, out IMethod method)
+		{
+			method = null;
 			if (ldloca.ChildIndex != 0)
 				return false;
 			if (ldloca.Variable.Type.IsReferenceType ?? false)
@@ -280,7 +296,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			switch (inst.Parent.OpCode) {
 				case OpCode.Call:
 				case OpCode.CallVirt:
-					var method = ((CallInstruction)inst.Parent).Method;
+					method = ((CallInstruction)inst.Parent).Method;
 					if (method.IsAccessor && method.AccessorKind != MethodSemanticsAttributes.Getter) {
 						// C# doesn't allow calling setters on temporary structs
 						return false;