Incorrect result is produced by DataFlowAnalysis.DataFlowsOut when local functions are involved

[AlekseyTs]

        [Fact]
        public void Test1()
        {
            var source =
@"
class P
{
    static void Main()
    {
        bool Dummy(params object[] x) {return true;}

        try {}
        catch when (TakeOutParam(out var x1) && x1 > 0)
        {
            Dummy(x1);
        }

        try {}
        catch when (Dummy(TakeOutParam(out var x14), 
                            TakeOutParam(out var x14), // 2
                            x14))
        {
            Dummy(x14);
        }

        static bool TakeOutParam(out int x) 
        {
            x = 123;
            return true;
        }
    }
}
";
            var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseDll, parseOptions: TestOptions.Regular);

            var tree = compilation.SyntaxTrees.Single();
            var model = compilation.GetSemanticModel(tree);

            var node = tree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().First();
            Assert.Equal("TakeOutParam(out var x1)", node.ToString());
            var flow = model.AnalyzeDataFlow(node);
            Assert.Equal("x1", flow.DataFlowsOut.Single().Name);
        }

Expected: x1 is returned

Observed: x1 and x14 are returned.

[xUnit.net 00:00:00.01] xUnit.net VSTest Adapter v2.4.1-pre.build.4059 (64-bit .NET Core 3.1.1)
[xUnit.net 00:00:02.27]   Starting:    Microsoft.CodeAnalysis.CSharp.Semantic.UnitTests
The active test run was aborted. Reason: Test host process crashed : Process terminated. Assertion failed.
   at Microsoft.CodeAnalysis.CSharp.DataFlowsOutWalker.Analyze(CSharpCompilation compilation, Symbol member, BoundNode node, BoundNode firstInRegion, BoundNode lastInRegion, HashSet`1 unassignedVariables, ImmutableArray`1 dataFlowsIn) in F:\GitHub\roslyn\src\Compilers\CSharp\Portable\FlowAnalysis\DataFlowsOutWalker.cs:line 40
   at Microsoft.CodeAnalysis.CSharp.CSharpDataFlowAnalysis.get_DataFlowsOut() in F:\GitHub\roslyn\src\Compilers\CSharp\Portable\FlowAnalysis\CSharpDataFlowAnalysis.cs:line 156
   at Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.Test1() in F:\GitHub\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\OutVarTests.cs:line 35332

No repro when TakeOutParam and Dummy are changed to be real methods vs. local functions. Still reproduces when only TakeOutParam is converted, but Dummy remains a local function.

[shyamnamboodiripad]

@AbhitejJohn @nohwnd @cvpoienaru looks like testhost process is crashing for above Roslyn unit test when the test fails. Is this expected behavior for testhost? Or is this likely to be something related to how the unit tests themselves are configured / some problem in xUnit?

[AbhitejJohn]

@shyamnamboodiripad , @AlekseyTs : I remember seeing some kind of a fail fast on test failure in the roslyn unit test infra where we killed the current process on certain failures. Could that be what's going on here?
We do have a feature in our backlog to help surface possible cause of the crash with mention of the test being run when the crash occurred. That could especially be useful when multiple tests are run in a session and the only report one sees is that a few tests ran and the process crashed.

[AlekseyTs]

@AbhitejJohn In this case a regular Debug.Assert failure causes the crash rather than a test failure.

[shyamnamboodiripad]

Looks like the behavior for Debug.Assert in net core is well known -
dotnet/runtime#15386

As @nohwnd points out in microsoft/vstest#2309 one workaround is to use a TraceListener.

Example:

using System;
using System.Diagnostics;
using Xunit;

namespace XUnitTestProject1
{
    public class UnitTest1
    {
        static UnitTest1()
        {
            Trace.Listeners.Clear();
            Trace.Listeners.Add(new MyTraceListener());
        }

        [Fact]
        public void Test1() => Debug.Assert(false);

        private class AssertException : Exception { }

        private class MyTraceListener : TraceListener
        {
            public override void Write(string message) => throw new AssertException();
            public override void WriteLine(string message) => throw new AssertException();
        }
    }
}

[shyamnamboodiripad]

@AlekseyTs Looks like the roslyn repo already has a ThrowingTraceListener that is supposed to be hooked up for all tests via app.config - See

roslyn/eng/config/test/Core/app.config

Line 9 in d7e4939

<add name="ThrowingTraceListener" type="Microsoft.CodeAnalysis.ThrowingTraceListener, Roslyn.Test.Utilities" />

I am not sure whether the app.config is really being hooked up (and if it is why that is not working). Perhaps @jaredpar or @tmat may know more about this...

[jaredpar]

I believe part of the problem is the .NET Core version of xUnit doesn't read app.config but instead prefers a json style configuration.

[AlekseyTs]

@agocke

It looks like not all scenarios are fixed. Here is how to repro in features/SimplePrograms branch:

1. Uncomment the assert (it was commented out pending the fix)
2. Run Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.GlobalCode_For_01 unit-test.

Observe:

The active test run was aborted. Reason: Test host process crashed : Process terminated. Assertion failed.
   at Microsoft.CodeAnalysis.CSharp.DataFlowsOutWalker.Analyze(CSharpCompilation compilation, Symbol member, BoundNode node, BoundNode firstInRegion, BoundNode lastInRegion, HashSet`1 unassignedVariables, ImmutableArray`1 dataFlowsIn) in F:\GitHub\roslyn\src\Compilers\CSharp\Portable\FlowAnalysis\DataFlowsOutWalker.cs:line 42
   at Microsoft.CodeAnalysis.CSharp.CSharpDataFlowAnalysis.get_DataFlowsOut() in F:\GitHub\roslyn\src\Compilers\CSharp\Portable\FlowAnalysis\CSharpDataFlowAnalysis.cs:line 156
   at Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.VerifyDataFlow(SemanticModel model, DeclarationExpressionSyntax decl, Boolean isDelegateCreation, Boolean isExecutableCode, IdentifierNameSyntax[] references, ISymbol symbol) in F:\GitHub\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\OutVarTests.cs:line 1124
   at Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.VerifyModelForOutVar(SemanticModel model, DeclarationExpressionSyntax decl, Boolean isDelegateCreation, Boolean isExecutableCode, Boolean isShadowed, Boolean verifyDataFlow, LocalDeclarationKind expectedLocalKind, IdentifierNameSyntax[] references) in F:\GitHub\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\OutVarTests.cs:line 997
   at Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.VerifyModelForOutVar(SemanticModel model, DeclarationExpressionSyntax decl, IdentifierNameSyntax[] references) in F:\GitHub\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\OutVarTests.cs:line 934
   at Microsoft.CodeAnalysis.CSharp.UnitTests.OutVarTests.GlobalCode_For_01() in F:\GitHub\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\OutVarTests.cs:line 23696

[nohwnd]

@AlekseyTs this was fixed in test.sdk nuget (16.6.0 I believe). This will get fixed after you update #44442

[AlekseyTs]

Our tests do not test compiler from SDK, they test the version built locally.

[nohwnd]

That is fine, but the Microsoft.NET.Test.SDK (that I called Test.SDK above), is what is bringing the test host that your tests will run in. You need to be at version 16.6.0 or newer to have test host that handles Debug.Assert gracefully.

[AlekseyTs]

The problem is the assert that is being thrown, not how test host reacts to it