Fix Left to Right semantics with fields and arrays in async

[YairHalberstadt]

Fixes #42464 #42755

Currently async code doesn't respect Left to right semantics when assigning to a field, an array index, or a combination of the two.

The reason is that these return locations, and locations cannot be stored across an async boundary.

We fix this by moving as much as we can to before the async boundary, copying everything over into temps, and then doing a dummy access to cause exceptions if necessary.

For example:

struct A
{
    int x;
}
...
var a = new int[1];
var index = 0;
a[index].x = await SomeMethod();

Is generated as:

var a = new int[1];
var index = 0;
var aTemp = a;
var indexTemp = index;
aTemp[indexTemp].x;
var awaitResult = await SomeMethod();
aTemp[indexTemp].x = awaitResult;

I believe this should preserve semantics, since array indexer and field accesses, with the same receiver and index, is guaranteed to be idempotent in terms of throwing exceptions (with a little bit of complexity to deal with fields of classes).

Note that this produces significantly worse codegen, since you have to create a lot of temps, and hoist a lot more variables. It may be decided that it's better to keep the current (I believe not-to-spec) semantics than to create more correct code, but worse performance.

Note #42738 is not fixed by this, and likely should be an error.

[333fred]

Can you add some more complex left-hand side examples, like ref assignment as a target or ref-returning methods?

Edit: Forgot refs can't be used with await for a minute 😅

[333fred]

Also, it looks like you have a failing test.

[333fred]

Finally, can you please edit your description to put Fix in front of the two issues that this fixes, and remove fix from in front of the issue this doesn't fix so github gets the right issue linking?

[YairHalberstadt]

Can you add some more complex left-hand side examples, like ref assignment as a target or ref-returning methods?

Those are or should be illegal in an async method.

Also, it looks like you have a failing test.

Yeah, I misunderstood semantics for non async methods, and need to make same changes to keep aligned with that.

[333fred]

Those are or should be illegal in an async method.

Yeah, I had just updated my comment :)

[333fred]

Are unsafe operators allowed on the left-hand side of an assignment there? ie a->b = await foo()

[333fred]

Or just a standard a = b = await c;?

[AlekseyTs]

@YairHalberstadt Could you please update the description with proposed code gen?
Also, is the current behavior different from behavior of the native compiler? #Closed

[AlekseyTs]

What about VB? #Closed

[AlekseyTs]

Done with review pass (iteration 1) #Closed

[YairHalberstadt]

Are unsafe operators allowed on the left-hand side of an assignment there? ie a->b = await foo()

Cannot await in an unsafe context

[YairHalberstadt]

Also, is the current behavior different from behavior of the native compiler?

How can I get hold of the native compiler?

[YairHalberstadt]

@AlekseyTs

The following C# program prints "Hello World" in Master, but does not print anything before the exception in Dev12.

using System;
using System.Threading.Tasks;

class Program
{
    public static void Main(string[] args)
    {
        Run().Wait();
    }

    public static async Task Run()
    {
        var a = new A();
        a.B.x = await Write();
    }

    public static async Task<int> Write()
    {
        Console.WriteLine("Hello World!");
        return 5;
    }
}

class A
{
    public B B
    {
        get
        {
            throw new Exception();
        }
    }
}

class B
{
    public int x;
}

The equivalent program in VB does not print anything before the exception:

Imports System
Imports System.Threading.Tasks

Class Program
    Public Shared Sub Main(args() As String)
        Run().Wait()
    End Sub
    
    Shared Async Function Run() As Task
        Dim a = New A()
        a.B.x = Await Write()
    End Function
    
    Shared Async Function Write() As Task(Of Integer)
                Console.WriteLine("Hello World!")
                return 5
    End Function
End Class

Class A
    Public ReadOnly Property B As B
        Get
            Throw New Exception()
        End Get
    End Property
End Class

Class B
    Public x As Integer
End Class

So this bug exists only in Roslyn C#, not in Native C# or VB, or Roslyn VB.

I have a feeling it was introduced by @VSadov's changes in https://github.com/dotnet/roslyn/pull/20883/files

[AlekseyTs]

How can I get hold of the native compiler?

It is on any Windows machine with old .NET framework installed. For example, on my machines it is here c:\Windows\Microsoft.NET\Framework\v4.0.30319\csc.exe. #Closed

[YairHalberstadt]

Thanks, but I managed to test it on VS 2013 in the end. See comment above.

[AlekseyTs]

