Skip to content
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.

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

Dataflow analysis thinks that we could get back in a loop with null node #26624

Closed
jcouv opened this issue May 4, 2018 · 7 comments
Closed

Comments

@jcouv
Copy link
Member

jcouv commented May 4, 2018

Update:

A test like c?.Count > 0 should tell us that c is not-null in the when-true branch.


public class GreenNode
{
    internal GreenNode? GetFirstTerminal()
    {
        GreenNode? node = this;

        do
        {
            GreenNode? firstChild = null;
            for (int i = 0, n = 1; i < n; i++)
            {
                var child = node.GetSlot(i); // warning CS8602: Possible dereference of a null reference.
                if (child != null)
                {
                    firstChild = child;
                    break;
                }
            }
            node = firstChild;
        } while (node?._slotCount > 0); // this check guarantees that node isn't null in the second iteration

        return node;
    }

    internal GreenNode? GetSlot(int i) => throw null;
    internal int _slotCount = 0;
}

Found in nullable dogfood.
Tagging @cston

@jcouv jcouv added this to the 16.0 milestone May 4, 2018
@sharwell
Copy link
Member

sharwell commented May 4, 2018

💡 it took me a bit to realize what happened when I reviewed this on a small screen. It would help to also comment the line that ensures node isn't null on the first iteration.

@cston
Copy link
Member

cston commented May 4, 2018

This looks like a variant of #25870.

@jcouv
Copy link
Member Author

jcouv commented Jul 26, 2018

The problem still seems to be there after the null-conditional access has been fixed.

class C
{
    void M()
    {
        C? c = this;
        do
        {
            c.ToString(); // warning CS8602: Possible dereference of a null reference.
            c = c.Next; // warning CS8602: Possible dereference of a null reference.
        }
        while (c?.Count > 0);
    }
    int Count => 0;
    C? Next => null;
}

@cston
Copy link
Member

cston commented Jul 26, 2018

The issue may be comparing with something other than null. The following results in warning for the > 0 case:

static void M(string? s)
{
    if (s?.Length != null) s.ToString();
    if (s?.Length > 0) s.ToString(); // warning
}

@jcouv
Copy link
Member Author

jcouv commented Jul 26, 2018

Tagging @333fred FYI

@333fred
Copy link
Member

333fred commented Jul 26, 2018

Yes, the propagation only runs as part of comparisons to null. We need to change that.

@jaredpar jaredpar added the Bug label Aug 30, 2018
@jaredpar jaredpar assigned jcouv and gafter and unassigned jcouv Jan 25, 2019
@jaredpar jaredpar modified the milestones: 16.0, 16.1, 16.0.P4, 16.1.P1 Jan 25, 2019
@jcouv
Copy link
Member Author

jcouv commented Mar 6, 2019

Closing as duplicate of #31906 (Infer nullability of receiver from ?. expression compared with non-null).
For example, s?.Length == 1 tells us that s is not-null in the when-true branch.

@jcouv jcouv closed this as completed Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants