rsdn / CodeJam

Set of handy reusable .NET components that can simplify your daily work and save your time when you copy and paste your favorite helper methods and classes from one project to another
MIT License
258 stars 35 forks source link

Nullable reference types support #129

Closed andrewvk closed 3 years ago

andrewvk commented 3 years ago

Feel free to apply changes from previous PR.

andrewvk commented 3 years ago

One more interesting point. R# automatically removes its own CanBeNull markup when adding "?" to a type. @NN---

NN--- commented 3 years ago

I paid attention to that:) As I said my concern is old ReSharper versions without good NRT support.

NN--- commented 3 years ago

@andrewvk So, do we agree that all ReSharper nullability attributes will be replaced with .NET nullability attributes ?

NN--- commented 3 years ago

@andrewvk Can we merge what we have now ?

andrewvk commented 3 years ago

There is some issue with nullability and autogenerated code. By default, C# compiler treat all generated code as #nullable disabled, regardless of project settings. In that case you should specify #nullable enabled explicitly. I fixed this in base T4 template, but did not rerun all templates. So, after I did it we still have hundreds of nullability warnings and errors (see last commit). image

NN--- commented 3 years ago

Ok. I can fix that too. There is some problem with build time limit . Is there any solution for this ?

andrewvk commented 3 years ago

If there is one, I don't know it :(

NN--- commented 3 years ago

I think GitHub actions can handle this. Need to try.

NN--- commented 3 years ago

@andrewvk in my project I added NAssert.NotNull which tells compiler to make variable non nullable and avoid useless ! . Do you think it worth adding it?

andrewvk commented 3 years ago

Can you explain in more detail what you mean by that? In any case, using ! in most cases is definitely not the best option.

NN--- commented 3 years ago

This assertion makes this code compiling

string? f();
void g(string) {}

string? a = f();
NAssert.NotNull(a);

// a here is not null
g(a); // OK
using System.Diagnostics.CodeAnalysis;
using NUnit.Framework;

namespace Tests
{
    public static class NAssert
    {
#pragma warning disable CS8777 // Parameter must have a non-null value when exiting.
        // The warning is due to fact we do not check for null and throw directly.

        #region NotNull

        /// <summary>
        /// Verifies that the object that is passed in is not equal to <see langword="null" />. Returns without throwing an
        /// exception when inside a multiple assert block.
        /// </summary>
        /// <param name="anObject">The object that is to be tested</param>
        /// <param name="message">The message to display in case of failure</param>
        /// <param name="args">Array of objects to be used in formatting the message</param>
        public static void NotNull([NotNull] object? anObject, string message, params object?[]? args)
        {
            Assert.NotNull(anObject, message, args);
        }

        /// <summary>
        /// Verifies that the object that is passed in is not equal to <see langword="null" />. Returns without throwing an
        /// exception when inside a multiple assert block.
        /// </summary>
        /// <param name="anObject">The object that is to be tested</param>
        public static void NotNull([NotNull] object? anObject)
        {
            Assert.NotNull(anObject);
        }

        #endregion

#pragma warning restore CS8777 // Parameter must have a non-null value when exiting.
    }
}
NN--- commented 3 years ago

Btw, IDictionary doesn't require "TKey : notnull" according to sources. https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IDictionary.cs

It means adding this constraint is not correct.

andrewvk commented 3 years ago

You mean that NUnit still doesn't have a NotNull attribute markup? Yeah, that's pretty weird. Of course, in this case it's worth adding your own variant.

andrewvk commented 3 years ago

image

NN--- commented 3 years ago

Yeah. I tried to reproduce this in a separate project but everything compiled. Probably it requires this constraint only for specific framework. Currently everything compiles but not in appveyor:( I’ll check how to improve it.

andrewvk commented 3 years ago

Core 3.0 library image Core 5.0 library image Shall we try to hide it under a compiler directive?

andrewvk commented 3 years ago

`#if NETCOREAPP3_1 where TKey : notnull

endif`

Build OK

NN--- commented 3 years ago

It is better to not introduce additional constraint if possble

NN--- commented 3 years ago

Fixed recursion and now the build works :) @andrewvk Is there anything left in PR ?