Annotate nullable reference types in public APIs

[daniel-lerch]

Is your feature request related to a problem? Please describe.
I just came across a NullReferenceException in my app which uses nullable reference types because I did not read MimeKit's documentation properly and missed that a type can be null.

Describe the solution you'd like
I consider nullable reference types a great feature of C# that makes you as a developer think twice whether you can really omit a null check. In my opinion it would be great if MimeKit would at least add nullable attributes to its public API. It might even make sense to enable nullable reference types for the entire code base.

Describe alternatives you've considered
N/A

Additional context
N/A

[jstedfast]

The problem is that I'm still trying to support .NET Framework which doesn't have this.

I'm not saying it can't be done, it's just a lot of work to #if/#else/#endif everything.

[Sicos1977]

As far as I know this should work in .net framework. You probably have to upgrade to language version 8 or higher.

[daniel-lerch]

I can confirm that nullable reference types as such do work with .NET Framework. Some very useful attributes like [NotNullWhen] or [MemberNotNull] require .NET Standard 2.1 or .NET 5.0.

I enabled this feature for some classes in #1068 and added #if statements to add attributes for more modern .NET versions.

I would be willing to contribute further pull request to enable nullable reference types for more classes. Enabling for the entire project would be too much work at once for me, though.

[jstedfast]

FWIW, I created a new branch for this work. I also added compatibility attributes to reduce #if/#else's littering the codebase.

I just need to find time to work on it more, haha.

I wonder if I should look into writing a Roslyn-based tool to automate this.

[daniel-lerch]

I continued to enable nullable reference types in more classes and am more than halfway through.

In some places static code analysis cannot ensure a type is not null. In these cases I used the null forgiving operator ! and added a comment why this value cannot be null.

Now I came accross an internal class ParameterList.NameValuePair which does not have a constructor.

1. The idiomatic solution whithout changing the code style would be to use required properties. These are not supported by all targeted .NET versions but PolySharp could help out. It also includes nullable attributes.
2. The easy alternative for these internal classes would be to add constructors with all required members.

Which way do you prefer @jstedfast?

[jstedfast]

Looks like ParameterList.cs line 882 is the only location that calls the .ctor, so it would probably be easier to just add a public .ctor to NameValuePair that took all the arguments.

[daniel-lerch]

More than halfway through was probably a bit optimistic. Some classes are not easy to get annotated without excessive usage of null forgiving operators (!) which I want to avoid.

MimeKit contains quite a few internal event EventHandler<T> .... Is there a specific reason to use EventHandler which requires type casts from object back to the actual sender type? InternetAddressList.Changed sender for instance is never null but with EventHandler it is not possible to put this into nullable attributes. Accordingly, MimeMessage.InternetAddressListChanged will complain that sender may be null. Shall I use null forgiving operators in these cases or create an EventHandler<TSender, TEventArgs> or directly use internal event Action<InternetAddressList> Changed?

[jstedfast]

I'd probably prefer EventHandler<TSender, TEventArgs> over internal event Action<InternetAddressList>

[daniel-lerch]

One more question: Many classes in MimeKit contain fields that are non-null except for when they get disposed e.g. FilteredStream.filters or MemoryBlockStream.blocks. I could either try to make these fields nullable and use [MemberNotNull (nameof (field))] on the CheckDisposed method. This would continue to enable us to set fields to null and the compiler will help to find mistakes. The other option would be make these fields non-nullable, not set them to null in Dispose and instead add unit tests to ensure that these fields are not accessed anymore and the class throws ObjectDisposedException for all methods. Which approach do you prefer?

[jstedfast]

Probably the latter - don't set them to null.

What would be your recommendation?