Checking if not-nullable reference is null should produce a warning

[TahirAhmadov]

Consider the following:

byte[] foo() => new byte[10];
......
byte[] a = new byte[10];
if (a != null) // we know "a" is not null, why would we check for null?
	Console.WriteLine("");
else
	Console.WriteLine(""); // this is unreachable code, from the perspective of nullability analysis

byte[] b = foo();
if (b != null) // same - we know "b" is not null, why would we check for null?
	Console.WriteLine("");
else
	Console.WriteLine(""); // same - this is unreachable code, from the perspective of nullability analysis

I think there should be warnings for these situations, either "unrechable code" or "invalid comparison" - similarly to when comparing a value type to null.
Warning CS0472 The result of the expression is always 'false' since a value of type 'int' is never equal to 'null' of type 'int?'
Warning CS0162 Unreachable code detected

Granted these exact warnings shouldn't be used, because at runtime it's possible that b becomes null, however I do think two new warnings are called for.

[canton7]

The nullability analysis actually took the opposite route.

public void M(string s) {
    Console.WriteLine(s.Length); // Fine

    if (s == null)
    {
        Console.WriteLine("Null");
    }

    Console.WriteLine(s.Length); // Warning
}

The nullability analysis learns from your if statements: if you test something for null, it figures that you probably have good reason to, and updates the null state for that variable.

[333fred]

For the 2nd, why not use the nullability of the variable declaration as a hint to start it off as a nullable flow state, even if a not-null expression is assigned to it?

For the exact reason I mentioned: there are valid reasons to start with a not null value, but assign it a null value later. It would be generally confusing if we respected the results of flow analysis only sometimes.

Just so that we understand what's being discussed here. The below is how I would expect it to work.

void foo(object x) { /* ... */ }

object? x = new object(); // no problem
foo(x); // flow analysis - x is not null, therefore no problem
x = null; // no problem
foo(x); // flow analysis - x may be null, therefore warning
if(x == null) { /* ... */ } // no problem

object x = new object(); // no problem
foo(x); // no need for flow analysis; x is not null, period; no problem
x = null; // warning: null assignment
foo(x); // no need for flow analysis; x is not null, period; no problem
if(x == null) { /* ... */ } // warning: unreachable code

var x = new object(); // var = object (not nullable); everything works same as above

The second pattern you show here is a coding style we intentionally support. Non-safety warnings, ie warnings that will not cause a null reference (such as assignment to a variable) are intentionally all CS8600, and we expect that some users, to avoid massive changes to their codebases, will disable that warning and leave these variables unaltered. The feature is about flow analysis, not about ignoring flow analysis.

[TahirAhmadov]

The second pattern you show here is a coding style we intentionally support. Non-safety warnings, ie warnings that will not cause a null reference (such as assignment to a variable) are intentionally all CS8600, and we expect that some users, to avoid massive changes to their codebases, will disable that warning and leave these variables unaltered. The feature is about flow analysis, not about ignoring flow analysis.

I think I understand what you mean, you're trying to make it easier for people to use most of the not-null improvement without forcing them to pay a high price for it. However, I think there's a mistake in judgement here. The team which disables one of these warnings simply for being annoying, will most likely not turn the feature on in the first place.

However, even if we accept that line of reasoning, we can simply "re-enable" the flow analysis in this case:

void foo(object x) { /* ... */ }

object x = new object(); // no problem
foo(x); // no need for flow analysis; x is not null, period; no problem
if(x == null) { /* ... */ } // warning: unreachable code
x = null; // warning: null assignment; also, we turn flow analysis on - x is now known to be "object?"
foo(x); // warning
if(x == null) { /* ... */ } // no problem

But, I'd reiterate that it was a mistake to make the language accommodating to "laziness". People can disable all kinds of warnings; bad code will result from it; the language can't bend over backwards to account for bad practices.

[333fred]

The point of the way we designed nullable was to get as many people to adopt it as possible. This means accommodating various coding styles.

The team which disables one of these warnings simply for being annoying, will most likely not turn the feature on in the first place.

At least one member of the LDM intentionally works this way with their own existing code, and several members of the LDM argued against even allowing locals to be annotated in the first place. We gain nothing by being more restrictive except alienating customers.

[TahirAhmadov]

The point of the way we designed nullable was to get as many people to adopt it as possible. This means accommodating various coding styles.

The team which disables one of these warnings simply for being annoying, will most likely not turn the feature on in the first place.

At least one member of the LDM intentionally works this way with their own existing code, and several members of the LDM argued against even allowing locals to be annotated in the first place. We gain nothing by being more restrictive except alienating customers.

I understand the gradual adoption goal, but the #nullable enable directive solves that. Once you enable a file, then you fix the methods over time, then next file, etc. You can even #nullable disable part of the file, right? There's no reason to enable nullability for the project, and then demand language do things differently because of the sudden amount of work introduced.

Every decision has pros and cons. This decision, while having a weak pro - the large amount of work needed to fix locals can effectively be reduced using file-by-file (or even by-section) nullability and this "style" isn't really needed - it also has a strong con: new code and in some cases existing code get strange rules going forward forever. Usually, breaking existing code is bad. In this case, though, we're paying this huge price, with a compiler switch, thousands of warnings, etc. Having paid this huge price, we should strive for perfectly.

[CyrusNajmabadi]

I understand the gradual adoption goal, but the #nullable enable directive solves that.

1. I don't think that's true.
2. Importantly, nullable is fundamentally a weak static check (and will remain that way on the current runtime/langauge for the forseeable future), while runtime checks are the only true strong runtime check. As long as the language is weak here, it is important that the strong way of writing the code not be penalized.

[huoyaoyuan]

This should be covered by CA1508.

[TahirAhmadov]

This is a good point. The only exception (no pun intended) should be to throw an exception like ArgumentNullException.

[TahirAhmadov]

On second thought, it's not a good point. For if(x==null) { /* ... */ }, this works. But for the equally (if not more) common if(x!=null) { /* ... */ }, this doesn't; there is no unreachable code here. But we still want to warn about useless comparison.

[TahirAhmadov]

I again ran into a bug because of this: in ASP.NET, Request.Query["p"].ToString() returns empty string or value, and never null, and I was checking for null (because I mistakenly thought ToString() returned string?).
Is there an analyzer for this? Otherwise, I'm afraid I'll need to write one myself.

[333fred]

I doubt it will be as simple as that just visiting and looking at flow states, as I expect a number of those API calls will return the state after, not the state before. And the state after will be maybe null.

[TahirAhmadov]

@333fred is it possible to find the previous statement and call the nullability result API on that one?

[RikkiGibson]

Yeah, but I think looking within the expression that does the null test is sufficient. Just look at the state after the operand expression, e.g. s in s == null. In a method like this one

void M(string s)
{
    if (s == null)
    {
        s.ToString();
    }
}

we expect a not-null state afterwards, which appears to be reflected in the IDE's display of the null state

[333fred]

@RikkiGibson I don't expect that this is result you'll get for every scenario that Tahir will want to cover.

[333fred]

@333fred is it possible to find the previous statement and call the nullability result API on that one?

If there is one. There is no guarantee there is.

[TahirAhmadov]

For those who are interested:

https://www.nuget.org/packages/TA.Analyzers.InvalidNullChecks/