Fix crash in SpillSequenceSpiller (#47428)

* Fix crash in SpillSequenceSpiller * Add similar test with an instance field * Add more tests
dotnet · Sep 9, 2020 · a3738c4 · a3738c4
1 parent 7b3d906
commit a3738c4
Show file tree

Hide file tree

Showing 3 changed files with 191 additions and 33 deletions.
diff --git a/src/Compilers/CSharp/Portable/Lowering/SpillSequenceSpiller.cs b/src/Compilers/CSharp/Portable/Lowering/SpillSequenceSpiller.cs
@@ -713,42 +713,46 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
 
             BoundExpression fieldWithSpilledReceiver(BoundFieldAccess field, ref BoundSpillSequenceBuilder leftBuilder, bool isAssignmentTarget)
             {
-                BoundExpression receiver;
                 var generateDummyFieldAccess = false;
-                if (field.FieldSymbol.ContainingType.IsReferenceType)
+                if (!field.FieldSymbol.IsStatic)
                 {
-                    // a reference type can always live across await so Spill using leftBuilder
-                    receiver = Spill(leftBuilder, VisitExpression(ref leftBuilder, field.ReceiverOpt));
-
-                    // dummy field access to trigger NRE
-                    // a.b = c will trigger a NRE if a is null on assignment,
-                    // but a.b.c = d will trigger a NRE if a is null before evaluating d
-                    // so check whether we assign to the field directly
-                    generateDummyFieldAccess = !isAssignmentTarget;
-                }
-                else if (field.ReceiverOpt is BoundArrayAccess arrayAccess)
-                {
-                    // an arrayAccess returns a ref so can only be called after the await, but spill expression and indices
-                    var expression = VisitExpression(ref leftBuilder, arrayAccess.Expression);
-                    expression = Spill(leftBuilder, expression, RefKind.None);
-                    var indices = this.VisitExpressionList(ref leftBuilder, arrayAccess.Indices, forceSpill: true);
-                    receiver = arrayAccess.Update(expression, indices, arrayAccess.Type);
-                    // dummy array access to trigger IndexOutRangeException or NRE
-                    // we only need this if the array access is a receiver since
-                    // a[0] = b triggers a NRE/IORE on assignment
-                    // but a[0].b = c triggers an NRE/IORE before evaluating c
-                    Spill(leftBuilder, receiver, sideEffectsOnly: true);
-                }
-                else if (field.ReceiverOpt is BoundFieldAccess receiverField)
-                {
-                    receiver = fieldWithSpilledReceiver(receiverField, ref leftBuilder, isAssignmentTarget: false);
-                }
-                else
-                {
-                    receiver = Spill(leftBuilder, VisitExpression(ref leftBuilder, field.ReceiverOpt), RefKind.Ref);
-                }
+                    Debug.Assert(field.ReceiverOpt is object);
+                    BoundExpression receiver;
+                    if (field.FieldSymbol.ContainingType.IsReferenceType)
+                    {
+                        // a reference type can always live across await so Spill using leftBuilder
+                        receiver = Spill(leftBuilder, VisitExpression(ref leftBuilder, field.ReceiverOpt));
+
+                        // dummy field access to trigger NRE
+                        // a.b = c will trigger a NRE if a is null on assignment,
+                        // but a.b.c = d will trigger a NRE if a is null before evaluating d
+                        // so check whether we assign to the field directly
+                        generateDummyFieldAccess = !isAssignmentTarget;
+                    }
+                    else if (field.ReceiverOpt is BoundArrayAccess arrayAccess)
+                    {
+                        // an arrayAccess returns a ref so can only be called after the await, but spill expression and indices
+                        var expression = VisitExpression(ref leftBuilder, arrayAccess.Expression);
+                        expression = Spill(leftBuilder, expression, RefKind.None);
+                        var indices = this.VisitExpressionList(ref leftBuilder, arrayAccess.Indices, forceSpill: true);
+                        receiver = arrayAccess.Update(expression, indices, arrayAccess.Type);
+                        // dummy array access to trigger IndexOutRangeException or NRE
+                        // we only need this if the array access is a receiver since
+                        // a[0] = b triggers a NRE/IORE on assignment
+                        // but a[0].b = c triggers an NRE/IORE before evaluating c
+                        Spill(leftBuilder, receiver, sideEffectsOnly: true);
+                    }
+                    else if (field.ReceiverOpt is BoundFieldAccess receiverField)
+                    {
+                        receiver = fieldWithSpilledReceiver(receiverField, ref leftBuilder, isAssignmentTarget: false);
+                    }
+                    else
+                    {
+                        receiver = Spill(leftBuilder, VisitExpression(ref leftBuilder, field.ReceiverOpt), RefKind.Ref);
+                    }
 
-                field = field.Update(receiver, field.FieldSymbol, field.ConstantValueOpt, field.ResultKind, field.Type);
+                    field = field.Update(receiver, field.FieldSymbol, field.ConstantValueOpt, field.ResultKind, field.Type);
+                }
 
                 if (generateDummyFieldAccess)
                 {

diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs
@@ -6319,5 +6319,102 @@ .locals init (int V_0,
   IL_009e:  ret
 }");
         }
+
+        [Fact, WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")]
+        public void AssignStaticStructField()
+        {
+            var source = @"
+using System;
+using System.Threading.Tasks;
+
+public struct S1
+{
+    public int Field;
+}
+
+public class C
+{
+    public static S1 s1;
+    static async Task M(Task<int> t)
+    {
+        s1.Field = await t;
+    }
+
+    static async Task Main()
+    {
+        await M(Task.FromResult(1));
+        Console.Write(s1.Field);
+    }
+}";
+            var verifier = CompileAndVerify(source, expectedOutput: "1");
+            verifier.VerifyDiagnostics();
+        }
+
+        [Fact, WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")]
+        public void AssignStaticStructField_ViaUsingStatic()
+        {
+            var source = @"
+using System;
+using System.Threading.Tasks;
+using static C;
+
+public struct S1
+{
+    public int Field;
+}
+
+public class C
+{
+    public static S1 s1;
+}
+
+public class Program
+{
+    static async Task M(Task<int> t)
+    {
+        s1.Field = await t;
+    }
+
+    static async Task Main()
+    {
+        await M(Task.FromResult(1));
+        Console.Write(s1.Field);
+    }
+}
+";
+            var verifier = CompileAndVerify(source, expectedOutput: "1");
+            verifier.VerifyDiagnostics();
+        }
+
+        [Fact, WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")]
+        public void AssignInstanceStructField()
+        {
+            var source = @"
+using System;
+using System.Threading.Tasks;
+
+public struct S1
+{
+    public int Field;
+}
+
+public class C
+{
+    public S1 s1;
+    async Task M(Task<int> t)
+    {
+        s1.Field = await t;
+    }
+
+    static async Task Main()
+    {
+        var c = new C();
+        await c.M(Task.FromResult(1));
+        Console.Write(c.s1.Field);
+    }
+}";
+            var verifier = CompileAndVerify(source, expectedOutput: "1");
+            verifier.VerifyDiagnostics();
+        }
     }
 }
diff --git a/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenAsyncTests.vb b/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenAsyncTests.vb
@@ -11440,6 +11440,63 @@ After Invoke").VerifyIL("Program.VB$StateMachine_0_Invoke.MoveNext", "
   IL_00d1:  ret
 }")
         End Sub
+
+        <Fact>
+        <WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")>
+        Public Sub AssignModuleStructureField()
+            Dim source = "
+Imports System
+Imports System.Threading.Tasks
+
+Public Structure S1
+    Public Field As Integer
+End Structure
+
+Module Program
+    Dim s1 As S1
+
+    Async Function M1(t As Task(Of Integer)) As Task
+        s1.Field = Await t
+    End Function
+
+    Sub Main()
+        M1(Task.FromResult(1)).Wait()
+        Console.Write(s1.Field)
+    End Sub
+End Module
+"
+            Dim compilation = CreateCompilation(source, options:=TestOptions.ReleaseExe)
+            CompileAndVerify(compilation, expectedOutput:="1")
+        End Sub
+
+        <Fact>
+        <WorkItem(47191, "https://github.com/dotnet/roslyn/issues/47191")>
+        Public Sub AssignInstanceStructureField()
+            Dim source = "
+Imports System
+Imports System.Threading.Tasks
+
+Public Structure S1
+    Public Field As Integer
+End Structure
+
+Class C
+    Dim s1 As S1
+
+    Async Function M1(t As Task(Of Integer)) As Task
+        s1.Field = Await t
+    End Function
+
+    Shared Sub Main()
+        Dim c = New C()
+        c.M1(Task.FromResult(1)).Wait()
+        Console.Write(c.s1.Field)
+    End Sub
+End Class
+"
+            Dim compilation = CreateCompilation(source, options:=TestOptions.ReleaseExe)
+            CompileAndVerify(compilation, expectedOutput:="1")
+        End Sub
     End Class
 End Namespace