Behavior of "var" quickinfo and nullable reference types

[ryzngard]

We have had a few reports spread across feedback tools. I'd like to create a specific issue to track the discussion and user impact. This is strongly related to discussion in #53078 and user feedbacks such as https://developercommunity.visualstudio.com/t/Incorrect-type-deduction-tooltip-for-var/

This is specifically on why we show string? in cases where using var is assigned from a string value. For example:

var x = "Split";
var splitString = x.ToString();

This does not apply only to string, but any reference type assignment. Current IDE behavior is captured below:

A few important things about this issue:

1. 53078 does a great job of the discussion on why the behavior exists, and the thought put into this as it adheres to language design. This is not a proposal to change the language design here, but rather how we as a tooling team can help users understand the design better.

2. This will be a good issue for tracking any confusion/feature requests in this area. That way we can gauge user interest in wanting this feature for prioritization.

I will mark this as "Needs Proposal" to signify that there is not a clear design moving forward, and we will see track closely user feedback to help us prioritizing coming up with a design to help users.

[jasonmalinowski]

So even though I understand the underlying language design and why we did it that way, I still get thrown off on occasion by this. I wonder if it's worth adding a little bit of text to quick info that says something like "The thing assigned to the var was string" in the example above, so it's clear that the return type here isn't necessarily what people think it is.

[Vannevelj]

I filed a feedback item around this. It particularly confuses me combined with inlay hints: I love them, as well as var and NRT, but combine them and I feel like I'm constantly questioning my code.

e.g. in this screenshot, both variables are guaranteed non-null:

[CyrusNajmabadi]

I personally think the inline-hints display is not only accurate, but desirable. It's good to know that objectSymbol is an INamedTypeSymbol? as i can, and should, be able to assign null to it.

I agree that QI could have additional text saying something akin to "known to be non-null at this point in te program" or somethingl ike that.

[jasonmalinowski]

@ryzngard Hmm, I would have expected the flow analysis to be shown though in your screenshot...it would have at a later use, right?

[jasonmalinowski]

It's good to know that objectSymbol is an INamedTypeSymbol? as i can, and should, be able to assign null to it.

I actually hate it -- I'm more likely to read that variable than write to it (at least in the sense that all variables are read, but not all written to) and it's now telling me I have to be careful before access. I can mouse over it, but that defeated the whole point of the inline hint.

[ryzngard]

@ryzngard Hmm, I would have expected the flow analysis to be shown though in your screenshot...it would have at a later use, right?

Not sure I follow. Did you want to see what flow analysis says on x.ToString()?

The code and gif are aligned. There's no usage after that or other assignments.

[CyrusNajmabadi]

i mean, you do have to be careful before access. unless at the access point we've proven it is non-null. but at that point we'll test and tell you if there's a problem. :)

[CyrusNajmabadi]

@jasonmalinowski i think this is because this is a decalration point, not a reference point.

[Vannevelj]

I would expect objectSymbol to be annotated as INamedTypeSymbol as long as it only has non-null assignments. The moment we do assign a value that might be null, then the inlay hint can be updated to the broader type. But as long as that doesn't happen, I want to see as narrow a type as we can guarantee.

If I have to choose between "knowing a variable with nullable type hint is actually non-null" and "knowing a non-null variable can be widened to nullable", I choose the latter. The latter is one-time knowledge that will set me up for any scenario in which I encounter this. The former is something I have to check every single time

[sharwell]

I'm also in favor of the current approach, primarily because that is the true static type of the variable and it's the only type of the variable according to the compiler. Changing it based on the assignment characteristics means we're reimplementing the nullable walker with intentional deviation from the actual nullable walker.

[CyrusNajmabadi]

I would expect objectSymbol to be annotated as INamedTypeSymbol as long as it only has non-null assignments. The moment we do assign a value that might be null, then the inlay hint can be updated to the broader type. But as long as that doesn't happen, I want to see as narrow a type as we can guarantee.

This is not the view of the language, and it seems outright misleading. e.g. if something says "i am not null" then i should nto be able to assign null to it :)

[CyrusNajmabadi]

The former is something I have to check every single time

Note: you should only have to check if the compiler tells you you need to check. That's the nice thing about the flow-analysis approach.

[ryzngard]

I think there's still a big difference in technically correct and how we can help users understand the behavior.

var x = "Hello" showing string? on hover is a thinker at the very least. We could treat it similar to how we do flow analysis and add something like Jason suggested. Maybe show

(local variable)string? x
x is not null here 

[jasonmalinowski]

i mean, you do have to be careful before access.

Sure, but if I'm writing that next line of code I don't want to write it, then get the nullable warning, and then realize I have to rewrite it from scratch because null was a potential output.

@jasonmalinowski i think this is because this is a decalration point, not a reference point.

I think this might be the useful improvement: count var where the it was made nullable (even if the original value wasn't) as a place we also show the reference point hint.

[CyrusNajmabadi]

We could treat it similar to how we do flow analysis and add something like Jason suggested. Maybe show

Yes. I'm in strong agreement on that. I thought we already did that tbh. At least, i thought i remember us deciding on htat in a design meeting like 3 years back to do that :D

[Eli-Black-Work]

I just filed https://github.com/dotnet/roslyn/issues/66937 and found that it's a duplicate of this issue 🙂

Agreed with @jasonmalinowski and @CyrusNajmabadi that adding an "x" is not null here tooltip to the variable declaration would be helpful 🙂

[Eli-Black-Work]

Pinging this thread to see if we can make some forward progress here 🙂

[CyrusNajmabadi]

@Bosch-Eli-Black PRs welcome :)

[HaloFour]

I think being "technically correct" is useless if it only results in the developer questioning the correctness of their own code.

At the very least I think the existing tooltips should be fixed so that null state is shown both at assignment and at usage.

[Liander]

I agree with @HaloFour. Here are my thoughts around it:

From the user perspective (when NRT is enabled), the fact that a reference value can be set to null is not an outcome from having a nullable reference type, but instead it is from the fact that the null state flow analysis allows multiple assignments to determine its type. Therefore the "technically correct" type (if it means the nullable reference type reported today) has no relevance to the user at all, and presenting that is very contradictory and confusing. It ignores having a null state analysis running on top of it which is the true effect facing the user. Instead it is the inferred type determined by the null state flow that is of interest . Given that, it is of user interest to know where the inferred type is actually defined (ie the expressions/statements influencing the type) and what that inferred type is.

From that one can try to make up some suggestions, such as:

• To always show the inferred type from the null state flow analysis when hovering on a variable. (Both at LHS and at RHS).
• When hovering on the variable at declaration point (at a LHS variable) and when the inferred type comes from multiple statements to also indicate/highlight those statements to show that they are connected in terms of defining the type.
• Bonus 1: To also do this indication/highlighting of connected statements for determining the inferred type when a statement is modified that is affecting this inferred type.
• Bonus 2: If there is an (different) intermediate nullable state at the point of interest to also show that (like 'x' is not null here).

If the inferred type is purely from the RHS expression at the declaration point there is no need to do anything special (like highlighting the expressions causing the inferred type) other than reporting the resolved type.

This would also bring us back to the more familiar behavior that it is interchangeable to explicitly specify an inferred type or to using a 'var'.

[CyrusNajmabadi]

@Liander as per above, we're happy to take contributions here.