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.02k stars 4.03k forks source link

Roslyn fails to learn from [MemberNotNullWhen] members used in pattern matching over tuple expressions #51797

Open TessenR opened 3 years ago

TessenR commented 3 years ago

Version Used:

Branch main (8 Mar 2021)
Latest commit eddde7c by Sam Harwell:
Merge pull request #51559 from sharwell/current-thread

Use Environment.CurrentManagedThreadId

Steps to Reproduce: Compile the following code:

#nullable enable
using System.Diagnostics.CodeAnalysis;

public struct Test
{
  [MemberNotNullWhen(true, nameof(Value))]
  public bool IsOk => Value != null;
  public string? Value { get; }

  public string M(bool b)
  {
    if ((IsOk, b) is (true, _))
    {
      return Value; // false warning
    }
  }
}

Expected Behavior: No warnings. The if statement explicitly checks that IsOk member is true which should guarantee that Value is not null.

Actual Behavior: False CS8603: Possible null reference return. is reported for return Value;

Notes Probably related to https://github.com/dotnet/roslyn/issues/50158

jaredpar commented 3 years ago

I believe this falls outside the scope of the intended design for MemberNotNullWhen as well as the rest of our conditional attributes. The underlying flow wasn't designed to track through tuple equality as you're doing here.

I'll let @jcouv confirm though.

jcouv commented 3 years ago

This is a flaw with analysis of switch expressions on tuples, not specific to attributes. See general example below. If we fix the general case, then the attribute case will just fall out.

To perform such analysis, we would need to keep track of multiple states (one per tuple element) before entering the analysis of switch, but currently we only have one state for the entire expression. I think we have some tests illustrating the problem (included in PR https://github.com/dotnet/roslyn/pull/51001 which fixed some issues with switch and patterns on conditional states).

Note: we do have an issue open for related problem with tuple equality (if ((s == null, 1) == (true, 1)) ... should behave like if ((s == null) == true) ...).

#nullable enable

public class C 
{
    public void M(string? s) 
    {
        _ = (s == null, 1) switch
        {
            (false, _) => s.ToString(), // warning CS8602: Dereference of a possibly null reference (on `s`)
            _ => null
        };
    }
}