Add attributes to Assert.NotNull method to tell compiler input is checked for null.

[manfred-brands]

Partially fixes #3376

[Dreamescaper]

Please take a look at this comment.
#3376 (comment)

Such attributes were never added, because NUnit does not guarantee that it will fail in case of null argument - because it might be inside Assert.Multiple block.

[manfred-brands]

@Dreamescaper Yes, I know NUnit doesn't throw inside an Assert.Multiple. The use case for that would be to test multiple variables for different values. I don't see a use case for an Assert.Multiple where one would assert a variable to be not null and then use it.

Yes for:

Assert.Multiple(() =>
{
    Assert.IsNotNull(variable1);
    Assert.IsNotNull(variable2);
    Assert.IsNotNull(variable3);
}

But not:

Assert.Multiple(() =>
{
    Assert.IsNotNull(variable1);
    Assert.IsNotNull(variable1.field1);
    Assert.IsNotNull(variable3);
}

[jnm2]

The team needs to have a conversation if we want to change the decision made at #3473 (comment). That's something I'm open to thinking about because I'm amenable to arguments based on multiple asserts being a sufficiently negligible pitfall. There would then be a lot of new annotations to add, including [MaybeNull] on the Assert.Null methods, [DoesNotReturn] on Assert.Fail, and multiple [NotNullIfNotNull(...)] on equality asserts.

[jnm2]

Note this documentation I added to explain why we decided not to add [NotNull].

[jnm2]

@rprouse Where would you recommend that we have this discussion?

[rprouse]

@jnm2 let's have a side discussion in Slack then bring it back here after we chat.

[jnm2]

I just noticed these new extension method Assert APIs. This is something we should also discuss. If we add these, we should add T : struct copies of these methods.

[manfred-brands]

If we change the language version to C#9 we can drop the T : class constraint and it will work for both.

[jnm2]

The return type would not be as convenient if we did that though. T? means "defaultable" rather than "nullable" if there is no class or struct constraint.

Having to do nullableValueType.AssertNotNull().Value would be unpleasant to me since we can avoid making the .Value part (logically an assertion itself) necessary.

[manfred-brands]

I stand corrected. Thanks. Disappointing that even though the return type is T it still returns T? I thought C#9 fixed this.

As constraints are still not part of the signature we cannot have two versions, one with where T : class and one with where T : struct. However, I managed to make the 2nd by explicitly declaring it as:

    public static T AssertNotNull<T>(this Nullable<T> source)
        where T : struct

[jnm2]

We actually can have both versions with the same name (method overloads) because the signature is still different for the T : struct version when the parameter is T?. Nullable<T> and T? are only different at the syntax level when T : struct, not different in meaning.
This is how I did it for Shouldly: https://github.com/shouldly/shouldly/blob/master/src/Shouldly/ShouldlyExtensionMethods/ShouldBeNullExtensions.cs#L21

The reason that C# 9 didn't fix this the way people hoped was that there is no sequence of bytes the compiler can emit that would work that way. The .NET runtime would have to gain a new feature and have a new way to encode ECMA-335 metadata that would not be compatible with previous runtimes and versions.
(The ability to call overloads like this is a newish C# compiler feature IIRC, but IIRC it was backported to all previous language versions. Don't quote me on this.)

[manfred-brands]

You are correct again. Thanks.

[jnm2]

No worries, glad I can help.

[rprouse]

I had a long discussion with @jnm2 about this in Slack. I'm okay with changing the legacy asserts in this way, just not adding new ones, so that part isn't a problem. My issue is that this is a partial fix that cannot have a complete fix within the framework. If we do this for Assert.NotNull in the framework, then people will expect it to work for Assert.That(actual, Is.NotNull) and might also expect it to work in multiple assert blocks.

I think that we could handle all of these things in the Analyzer project, so it is the best place for that. Unfortunately, there are also some technical difficulties there, so let's leave this PR open for now and see how the work goes in the Analyzer project.

nunit/nunit.analyzers#157

I'll post this comment to the issue too so that if we close this PR, it will also be tracked there.

[manfred-brands]

@rprouse I have implemented the DiagnosticSuppressor in NUnit.Analyzers and it works a charm.