Support C#8 NRT (Nullable Reference Types) in generated code

[amis92]

It was brought to my attention that generating code in NRT-enabled projects raises warnings.

Reference: amis92/RecordGenerator#110

---

From the issue linked above:

Looks like the dotnet team decided auto-generated code needs to explicitly opt-in into nullable reference types:

public partial class TestClass
{
    public string? SomeTestProperty { get; }
}

In a project with

<LangVersion>8.0</LangVersion>
<Nullable>enable</Nullable>

this generates without issues and is actually usable, but the following compiler warning is listed for the generated file. One warning per line that uses ? on a reference type.

Warning CS8669 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. Auto-generated code requires an explicit '#nullable' directive in source.

---

What can/should we do about it? We'll definitely need to get aware of the Nullable project property. What else can we do to simplify/enable nullable generated code?

[AArnott]

To the extent that we can generate nullable ref annotated code regardless of the rest of the project, I think we should. But emitting the #nullable enable directive shouldn't be done unless the actual generator is participating or else we're emitting nullable annotations even if no ? is added anywhere.

So IMO this is something each individual generator should do if it wants to. A generator that does not want to participate would need to shed the ? annotations as appropriate wherever it copies types around from user code.

[amis92]

So let's consider a couple of scenarios for C#8 projects:

1. Project is nullable-oblivious - no problem, same as pre-8.0
2. Project is nullable-enabled, generator is oblivious:

• case as the OP's report linked, nullable declarations "leak" to the generated code.
• fix: ignore the specific CS8669 warning
• can we do better? Generator could attempt to "strip" the annotations but that requires generator to be updated. CGR could "wrap" any code from oblivious generators in a pragma that disables that warnings, making using "older" generators painless?

3. Project is nullable-enabled, generator attempts to as well:

• right now, generator would have to read project file on it's own - we don't provide information whether project uses Nullable or what langversion it targets.
• assuming the generator knows these, it can "switch" to produce nullable-conscious code and wrap it in #nullable enable directives to become independent of other generators
• framework can provide a concept of generator manifest that allows generators to declare nullable-compatibility and automatically wrap generated code in appropriate directives

4. Project is nullable-disabled but a source file on which generator is triggered is #nullable enable (that can be common in migrating projects)

• CGR or generator has to analyze code in the source file for nullability directives and switch behavior accordingly
• not solvable automatically, unless there's a single #nullable enable directive before first member is declared?
• still it's not clear whether generated code (which may be a totally separate class) is supposed to be annotated or not

I'd like to state my goals:

• make old, nullable/c#8-oblivious generators simply work™ - at best without any new warnings generated (auto-wrap old generator's code with warning-ignoring pragmas)
• make new generators still simple to develop with nullability in mind and keep them working in nullable-disabled projects

I honestly believe we should somehow automatically handle legacy generators, like the suggested wrapping.

For new generators, I really don't know which way to go. Generators using Roslyn API get nullability annotations on types they read from source code for free, but without #nullable enable those annotations are ignored. (docs: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references)

On the other hand, generating nullable directives on pre-C#8+ projects is also bad(errors), so we need to know LangVersion.

