roslyn icon indicating copy to clipboard operation
roslyn copied to clipboard

Quick info says calling an oblivious method is non-null

Open jasonmalinowski opened this issue 6 years ago • 13 comments

image

Expected: there is no message in quick info, since the flow state would be oblivious Actual: we get a message, which implies that GetTypeInfo returned non-null as the flow state

jasonmalinowski avatar Jul 04 '19 01:07 jasonmalinowski

I talked with @gafter briefly about this. This is intended behavior. An oblivious value is being assigned to an explicitly non-null variable. Therefore, we presume that the user knows what they're doing and we say that it's not null.

333fred avatar Jul 08 '19 20:07 333fred

@333fred My mental model was "the compiler doesn't really look at top level nullability of locals, it's always ignored and the flow state is always computed regardless". My mental model was wrong in the case of oblivious being assigned something? To be clear I'm not pushing back on the design, just trying to understand it.

jasonmalinowski avatar Jul 09 '19 17:07 jasonmalinowski

Yes, the compiler tries to lose oblivious as soon as it's able to.

333fred avatar Jul 10 '19 21:07 333fred

This is a serious deficiency of Quick Info. Once an oblivious value is assigned to a variable, we can no longer show "'item' is not null here". It's acceptable to show "The nullability of 'item' is not known here", or to show nothing at all, but it's not fine to make claims about the nullability of APIs that contain no information about it.

sharwell avatar Jan 31 '20 22:01 sharwell

I'd like to +1 on revisiting this. I'm trying to write a classifier that shows the nullability status of a local as it travels through a method. I'm using the current APIs, green means NotNull and orange means MaybeNull:

image

I've pointed out where the API says it cannot be null, but where it of course can be null.

Regarding @333fred and @gafter 's response to the earlier version of this:

An oblivious value is being assigned to an explicitly non-null variable. Therefore, we presume that the user knows what they're doing and we say that it's not null.

In my case, oblivious is being assigned to an explicitly nullable variable, so it's even weirder that it comes back NotNull...

dpoeschl avatar Jan 31 '20 22:01 dpoeschl

I'm absolutely for changing this -- the original design strongly felt of an implementation detail leaking out of the API.

jasonmalinowski avatar Jan 31 '20 22:01 jasonmalinowski

+1 for this issue! It nearly got me in code interacting with a non-"nullable enabled" third party lib...

Quoting @stephentoub:

Showing incorrect information is worse than showing nothing at all. If it may or may not be null, it may be null.

I could not agree more! And by the way, maybe the target milestone should be updated: can this be fixed (or mitigated) for 16.6?

odalet avatar Apr 04 '20 14:04 odalet

@odalet

And by the way, maybe the target milestone should be updated: can this be fixed (or mitigated) for 16.6?

If you think you could do this with a clean design we’d accept in the next week or so, yes.

gafter avatar Apr 04 '20 14:04 gafter

Got it, apologies for being too pushy... Roslyn is quite the piece of code and I wouldn't know where to start... I suppose I'll just back down for now and hope someone with the know-how wil try to tackle the issue.

odalet avatar Apr 04 '20 14:04 odalet

This is especially a big problems with using 3rd party libs that aren't annotated like WPF. For instance this intellelisense makes it clear how misleading it is. For example the Content property on the ContentPresenter:

image

dotMorten avatar Sep 06 '20 15:09 dotMorten

@dotMorten Note that even after this is fixed, it's possible for situations like the screenshot to occur. For example:

if (this.Content is not null)
{
  _ = this.Content; // Here 'Content' is known to not be null, but documentation will still say the default
}

sharwell avatar Sep 06 '20 15:09 sharwell

It's been a year, any movement on this?

artempushkin avatar Sep 08 '21 22:09 artempushkin

+1 Sadly, nullable feature cannot be safely used when any 3d party lib (including wpf) could break the analysis. Any unannotated code should be treated as dangerous.

AndreyMikhailov avatar Apr 30 '22 21:04 AndreyMikhailov