Definite assignment bug when not awaiting an async closure

[YairHalberstadt]

Version Used: master as of 10th march 2020

Steps to Reproduce:

using System;
using System.Threading.Tasks;

public class C {
    public void M() {
        bool a;
        M1();
        Console.WriteLine(a);
        async Task M1()
        {
            if ("" == String.Empty)
            {
                throw new Exception();
            }
            else
            {
                a = true;
            }
        }
    }
}

Expected Behavior:

error CS0165: Use of unassigned local variable 'a'

Actual Behavior:

Compiles without error

[jnm2]

Also repros in VS 16.6-p4

[svick]

What makes you think this is a bug? The local function doesn't contain any awaits itself, so it is guaranteed to execute fully even if you don't await it. If you add an await to the beginning of the local function, the error is reported correctly.

[canton7]

@svick Right, but the local function throws an exception, and so never assigns to a. However once M1 has finished synchronously executing, you run Console.WriteLine(a), which accesses a -- but a hasn't been assigned to yet.

[YairHalberstadt]

@svick

The following program prints false, instead of throwing. If SkipLocalsInit was applied, it would be non-deterministic.

using System;
using System.Threading.Tasks;

namespace ConsoleApp10
{
    class Program
    {
        static void Main(string[] args)
        {
            bool a;
            M1();
            Console.WriteLine(a);
            async Task M1()
            {
                if ("" == String.Empty)
                {
                    throw new Exception();
                }
                else
                {
                    a = true;
                }
            }
        }
    }
}

[svick]

@canton7 @YairHalberstadt Thanks, you're right. The part that I missed (and probably also the mistake in the compiler's analysis) is that an exception thrown from the synchronous part of an async function is not rethrown if you don't await it.

[jcouv]

Assigned to @agocke to advise

[agocke]

