False nullability warnings caused by order of local function declarations in containing method

[TessenR]

Version Used:

Branch main (2 Aug 2021)
Latest commit 914d69c by Julien Couvreur:
Track "raw string literals" feature (#55307)

Steps to Reproduce:

Compile the following code:

using System.Diagnostics.CodeAnalysis;

class C
{
  void Test1()
  {
    var x = "";
    Local1();
    Fail();
    Local2();
    return;

    void Local2()
    {
      if ("".Length > 0)
        Local2();

      x.ToString(); // false CS8602: Dereference of a possibly null reference.
    }

    void Local1() => Local2();
  }

  void Test2()
  {
    var x = "";
    Local1();
    Fail();
    Local2();
    return;

    void Local1() => Local2();

    void Local2()
    {
      if ("".Length > 0)
        Local2();

      x.ToString(); // no warnings
    }
  }

  [DoesNotReturn]
  public void Fail() => throw null!;
}

Expected Behavior: I'd expect no warnings in both methods. At the very least warnings in both methods should be the same as methods are exactly the same

Actual Behavior: Test1 reports false CS8602: Dereference of a possibly null reference for x.ToString() in Local2 Test2 doesn't report any warnings.

Notes: Note that both methods are exactly the same except for the order of local function declarations which don't execute any code so analysis results must be the same. Note that x can never be nullable in both methods and Local2 can be invoked in both methods.

It looks like in Test1 the following sequence of events occur:

• Roslyn finds call sites to both Local1 and Local2
• A call site to Local2 is unreachable but Roslyn proceeds with inspecting it
• Roslyn seems to follow the order of declarations when processing local functions so it starts with Local2 as it's declared first
• Since the only known call site of Local2 is unreachable Roslyn assumes default state for each variable which is Nullable for var x since var is always declared as nullable
• Roslyn finds a new reachable call site of Local2 within Local2 itself. The state of x at this call site is nullable because Roslyn thinks that Local2 is always unreachable (see the step above)
• Roslyn inspects Local1 and finds a new reachable call site of Local2
• At this point Roslyn believes that it has two reachable call sites for Local2, so it merges them and infers that x might be nullable on some call sites

In Test2 local functions are inspected in different order so the following sequence of events occur:

• Roslyn finds call sites to both Local1 and Local2
• It inspects Local1 first as it's declared first in the method
• Roslyn finds a new reachable call site of Local2 within Local1 and disregards the original unreachable call site
• Roslyn proceeds with inspecting Local2 with the only known reachable call site where x is known to be non-nullable
• The recursive call within Local2 is concluded to have the exact same state with non-nullable value of x

The problem can be fixed by adding the following rule: local functions with reachable call sites are inspected before any local functions which only have unreachable call sites.

[RikkiGibson]

We might be able to fix this at the same time as #44805.