Skip to content

Add member 'Principal' and 'Properties' to MemberNotNullWhenAttribute #34883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 3, 2021
Merged

Add member 'Principal' and 'Properties' to MemberNotNullWhenAttribute #34883

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f18d6c
Contributes to #5680
msschl Jul 30, 2021
46ed0ec
Same for the Properties property
msschl Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Http/Authentication.Abstractions/src/AuthenticateResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ protected AuthenticateResult() { }
/// <summary>
/// If a ticket was produced, authenticate was successful.
/// </summary>
[MemberNotNullWhen(true, nameof(Ticket))]
[MemberNotNullWhen(true, nameof(Ticket), nameof(Principal), nameof(Properties))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)
                );
        }
        ```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)
                );
        }

Copy link
Contributor

@pranavkm pranavkm Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

public bool Succeeded => Ticket != null;

/// <summary>
Expand Down