Nullable state tracking for the ternary operator

[svick]

Consider the following code:

if (response != null ? response.IsSuccessStatusCode : false)
{
    _ = response.Content;
}

This currently produces "warning CS8602: Dereference of a possibly null reference" in the body of the if, even though response can't possibly be null there.

This code can be simplified to the following:

if (response != null && response.IsSuccessStatusCode)
{
    _ = response.Content;
}

This version does not produce any warnings.

So, my question is: should nullability tracking be changed so that it also works for the ternary operator in situations like the above (i.e. where one of the possible results is a constant boolean)?

One argument for why that's not necessary is that all such cases can (and should?) be simplified to use && or ||, which track nullability properly. But personally, I think the language should work well even for code that's still being written, before it's refactored to its final form, so it makes sense to support this.

[dhindrik]

But I guess many people will treat that types of warnings as errors. At least I'm doing that right now.

[alrz]

It's just that ?: only affects its own branches, the compiler doesn't look into subexpressions while it splits states for the enclosing if.

This is true for definite assignment too,

if (o is string s == true) {
   _ = s; // error
}

I think extending that would be more involved that it seems.

[svick]

@alrz

the compiler doesn't look into subexpressions while it splits states for the enclosing if.

It does do that for some operators, like the && in the initial post or in if (foo != null && bar != null). What I'm asking here is more complicated than that, but not by much. I'm pretty sure it's not anywhere close to requiring a theorem prover.

[RikkiGibson]

The pattern I would recommend here is response?.IsSuccessStatusCode == true or response?.IsSuccessStatusCode ?? false, both of which we support in nullable flow analysis today.

It feels like when one of the conditional expression branches is a boolean constant, we should be able to propagate the flow state from the non-constant branch back up to the if-statement's condition somehow.

I think code which uses ?: and boolean constants instead of "simpler" && or || is still good to recognize in nullable analysis.

@svick, do you mind if I just move this issue to roslyn? I think it would be a reasonable thing to put on our nullable backlog.

[alrz]

I'm pretty sure it's not anywhere close to requiring a theorem prover.

Sure, if you want to cherry pick from a whole set of possible forms - there will be uncovered cases. though I agree that's better than not doing anything.

[svick]

@RikkiGibson Feel free to move this to roslyn.

[svick]

@alrz Cherry picking forms is pretty much how NRTs are designed, so I think it makes sense to discuss new forms to pick. Though with that approach, there has to be a cutoff somewhere (probably based on how common a form is and how hard would it be to support it), otherwise you're pretty much just implementing a theorem prover, badly.

[RikkiGibson]

Once we come to a consensus internally on this, I'll either move to Roslyn or close. Thanks!

[RikkiGibson]

I see no reason why we shouldn't do this for nullable, but doing it for some other forms of analysis (such as definite assignment of 'out' arguments described in dotnet/csharplang#3485) may require spec changes.

[RikkiGibson]

This will be resolved by the "improved definite assignment" feature in C# 10. dotnet/csharplang#4465 #51463

The feature branch currently handles this scenario. SharpLab