The part that I missed (and probably also the mistake in the compiler's analysis) is that an exception thrown from the synchronous part of an async function is not rethrown if you don't await it.

Oops. Yup, that's what I overlooked. I suppose one fix is that calls to async local functions could always be considered reachable at the end.

[jcouv]

Assigned to @RikkiGibson since he's been deep in definite assignment recently and Milestone=Compiler.Next. Feel free to further triage/move if needed.

[RikkiGibson]

I think the solution Andy suggested is good and we should be able to knock it out before too long.

[AlekseyTs]

I did some digging into implementation of flow analysis around local functions. Here is what I found.

Finding #1.

For the purpose of definite assignment analysis, await expressions and the like are treated in local functions as returns. This is achieved by registering a pending branch while visiting await expression here (https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/FlowAnalysis/AbstractFlowPass.cs,2583) and joining state from all of them in the loop here (https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/FlowAnalysis/AbstractFlowPass_LocalFunctions.cs,131).

The effect of this can be observed with this code (https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgYAYAEMEFYDcqqAxAA4BOAhgOYC2lmA7peQHZivWYAmYAzpQBGEAKaYAwgGUALOgTTMAekWYAQiIDGlAK58xwABb9MWiBEzHWAe2DYAnCO4AaTCIAem7cDBXWmKwBmmIZiGtrk5CKstrQihlbcJr7erNoifJiCIgFWkcEGoZRmFhkaVrSkosCOAHQSvnxg3CLkmJSklQCeHFwhmADkMHb9/qQtlMC5wVb5YpF82hC2gbMmRRA1JBQ09Ews7Jw8/EKiEpIIdnYAHEoqACpGGTA3sfGJEJQaANYZg8Oj40m5AylFYiUYkHM5G0fj4nVYGgM5F8Vl0EE6dXEDSaLWwWD6fxGVjGVCB03smGsrAAtMIrN8epgAIIABQAkmszHwXFM/tgAGw1ABKMIAFDUJQBKEaTHgzcQsgCqtNRYKYuS+/j8DEEny+1GRMMShkilG4mxQqBgAGZsHAJKgAN6oTCu7C2mAKACyoslLrdzpQbuDmSsVnMlEIQZDrq9CF9UZjrpwdlFlEliaT/pjzwFmDjvuzIcDSZDYCCooARJXMABeWuYSTAcg9GoAUQqwE6fujpddJb7ObsApqAE0wCIINwE0XSwBfIi9vsMBvNtKZmMLpdbudAA):

using System;
using System.Threading.Tasks;

#pragma warning disable CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.
#pragma warning disable CS1998 // This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.

public class C
{
    public void M()
    {
        bool a;
        M1();
        Console.WriteLine(a); // <- error CS0165: Use of unassigned local variable 'a'
        
        async Task M1()
        {
            if ("" == String.Empty)
            {
                await Task.Yield();
            }

            a = true;
        }
    }
}

Compiler’s behavior looks reasonable. ‘M1’ is not awaited, therefore, ‘a’ can be accessed before it is assigned in the local function.

Finding #2.

We do not make any adjustments to the analysis depending on whether an async local function is awaited or not. This can be observed with the following code (https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgYAYAEMEFYDcqqAxAA4BOAhgOYC2lmA7peQHZivWYAmYAzpQBGEAKaYAwgGUALOgTTMAekWYAQiIDGlAK58xwABb9MWiBEzHWAe2DYAnCO4AaTCIAem7cDBXWmKwBmmIZiGtrk5CKstrQihlbcJr7erNoifJiCIgFWkcEGoZRmFhkaVrSkosCOAHQSvnxg3CLkmJSklQCeHFwhmADkMHb9/qQtlMC5wVb5YpF82hC2gbMmRRA1JBQ09Ews7Jw8/EKiEpIIdnYAHEoqACpGGTA3sfGJEJQaANYZg8Oj40m5AylFYiUYkHM5G0fj4nVYGgM5F8Vl0EE6dXEDSaLWwWD6fxGVjGVCB03smGsrAAtMIrN8epgAIIABQAkmszHwXFM/tgAGw1ABKMIAFDUJQBKEaTHgzcQsgCqtNRYKYuS+/j8DEEny+1GRMMShkilG4mxQqBgAGZsHAJKgAN6oTCu7C254CzAAWVFkpdbudKDdIcyVis5kohGDoddQx9CD90djcYuosokuTKYDsc9MH5Cb9OdDQZTobAQVFACIq5gALx1zCSYDkHo1ACiFWAnX9MbLrtL/dzdgFNQAmmARBBuEni2WAL5EPv9hiNltpLOxxfL7fzoA===), which simply adds await at the local function’s call site to the example above:

using System;
using System.Threading.Tasks;

#pragma warning disable CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.
#pragma warning disable CS1998 // This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.

public class C
{
    public async Task M()
    {
        bool a;
        await M1();
        Console.WriteLine(a); // <- still report error CS0165: Use of unassigned local variable 'a'
        
        async Task M1()
        {
            if ("" == String.Empty)
            {
                await Task.Yield();
            }

            a = true;
        }
    }
}

The error is still reported even though await in ‘M1’ now interrupts execution in ‘M’, and, once execution resumes, ‘a’ is going to be definitely assigned.
One might find this compiler behavior surprising, but, I guess, it is a reasonable simplification to go with the most conservative analysis. Because, for example, we do not know when, if at all, a delegate created from a local function going to be awaited.

Back to exceptions

In my opinion, for the purpose of definite assignment analysis, explicit throws inside an async local function are somewhat similar to awaits. I.e., if the local function is not awaited, a throw is equivalent to a return from the local function. If the local function is awaited, a throw is equivalent to a throw at the local function’s call site.

The analysis will be more accurate if we also detect situations when thrown exceptions are definitely caught inside the local function. For example, catch all or catch the same (or base) exception type. But we will have to track rethrow situations as well.

The analysis will be even more accurate if we also analyze finally-es, those might still assign things

The analysis will be even more accurate if we start adjusting analysis for definitely awaited call sites.

[AlekseyTs]

We had a short e-email discussion about this issue, here is a brief summary:
• At first blush it would seem that we should treat explicitly thrown exceptions in async local functions the same as awaits: They are points where execution may return to the caller (who may not await and thus propagate the exception), so we should consider the code after the throw statement as not necessarily having executed when the calling function resumes.
• However, exceptions are worse than awaits in that they can be implicitly thrown. Why should explicitly thrown exceptions be treated any differently than implicitly thrown ones – which can happen pretty much anywhere?
• To be safe, then, the upshot would be that the caller of an async local function cannot expect any of the callee’s code to execute ever!

This may seem draconian in some places – especially when the async local function is immediately being awaited. But if we want to fix this, we should introduce a notion of “immediately awaited calls”: When an async local function call is “immediately awaited” we treat it as if the body was invoked synchronously, any exceptions are propagated, in their absence code executes to the end, etc. (Async iterators would be treated like synchronous iterators, whatever that means).

Either way the fix will be a breaking change. If we do the “immediately awaited calls” thing it will be more work, but we suspect the break will also be a lot smaller.

[agocke]

To be safe, then, the upshot would be that the caller of an async local function cannot expect any of the callee’s code to execute ever!

Sounds like the right change to me. Similar to the analysis we do for try/catch/finally, where none of the code in the try block is assumed to have been run in the catch/finally, in case a ThreadAbort happened before the first instruction.