From f5eb00c0878ca6edcbc0ecf9b64bcb7a43b290d3 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Thu, 29 Mar 2018 00:26:09 -0700 Subject: [PATCH] Struct this should be saved to temp in tuple equality (#25390) --- ...ocalRewriter_CompoundAssignmentOperator.cs | 9 +++- ...writer_DeconstructionAssignmentOperator.cs | 2 +- .../Emit/CodeGen/CodeGenDeconstructTests.cs | 50 +++++++++++++++++++ .../Emit/CodeGen/CodeGenTupleEqualityTests.cs | 38 ++++++++++++-- 4 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs index 4296c4de492..779fd9e0669 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs @@ -661,11 +661,14 @@ private BoundExpression BoxReceiver(BoundExpression rewrittenReceiver, NamedType /// l += goo(ref l); /// /// even though l is a local, we must access it via a temp since "goo(ref l)" may change it - /// on between accesses. + /// on between accesses. + /// + /// Note: In `this.x++`, `this` cannot change between reads. But in `(this, ...) == (..., this.Mutate())` it can. /// internal static bool CanChangeValueBetweenReads( BoundExpression expression, - bool localsMayBeAssignedOrCaptured = true) + bool localsMayBeAssignedOrCaptured = true, + bool structThisCanChangeValueBetweenReads = false) { if (expression.IsDefaultValue()) { @@ -681,6 +684,8 @@ private BoundExpression BoxReceiver(BoundExpression rewrittenReceiver, NamedType switch (expression.Kind) { case BoundKind.ThisReference: + return structThisCanChangeValueBetweenReads && ((BoundThisReference)expression).Type.IsStructType(); + case BoundKind.BaseReference: return false; diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs index c5b564ee498..4a6f8353f12 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs @@ -271,7 +271,7 @@ private static bool IsTupleExpression(BoundKind kind) private BoundExpression EvaluateSideEffectingArgumentToTemp(BoundExpression arg, ArrayBuilder effects, ref ArrayBuilder temps) { - if (CanChangeValueBetweenReads(arg, localsMayBeAssignedOrCaptured: true)) + if (CanChangeValueBetweenReads(arg, localsMayBeAssignedOrCaptured: true, structThisCanChangeValueBetweenReads: true)) { BoundAssignmentOperator store; var temp = _factory.StoreToTemp(arg, out store); diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs index 439348619fc..12b56b3a3f6 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs @@ -8279,5 +8279,55 @@ class C Assert.Null(info.Method); Assert.Empty(info.Nested); } + + [Fact] + public void TestDeconstructStructThis() + { + string source = @" +public struct S +{ + int I; + public static void Main() + { + S s = new S(); + s.M(); + } + public void M() + { + this.I = 42; + var (x, (y, z)) = (this, this /* mutating deconstruction */); + System.Console.Write($""{x.I} {y} {z}""); + } + void Deconstruct(out int x1, out int x2) { x1 = I++; x2 = I++; } +} +"; + var comp = CreateCompilation(source, options: TestOptions.DebugExe); + CompileAndVerify(comp, expectedOutput: "42 42 43"); + } + + [Fact] + public void TestDeconstructClassThis() + { + string source = @" +public class C +{ + int I; + public static void Main() + { + C c = new C(); + c.M(); + } + public void M() + { + this.I = 42; + var (x, (y, z)) = (this, this /* mutating deconstruction */); + System.Console.Write($""{x.I} {y} {z}""); + } + void Deconstruct(out int x1, out int x2) { x1 = I++; x2 = I++; } +} +"; + var comp = CreateCompilation(source, options: TestOptions.DebugExe); + CompileAndVerify(comp, expectedOutput: "44 42 43"); + } } } diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs index 292d0fb74b6..5fbb3a5d403 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs @@ -648,13 +648,43 @@ S Mutate() public static implicit operator S(int value) { return new S() { I = value }; } public static bool operator==(S s1, S s2) { System.Console.Write($""{s1.I} == {s2.I}, ""); return s1.I == s2.I; } public static bool operator!=(S s1, S s2) { throw null; } + public override bool Equals(object o) { throw null; } + public override int GetHashCode() { throw null; } }"; + var comp = CompileAndVerify(source, expectedOutput: "1 == 1, 2 == 2, True"); + comp.VerifyDiagnostics(); + } - // https://github.com/dotnet/roslyn/issues/25488 - // We need to create a temp for `this`, otherwise it gets mutated + [Fact] + public void TestThisClass() + { + var source = @" +public class C +{ + public int I; + public static void Main() + { + C c = new C() { I = 1 }; + c.M(); + } + void M() + { + System.Console.Write((this, 2) == (2, this.Mutate())); + } - var comp = CompileAndVerify(source, expectedOutput: "2 == 1, False"); - //comp.VerifyDiagnostics(); + C Mutate() + { + I++; + return this; + } + public static implicit operator C(int value) { return new C() { I = value }; } + public static bool operator==(C c1, C c2) { System.Console.Write($""{c1.I} == {c2.I}, ""); return c1.I == c2.I; } + public static bool operator!=(C c1, C c2) { throw null; } + public override bool Equals(object o) { throw null; } + public override int GetHashCode() { throw null; } +}"; + var comp = CompileAndVerify(source, expectedOutput: "2 == 2, 2 == 2, True"); + comp.VerifyDiagnostics(); } [Fact] -- GitLab