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

Incorrect reference nullability warning when deconstructing result of the function #49313

Open 13xforever opened 3 years ago

13xforever commented 3 years ago

Version Used: Dotnet SDK 5.0.0

Steps to Reproduce: Not sure if relates to #35349, but here's a repro:

using System;
using System.Threading.Tasks;

#nullable enable

public class C {
    public void M()
    {
        Result? r = null;
        r = r.NonNullableResult(); // result is marked as MayBeNull here
        (r, _) = r.MaybeNullableResult(); //warning CS8604: Possible null reference argument for parameter 'input' 
    }
}

public class Result
{
    public int Value {get; set;}
}

internal static class Extensions
{
    public static Result NonNullableResult(this Result? input)
        => input is null ? new() : input;

    public static (Result? result, bool extra) MaybeNullableResult(this Result input)
    {
        Result? tmp = input;
        try
        {
            input.Value = new Random().Next(42);
        }
        catch
        {
            tmp = null;
        }
        return (tmp, tmp is null);
    }
}

Expected Behavior: No warning given

Actual Behavior: I'd expect the result of NonNullableResult() to be not null, at least by the time MaybeNullableResult() is called on it. if you change code to be

var tmp = r.MaybeNullableResult(); 
(r, _) = tmp;

warning goes away, which is kinda confirms it?

jaredpar commented 3 years ago

Think this repro can be simplified to the following

using System;
using System.Threading.Tasks;

#nullable enable

public class C {
    static (C?, bool) M(C p) => throw null!;

    void Repro() {
         C? local = new C();

         // Warns
         (local, _) = M(local);

         // Does not warn
         local = new C();
         (_, _) = M(local);
    }
}

The core problem here is that we appear to be using the result of the deconstruction to update the state of the local before it is used in the call expression where it should be updated after.

jnm2 commented 3 years ago

If you're interested in non-contrived code samples, (state, _) = Reduce(state, ...) happens frequently in test code for one project.