Done with review pass (iteration 7), C# side of changes LGTM #Closed

[YairHalberstadt]

I've now fixed the bug in VB and copied over the tests

[YairHalberstadt]

I haven't actually changed anything on Calls, since previously we would always check side effects, and now we explicitly pass shouldCheckSideEffects:=True for all cases other than assignments.

However I added a couple of basic tests as sanity checks to confirm this.

[YairHalberstadt]

This is the only actual difference. We would load z and then pop it to trigger side effects (irrelevant here since the Closure can't be null, but incorrectly since the NRE only occurs on assignment)

[AlekseyTs]

shouldCheckSideEffects As Boolean, sideEffectsAlreadyChecked As Boolean [](start = 89, length = 71)

It feels like the two parameters are confusing and unnecessary complicate things. Could we get away with just one parameter, perhaps called "isAssignmentTarget" as in C#? #Closed

[YairHalberstadt]

Nope. This is called from multiple code paths, not just assignments to fields. We require 3 state logic here: should we cause side effects, and if so have we already done something that will trigger these side effects recursively. #Closed

[YairHalberstadt]

I could change this to an enum if that would be clearer? #Closed

[AlekseyTs]

Nope. This is called from multiple code paths, not just assignments to fields. We require 3 state logic here: should we cause side effects, and if so have we already done something that will trigger these side effects recursively.

I still think there is a way to simplify this. It looks like we only want to customize side-effects evaluation for an assignment case.

• Let's have just one parameter, evaluateSideEffects
• VisitBinaryOperator passes True for it.
• VisitLoweredConditionalAccess passes True for it.
• SpillArgumentListInner passes True for it.
• SpillValue overloads above pass the value through
• In this function Case BoundKind.Sequence and Case BoundKind.SpillSequence just pass it through
• Case BoundKind.ArrayAccess evaluates side-effects only if the parameter is True
• Case BoundKind.FieldAccess evaluates side-effects only if the parameter is True and the receiver is not known to be a value type.
• It also passes True if evaluateSideEffects was true and it is not going to evaluate side-effects, False otherwise.
• ProcessRewrittenAssignmentOperator passes false and evaluates side-effects itself as follows:
  Starting with the node ProcessRewrittenAssignmentOperator gets back, if the node is a field access and its receiver is not known to be a value type, it evaluates the field access and stops. Otherwise, if the node is a field access, the process is repeated for the receiver.

Would this work?

---

In reply to: 406469496 [](ancestors = 406469496)

[AlekseyTs]

Perhaps we need to skip one field access in ProcessRewrittenAssignmentOperator for the purpose of side-effect evaluation. I think you should get the idea.

---

In reply to: 406508448 [](ancestors = 406508448,406469496)

[YairHalberstadt]

I've gone for a slightly different refactoring that I think should be easier to follow without duplicating the logic for checking side effects. #Closed

[AlekseyTs]

I've gone for a slightly different refactoring that I think should be easier to follow without duplicating the logic for checking side effects.

I think this is much better.

---

In reply to: 406730966 [](ancestors = 406730966)

[AlekseyTs]

Done with review pass (iteration 9) #Closed

[AlekseyTs]

Dim newReceiver As BoundExpression = SpillValue(fieldAccess.ReceiverOpt, [](start = 24, length = 72)

Suppose we are assigning to a.b.c where a is a local, b and c are fields in structures.
First, we get here for c with shouldCheckSideEffects:=False, sideEffectsAlreadyChecked:=False, checkSideEffectsHere is False, side-effects are not evaluated below
Then, we get here for b with shouldCheckSideEffects:=True, sideEffectsAlreadyChecked:=False, checkSideEffectsHere is True, side-effects are going to be evaluated below
Do we really need to evaluate them? Why? What am I missing?
#Closed

[YairHalberstadt]

Here's another case which I'm not sure what to do for:

The following syncronous program:

Imports System
Imports System.Threading.Tasks

Public Class Program
    Public Shared Sub Main(ByVal args As String())
        Run()
    End Sub

    Public Shared Sub Run()
        A.x.y = Write()
    End Sub

    Public Shared Function Write() As Integer
        Console.WriteLine("Hello World!")
        Return 5
    End Function
End Class

Structure A
    Private Shared Function [Throw]() As A
        Throw New Exception()
    End Function

    Public Shared x As A = [Throw]()
    Public y As Integer
End Structure

Throws without printing.

The asynchronous version:

Imports System
Imports System.Threading.Tasks

Public Class Program
    Public Shared Sub Main(ByVal args As String())
        Run().Wait()
    End Sub

    Public Shared Async Function Run() As Task
        A.x.y = Await Write()
    End Function

    Public Shared Async Function Write() As Task(Of Integer)
        Console.WriteLine("Hello World!")
        Return 5
    End Function
End Class

Structure A
    Private Shared Function [Throw]() As A
        Throw New Exception()
    End Function

    Public Shared x As A = [Throw]()
    Public y As Integer
End Structure

Prints and then throws on the native version, but throws without printing on roslyn.

I think roslyn's behaviour is correct?

Meanwhile for C# the equivalent asynchronous program crashes the native compiler, but prints and then throws in roslyn. I'm guessing correct semantics are same as VB, and it should throw without printing?

[AlekseyTs]

Meanwhile for C# the equivalent asynchronous program crashes the native compiler, but prints and then throws in roslyn. I'm guessing correct semantics are same as VB, and it should throw without printing?

Is the exception thrown by the static constructor and the difference in behavior is due to the fact that it is triggered at a slightly different time? I am not even sure if we should worry about duplicating side-effects like that simply because it is unlikely that anyone would have a dependency on not going too far based on when the static constructor throws. The program is likely "dead" anyway. Is there even a reliable way to recover from an exception thrown by a static constructor? #Closed

[AlekseyTs]

If evaluateSideEffects Then [](start = 24, length = 27)

Do we want to evaluate side-effects before evaluating the RHS when we assign directly to the array element? This looks inconsistent with what C# is doing. #Closed

[AlekseyTs]

fieldAccess.FieldSymbol.ContainingType.IsReferenceType [](start = 105, length = 54)

Is this the right condition? Could this be a generic type parameter that is not known to be a reference type substituted with a reference type at runtime? #Closed

[YairHalberstadt]

It is not possible to assign to a field of a type parameter unless the type is constrained to a specific type. Only three type constraints have child types which can be reference or value types: object, ValueType, and Enum. None of them have fields.
However to be defensive we could check !IsValueType in case e.g. some runtime has a field on object, ValueType, or Enum. It will not cause any observable changes, but may lead to worse performance in such a case because we evaluate a side effect needlessly. #Closed

[AlekseyTs]

However to be defensive we could check !IsValueType in case e.g. some runtime has a field on object, ValueType, or Enum. It will not cause any observable changes, but may lead to worse performance in such a case because we evaluate a side effect needlessly

I don.t think there is a need for the change. I don't know what I was thinking. ContainingType is always a named type, never a type parameter. Therefore, we always know whether it is a reference type or not.

---

In reply to: 407885864 [](ancestors = 407885864)

[AlekseyTs]

Done with review pass (iteration 11) #Closed

[YairHalberstadt]

Is the exception thrown by the static constructor and the difference in behavior is due to the fact that it is triggered at a slightly different time?

No the compiler fails to compile.

I am not even sure if we should worry about duplicating side-effects like that simply because it is unlikely that anyone would have a dependency on not going too far based on when the static constructor throws.

The same would apply if the static constructor printed instead of threw.

[AlekseyTs]

If evaluateSideEffectsHere Then [](start = 24, length = 31)

It looks like C# doesn't evaluate side-effects for an access to a static field. Should we be consistent? #Closed

[AlekseyTs]

I guess Unspillable check above would catch that.

---

In reply to: 408139568 [](ancestors = 408139568)

[AlekseyTs]

Is the exception thrown by the static constructor and the difference in behavior is due to the fact that it is triggered at a slightly different time?

No the compiler fails to compile.

I am confused. You said: "for C# the equivalent asynchronous program crashes the native compiler, but prints and then throws in roslyn." What does it mean: " the compiler fails to compile"? Are you talking about native compiler? I don't think we are going to do anything about that at this point. I was asking about difference in behavior between sync and async execution.

I am not even sure if we should worry about duplicating side-effects like that simply because it is unlikely that anyone would have a dependency on not going too far based on when the static constructor throws.

The same would apply if the static constructor printed instead of threw.

This doesn't change my opinion. An invocation of a static constructor is implicit and happens only once during program execution. When that happens isn't stated explicitly in the expression. Therefore, I find dependency on this side-effect happening at specific point in time very unlikely. #Closed

[AlekseyTs]

LGTM (iteration 12)

[AlekseyTs]

@333fred For the second review, it looks like you reviewed some earlier iterations.

[333fred]

Thanks for the contribution @YairHalberstadt!