search

nhibernate / fluent-nhibernate

Fluent NHibernate!
BSD 3-Clause "New" or "Revised" License
1.66k stars 686 forks source link

Add Nullable to Automapping #678

Open bethmaloney opened 4 months ago

bethmaloney commented 4 months ago

This PR begins the process of adding nullability support to fluent-nhibernate.

Nullable Reference is a compiler feature that provides warning of potential null reference errors. This feature has been enabled by default since .NET Core 6 and many 3rd party libraries have since added support for this feature.

I've began by enabling Nullable in the Automapping folder and resolving any errors. Most of the work was simply adding annotations but I did occasionally have to add a null check or refactor the code to allow the static analyser to determine that something was not nullable.

You'll note that there is a few Activator.CreateInstance(type)! with the null forgiving operator. Activator.CreateInstance will only return a null for nullable value types which we never activate (they're all classes) so it's safe to use the null forgiving operator.

This PR requires #668 to be merged in as .Net framework/standard does not include the nullability attributes in the standard library so we can't add support without adding a third party NuGet library that adds these attributes to the lower versions.

I'm using the pre-processor directive

#if USE_NULLABLE
#nullable enable
#endif

This allows us to disable nullability for release builds until the migration is complete. Once the migration is complete we can remove the directives.

Let me know if there's any issues with this approach. If there's not I'm happy to migrate the rest of fluent-nhibernate over.

bethmaloney commented 4 months ago

DeepSource is failing due to a code smell in an area of code I haven't modified. Not sure if I should be fixing the issue or if we can ignore it?

hazzik commented 4 months ago

Thanks for the contribution.

This PR requires https://github.com/nhibernate/fluent-nhibernate/pull/668 to be merged

No, it is not needed. The project uses Polyfill that provides all required attributes.

bethmaloney commented 4 months ago

My understanding is that polyfill will make the nullable attributes available but don't add them to the base library. Eg in .net 8 string.IsNullOrEmpty will mark a string as being non null. However in .net 4.6 this won't happen as the function is missing the required attributes.

We'd need some sort of package that could go and rewrite/add the attributes to the base library. Otherwise we'll get nullability errors in earlier versions of .net.