Show nullability flow analysis in Quick Info

[jasonmalinowski]

If you mouse over a local, field, etc., and we have computed nullable flow analysis for it, we'll tell you whether the compiler thinks the item is null or not at that point.

The symbol shown on the first line is always shown with the declared nullability; we decided to keep it that way out of concerns that it differing from the declared nullability (without a clear statement of what is different) would be too confusing -- a simple, straightforward sentence more easily expresses the message, and putting 'here' in the message clarifies that it's a piece of information depending on the exact invocation point, which is somewhat a first for Quick Info.

I don't like that this is rebinding the symbol after we lost it in the quick info infrastructure. There is more cleanup I'd like to do here of the Quick Info code but that'll wait for another PR -- it went into weeds far deeper than I expected at first.

Closes #36191.

[CyrusNajmabadi]

The symbol shown on the first line is always shown with the declared nullability; we decided to keep it that way out of concerns that it differing from the declared nullability (without a clear statement of what is different) would be too confusing

Why do we think this? It should be noted that typescript supports that fine without issue:

[CyrusNajmabadi]

a simple, straightforward sentence more easily expresses the message

Enh... i think we're not giving users enough credit here. I think they'll understand immediately after 10 seconds using the feature: ok, it's telling me the info "here" (which seems to be intuitive since that's where they asked for the info).

[CyrusNajmabadi]

Going to hold off on review until design question is resolved.

[jasonmalinowski]

@CyrusNajmabadi We landed on this design after considering those questions, and one thing we did take as an assumption is that over time we may want to reduce the display as people do get more comfortable with it. We agreed that once people get the mental model then yes they'll figure it out easily -- but how they were going to get there in the first place was a concern. We very much consider this a "point in time" even if that point in time is just the next few point releases.

This is also addressing a specific problem that we don't have notation to distinguish "this is not null" and "this is oblivious". If quick info shows "string foo", that means either "this is non-null" or "we don't know because the library you called into isn't annotated In design meetings and such we've often done things like string~ or string! to distinguish but we didn't want to display notation that wasn't "valid" code in that sense, and using random characters that aren't valid code doesn't give a way for people to first learn what's going on. Once the majority of the ecosystem is annotated and people are regularly working "in the new world" we suspect the oblivious case may become rare enough it's just not worth dealing with; until then there's going to be a hybrid world.

I would have also imagined that TypeScript the concept of "types change at use" isn't new to the language -- even if the nullability concept was added later, I assume "the type of this can change" was there from the very start?

[CyrusNajmabadi]

I would have also imagined that TypeScript the concept of "types change at use" isn't new to the language -- even if the nullability concept was added later, I assume "the type of this can change" was there from the very start?

It was new to the language. It only came in TypeSCript 2.0 when control-based-flow-analysis was added. Prior to that, there was no such concept and you'd always just see the declaration information.

[CyrusNajmabadi]

@CyrusNajmabadi We landed on this design after considering those questions, and one thing we did take as an assumption is that over time we may want to reduce the display as people do get more comfortable with it. We agreed that once people get the mental model then yes they'll figure it out easily -- but how they were going to get there in the first place was a concern. We very much consider this a "point in time" even if that point in time is just the next few point releases.

I'd err in the other direction. Just supply the accurate information about what the type is at that point. if it turns out people are confused by this (which i doubt), then we could add information in.

It seems highly speculative. Honestly, i really don't think people will be confused here. :)

[jasonmalinowski]

Making this internal as this is still considered a preview experience and indeed may change. Will make it public in the 16.3 branch if this PR ships in 16.2. If it first goes into 16.3 then I'll change this prior to merge.

[ryzngard]

Make sure to add a follow up issue

[jasonmalinowski]

@jinujoseph To give a heads up on this one, this is one small piece the compiler team would like to have in 16.2; it's an enhancement to Quick Info to show some additional nullable information. It's written to only trigger under @333fred's feature flag for risk reduction; technically this isn't a "bugfix" but it's actually less risk than a bug fix. So looking for 16.2 M2 approval for this one.

@dotnet/roslyn-ide @ryzngard code reviews?

[ryzngard]

This code looks good for the current design. I'd say the benefit of this design is that it makes a noticeable visual difference that may surprise users (in a good way). Otherwise, I agree with @CyrusNajmabadi that I'm not convinced users would be confused, but I think iterative design would be useful on that front.

[ryzngard]

switch expression would look good here :)