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

Nullable Reference Types: Indirect indicators of "nullness" should inform flow analysis #27011

Closed
PathogenDavid opened this issue May 21, 2018 · 8 comments
Labels
Area-Compilers Area-Language Design Bug Feature - Nullable Reference Types Nullable Reference Types Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@PathogenDavid
Copy link
Contributor

Version Used: 05/14/18 Nullable Reference Types Preview (csc reports 2.8.0.62830 (e595ee27)) with Visual Studio 15.7.1

Demonstration Code:

void Test(Object obj)
{
    string? str = obj as string;
    bool isString = str != null;

    if (isString)
    { Console.WriteLine(str.ToString()); } // warning CS8602: Possible dereference of a null reference.
    else
    { Console.WriteLine("`o` is not of type `string`"); }
}

Expected Behavior:

No warning is issued.

Actual Behavior:

Possible dereference of a null is reported for str.ToString()


I imagine this is extremely non-trivial, and perhaps even outside of what the Roslyn team hopes to accomplish with the initial release of C# 8.0, but I couldn't find any issue discussing this capability.

In the context where I discovered this in our code base it was simple enough to refactor things to be if (obj is string str) instead, but I felt like it was still worth bringing up since it still feels like a "Dumb compiler, it's clearly not null!"-type situation.

@PathogenDavid
Copy link
Contributor Author

PathogenDavid commented May 21, 2018

Inspired from the code base I'm testing the preview on, here's a slightly more complicated example where the "deciding variable" is a read-only field:

public class TestClass
{
    private readonly bool isFooEnabled;
    private readonly string? foo;

    public TestClass(string? x)
    {
        foo = x;
        isFooEnabled = foo != null;
    }

    public void UseFoo()
    {
        if (isFooEnabled)
        { Console.WriteLine(foo.ToString()); } // Currently CS8602, should be no warning.
        else
        { Console.WriteLine("<Foo is disabled>"); }
    }
}

isFooEnabled might be a field that represents a general state of the type that implies multiple things, not just that foo is not null.

In the case I was basing my example off of, the field is read-only, but I imagine it could be decided it was never written to false after being written true, I'm sure that gets even more crazy than the original suggestion though.

@PathogenDavid
Copy link
Contributor Author

Here's another similar one that is actually closer to what our use case looks like:

public class TestAbstractClass2
{
    protected readonly bool isFooEnabled;

    protected TestAbstractClass2(bool enableFoo)
        => isFooEnabled = enableFoo;
}

public class TestChildClass2 : TestAbstractClass2
{
    private readonly string? foo;

    public TestChildClass2(bool enableFoo)
        : base(enableFoo)
    {
        if (isFooEnabled)
        { foo = "Hello!"; }
        else
        { foo = null; }
    }

    public void UseFoo()
    {
        if (isFooEnabled)
        { Console.WriteLine(foo.ToString()); } // Currently CS8602, should be no warning.
        else
        { Console.WriteLine("<Foo is disabled>"); }
    }
}

(Basically the presence of foo is decided by state defined by the abstract base class.)

@jcouv
Copy link
Member

jcouv commented May 25, 2018

Tagging @cston

@gafter gafter added this to the 16.0 milestone May 31, 2018
@jaredpar jaredpar added the Bug label Aug 30, 2018
@jaredpar jaredpar modified the milestones: 16.0, 16.1.P1 Jan 26, 2019
@jcouv
Copy link
Member

jcouv commented Mar 6, 2019

This is indeed very non-trivial and currently by-design.

@jcouv jcouv added the Resolution-By Design The behavior reported in the issue matches the current design label Mar 6, 2019
@PathogenDavid
Copy link
Contributor Author

Sad to hear, but not horribly surprising.

Perhaps down the line manual annotations could be added to let the developer inform the compiler manually, similar to the ones described here.

private readonly bool isFooEnabled;
[NotNullWhenTrue(nameof(isFooEnabled))]
private readonly string? foo;

@jcouv
Copy link
Member

jcouv commented Mar 7, 2019

I understand. This would require the compiler to not only track whether some variables can contain null, but whether some variables contain true (that's a whole new feature). Only then would such an annotation (relating two variables) be useful...

@PathogenDavid
Copy link
Contributor Author

PathogenDavid commented Mar 7, 2019

I was thinking of it more as the annotation would allow the compiler to assume

if (isFooEnabled)
{ Console.WriteLine(foo.ToString()); } // Currently CS8602, should be no warning.

is logically equivalent to

if (foo != null)
{ Console.WriteLine(foo.ToString()); } // Would be no warning.

This doesn't help my first example with the local variable, but I think it'd help a lot in the case of fields.

Basically the annotation removes the need for the compiler to do flow analysis between separate methods to derive facts like "This field implies something about other ones.".


(As for why the code that prompted me to file this issue wasn't written like the latter, I suspect isFooEnabled was something more complex that implied things about other state besides foo. Unfortunately I can't find exactly where it came up anymore. I tagged most of our NRT test branch with GitHub issue numbers when I filed stuff, but I guess I missed this one.)

@gafter gafter modified the milestones: 16.1.P1, 16.1.P3 Apr 9, 2019
@jaredpar
Copy link
Member

jaredpar commented Apr 9, 2019

Closing as this is indeed currently by design. If we want to iterate on the design here we should open a new issue on csharplang for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Area-Language Design Bug Feature - Nullable Reference Types Nullable Reference Types Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

4 participants