Embedded activation of C#8 and NUllable feature in the package.

[mnivet]

I discover this package after having read this blog post:
https://endjin.com/blog/2020/07/dotnet-csharp-8-nullable-references-supporting-older-runtimes

It clearly simplify a lot the support of a private enterprise library because before that we were using a lot of #if instructions to annotate the library for recent frameworks and keep supporting older frameworks.

But in that blog post there is a trick which is not easy to guess and on which this package could help:
The variability of the activation level of the feature: <Nullable>enable</Nullable> vs <Nullable>annotations</Nullable>

Note that we have a lot of libraries in which we want to add support of nullable, and that this scenario is probably the most common one for people looking for a package like that one. So I though it could be great if the package natively guide us in that direction.

It worked fine in multiple use cases I've tested, and we can override it in csproj if we want.

The only limitation i've found is on legacy csproj (not SDK style) where the Nullable tag seems to be not interpreted, but the LangVersion tag works so even in that case it's already a better experience for the developer since he could work with nullable right after having added the package (by using #nullable directive in the cs files), it's no more required for him to set the LangVersion manually.

[manuelroemer]

Hey, sorry for the late reply. I totally missed the notification for this PR. Feel free to @ me the next time if I don't reply after a few days.

I see where you are coming from with this PR and I agree that something like this may make adopting nullable reference types easier for some people, but I think that making these changes would introduce feature creep to the package. Imo, NuGet packages should not arbitrarily change project system defaults. This can quickly become an issue for a user who needs to trace down why a default value established by .NET's project system is suddenly different. It's also impossible to predict every single use-case with this package. Maybe the proposed defaults will create some issues for some exotic project configuration. I think that the best route is to simply not change any kind of defaults, if possible.

In addition, I think that your desired behavior is easily replicated per-project by using a Directory.Build.prop file setting the following values for all C# projects in your solution:

<LangVersion>latest</LangVersion>
<Nullable>annotations</Nullable>

By creating such a file, you don't have to (re-)configure NRTs in each project, making the whole setup for a solution incredibly simple and quick.

If that doesn't satisfy your needs, I also have another suggestion, but it's untested and I have no idea whether it actually works! If you want to have a NuGet package which provides these project defaults, you could try to create an entirely new NuGet package which is a dev-dependency and references Nullable. When referenced, NuGet might be smart enough to integrate both the *Attributes.cs files and the *.targets files.
But again, I don't really know whether that works. Experimenting with a local package without uploading anything to nuget.org is probably a good idea here.

I am going to close the PR for the reasons named above, but I want to thank you for taking the time to create the PR. I really appreciate the intention and the fact that you wanted to contribute here! ❤️ I just think that such changes are too heavy for a slim package like Nullable.

[mnivet]

@manuelroemer Ok thanks for your response.
I understand your point of view, so I will probably go for the Directory.Build.prop solution in our projects for now, even if I'm not really found of that feature because it's a "magic" feature IMHO. If you don't known the convention you may not notice that such file, that is reference nowhere, has impacts on your projects… Even projects that do not yet use Nullable (typical case of a migration done project by project in a big solution). It's why I would have preferred not to go in that direction.
But I also read more and more people using that feature, so if the magic start to become well known by the community it may be less problematic that I though.

So if not changing the Nullable package is your opinion (which is perfectly valid), could we at least give more explanations in the readme of this repo about the projects properties that need to be setup in addition of the package, and how they could be setup in a Directory.Build.prop ?
At least if the package do not auto define those properties the repo would provide all needed information for developers to really start using nullable feature over this package.
I can do a PR in that direction if you're agree.

Finally for the solution of creating a package that reference Nullable one, I've tried before doing this PR, but it seems that contentFiles are not consumed in that case.
I think that transitive copy of ContentFiles is not done by default because of that typical case:
Project P1 reference a package with ContentFiles, and a unit test project P2 reference project P1.
In that typical case it's clear that by default we don't want ContentFiles to be included in P2!

I look a bit and found that we can still change that default behaviour with some modifications in Nullable package: https://stackoverflow.com/a/61297839
And since nullable package is a dev-dependency and any package that reference it should also be dev-dependency the additional target won't be consumed even by the project P2 and maybe it could work and not generate strange scenarios.
But I'm not sure of that...
Even if I'm not sure, I share the idea with you to get your opinion because I would be enjoy to have packages like Nullable.EnableLastest, Nullable.EnableCS8 or Nullable.EnableCS9 (or something like that), so I can choose easily with a single line in my projects (and really by project) how I want to activate Nullable feature.
It's what I wanted to do first, but the fact that it wasn't possible by default make me do that PR.

A last idea that comes to my mind: maybe the best to create those packages is to not reference Nullable but to fully include it's content in each package. Naming would semantically be enough. Would you be agree if this repo produce those multiple packages ? So we don't have to synchronize some forks to do that...
It would allow to keep the Nullable package pure as you want it to be, so it can be used in any scenarios, and also provide some advanced packages for people like me that will want an easy single line setup in projects.

[mnivet]

One more remarks:
After some tests, using the Directory.Build.props file is clearly not a working solution to get variability on the Nullable activation in function of the target for multitargeted projects.
To achieve a variability on the Nullable activation like I've done in this PR we need to write something like that:

<Project>
  <Choose>
    <When Condition="'$(TargetFramework)' == ''">
    </When>
    <When Condition="$(TargetFramework.StartsWith('net2'))">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <When Condition="$(TargetFramework.StartsWith('net4'))">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <When Condition="$(TargetFramework.StartsWith('netstandard1'))">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <When Condition="'$(TargetFramework)' == 'netstandard2.0'">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <When Condition="$(TargetFramework.StartsWith('netcoreapp1'))">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <When Condition="$(TargetFramework.StartsWith('netcoreapp2'))">
      <PropertyGroup>
        <Nullable>annotations</Nullable>
        <LangVersion>8.0</LangVersion>
      </PropertyGroup>
    </When>
    <Otherwise>
      <PropertyGroup>
        <Nullable>enable</Nullable>
      </PropertyGroup>
    </Otherwise> 
  </Choose>
</Project>

However conditions on the target projects are not working when put in Directory.Build.props file (the file is loaded and interpreted too soon in the msbuild pipeline).
But it works if written in a Directory.Build.targets file.

In real I don't need all that conditions since I don't use all that targets in the projects I should maintain. But it's enough complex to make me want to hide that complexity in some package.

[manuelroemer]

Hi, thanks for the reply! I want to tackle your last point first:

After some tests, using the Directory.Build.props file is clearly not a working solution to get variability on the Nullable activation in function of the target for multitargeted projects.
To achieve a variability on the Nullable activation like I've done in this PR we need to write something like that.
[...]

I must ask you a follow-up question here: Why aren't you simply using the following Directory.Build.props?

<PropertyGroup>
  <LangVersion>latest</LangVersion>
  <Nullable>annotations</Nullable>
</PropertyGroup>

This is exactly the same configuration which you would also get via the NuGet package if I had accepted this PR.
And if that doesn't work for your project (i.e. if you'd have to overwrite these settings again anyway for certain target frameworks), what benefit would such a configuration have in a NuGet package?