Requiring all generators that want to support nullability to have two execution paths, one for non-nullable and the other for nullable-enabled, also sounds like a bad idea. So maybe in the framework we should manage these, by stripping annotations when necessary (pre-C#8 langver)?

So many questions, so little answers.

[AArnott]

IMO we shouldn't try to do (much) more magic than Roslyn would be willing to do.

The only mandatory goal IMO is this:

Make it possible to write generators that run properly in a C# 8 world. This includes intelligently adding/removing nullable annotations while generating source code. It also includes allowing the generator to look up the nullable setting for the project and source file from which code is being generated, and any other source file it may need.

An optional stretch goal is make older generators Just Work. This seems to me to likely be a bug farm. We're giving the generators the CSharpCompilation so they can look up anything already, and copy type names (including annotations) from anywhere. We can't intercept that. The only thing we could do is post-process what they generate in order to remove annotations, but how do we know those annotations were not intentional? I suppose we could remove them if no #nullable enable directive is at the top of the file. I don't think I'd go beyond that. Did you have a proposal for going further or does this seem sufficient? You mention a generator 'declaring' support for nullability, but wouldn't its inclusion of #nullable enable in the generated file do that? Or are you thinking about generators that contribute only portions of a file?

generator would have to read project file on it's own - we don't provide information whether project uses Nullable or what langversion it targets.

Why do you say that? We hand the generator the CSharpCompilation and that includes the LanguageVersion property and NullableContextOptions property. They also have access to the full source file with the attribute and in fact every other source file, so they can look for #nullable enable as well.
I don't think we should make it any "easier" to do it right besides document that they should, but I can't think of ways we would make it easier anyway. What did you have in mind?

[amis92]

IMO we shouldn't try to do (much) more magic than Roslyn would be willing to do.

Agreed.

The only mandatory goal IMO is this:
Make it possible to write generators that run properly in a C# 8 world. This includes intelligently adding/removing nullable annotations while generating source code. It also includes allowing the generator to look up the nullable setting for the project and source file from which code is being generated, and any other source file it may need.

Definitely, I agree. We should probably have some guidance on that, though.

An optional stretch goal is make older generators Just Work. This seems to me to likely be a bug farm. We're giving the generators the CSharpCompilation so they can look up anything already, and copy type names (including annotations) from anywhere. We can't intercept that. The only thing we could do is post-process what they generate in order to remove annotations, but how do we know those annotations were not intentional? I suppose we could remove them if no #nullable enable directive is at the top of the file. I don't think I'd go beyond that. Did you have a proposal for going further or does this seem sufficient? You mention a generator 'declaring' support for nullability, but wouldn't its inclusion of #nullable enable in the generated file do that? Or are you thinking about generators that contribute only portions of a file?

Yeah, so I think you have a point. Still, reading that made me think we should really have some guidance/samples on how to write code generation that behaves well both in non-nullable and nullable projects, and take into account that the project itself may be compiled using older Roslyn in the end step (like, 2.x version). I really have no idea how to do that, or where to start.

generator would have to read project file on it's own - we don't provide information whether project uses Nullable or what langversion it targets.

Why do you say that? We hand the generator the CSharpCompilation and that includes the LanguageVersion property and NullableContextOptions property.

@AArnott We're building CSharpCompilation ourselves. So, unless we pass the compilation switches (like Nullable MSBuild property results in) into our CLI, we can't properly build it. Obviously, we just need to pass it in and make Compilation correctly. But it's a thing we need to take into account. And LangVersion as well? I'm not sure where it "ends". How much more do we have to pass in and configure the Compilation with. Just those two will suffice? What else has important impact?

[manne]

@AArnott @amis92 Does my pr #190 addresses your concerns

[manne]

my "algorithm" checks if the nullable context for the annotated class is enabled. If it is, then the nullability is respected, otherwise not.
"#nullable ..." cannot be used, if the project is pre C# 8.0

[manne]

@amis92

Looks like the dotnet team decided auto-generated code needs to explicitly opt-in into nullable reference types:

you can find "the" reason here https://github.com/dotnet/roslyn/blob/70e158ba6c2c99bd3c3fc0754af0dbf82a6d353d/docs/features/nullable-reference-types.md#generated-code

[manne]

It was brought to my attention that generating code in NRT-enabled projects raises warnings.

Reference: amis92/RecordGenerator#110

From the issue linked above:

Looks like the dotnet team decided auto-generated code needs to explicitly opt-in into nullable reference types:

public partial class TestClass
{
    public string? SomeTestProperty { get; }
}

In a project with

<LangVersion>8.0</LangVersion>
<Nullable>enable</Nullable>

this generates without issues and is actually usable, but the following compiler warning is listed for the generated file. One warning per line that uses ? on a reference type.

Warning CS8669 The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. Auto-generated code requires an explicit '#nullable' directive in source.

What can/should we do about it? We'll definitely need to get aware of the Nullable project property. What else can we do to simplify/enable nullable generated code?

@amis92 I could not yet get the pain of the situation. Referring to this issue and to your comments in PR #190.

What can/should we do about it? We'll definitely need to get aware of the Nullable project property. What else can we do to simplify/enable nullable generated code?

The Nullable property of the project file should not be relevant. The nullable context of class/member/field/... can be retrieved without this Nullable project property. I created a short gist depicting this scenario. The gist contains a linqpad file. Linqpad does not have a project, and the code within this file does not create a project (?).

That gist, like the pr #190, uses SemanticModel.GetNullableContext(Int32) to retrieve the state of "nullable" at the specific position within the "source". Based on this nullable state, the generator can opt-in for the nullability with the trivia "#nullable enable".

Running the NullableGenerator from PR #190

• without adding "#nullable enable" to the generated code, roslyn warns for each non nullable usage.
• with adding "#nullable enable" to the generated code, roslyn does not warn.

Does this information helps?
Or did I miss an aspect of this issue/discussion?

[manne]

I investigated further
https://gist.github.com/manne/7646b01b7e72664654063d90adec890d#file-sroslyn_nullable_context-linq

        public class MyClass
        {
            public string? Get() => null;
            
        #nullable enable
            public string? Get() => null;
            
        #nullable disable
             public string GetNonNull() => null;
        }

Retrieving the nullable context for the first Get results in ContextInherited. And the actual value comes from the project file. As stated here https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.nullablecontext?view=roslyn-dotnet#fields

[AArnott]

The Nullable property of the project file should not be relevant. The nullable context of class/member/field/... can be retrieved without this Nullable project property

@manne I think the point @amis92 made earlier is relevant here: you can't rely on the Nullable attribute from the Roslyn API today because we initialize Roslyn's Compilation based on a limited understanding of what is in the project file. Therefore when your generator interrogates the Roslyn API for whether nullability is active, you might be told 'no' when the answer is 'yes' because of something in the project file that we didn't tell Roslyn about.

@amis92 The best way forward is probably to pass the entire compilation command line to our task and ask Roslyn to parse it to create the Compilation. VS itself does this to initialize their language service.

[amis92]

pass the entire compilation command line to our task and ask Roslyn to parse it

@AArnott that'd be amazing, but I have no idea how to do that. 🤷‍♂️

[AArnott]

Our targets could invoke the Csc task, and fetch the CommandLineArgs task output property:
https://github.com/dotnet/roslyn/blob/6a099895d6bbe76ffdb80a9557ae464d2aba147b/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L137

Passing in true for SkipCompilerExecution I think will make the effect of the task just be to produce a single string with the command line.
https://github.com/dotnet/roslyn/blob/6a099895d6bbe76ffdb80a9557ae464d2aba147b/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L118

Then you can pass that command line string to our own task/tool so that it can ask the CSharpCompilation to parse it and initialize everything.
We'd still need to maintain our Csc task invocation to mostly match what is the the standard csharp targets (e.g. add Nullable="$(Nullable)" parameter when that's invented) but that would be a very simple change and isn't required very often.

We might want to use a "response file" approach instead of passing a huge string around, particularly since Windows as limitations of how long a command line can be.