Nullable annotations

[gfoidl]

Added C# 8 nullability annotations.

TBH I don't know how it plays with older C# compilers that don't support nullable with the current setup (nuspec), and if there's a good and robust way to detect the "compiler features". If there's no way to make this backwards-compatible this PR should be closed.

That's why this PR is marked as draft right now.

[khellang]

Thanks for this, @gfoidl! 🙏🏻

TBH, I have no clue how this would work out given a compiler that doesn't support nullable reference types. I assume you'd get compilation errors.

AFAIK, you can pivot on language and TFM for content files, but those are orthogonal from LangVersion.

At the same time, this package is versioned, so if you're using an older language version, you could just stick with an older version of the package 😅

[gfoidl]

At the same time, this package is versioned, so if you're using an older language version, you could just stick with an older version of the package

That's the easiest and most straightforward way 👍

I don't knows whats the best approach for this. But with the embrace of nullability in C#, I think this (very useful and handy) package should support it. Leaving the decision about the way to go by you :wink:

[khellang]

Hmm, I found another issue. When included in targets that don't have the nullable attributes, i.e. anything below netcoreapp3.0 and netstandard2.1, it won't compile. I tried to just add the attributes to the file, which seems to compile fine even on platforms that have them already:

https://github.com/khellang/MimeTypes/blob/331ed34c4cee379f78c0d5df677a8f84f741bd4a/src/MimeTypes/MimeTypes.cs.pp#L1066-L1085

[khellang]

I pushed 2.0.0 to NuGet 👌

[gfoidl]

Ahh, exactely, I forgot about this (workaround) -- I've used them before. Thanks for pointing out and fixing it.

I pushed 2.0.0 to NuGet

🎉 Perfect 🥇

[gfoidl]

A small flaw, now for .NET Core 3.1

warning CS0436: 
The type 'NotNullWhenAttribute' in 'D:\...\obj\Debug\netcoreapp3.1\NuGet\1C9FABAE087B3F8E1C44C43A9949DCE5C328D559\MimeTypes\2.0.0\MimeTypes.cs' 
conflicts with the imported type 'NotNullWhenAttribute' in 'System.Runtime, Version=4.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. 
Using the type defined in 'D:\...\obj\Debug\netcoreapp3.1\NuGet\1C9FABAE087B3F8E1C44C43A9949DCE5C328D559\MimeTypes\2.0.0\MimeTypes.cs'.

So in https://github.com/khellang/MimeTypes/commit/331ed34c4cee379f78c0d5df677a8f84f741bd4a these should be put under #ifdefs. I'll prepare a PR for this.

[khellang]

Hmm. Yeah, that's what I was afraid of happening, but when I tested, I didn't get any warnings 🤔

these should be put under #ifdefs

Yeah, I thought about that as well, but since there's no way to define open-ended ifdefs, I couldn't figure a good way to write them in a future proof way.

[gfoidl]

to write them in a future proof way.

My idea is to add a target for netstandard2.1, which will be picked by .NET Core 3.1 and newer. So #if !NETSTANDARD2_1 should work. I'll test it though.

[khellang]

But if you explicitly target netcoreapp3.1, it won't define NETSTANDARD2_1, right?

[gfoidl]

Hm, good question. I'll prepare a test-matrix and show the results here (or in the PR I'll open).

[khellang]

Also, what happens if (it won't happen, but let's pretend) .NET Standard 2.2 comes out. It will still have the attributes, but won't define NETSTANDARD2_1 and the attributes will be compiled again.

[khellang]

Hmm, I think I have an idea. Since contentFiles can pivot on TFM, I wonder if they are picked for future versions of a TFM, i.e. if you have contentFiles/cs/net45/MimeTypes.cs, will that also be picked for all future versions of .NET Framework?

If that's the case, we could produce two different files; one with the annotations and one without, then include them for the specific targets:

[gfoidl]

Your ideas sounds good.

will that also be picked for all future versions of .NET Framework?

I can't tell for sure from top of my head. For net45 example I'm pretty sure that say net48 will use the content from the net45 "folder" -- same as it does with assemblies from the package.

So with your layout it should be (not verifyed):

tfm	file
net5.0	contentFiles/cs/netcoreapp3.0/MimeTypes.cs.pp
netcoreapp3.1	contentFiles/cs/netcoreapp3.0/MimeTypes.cs.pp
netcoreapp3.0	contentFiles/cs/netcoreapp3.0/MimeTypes.cs.pp
netcoreapp2.2	contentFiles/cs/any/MimeTypes.NullableAttributes.cs.pp
netcoreapp2.1	contentFiles/cs/any/MimeTypes.NullableAttributes.cs.pp
netstandard2.1	contentFiles/cs/netstandard2.1/MimeTypes.cs.pp
netstandard2.0	contentFiles/cs/any/MimeTypes.NullableAttributes.cs.pp
net48 / net56	contentFiles/cs/any/MimeTypes.NullableAttributes.cs.pp

If that is verifyed, then this is the best and most future proof approach.

---

Current state:

tfm	state
net5.0	:warning:
netcoreapp3.1	:warning:
netcoreapp3.0	:warning:
netcoreapp2.2	:heavy_check_mark:
netcoreapp2.1	:heavy_check_mark:
netstandard2.1	:warning:
netstandard2.0	:heavy_check_mark:
net48	:heavy_check_mark:

Looks like we have to specify each tfm in a white-/black-list way

#if !(NET5_0 || NETCOREAPP5_0 || NETCOREAPP3_1 || NETCOREAPP3_0 || NETSTANDARD2_1)
// ...
#endif

If your ideas works, I prefer your solution.

what happens if ... .NET Standard 2.2 comes out

Let's rephrase it: what happens if a new tfm comes out?

I remember an issue, but I don'f find it right now. Think it was in msbuild-repo, and it's about improving the toolling to support tfm-ranges like #if NETCOREAPP > 3.1 or anything like that. So for future version this package should be updated once this "range support" is available.

But if you explicitly target netcoreapp3.1, it won't define NETSTANDARD2_1, right?

(Unfortunately) right -- it's the correct behavior though.

[khellang]

➜ dotnet build
Microsoft (R) Build Engine version 16.5.0+d4cbfca49 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 386.84 ms for /Users/khellang/Code/ConsoleApp2/ConsoleApp2/ConsoleApp2.csproj.
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/net45/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netstandard2.0/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netcoreapp2.1/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netcoreapp3.0/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netstandard2.1/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netcoreapp2.2/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netcoreapp3.1/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/netstandard1.0/ConsoleApp2.dll
  ConsoleApp2 -> /Users/khellang/Code/ConsoleApp2/ConsoleApp2/bin/Debug/net48/ConsoleApp2.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:02.12

I think we have a winner 😅

[gfoidl]

Yep 👍

Just verifyed the same.

ls -laR obj/Debug (and cleaned up a bit) gives:

drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 net48/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 net5.0/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netcoreapp2.1/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netcoreapp2.2/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netcoreapp3.0/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netcoreapp3.1/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netstandard2.0/
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:21 netstandard2.1/

./net48/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 58852 Mai 28 14:18 MimeTypes.NullableAttributes.cs

./net5.0/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 57754 Mai 28 14:18 MimeTypes.cs

./netcoreapp2.1/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 58852 Mai 28 14:18 MimeTypes.NullableAttributes.cs

./netcoreapp2.2/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 58852 Mai 28 14:18 MimeTypes.NullableAttributes.cs

./netcoreapp3.0/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 57754 Mai 28 14:18 MimeTypes.cs

./netcoreapp3.1/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 57754 Mai 28 14:18 MimeTypes.cs

./netstandard2.0/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 58852 Mai 28 14:18 MimeTypes.NullableAttributes.cs

./netstandard2.1/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes:
total 0
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:18 ../
drwxr-xr-x 1 gfoidl 197121 0 Mai 28 14:18 5.0.0/

./netstandard2.1/NuGet/C98C7C07DD58E9A32C3D6821062323CFEEFEA854/MimeTypes/5.0.0:
total 60
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ./
drwxr-xr-x 1 gfoidl 197121     0 Mai 28 14:18 ../
-rw-r--r-- 1 gfoidl 197121 57754 Mai 28 14:18 MimeTypes.cs

Brillant idea 💯

[gfoidl]

Side question: should there be CI to verify this on build? Although for this simple package I think that would be more like shooting with cannons at sparrows.

[khellang]

Side question: should there be CI to verify this on build? Although for this simple package I think that would be more like shooting with cannons at sparrows.

Yeah, it probably should. I just haven't bothered setting it up for this small project 😅

Just pushed 2.0.1 to NuGet 👍

[gfoidl]

Just pushed 2.0.1 to NuGet

Thank you very much! My live-project (from the warnings above) is now happy, too 😄

I just haven't bothered setting it up for this small project

TBH: I wouldn't have set it up too, for the same reason. Further changes are infreqeunt, so manual tests / review will do the job too -- except in this case, but may you can keep your test-setup (or check it in), so it's easy to repro / reuse.