Add member 'Principal' and 'Properties' to MemberNotNullWhenAttribute

[msschl]

Contributes to #5680

[msschl]

The ctor in AuthenticationTicket throws an ArgumentNullException when ClaimsPrincipal is null. Hence, when the Ticket is set in the AuthenticateResult i.e., Succeeded is true, the Principal member is also not null.
See AuthenticationTicket.cs#L24

[msschl]

The same is true for the Properties property. If the Ticket is set i.e., Succeeded in the AuthenticateResult is true, the Properties property is not null.
See AuthenticationTicket.cs#L29

[pranavkm]

I'm surprised the compiler allow this as-is. You cannot demonstrate that Properties is not null if this returned true.

[msschl]

Ah you mean because of the protected settter on the property Properties?

[pranavkm]

Sorry took me a bit to try this out: using MemberNotNull / MemberNotNullWhen on a method causes the compiler to ensure non-nullness at exit, but this is not enforced when the attribute is applied to properties.

@jcouv, is that the expected behavior?

[jcouv]

@pranavkm Please provide a small/standalone scenario to clarify what you mean. Those attributes also work on properties as illustrated in this test:

        [Fact]
        public void MemberNotNullWhenTrue_Property()
        {
            var c = CreateNullableCompilation(new[] { @"
using System.Diagnostics.CodeAnalysis;
public class C
{
    public string field1;
    public string? field2;
    public string field3;
    public string? field4;

    public C()
    {
        if (IsInit)
        {
            field1.ToString();
            field2.ToString();
            field3.ToString(); // 1
            field4.ToString(); // 2
        }
        else
        {
            field1.ToString(); // 3
            field2.ToString(); // 4
            field3.ToString(); // 5
            field4.ToString(); // 6
            throw null!;
        }
    }

    public C(bool b)
    {
        IsInit = b;
        field1.ToString(); // 7
        field2.ToString(); // 8
        field3.ToString(); // 9
        field4.ToString(); // 10
    }

    [MemberNotNullWhen(true, nameof(field1), nameof(field2))]
    bool IsInit { get => throw null!; set => throw null!; }
}
", MemberNotNullWhenAttributeDefinition }, parseOptions: TestOptions.Regular9);

            c.VerifyDiagnostics(
                // (16,13): warning CS8602: Dereference of a possibly null reference.
                //             field3.ToString(); // 1
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field3").WithLocation(16, 13),
                // (17,13): warning CS8602: Dereference of a possibly null reference.
                //             field4.ToString(); // 2
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field4").WithLocation(17, 13),
                // (21,13): warning CS8602: Dereference of a possibly null reference.
                //             field1.ToString(); // 3
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field1").WithLocation(21, 13),
                // (22,13): warning CS8602: Dereference of a possibly null reference.
                //             field2.ToString(); // 4
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field2").WithLocation(22, 13),
                // (23,13): warning CS8602: Dereference of a possibly null reference.
                //             field3.ToString(); // 5
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field3").WithLocation(23, 13),
                // (24,13): warning CS8602: Dereference of a possibly null reference.
                //             field4.ToString(); // 6
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field4").WithLocation(24, 13),
                // (32,9): warning CS8602: Dereference of a possibly null reference.
                //         field1.ToString(); // 7
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field1").WithLocation(32, 9),
                // (33,9): warning CS8602: Dereference of a possibly null reference.
                //         field2.ToString(); // 8
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field2").WithLocation(33, 9),
                // (34,9): warning CS8602: Dereference of a possibly null reference.
                //         field3.ToString(); // 9
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field3").WithLocation(34, 9),
                // (35,9): warning CS8602: Dereference of a possibly null reference.
                //         field4.ToString(); // 10
                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "field4").WithLocation(35, 9)
                );
        }
        ```

[jcouv]

@pranavkm My bad. Upon re-reading your comment, I understood the question. It's about enforcement inside the property body. Let me double-check.

[jcouv]

@pranavkm Confirmed that we also enforce inside property bodies. Here's an example:

        [Fact]
        public void MemberNotNullWhenTrue_EnforcedInMethodBody_Property()
        {
            var c = CreateNullableCompilation(new[] { @"
using System.Diagnostics.CodeAnalysis;
public class C
{
    public string field1;
    public string? field2;
    public string field3;
    public string? field4;

    C() // 1
    {
        if (!IsInit) throw null!;
    }

    [MemberNotNullWhen(true, nameof(field1), nameof(field2))]
    bool IsInit
    {
        get
        {
            bool b =  true;
            if (b)
            {
                return true; // 2, 3
            }
            field2 = """";
            return false;
        }
    }
}
", MemberNotNullWhenAttributeDefinition }, parseOptions: TestOptions.Regular9);

            c.VerifyDiagnostics(
                // (10,5): warning CS8618: Non-nullable field 'field3' is uninitialized. Consider declaring the field as nullable.
                //     C() // 1
                Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C").WithArguments("field", "field3").WithLocation(10, 5),
                // (23,17): error CS8772: Member 'field1' must have a non-null value when exiting with 'true'.
                //                 return true; // 2, 3
                Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return true;").WithArguments("field1", "true").WithLocation(23, 17),
                // (23,17): error CS8772: Member 'field2' must have a non-null value when exiting with 'true'.
                //                 return true; // 2, 3
                Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return true;").WithArguments("field2", "true").WithLocation(23, 17)
                );
        }

[pranavkm]

So here's what I am seeing:

For this case:

public class Test
{
    public string? Prop1 { get; set; }
    public string? Prop2 { get; set; }

    [MemberNotNullWhen(true, nameof(Prop1), nameof(Prop2))]
    public bool Function() => true;
}

the compiler produces these warnings (as I'd expect it to):

1>Program.cs(15,9,15,21): warning CS8775: Member 'Prop1' must have a non-null value when exiting with 'true'.
1>Program.cs(15,9,15,21): warning CS8775: Member 'Prop2' must have a non-null value when exiting with 'true'.

But if there's a body, I get no diagnostics:

public class Test
{
    public string? Prop1 { get; set; }
    public string? Prop2 { get; set; }

    [MemberNotNullWhen(true, nameof(Prop1), nameof(Prop2))]
    public bool Function() => Prop1 is not null;
}

The second thing let's me write problematic code like so:

static void SomeOtherFunc()
{
    var test = new Test { Prop1 = "Hello", Prop2 = null };

    if (test.Function())
    {
        test.Prop2.ToString();
    }
}

[jcouv]

@pranavkm The problem is not with method vs. property, but rather what kind of logic is inside the method body.
public bool Property => true; warns just like public bool Function() => true;.
public bool Property => Prop1 is not null; and public bool Function() => Prop1 is not null; don't warn.

This limitation is by-design. When it comes to enforcement of attributes within a method body we had to choose a trade-off between stricter enforcement and possibly annoying diagnostics. I don't remember the details of why MemberNotNul/MemberNotNullWhen were especially problematic (produced annoying warnings that are difficult to suppress), but we restricted the enforcement to return statements that involve constants. We should have some LDM meeting notes on this.
Here's the relevant PR: dotnet/roslyn#48425