Roslyn: Nullable state tracking for the ternary operator

[svick]

Consider the following code:

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

[This currently produces "warning CS8602: Dereference of a possibly null reference"](https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxAN0QSwGYE8A+AAgAwAEhAjAHQBycMVAEjDAA4DcAsAFA8DEAOwCuAGxERgIuKTgCJUnj0IAmUgGEeAbx6ld5ACykAsgApmbAEpworAPYCocI9agQA5tITW7DuAEodPW1uPVDSHFITLxt7R1IAQgBeUmExUgB+UmifRyoASSgAZSEAYxKXQpgIGCEoNVsAE2kQUmwIEUcAkLDdYJ6egH1SZOzYuCp6gXgprm6egF9A3UXueaA=) in the body of the `if`, even though `response` can't possibly be `null` there.

This code can be simplified to the following:

```c#
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!

[KennethObinna]

Thanks for the post.

Please I need FDX SDK PRO for windows executable file. Or is there setup to
install to have FDX SDK Pro for windows in my Program Files Folder . Please
help.

On Mon, Sep 9, 2019 at 10:31 PM Rikki Gibson [e-mail]
wrote:

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

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/csharplang/issues/2764?email_source=notifications&email_token=AI6GSCQHRKBKV5B4UEIJHT3QI26BNA5CNFSM4ISOZ252YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6JDGIA#issuecomment-529675040,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI6GSCWNBNYQFOY7GK5OEYDQI26BNANCNFSM4ISOZ25Q
.

[yaakov-h]

Please don't spam threads with unrelated and blatantly-off-topic requests.

[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.