Improving IDE experience around nullable-oblivious APIs

[jcouv]

We've discussed some options. There was great interest in #8 and we wanted to explore #7 and #2-5 (tweaking QuickInfo). But it was not clear what follow-up work was planned, so I'm filing a tracking issue for clarification.

🔗 Also filed as AB#1294226
🔗 Also filed as AB#1348241

1. Do nothing.
2. Change Quick Info to say something else that’s softer than “this is not null”.
3. Remove Quick Info if there's oblivious somewhere in the method/property/whatever body.
4. Don't show Quick Info for the not null case at all.
5. Change Quick Info to say something else with the IDE doing it’s own analysis.
6. Compiler adds oblivious flow analysis.
7. Analyzer that warns on any oblivious code that isn't !ed.
8. Highlight the oblivious calls.
9. Show oblivious information in Quick Info, for example if you mouse over a method call, and that call is oblivious (either oblivious returning or oblivious accepting) say something along the lines of “this call is not nullable aware”.

Tagging @dpoeschl @ryzngard @jasonmalinowski @333fred

[genlu]

Design meeting notes:

• Revisit and evaluate the prototype from @dpoeschl on (8)
• Investigate using roslyn DFA APIs to track oblivious as an analyzer, which is pretty much (5)

[jnm2]

This has gotten me into several NREs because the positive wording made me think the API was annotated.

[MelbourneDeveloper]

Amen to this!

I logged this issue

Essentially, this problem is impacting the whole NRT ecosystem. We are told that we can upgrade a library bit by bit but any consumers of the library will not know whether or not the variable is nullable oblivious or not. That's absolutely critical for the process.

The experience of implementing NRT in a given project is currently quite frustrating. Most people won't really know why it's so frustrating. The problem is that people are navigating the library with a blindfold on.

This should say something like "It is not known if ‘Test2 is null here’

PS: If someone sees this, and says "But, this is a Visual Studio issue, and not a Roslyn issue", please take this up with the Visual Studio team who closed my issue because it's a duplicate of this issue.

[allisonchou]

Design meeting notes (3/22): We'd like to use Andrew's value tracking work as a prototype to see if an oblivious API is an input to the value the user hovered over.

[tuklein]

I found this thread because I was just migrating a project to nullable reference types and ran into unexpected NREs.

Fix #7 would be great and would have prevented bugs in our code. UI-only fixes would be nice but insufficient if they rely on the author noticing them.

What's not clear to me is why this behavior was designed to begin with. The documentation does clearly state "the default state of a nullable-oblivious variable is not-null.", but to me it would make more sense for a nullable oblivious variable to be treated as maybe-null (assume the worst).

[jtschuster]

Is this issue still being considered? I've hit numerous bugs from NRE's in the linker because we use null-oblivious libraries in a nullable context and assume no warnings means we're good. For me it feels like making null-oblivious the default for unannotated libraries defeats the purpose. It feels like if you have nullable enabled and don't get warnings, you shouldn't have to guard, but once you have a single unannotated library, all bets are off and there still could be a null at any point in the program, and you still have to add guards.

If you don't think the default should be changed, maybe this could in an analyzer or only enabled with an MSBuild property / compiler flag?

[nathanpovo]

Is it currently possible to use roslyn analysers to do this?

It was suggested in dotnet/csharplang#2244 to use a custom roslyn analyser to solve the problem but it is not clear if an analyser is capable of knowing the nullability context of various parts of the code.

[CyrusNajmabadi]

@nathanpovo A custom analyzer would be able to do this afaict.

[nathanpovo]

@CyrusNajmabadi the problem I am running into is that you cannot determine the nullable oblivious context of external code (not sure if this is just not possible or I did not find the correct documentation for it).

How do you differentiate between these 2 properties?

#nullable enable

public class NullableEnabledClass
{
    public string Test { get; set; } = string.Empty;
}

#nullable disable

public class NullableObliviousClass
{
    public string Test { get; set; }
}

Roslyn is considering both of these properties to be "not annotated" and "not null".

This results in it being impossible to differentiate the following string initializations; I would like to be able to show a warning for the final line of this code

#nullable enable

NullableEnabledClass nullableEnabledClass = new();
NullableObliviousClass nullableObliviousClass = new();

// This is fine
string nonNullString = nullableEnabledClass.Test;
// This should cause my analyser to show a warning/error
string nonNullButNotReally = nullableObliviousClass.Test;

[CyrusNajmabadi]

@RikkiGibson this seems odd. I would have expected a different nullannotated state here.

[nathanpovo]

@CyrusNajmabadi I have written a small test for this here https://gist.github.com/nathanpovo/6db2029dc5bc5a117d98885bdf16bfc3

This is the output I get

string nonNullString = new NullableEnabledClass().Test;
Type info on the following syntax: = new NullableEnabledClass().Test
Initializer type:                  string
Initializer type annotation:       NotAnnotated
Initializer type flow state:       NotNull

string? nullString = Test;
Type info on the following syntax: = Test   
Initializer type:                  string   
Initializer type annotation:       Annotated
Initializer type flow state:       MaybeNull

string nonNullButNotReally = new NullableObliviousClass().Test;       
Type info on the following syntax: = new NullableObliviousClass().Test
Initializer type:                  string
Initializer type annotation:       NotAnnotated
Initializer type flow state:       NotNull

[CyrusNajmabadi]

What do you get if you ask the IPropertySymbol representing NullableEnabledClass.Test or NullableObliviousClass.Test what the .NullableAnnotation is on it's return type?

[nathanpovo]

I get this output:

TestApp.Program
public static string? Test { get; set; } = null;
Property symbol:                  TestApp.Program.Test
Property symbol type annotation:  Annotated

TestApp.NullableEnabledClass
public string Test { get; set; } = string.Empty;
Property symbol:                  TestApp.NullableEnabledClass.Test
Property symbol type annotation:  NotAnnotated

TestApp.NullableObliviousClass
public string Test { get; set; }
Property symbol:                  TestApp.NullableObliviousClass.Test
Property symbol type annotation:  None

Code used to generate this can be found here https://gist.github.com/nathanpovo/6db2029dc5bc5a117d98885bdf16bfc3

---

Actually, found a better way of doing this:

string nonNullString = new NullableEnabledClass().Test;
Type info on the following syntax: = new NullableEnabledClass().Test
Initializer type:                  string
Initializer type annotation:       NotAnnotated
Initializer type flow state:       NotNull
Property symbol:                   TestApp.NullableEnabledClass.Test
Property symbol type annotation:   NotAnnotated

string? nullString = Test;
Type info on the following syntax: = Test
Initializer type:                  string
Initializer type annotation:       Annotated
Initializer type flow state:       MaybeNull
Property symbol:                   TestApp.Program.Test
Property symbol type annotation:   Annotated

string nonNullButNotReally = new NullableObliviousClass().Test;
Type info on the following syntax: = new NullableObliviousClass().Test
Initializer type:                  string
Initializer type annotation:       NotAnnotated
Initializer type flow state:       NotNull
Property symbol:                   TestApp.NullableObliviousClass.Test
Property symbol type annotation:   None

[CyrusNajmabadi]

Right. That matches my expectation. So you should be able to tell what's going on. In the null-oblivious case, the source property as a 'None' annotation. So you can tell it's oblivious. So if you read oblivious and assign into something annotated as 'non-null', you can warn in your analyzer.

[nathanpovo]

Are NullableAnnotation.None and the internal NullableAnnotation.Oblivious equivalent?

This is this internal TypeWithAnnotations property that I can see in my debugger but cannot access in the code

[CyrusNajmabadi]

Yes. Taht should be the case. It is doc'ed as:

        /// <summary>
        /// The type is not annotated in a context where the nullable feature is not enabled.
        /// Used for interoperation with existing pre-nullable code.
        /// </summary>
        Oblivious,

Whereas 'None' states:

        /// <item><description>The symbol producing the expression comes from a method that has not been annotated, such as invoking a C# 7.3 or earlier method, or a method in this compilation that is in a disabled context.</description></item>

[mlessard-appcom]

@nathanpovo Hello! Did you end up writing or finding an analyzer that will warn on any assignment of an null-oblivious value to a non-null variable? I would be very interested in such an analyzer. Thanks!

[nathanpovo]

@nathanpovo Hello! Did you end up writing or finding an analyzer that will warn on any assignment of an null-oblivious value to a non-null variable? I would be very interested in such an analyzer. Thanks!

Thanks for reminding me about this. I had completely forgot about it.

I never ended up finding anything that met my needs so I started to write one myself. I spent quite some time on it (figuring out how to write roslyn analyser is not as simple as it sounds...) and managed to get somewhere but then I got busy with other things and stopped working on it.

These past few days I had some free time so I thought I would give it another go.

In case anyone is curious, you can find the code here https://github.com/nathanpovo/NullableReferenceTypes.StrictMode

Note that the project is still in the very very early stages.

[mlessard-appcom]

This is great. My experience with custom analyzers is also limited, but I'd like to look into this when I get the time.