New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Behavior of "var" quickinfo and nullable reference types #63959
Comments
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. |
I personally think the inline-hints display is not only accurate, but desirable. It's good to know that objectSymbol is an 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. |
@ryzngard Hmm, I would have expected the flow analysis to be shown though in your screenshot...it would have at a later use, right? |
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. |
Not sure I follow. Did you want to see what flow analysis says on The code and gif are aligned. There's no usage after that or other assignments. |
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. :) |
@jasonmalinowski i think this is because this is a decalration point, not a reference point. |
I would expect 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 |
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. |
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 :) |
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. |
I think there's still a big difference in technically correct and how we can help users understand the behavior.
|
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.
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. |
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 |
I just filed #66937 and found that it's a duplicate of this issue 🙂 Agreed with @jasonmalinowski and @CyrusNajmabadi that adding an |
Pinging this thread to see if we can make some forward progress here 🙂 |
@Bosch-Eli-Black PRs welcome :) |
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. |
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:
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'. |
@Liander as per above, we're happy to take contributions here. |
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 astring
value. For example: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:
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.
The text was updated successfully, but these errors were encountered: