[Design] Enhance QuickInfo with Inferred Nullability

[333fred]

With the upcoming Nullable Reference Types feature, we'll need to consider how the IDE will expose information about inferred nullability to the user. This issue tracks our thoughts on what the experience should be, as well some interesting scenarios that will help drive our design. This list of scenarios will be updated as we come up with more.

Scenarios

Scenario 1:

string? GetMaybeString();
...
var s = GetMaybeString();
if (s == null) throw;
s.ToString(); // Hover over s here

Today, the ToDisplayParts API has no concept of "location" of the string being displayed. This is fine in the present world, but with nullable reference types the inferred nullability of s changes as the method continues. If you hover over the var at the declaration, it should be string?. However, later on in the method, we infer that s is non-null, so quickinfo should display that fact in some mechanism.

Scenario 2:

string? GetMaybeString();
T FluentApiCall<T>(T t);
...
var s = GetMaybeString();
if (s == null) throw null;
var s1 = FluentApiCall(s);
s1.ToString();

Here, the type s1 will change after initial binding, which is something new introduced with this feature. Initial binding will do the simple thing, and flow the type of s through the type parameter T, which means that s1 and T will be string?. After null analysis however, the null state will flow through, and the type of s1 and T will be string, as the user has done a null-check ahead of using s (which is why they won't get a warning on the s1.ToString() call, which they would have gotten if s1 was string?). We'll need to consider how to ensure that QuickInfo gets info from after null analysis, to make sure that it isn't displaying incorrect info.

Scenario 3:

string ObliviousAPI(); // Defined in a pre-C#8 assembly
...
var str = ObliviousAPI();

Based on the latest LDM meeting, the "oblivious" state of the return value lasts only until the value is used. What that means is that type type of str is string, ie non-null. However, hovering over the ObliviousAPI call should somehow indicate that the string it returns is actually coming from an unannotated API.

Scenario 4:

string? GetMaybeString();
...
string s = GetMaybeString();
s.ToString();

We track inferred nullability regardless of declaration type, so hovering over s in s.ToString() here should inform the user that s could be null.

Scenario 5:

var a = new[] { maybeNull, notNull }; // string?[]
if (maybeNull == null) throw null;
var b = new[] { maybeNull, notNull }; // string[]

The nested nullability here is a very similar scenario to 3, where additional analysis will change the initial type of b.

Scenario 6:

string? s = String.Empty;

What do we show if you hover over s? Should this show string?, or string?

Scenario 7:

void M<T>(T t, out T t2)
{
    t.ToString(); // warn here, as t could be null
    if (t == null) return;
    t.ToString(); // No warning here
    t2 = default; // Warning here: T could be non-nullable
}

We need to consider how to convey the above info to the user through quick info and diagnostics. Should hovering over the first t.ToString() display the type as T??

Display Options

There are a few different tactics we could take for displaying inferred nullability. These examples show what QuickInfo would look like when hovering over the s in the s.ToString() call in Scenario 1.

1. Display the nullability info below the main QuickInfo section

This would allow the nullability information to be async, which could help improve IDE responsiveness. The idea is that it would look something like this:

(local variable) String? s
Inferred Nullability: Analyzing...

An advantage of this approach is that we could run nullability analysis in the background, and update the Analyzing... field when it completes. However, this will run into issues with Scenario 2, as the Type in the first line will be incorrect until nullability analysis has finished. This might be simpler to implement, as the existing SymbolDisplayVisitor wouldn't need to know anything about the concept of location, and the QuickInfo display classes would be responsible for querying the SemanticModel about nullability at that position separately. Additionally, I believe that separating the inferred nullability like this will help the user understand how the feature works, and have an obvious way of seeing feedback from the nullability walker.

2. Replace the type in the standard QuickInfo with post-analysis info

This would run nullability analysis on the code section in question on hover, and the existing type section would be updated with the results. It would look like something like this (exactly the same as quickinfo today, except that the type is the inferred type at the location):

(local variable) String s

This addresses the problem of Scenario 2, and ensures that all nullability display will be consistent. However, it could introduce confusion for the user in Scenario 1, as hovering over s at different program locations will produce seemingly different results. This is the same method that typescript uses today.

3. Hybrid Display of Nullability Information

This approach is similar to 2, but approaches the display in a different way that is more of a hybrid with 1. It would look something like this:

(local variable) String? s
Inferred Nullability: Not Null

Nullability analysis is run ahead of time, to ensure that Scenario 2's problem is solved. For variables that are declared as nullable (or non-nullable variables we have inferred to potentially be null), we display their inferred nullability in quickinfo at that point of the program. Like my argument in approach 1, I think this format will help the user understand more about how the feature works and have an easier time of getting feedback from the compiler on null states.

@dotnet/roslyn-ide @jinujoseph @jcouv @cston @jaredpar

[cston]

We'll probably want to display the inferred nested nullability as well as top-level nullability.

var a = new[] { maybeNull, notNull }; // string?[]
if (maybeNull == null) return;
var b = new[] { maybeNull, notNull }; // string[]

[333fred]

@cston is there a scenario where nullability analysis will change the nested nullability from the initially-inferred nullability? In that example, initial correct type of b is string[], and I don't believe there's a situation where we'll infer it to string?[], correct? Similarly for a, is there ever a situation where we will infer that it's become string[]?

[jcouv]

There are such scenarios:

string element = null!;
var array = MakeArray(element); // the initial binding yields `string[]` for `var`, but the flow analysis will yield `string?[]`

The reverse scenario would be more common (doesn't involve suppression operator):

string? element = null;
Debug.Assert(element != null);
var array = MakeArray(element); // `string?[]` during binding, but `string[]` in flow analysis

[CyrusNajmabadi]

Today, the ToDisplayParts API has no concept of "location" of the string being displayed. This is fine in the present world, but with nullable reference types the inferred nullability of s changes as the method continues. If you hover over the var at the declaration, it should be string?. However, later on in the method, we infer that s is non-null, so quickinfo should display that fact in some mechanism.

I don't think this is an issue with ToDisplayParts. I think this is an issue with the current symbolic API. Right now symbols are very location independent. so if i have:

var s = ...
s.Whatever();
// more code
s.Whatever();

then in both those cases we say that is the 's' local. And that the 's' local has type 'T'. However... this is somewhat strange. Because our API is effectively lying. Why it is true to say that we have the 's' local, saying that it has hte same type in both places isn't really true. The type is actually something different.

To me, it would make sense that if you asked for info about the first 's' and about the second 's' you would get different answers. Or, at least, there would be a different question you could ask. Something like GetSymbolInfo(node, withNullabilityInfoComputed). Then, you would simply do ToDisplayParts on that symbol you got back, and it would answer properly.

[CyrusNajmabadi]

However, it could introduce confusion for the user in Scenario 1, as hovering over s at different program locations will produce seemingly different results.

I'm skeptical this is actually a problem. It seems like it's actually what i want. If i'm hovering, i'm asking: what does this mean right here. And the language is telling me. If that can change, when i'm lower in the code, that's fine.

For exapmle, say i have this code:

If i hover over the first 'i' i see:

if i do the second, i see:

I'm hovering over an 'i' at different locations, and i get differnet results :) To me there is no real distinction between "it's telling me i have different symbols" vs "it's telling me i have the same symbol, but the types are different.". They're both ways of saying: here's what hte language thinks is going on right here.

[CyrusNajmabadi]

This would allow the nullability information to be async, which could help improve IDE responsiveness.

I am highly skeptical there is a responsiveness concern. I would like to see data showing what the problem actually is.

Note: that the current design of the compiler is that is already effectively has to bind full lambdas if semantic info is asked inside them. And we're able to do that, even in complex large cases, while meeting the demands of Completion (which has much stricter perf goals vs quick-info). Given that, unless nullability tracking analysis is super expensive, it seems like we shouldn't have a problem here.

Also, remember that this is quick-info. it's already an async, non-blocking, user-invoked feature. So if this took a few hundred ms more in some cases, that's really not a problem in practice. Users would barely be able to tell.

[CyrusNajmabadi]

I'd vote to start with '2'.

[333fred]

@jcouv that's not what I'm talking about. I'm not talking about when flow analysis changes "correct" type of a variable. In your examples, the correct type of array will always be string[], from declaration onwards. What I mean is when flow analysis changes the inferred nullability after declaration. Is there any scenario where the same variable can be declared as a string?[] and then later on in the program be treated as string[]? ie, would something like this work (assuming appropriate annotations on the methods)?

string?[] array = GetMaybeStringArray();
array.All(AssertNonNull); // Or some other method of checking all elements for null
TakesNonNullStringArray(array); // Trying not to get a warning here

@CyrusNajmabadi, I'd also guess that responsiveness won't be an issue. But 1 does give the option to make it async if it does end up being a problem. I was simply enumerating the possibilities.

Personally, I'd vote to start with 3. I really do think the increased transparency of "well, it was declared as (non)nullable, but we've inferred that it's changed at this point in the code" will help users grok what is going on.

[CyrusNajmabadi]

Here is how typescript handles it:

You just hover, and TS tells you what it thinks the thing is right there. That seems pretty sane and logical to me :)

[CyrusNajmabadi]

@CyrusNajmabadi, I'd also guess that responsiveness won't be an issue. But 1 does give the option to make it async if it does end up being a problem. I was simply enumerating the possibilities.

Understood. And thanks for writing that all up. Was just trying to give my perspective on things, and to hopefully help avoid work that might be unnecessary :)

[jcouv]

@333fred That can't happen, indeed.

[cston]

@333fred, flow analysis is used to infer both the top-level nullability and nested nullability of locals at the point of declaration. After that point, flow analysis tracks top-level nullability only, so top-level nullability may change but nested nullability will not.

[333fred]

That's what I assumed. It matters for the display in option 1 or 3, as if nested nullability could change that would complicate the display.

[333fred]

Updated with scenario 6 from talking with @sharwell.

[jcouv]

@333fred One more scenario to consider: how will we display unconstrained T before and after null-testing?

Chuck is suggesting to just display T for both cases. It's pretty rare to have a T that was tested for null and the user wrote T.

[333fred]

@jcouv added scenario 7. One thing that I'd guess is that it will be a bit more work to make it display T in all cases, and I suspect that it would not be helpful for analyzer authors.

[jasonmalinowski]

Removing form IDE design review because this has already been designed.

[jasonmalinowski]

Closing -- the enhancements planned have been completed. Any further changes would be new work.