-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Comments
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>"); }
}
}
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. |
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 |
Tagging @cston |
This is indeed very non-trivial and currently by-design. |
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.
|
I understand. This would require the compiler to not only track whether some variables can contain |
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 |
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. |
Version Used: 05/14/18 Nullable Reference Types Preview (
csc
reports2.8.0.62830 (e595ee27)
) with Visual Studio 15.7.1Demonstration Code:
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.The text was updated successfully, but these errors were encountered: