There is no API to determine whether we are in a #nullable enable context

[jasonmalinowski]

If the IDE needs to know if we're in a #nullable enable'd context, there's no API today to do so. This comes up in a bunch of cases: if we're generating code from one file to another and one file has it enabled but the other doesn't, we need to know to drop annotations or add #nullable enable or other things.

[333fred]

API Proposal:

[Flags]
public enum NullableContext
{
    Disabled,
    WarningsOnly = 0b01,
    AnnotationsOnly = 0b10,
    Enabled = WarningsOnly | AnnotationsOnly
}

public static class NullableContextExtensions
{
    public bool WarningsEnabled(this NullableContext context);
    public bool AnnotationsEnabled(this NullableContext context);
}

public class CSharpSemanticModel : SemanticModel
{
    public NullableContext GetNullableContextAtPosition(int position);
}

public class CSharpCompilation : Compilation
{
    public NullableContext GetDefaultNullableContext();
}

This is very similar to the current internal APIs, except that instead of a struct with two bool? we collapse this to a simple enum. In order for this to work, we need put the API on SemanticModel instead of a SyntaxTree, as we need to know what #nullable restore means in the context of the current compilation. A second option would be to add a Restore value to the NullableContext enum, move the API to CSharpSyntaxTree, and require the user to figure out what restore means. This seems less than intuitive to me.

As a followup, we should also consider adding methods to the syntax factory that take a current nullable state, a target nullable state, and a syntax node, and wraps that node in the correct declarations to give it the specified state, but I'm not proposing anything concrete there for now and will leave that for later.

@jasonmalinowski @ryzngard @jcouv @gafter @agocke could you please take a look at this proposal and leave any initial thoughts, and then we can schedule a quick design review after initial feedback.

[333fred]

Proposal 2:

public enum NullableContext
{
        Enabled,
	WarningsOnly,
	AnnotationsOnly,
	Restore,
	Disabled
}

public class CSharpSyntaxTree : SyntaxTree
{
    public NullableContext GetNullableContextAtPosition(int position);
}

public class CSharpCompilation : Compilation
{
    public NullableContext GetDefaultNullableContext();
}

[ryzngard]

and require the user to figure out what restore means. This seems less than intuitive to me.

I agree, is not intuitive here. What's the argument for needing the NullableContext without a compilation?

[jcouv]

I rather like proposal 1 (simpler), assuming it satisfies use cases. But I'm having trouble understanding whether users of this API would need the restore option.

So it would help if we could identify our own use cases for the IDE and confirm that proposal 1 suits those needs.
Is there any scenario where the IDE would need to generate a #nullable restore as opposed to a #nullable enable|disable?

[jasonmalinowski]

Does restore restore to the previous use in the file or back to the project settings?

So if the IDE is generating any new code that it has to surround with a #nullable enable in existing files, we're going to need to stick a #nullable <whatever it was before> at the end to make sure we don't screw up the code after that point. If restore restores to the project then yes we'd need to distinguish restore vs. the underlying meaning.

Not sure why this isn't a separate boolean for each axis being returned as a struct though. If we have that enum we're probably going to immediately write a "IsAnnotationsEnabled" extension method that compares to Enabled/AnnotationsOnly and IsWarningsEnabled that compares to WarningsOnly/Enabled.

[jasonmalinowski]

Also, can I do a restore on just warnings or just annotations?

[jcouv]

Does restore restore to the previous use in the file or back to the project settings?

It restores to the project settings.

Also, can I do a restore on just warnings or just annotations?

Yes, you can restore the warnings and the annotations contexts independently.

[agocke]

I think GetDefaultNullableContext will be a problem since generated files have a different default, don't they?

[333fred]

GetDefaultNullableContext should be fine with the initial api proposal. If you get a semantic model for a generated file and ask for the nullable context at a position, you'll get the current context, even if that's different from the project defaults. We could add another compilation API GetDefaultNullableContextForGeneratedFiles if we want to expose the generated file default as well.

[agocke]

Maybe the name was just screwing me up. The Compilation-level GetDefaultNullableContext is just asking what the compilation setting is, then? The file default without explicit annotation may be diferent?

[333fred]

@jasonmalinowski, I've updated the original proposal to make the enum flags, and added extension methods to determine whether warnings/annotations are enabled. Does that satisfy your concern about the booleans?

[333fred]

The Compilation-level GetDefaultNullableContext is just asking what the compilation setting is, then? The file default without explicit annotation may be diferent?

Correct. GetDefaultNullableContext is basically asking the project-level setting is. We could change the name to GetProjectLevelNullableContext, if you think that's a better name?

[agocke]

Yeah, GetProjectLevelNullableContext sounds good. One more question: do you instead want to make it a property on CompilationOptions? That feels like the standard place for this API

[jasonmalinowski]

I've updated the original proposal to make the enum flags, and added extension methods to determine whether warnings/annotations are enabled. Does that satisfy your concern about the booleans?

Why not just return a struct? Also means if/when you ever add another mode there's a logical way to expand the API.

[333fred]

Why not just return a struct?

Because using an enum will make followup generation APIs easier. I do think the SyntaxFactory will want an API that takes a syntax node, an enum for target nullability, and an enum for original nullability, and returns a syntax node wrapped with the correct pragmas to set to the target nullability and return to the original nullability after the syntax.

[333fred]

do you instead want to make it a property on CompilationOptions? That feels like the standard place for this API

@agocke that would be fine with me. @jasonmalinowski, how do you feel about that from the IDE side?

[agocke]

@333fred Wait, don't we already have http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/CSharpCompilationOptions.cs,41? How would this API be different from the NullableContextOptions?

[jasonmalinowski]

Because using an enum will make followup generation APIs easier. I do think the SyntaxFactory will want an API that takes a syntax node, an enum for target nullability, and an enum for original nullability, and returns a syntax node wrapped with the correct pragmas to set to the target nullability and return to the original nullability after the syntax.

In that case, doing a struct is just as easy as an enum: the consumer of such an API isn't constructing anything directly. They're just calling the existing APIs and passing it around.

[333fred]

@agocke I did not realize we had that. However, it will need to have the annotations setting added.

[agocke]

source.roslyn.io is a little out of data: https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/NullableContextOptions.cs

The latest version includes the cross product of warnings x annotations

[333fred]

In that case, doing a struct is just as easy as an enum: the consumer of such an API isn't constructing anything directly. They're just calling the existing APIs and passing it around.

@jasonmalinowski I don't think that's quite true, because you can't introduce a refactoring that explicitly turns the feature on at a specific point as easily as you could with an enum.

[333fred]

@agocke I think I would still propose that the contents of that enum be structured the way I did in this proposal, using Flags and having the extension methods.

[agocke]

That's fine with me

[333fred]

After meeting with @jasonmalinowski, @ryzngard, @jcouv, and @agocke, we have an updated API proposal. We observed a few things in discussion:

1. In order to not mess with a file, the IDE is going to need to know not just whether things are enabled, but also if the current context is inherited from the containing project.
2. The file-level context is necessarily more complicated than the compilation-level context, as you have to describe this inheritance at in a syntax tree, but not on a compilation.
3. There are 2 axes of information, and all crosses are valid.
   a. Are warnings or annotations enabled?
   b. Were warnings or annotations inherited from the compilation?

Given these observations, we have an updated proposal below:

// The first two bits communicate whether the state is inherited. The third and fourth bits
// communicate the state of the nullable feature
[Flags]
public enum NullableContext
{
    WarningsContextInherited = 0b0001,
    AnnotationsContextInherited = 0b0010,
    // Set by default at the start of a file
    ContextInherited = WarningsContextInherited | AnnotationsContextInherited,
    WarningsEnabled = 0b0100,
    AnnotationsEnabled = 0b1000,
    Enabled = WarningsEnabled | AnnotationsEnabled
}

// Simple helper methods to do the bitwise operations
public static class NullableContextExtensions
{
    public bool WarningsEnabled(this NullableContext context);
    public bool AnnotationsEnabled(this NullableContext context);
    public bool WarningsInherited(this NullableContext context);
    public bool AnnotationsInherited(this NullableContext context);
}

class CSharpSemanticModel
{
    public NullableContext GetNullableContext(int position);
}

public enum NullableContextOptions : byte
{
    Disable,
    Enable,
    Warnings,
    Annotations
}

public static class NullableContextOptionsExtensions
{
    public bool WarningsEnabled(this NullableContextOptions context);
    public bool AnnotationsEnabled(this NullableContextOptions context);
}

public class CSharpCompilationOptions
{
    public NullableContextOptions NullableContextOptions { get; private set; }
}

We did consider a GetSyntaxNullableContext(int position), and nothing in this proposal would block such an API, but we decided that there is no need for such an API at this point in time. If we or a customer encounters such a scenario later, we can add the API at that time.

Another important point is that the default file-level context is not always the same as the compilation context, namely in generated files. This is representable in the API, but will have to be doc'd so that people don't always assume that the "inherited" nullability state is the same as the compilation-level state.

I'm not fully satisfied with the name of this second enum: it's not meaningfully different from NullableContext, which I think will be confusing for API users. I'm proposing CompilationNullableContext as an alternative.

/cc @cartermp and @CyrusNajmabadi, would love to hear your thoughts on this proposal as well.

[CyrusNajmabadi]

/cc @cartermp and @CyrusNajmabadi, would love to hear your thoughts on this proposal as well.

There are 2 axes of information, and all crosses are valid.
a. Are warnings or annotations enabled?
b. Were warnings or annotations inherited from the compilation?

As a customer of the language, i have to admit i'm super confused about what's going on here. It feels very tough to know about all the options available and how i'm supposed to use them properly. Is that information explained anywhere?

For example, what are annotations? How are they different from warnings? Why is tehre a split between them?

When we did this in TS, we just had a single bool option to opt into this all or not. That was it.

Now, even if a single-bool option at the compilation level isn't sufficient, why is there a need for such fine-grained control at a narrow level?

[CyrusNajmabadi]

Basically:

1. i need to understand the language story here.
2. that will impact the language service story.
3. that will help determine what APIs are needed.

[cartermp]

@333fred

I'm proposing CompilationNullableContext as an alternative.

Just to be clear, is this for the first or second enum? Generally I think the split between "what the compiler has to care about" and "what things that are not the compiler has to care about" is a good distinction. I probably just need to wrap my head around this a bit more to have more of an opinion.

@CyrusNajmabadi

When we did this in TS, we just had a single bool option to opt into this all or not. That was it.

Although there are more options here, the one that we'll be documenting and recommending is a single option:

For a source file:

// To enable it for the file (or scope) you want
#nullable enable

// To enable it for the file (or scope) you don't want to enable it for
#nullable disable

For a project file or Directory.build.props file:

<!-- To enable for the project or directory -->
<Nullable>enable</Nullable>

<!-- To disable for the project or directory (implies it's enabled at a higher scope) -->
<Nullable>disable</Nullable>

There's also NoWarn for specific warnings but that's a bit niche.

The rest are basically useful for Microsoft employees and power users/advanced component authors. We'll need to flow all the other options through the API and enable them via tools, but enable/disable are the ones that matter for the large majority of C# users, and it will be the one that is blogged and emphasized in documentation (the others will be documented, but not emphasized/recommended).

[333fred]

Just to be clear, is this for the first or second enum?

@cartermp this is for the second enum, instead of NullableContextOptions. These are both things the compiler has to care about. This option is for what you have set in the project file, and the NullableContext enum is for what you have in a source file.

@CyrusNajmabadi as Phil said, most these options won't actually matter in practice, which is why we're providing extension methods to help you ask the questions that the IDE/analyzers actually care about.

[CyrusNajmabadi]

@CyrusNajmabadi as Phil said, most these options won't actually matter in practice,

I would still like to understand them :D

[agocke]

I think the name NullableContextOptions is fine

[333fred]

@CyrusNajmabadi the best explanation of the nullable context stuff we have currently is here: https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-13.md

[333fred]

I've created a draft PR with the latest proposal here: #37017

[333fred]

This has been added.