[manuelroemer]

Now to get to the other points (I'll just go through them in order):

I will probably go for the Directory.Build.prop solution in our projects for now, even if I'm not really found of that feature because it's a "magic" feature IMHO.

To be honest, I disagree with this claim. Directory.build.props/targets is an established standard in the .NET ecosystem and really easy to learn/spot (you can, for example, just add the file to your solution and the file will be visible to everyone in VS). It's also used by a lot of projects, so I'd argue that it's not exactly magic/unknown by developers. And if there's a dev on the team who doesn't know the feature it's easy enough to explain: It's just a file which allows you to globally configure project settings for all of your projects at once.

On the other hand, having project settings being written by a NuGet package is magic. It's something that rarely happens (if ever) and that your typical developer doesn't expect. And in comparison to a Directory.Build.props file, you cannot easily find the source, because the .targets file is typically hidden in a completely different location than your project. I'm not saying that this is per se bad, but it's certainly much more magic than a Directory.Build.props file.

At last, a personal opinion. Since I don't know your exact project setup, it might not apply to you at all. I'm simply saying the following because it might help to understand my point of view on the discussion.
Regarding the mentioned use case, i.e. migrating an existing solution project by project: In my opinion it's best to be explicit in such a scenario, i.e. to not globally enable NRTs, but do it on a project-per-project basis. This is not a lot of work and tells each developer the exact state of the current project:

• NRTs are disabled. -> The project is not enabled.
• NRTs are enabled via <Nullable>annotations</Nullable>. -> The project is being migrated at the moment.
• NRTs are enabled via <Nullable>enable</Nullable>. -> The project is fully migrated.
  Once the migration is finished, you can globally enable NRTs and be done with it!

So if not changing the Nullable package is your opinion (which is perfectly valid), could we at least give more explanations in the readme of this repo about the projects properties that need to be setup in addition of the package, and how they could be setup in a Directory.Build.prop ?
[...]
I can do a PR in that direction if you're agree.

Of course, that sounds great! Any contribution here is much appreciated. 🍻
If you'd like to add additional info to the README, I suggest doing it in the Quickstart part, ideally as step 4. E.g. 4. Optional: Enable Nullable Reference Types in your project(s).

I think that we should:

• Link to Microsoft's documentation about the topic.
• Quickly highlight the two options of enabling the feature (#nullable enable in code vs. <Nullable>enable/annotations</Nullable> in a project file).
• Add a section about how a Directory.Build.props file might be useful. I wouldn't make it too complicated here though because each project setup is different and everybody will have different requirements. I'd probably just say what a Directory.Build.props file does and give a 4-line example of how you can globally enable NRTs with it.

Do you have any additional suggestions/wishes? I'm always open to dicuss them - the above is just the result of brainstorming quickly!

I look a bit and found that we can still change that default behaviour with some modifications in Nullable package: https://stackoverflow.com/a/61297839

This is actually really interesting - thanks for the link! I will have a look at it when I find the time. I want to introduce a .targets file anyway in order to fix #11. But this will be quite a bit of work and require a script to generate the file. I'll probably not do that until I'm on holidays though.

If you need the changes immediately, you can consider forking the project, but I really recommend you to abuse the Directory.Build.props/targets first or alternative just enable NRTs on a per project basis. I guarantee you that it's an easier and more flexible solution than any package can ever provide!

[manuelroemer]

That was a lot of text - I hope that all of this helps you in some way/that we can come to a good solution here. I'm quite eager to hear your thoughts on the topic! 😄

[mnivet]

First I want to says that it's a pleasure to exchange with you. It's long text yes, but if it's needed to find the best solutions it's worth it.

So first, why I don't want to have just a simple Directory.Build.Props ?
It's because, like explain in the blog I linked in the first message, when your are working on a multi-targeted library, enabling the feature globally cause false positive warnings for frameworks that are not annotated, while build against the recent annotated frameworks tell us that it's ok.

This typically happens when using string.IsNullOrEmpty()
Here a minimal project and source code that can be used to show the issue:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net472;netcoreapp3.1</TargetFrameworks>
    <Nullable>enable</Nullable>
    <LangVersion>8</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Nullable" Version="1.3.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>
  
</Project>

using System.Linq;

class Program
{
    static void Main(string[] args)
    {
        string name = SanitizeHelloTo(args.FirstOrDefault());
        System.Console.WriteLine($"Hello {name}!");
    }

    private static string SanitizeHelloTo(string? to)
    {
        if (string.IsNullOrWhiteSpace(to))
            return "World";
        return to.Trim();
    }
}

If we build that sample we get one warning on last line (to.Trim()): CS8602 Dereference of a possibly null reference.
But only for net472 not for netcoreapp3.1!
Because netcoreapp3.1 is annotated which allow to the compiler to known that the call to string.IsNullOrWhiteSpace will produce false if tois null which means that we won't execute the to.Trim()
But net472 is not annotated . The compiler can't guess the behaviour of string.IsNullOrWhiteSpace against that framework and produce a warning.

So the solution is to have a variable activation of the nullable feature in function of the target framework, which is what this PR was trying to achieve. If you look back at it in details it do not affect the same default to all frameworks.
For legacy frameworks we just do : <Nullable>annotations</Nullable>
And for annotated frameworks (netcoieapp3.0 or more recent) we do <Nullable>enable</Nullable>

This allow to get warnings only against annotated frameworks, and remove false positive warnings made by build from legacy frameworks. We can says that legacy frameworks builds trust recent frameworks builds to produce the warnings.

And it's why I would have preferred NuGet packages: because they are more adapted to select thinks in function of the targeted framework than Directory.Build.props/targets or csproj.

So the whole point is not to use <Nullable>annotations</Nullable> to says that a migration is in progress but really to use it as a default activation state for legacy frameworks. We want to be able to write nullable code for legacy frameworks. It's the goal of this Nullable package after all! And it's what <Nullable>annotations</Nullable> do: just allow to write nullable code but don't raise warnings. it's the minimal activation level. And fully enable the feature (including warnings) for annotated frameworks.

But now I explain all that more in details (to you and also another time to myself) I start to wonder if in real what we want is not to activate nullable warnings only for the most recent framework of a project.
Which means that we may write something more simple in a Directory.Build.targets or csproj file by just doing a condition on the most recent framework targeted by the project or solution.
eg:

<Project>
  <PropertyGroup>
    <LangVersion>latest</LangVersion>
  </PropertyGroup>
  <Choose>
    <!-- just this condition must be adapted in each project/solution in function of the most recent framework that will be targeted -->
    <When Condition="'$(TargetFramework)' == 'netcoreapp3.1'">
      <PropertyGroup>
        <Nullable>enable</Nullable>
      </PropertyGroup>
    </When>
    <Otherwise>
      <PropertyGroup>
        <Nullable>annotations</Nullable>
      </PropertyGroup>
    </Otherwise> 
  </Choose>
</Project>

because net5 is probably more annotated than netcoreapp3.x, the issue I got with legacy frameworks may also apply to netcoreapp3.x versus net5.
So maybe, providing such template somewhere in the documentation is a better idea than my nuget packages idea?
Because even in a nuget package we can't guess which is the most recent framework that is targeted by a project, which means that we won't be able to set <Nullable>enable</Nullable> only for that framework and stay at <Nullable>annotations</Nullable> state for other frameworks.

Now, I see just one things that this nuget package can do to simplify this, which is to make this package set <Nullable>annotations</Nullable> for all target frameworks (not variable like I did in this PR) so this template could be simplified to:

<Project>
  <PropertyGroup>
    <LangVersion>latest</LangVersion>
    <!-- This condition must be adapted in each project/solution in function of the most recent framework that will be targeted -->
    <Nullable Condition="'$(TargetFramework)' == 'netcoreapp3.1'">enable</Nullable>
  </PropertyGroup>
</Project>

But this would require that you change your mind about changing default values for some properties...

I'm please that you were not agree to go in that direction first since anyway the solution will be better than what I suggest first.
But now I will try to make you change your mind by giving you an example of packages that change default values for some properties: the Microsoft.NET.Test.Sdk package.
If you download it and look inside you can see that is set the <IsTestProject> property to true, property which is used for example by the pack target to exclude that type of project from packaging. So if Nullable package should override the default value for the <Nullable>prop it won't be the first package to do that. Even Microsoft do that!

[mnivet]

And by the way, for a migration in progress, using <Nullable>annotations</Nullable> may not be a good option...
Since it won't produce warnings, you won't be able to know where you need to perform changes :(

I think that it's better to really enable the feature and disable it in all existing files by adding a #nullable disable pre-processor directive at the top of each file, like suggested here : https://www.meziantou.net/csharp-8-nullable-reference-types.htm#adding-nullable-anno
And then progressively remove that #nullable disable directives, and fix the warnings file by file.

So yes it do not show when a project is in progress or finished, but at least we can found where we have things to migrate, and track the progression by counting the number of files that are still starting by this directive.

[mnivet]

@manuelroemer I won't bother you anymore with this PR, I'm now fully convinced that we should not modify the package like that.
And about the update of the quick start section I've open a new PR: #15

Thanks for your patience

[manuelroemer]

Sorry again for the late reply, I had limited access to my PC for the last week.
If everything's resolved so far, I'm happy - thank you a lot for creating the PR! That may be very helpful for other people who didn't have much contact with NRTs yet.

I also want to tell you that it's been a very pleasant exchange in my opinion - thank you a lot for your thorough answers and for taking the time to discuss everything!