Roslyn: Nullable reference types cause exponential compile / analysis time even when disabled

[TessenR]

Version Used:

Microsoft Visual Studio Enterprise 2019 Preview
Version 16.2.0 Preview 1.0
VisualStudio.16.Preview/16.2.0-pre.1.0+28917.182
Microsoft .NET Framework
Version 4.7.03056

Steps to Reproduce:

Create a new solution.
Paste the following code into it:

using System;
using System.Runtime.CompilerServices;
class C
{
  static void Main(string[] args)
  {

  }

  private C f;

  void M(C c)
  {
c.f = c;
c.NotNull(
  x => x.f.NotNull(
    y => y.f.NotNull(
      z => z.f.NotNull(
        q => q.f.NotNull(
          w => w.f.NotNull(
            e => e.f.NotNull(
              r => r.f.NotNull(
          _ =>
          {

            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);
            "".NotNull(s => s);

            return "";
          }))))))));

  }

}

static class Ext
{
  public static extern V NotNull<T, V>([EnsuresNotNull] this T t, Func<T, V> f);
}

namespace System.Runtime.CompilerServices
{
  public class EnsuresNotNullAttribute : System.Attribute { }
}

Try to compile it with
<LangVersion>8.0</LangVersion>
and
<LangVersion>7.3</LangVersion>

Expected Behavior:
Compile time does not change much if you change the project's lang version.

Actual Behavior:
The project compiles in ~2 seconds on my machine if I use C# 7.3

> msbuild ConsoleApp4.sln -property:Nullable=disable


  Program.cs(48,26): warning CS0626: Method, operator, or accessor 'Ext.NotNull<T, V>(T, Func<T, V>)' is marked externa
l and has no attributes on it. Consider adding a DllImport attribute to specify the external implementation.

    1 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.44

It takes ~15 minutes to compile with C# 8.0 on the same machine even though nullable reference types are disabled and not mentioned anywhere in the project.

> msbuild ConsoleApp4.sln -property:Nullable=disable

  Program.cs(48,26): warning CS0626: Method, operator, or accessor 'Ext.NotNull<T, V>(T, Func<T, V>)' is marked externa
l and has no attributes on it. Consider adding a DllImport attribute to specify the external implementation.

    1 Warning(s)
    0 Error(s)

Time Elapsed 00:14:44.28

Notes
It's also quite hard to do literally anything with this file since Roslyn keeps analyzing it restarting the analysis at every change you make causing constant high CPU usage.

You can change the compilation time by a factor of 2 by adding / removing top level .NotNull calls.
Roslyn analyses arguments of methods with flow annotations such as [EnsuresNotNull] at least twice in order to first report errors / warnings and then infer variables' states after the call. Consequently it analyses any argument expression in annotated call chains at least 2^N times where N is the number of annotated calls. Compile/analysis time can be dragged even lower by adding more code to analyze or cycles that will cause multiple data flow passes over the whole method.

I'm also not sure why Roslyn runs this analysis at all despite nullable reference types being explicitly disabled for this project.

[gafter]

/cc @jcouv

[TessenR]

@jcouv
I've noticed that there's a pull request that fixes this issue. It looks like the second pass over annotated calls' arguments was removed. Consequently all side effects during arguments' evaluation will not take flow annotations into account. You can check it with the following code:

#nullable enable
using System.Runtime.CompilerServices;
class C
{
  void M(string? s1, string? s2)
  {
    Test(s1, s2 = s1);
    s1.ToString();
    s2.ToString();
  }

  void Test([EnsuresNotNull] string? s1, string? s2) { }
}

namespace System.Runtime.CompilerServices
{
  public class EnsuresNotNullAttribute : System.Attribute { }
}

This code does not have any warnings in the current master branch as well as the latest preview of VS. You can check this behavior in a slightly outdated master branch via sharplab.io:

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+BiAdgVwBtCJhC4ACOXU8gWACgABABgqYEYA6AJX1xgBLMHC4BhAPZgADoPIIAyogBuggMZwoAbkZMATBTGMA3owrsALBQCyACk4sA/BSgck7Dk5d6AlGYqmDObmACqaMLau7lAGALwuHD46QcGuXCESCjAIgrgA5rZJ/uYx6ZnZuQVFKQC+/v5MVmGwtgDaAKK4UPgImgByEjB9RIQAuh5eURPOMT4BFHUMi4w0IlDSEBoevPxCIuJSsvJKCKoaUCYNAMzsBp3dvVADQyMAgjAVwPjwFCDb759vpRjAtGDUgA==

However, the following test fails in jcouv:cond-attributes branch

        [Fact]
        public void AnnotatedCallSideEffects()
        {
            var source =
@"#nullable enable
using System.Runtime.CompilerServices;
class C
{
  void M(string? s1, string? s2)
  {
    Test(s1, s2 = s1);
    s1.ToString();
    s2.ToString();
  }

  void Test([NotNull] string? s1, string? s2) { }
}";
            var comp = CreateCompilation(new[] {source, NotNullAttributeDefinition });
            comp.VerifyDiagnostics();
        }

As s2 is no longer considered non-null after the call:

Expected:
Actual:
                // (9,5): warning CS8602: Dereference of a possibly null reference.
                //     s2.ToString();
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(9, 5)
Diff:
++>                 Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(9, 5)

I've used the following commit for this test:

commit de944d1c0ae19a0bdd05481f31e41d8c6aaf9dcf
Author: Julien Couvreur <[email protected]>
Date:   Mon Jun 3 07:07:14 2019 -0700

    MaybeNullWhen and NotNullWhen attribute affects callers

Could you please comment on whether this regression is intended or not? I've read through the referenced language design notes but was unable to find mentions of this issue there.

If this is intended you might also want to close https://github.com/dotnet/roslyn/issues/34875 as well since its both actual and expected behavior rely on the side effects taking flow annotations into account.

[jcouv]

It was intentional on my part, and I'll start a thread to confirm with LDM.
I suspect we will stick with the design implemented in that PR (ie. post-conditions are used last). A few reasons/factors:

• it is important to fix the complexity issue (thanks for reporting OP by the way!), and I suspect that it would come back if we apply post-condition attributes earlier in the analysis (see example below where we'll need to evaluate arguments while we're in a split/conditional state, because of a conditional attribute on the first parameter),
• even if we could solve that, our infrastructure for analyzing conversions would make this difficult to implement,
• the scenarios were the difference is observable are probably rare and it is conceptually simpler to explain that post-conditions come into play last in all cases. Consider [MaybeNull]string s which we could theoretically learn from sooner vs. [MaybeNull]ref string s which is an assignment and comes last.

```C#
void M(string? s1, string? s2)
{
if (Test(s1, s2 = s1))
{
s1.ToString();
s2.ToString();
}
}

void Test([NotNullWhen(true)] string? s1, string? s2) { }
```