Incorrect NRT tooltip when doing null check

[vsfeedback]

This issue has been moved from a ticket on Developer Community.

---

Consider this code:

#nullable enable

public class Example
{
    private async Task MainAsync()
    {
        var user = new object();

        user.GetHashCode();

        // When hovering over `user`, the tooltip says "'user' may be null here", which is wrong.
        if (user is null)
            throw new InvalidOperationException("Shouldn't happen");
    }
}

When I hover over user in if(user is null), the tooltip says 'user' may be null here, which is incorrect. If I hover over user in user.GethashCode(), the tooltip correctly says 'user' is not null here.

Please let me know if you need any more info 

---

Original Comments

Feedback Bot on 2/2/2023, 11:56 PM:

(private comment, text removed)

Tracy Wang [MSFT] on 2/3/2023, 01:39 AM:

(private comment, text removed)

Feedback Bot on 2/3/2023, 01:40 AM:

(private comment, text removed)

---

Original Solutions

(no solutions)

[CyrusNajmabadi]

I believe this is by design. Once you start checking for null again, the compiler assumes you know more than it, and it reverts back to thinking the value is potentially null. @RikkiGibson to confirm and close if so.

[RikkiGibson]

Thanks for filing this issue. Cyrus is correct here. Generally these tooltips use the state of the variable after the expression.

[Eli-Black-Work]

Hi @CyrusNajmabadi and @RikkiGibson, I'm this issue's original reporter 🙂 I'd like to explain my workflow and why this feels confusing to me.

Often, I'll see a null check in our code that seems unnecessary. To begin my investigation, I hover over the variable in the null check and see whether the compiler reports it as nullable or not. If the compiler says that the variable might be null at this point, I end my investigation. If it says it isn't null at this point, I further investigate removing the null check.

The problem is that the compiler always reports that the variable might be null in two places:

• Inside null checks (this issue)
• At the declaration site (https://developercommunity.visualstudio.com/t/Inline-hints-displays-incorrect-nullabil/10269984)

This means that when we have code like below, it's impossible to easily tell if the variables might be null or not:

User GetUser()
{
    return new User();
}

// The tooltip on `user` shows it of type User?, even though GetUser() returns a non-null type
var user = GetUser();

// The tooltip on `user` says "'user' may be null here"
if (user is null)
	throw new InvalidOperationException("Shouldn't happen");

When I pointed out to my coworker that the null check wasn't needed, they responded by looking at the tooltip on user in if (user is null) and saying "See, it says 'user may be null at this point', so the null check is needed", which makes me think that this is affecting more people than just me 🙂

[RikkiGibson]

Let's reopen to track down any record of what motivated us to choose the state we did for public API and see if there's any opportunity to improve.

[jasonmalinowski]

I think I've been bitten by this once or twice too. If we can detect this scenario with the public API as it is today it might be easy to tweak the quick info text to either explain this a bit more. ("The compiler doesn't think this can be null, but will assume it can be since you're checking for it", but wordsmithed to something better.)

[RikkiGibson]

@chsienki @333fred in case they can provide additional context.

[333fred]

This is extremely similar to the types of problems that #44063 talks about.

[Eli-Black-Work]

I think the original motivation was probably for this type of code:

User user = GetUser();

if (user is null)
    _service.Start();
else
    _service.Start();

// It's probably reasonable to warn here that `user` might be null
Console.WriteLine(user.Id);

[jasonmalinowski]

Yeah, the language behavior makes sense; and generally the API behavior also makes sense...it just collides to a confusing behavior here in the IDE. Sure, you can kinda figure it out if you know the language design, but ideally our IDE would be clear to non-language-designers too.

[Eli-Black-Work]

Possible solution: Have flow analysis mark variables as possibly null starting with the statement after the null check.

Original example:

User GetUser() => new User();

var user = GetUser();

// Flow analysis hasn't marked `user` as possibly null yet, so the tooltip would say "'user' is not null here"
if (user is null)
        //  Starting with this statement, `user` might be null
	throw new InvalidOperationException("Shouldn't happen");

And:

User user = GetUser();

// Tooltip: '"user" is not null here'
if (user is null)
    _service.Start();
else
    _service.Start();

// Warning: '"user" might be null here'
Console.WriteLine(user.Id);