dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.03k forks source link

Strange behaviour in async method #32276

Open vsfeedback opened 5 years ago

vsfeedback commented 5 years ago

I have a solution targeting NetCoreApp2.0.

Lately I've seen that when running an async method, the debugger shows me variables that are non allocated (out of scope).

If this variable is an IQueryable<string> it will show show up in the 'Locals window' as NullReferenceException.

If I manually set the 'next statement' to first statement in the block containing this IQueryable<string>, a NullReferenceException will be thrown when accessing the variable.

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/238451/strange-behaviour-in-async-method.html VSTS ticketId: 604787 These are the original issue comments: (no comments) These are the original issue solutions: (no solutions)

ivanbasov commented 5 years ago

@cston , here is an interesting EE issue.

Running the following code under .NET Core:

using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Threading.Tasks;

namespace ConsoleApp101
{
    class Program
    {
        static void Main(string[] args)
        {
            var x = TestItAsync(true);
        }

        private class Test
        {
            public string Id { get; set; }
        }

        private static async Task<int> TestItAsync(bool doIt = false)
        {
            var testData = new List<Test> { new Test() };

            // Set breakpoint to line below => if (doIt)
            Debugger.Break();
            if (doIt)
            {
                // Set next statement to line below => var testIds = (IQueryable<string>)null;
                var testIds = (IQueryable<string>)null;

                if (testIds.Any())
                {
                    testData = testData.Where(x => testIds.Contains(x.Id)).ToList();
                }
            }

            return await Task.FromResult(0);
        }
    }
}

and stopping at the breakpoint, the testIds variable is displayed in the list of locals even if it is not yet in the scope. As I see, the following method http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.ExpressionEvaluator.ExpressionCompiler/CompilationContext.cs,1430 adds an extra display class type for an internal field, and then this an extra field to the scope. I assume that there should be some extra check if fieldKind is DisplayClassLocalOrField before adding the field to displayClassTypes. Could you please tale a look?

cston commented 5 years ago

@ivanbasov, testIds is emitted as a field on a compiler-generated display class, and an instance of that display class is field on the state machine generated for the async method. Since the are no longer locals in the state machine MoveNext() method for either testIds or the display class, we may not have the scope information from the source method that would allow limiting the scope of testIds in the EE.

tmat commented 5 years ago

We should have that information. The compiler emits scope information for lifted variables to the PDB: https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/specs/PortablePdb-Metadata.md#state-machine-hoisted-local-scopes-c--vb-compilers. There might be some issue with that.

ivanbasov commented 5 years ago

@csto, do you agree with @tmat that we may have the scope information? Can this help us?

andrewjsaid commented 4 years ago

I have run into very similar behaviour in a program similar to the following. The NullReferenceException occurs because the display class is null. Only the if statement is skipped in this scenario.

class Program
{
    static async Task Main()
    {
        var p = new Program();
        await p.TestAsync(false);
    }

    public async Task TestAsync(bool enabled)
    {
        Debugger.Break();
        if (enabled)
        { // Set next statement to this line, then continue running.

            var obj = new object(); // <-- This line throws NullReferenceException

            if ((new object[0]).Any(x => x == obj))
            {
                await Task.Delay(1);
            }
        }
    }
}

I would imagine skipping over an if statement is a relatively common when debugging, and the unexpected exception is very misleading. The RHS of the expression could be a complex expression and investigating which part threw the NRE can be time-consuming.

If fixing this is too much effort, I think disallowing "Set Next Statement" in these situations would be preferable to the current behaviour.