Support c# 8 nullable reference

[iron9light]

Just found this dotnet/standard#1359 .
Maybe this lib can do similar things for c# 8 nullable reference.

[jnyrup]

This is indeed an interesting area, which I'm following closely.
For anyone else interested here a blog describing the features.

If we add this, it should align with #1074.
As these Attributes will be available from netcoreapp3.0, we would need to think about which frameworks we target.
Implementing #902 would e.g. require netcoreapp2.1 or netstandard2.1.

Related issues about nullability:
#1058
#1039

[iron9light]

I don't know the implementation details.
But I do know that: netstandard 2.0 will be the main target framework for most live libraries and projects since the .Net Framework will not support netstandard 2.1 .
And many people will use 8.0 with netstandard 2.0, not everyone will use Resharper, it's not free.
Support nullable reference should be a must-have (if it's possible). obj.Should().NotBeNull() is too common in the test code.

[jnyrup]

Be aware that these attributes will also not be available in .netstandard 2.0.
See Philip Carters comment.

[iron9light]

Hey Jonas, The same as with .NET Framework. Async enumerables, Index and Range support, Default Implementations, and Nullable Annotations in first-party libraries.

Do you mean this? There're some BCL libraries, e.g. https://www.nuget.org/packages/Microsoft.Bcl.AsyncInterfaces . Maybe there will be one for those attributes.

[jnyrup]

This comment:

... It's currently a bit tricky when multitargeting. We've updated the specification for .NET Standard 2.1 to include all of the more advanced attributes so that you can share that code across .NET Core and Xamarin. But the .NET Standard 2.0 spec won't be updated, so if you use that then you won't have access to these types. Similarly, since they aren't on .NET Framework, any specific target for net4X will not see these types.

If I we're to add the attributes, I would probably wait until there is more information available about how to use them in multi-targeting libraries.

[dennisdoomen]

And apparently some folks managed to get it to work with .NET Standard. See https://twitter.com/ddoomen/status/1160171490081419271

[danielmpetrov]

So now that .NET Core 3.0 has officially launched yesterday, I was watching the "What's new in C# 8.0 - Part 1" by C# Language Designer Mads Torgersen, and towards the end (2:19:09) he mentions library writers.

He recommends multi-targeting (FA already is), and manually enabling C# 8 by editing the project file. Not all features are guaranteed to work, but nullable reference types should, in his words, work just fine.

I, personally, am very excited for this feature

The clip I'm referring to (with timestamp) is this one: https://www.youtube.com/watch?v=W8yL8vRnUnA&t=8349

[dennisdoomen]

@jnyrup do we need to do anything here?

[jnyrup]

I did play with it in https://github.com/jnyrup/fluentassertions/commits/nullableReferenceTypes
As nullability is not part of the method signature we should be able to do this after shipping 6.0 without introducing breaking ABI/API changes.

It should at most be able to give compile-errors for FA users if we change an annotation which produces a warning and they have WarningsAsErrors enabled.

[POnakS]

I think the clue of this is about not annotations on types, but about attributes. I.e.

variable.Should().NotBeNull();
variable[5].Should().Stuff(); // variable may be null is generated here

[scott-moxham]

I'm having exactly the same issue with the same pattern as @POnakS.
It's frustrating that I'm having to add a lot of ! in my code after checking for null.

[BEagle1984]

We are having the same issue with nullable reference types. Fixing it via attributes isn't trivial as the Should().NotBeNull() construct makes it impossible to properly set the needed attributes.

My proposal is to create an extra extension method ShouldNotBeNull() and properly decorate it with the NotNullAttribute etc.

using System;
using System.Diagnostics.CodeAnalysis;
using FluentAssertions.Primitives;

#nullable enable

namespace FluentAssertions
{
#if NETCOREAPP3_0

    public static class AssertionNullableExtensions
    {
        /// <summary>
        /// Asserts that the current object has been initialized.
        /// </summary>
        /// <param name="because">
        /// A formatted phrase as is supported by <see cref="string.Format(string,object[])" /> explaining why the assertion
        /// is needed. If the phrase does not start with the word <i>because</i>, it is prepended automatically.
        /// </param>
        /// <param name="becauseArgs">
        /// Zero or more objects to format using the placeholders in <paramref name="because" />.
        /// </param>
        public static AndConstraint<ObjectAssertions> ShouldNotBeNull(
            [NotNull] this object? actualValue,
            string because = "",
            params object[] becauseArgs)
        {
            var result = actualValue.Should().NotBeNull(because, becauseArgs);

            if (actualValue == null)
            {
                throw new ArgumentNullException(nameof(actualValue)); // Will never be thrown, needed only to trick the compiler
            }

            return result;
        }
    }

#endif
}

This extension properly shutdowns the warning.

object? someObject = null;
someObject.ShouldNotBeNull();
var result = someObject.ToString(); // No warnings here

I know this isn't perfectly fitting into the API but seems like an acceptable compromise to me. WDYT?

Let me know if it sounds good and I'll drop a PR (code is ready).

EDIT: cc @jnyrup

[BEagle1984]

See draft PR -> #1689

[dennisdoomen]

This also works using the example test from #1689

/// <summary>
/// Returns an <see cref="ObjectAssertions"/> object that can be used to assert the
/// current <see cref="object"/>.
/// </summary>
[Pure]
public static ObjectAssertions Should([System.Diagnostics.CodeAnalysis.NotNull] this object actualValue)
{
    return new ObjectAssertions(actualValue);
}

[BEagle1984]

Yes @dennisdoomen, it works but it’s a lie. Calling Should doesn’t guarantee that actualValue will not be null afterwards.
You didn’t enable the NRT and/or the actualValue argument isn’t declared as nullable, that’s why you don’t get a warning.

[dennisdoomen]

But when you use FA that's not relevant. That's why you use it in the first place.

[BEagle1984]

I see what you mean but it's wrong in my opinion. You are just suppressing the compiler null check without actually checking for null on your part. It's not very correct.
I know it's kind of a special case, I know "it's just test code", but I think you shouldn't allow for cases where a NullReferenceException is thrown at runtime if NRT is enabled.

User? user = null;
user.Should().NotBeOfType<SuperUser>(); // This wrongly disables the null check
user.IsEnabled.Should().BeTrue(); // This throws a NullReferenceException

[dennisdoomen]

But all FA APIs are already prepared to check for that. So if user would be null, NotBeOfType<SuperUser>() would throw with the appropriate exception.

So although I see what you're trying to accomplish, I don't think with the approach.

[BEagle1984]

If would have thought that NotBeOfType wouldn't consider null a violation. Nice to know.

The principle is still valid. The following code throws a NullReferenceException (this time around I actually tried it before posting).

[Fact]
public void Test1()
{
    User? someObject = null;
    User? someOtherObject = null;
    someObject.Should().BeSameAs(someOtherObject);
    someObject.IsEnabled.Should().BeTrue();
}

As said in the first post, I understand that my proposed solution is not really in line with the FA API. I also don't like it a lot.
I unfortunately don't see a better alternative though and manually suppressing the null check is just annoying.

It's not a big deal and we can in the worst case create this ShouldNotBeNull extension on our side.

If you can think of a better way to fix this, even better.

[dennisdoomen]

The following code throws a NullReferenceException

And this time it's correct, since you're trying to get a property from a null object. Or are you saying that the hacky Should I proposed earlier would disable the null reference check on the last line? If so, I see your point, but I still don't see the value of polluting the FA API to work around that. In the end it's a unit test, and it'll fail. It's not like it's going to affect the production code.

[BEagle1984]

Yes, I mean that your hack in the Should method disables the null reference check at compile time and I therefore disagree with it. I understand it might not be important to you and "it's just test code" but let me decide on my own. If I don't care about he check at all I can disable the NRT in the test project.

And again I understand your point about the API pollution. I unfortunately don't have a better idea to solve this annoyance without rethinking the API.
For sure, as a user of your lib, it's a bit annoying that this isn't properly handled.

[dennisdoomen]

I respect your choices and decisions. But it's a combination of the API choices we made in the past and the way Microsoft decided to implement nullable reference types. For now, I don't see any solution I feel comfortable with, other than just disabling by making Should disable it.

[BEagle1984]

I see. I totally understand your point of view. In this case, I'd just leave it alone. 😉

[CzBuCHi]

Not sure if it belongs here, but found another construct that need some love:

object value = Func(); // usually from nuget package without nullable enabled ...

value.Should().BeNull();  // here i get 2x possible null reference exception:
// value can be null
// value.Should() can be null

currently i can 'fix' it by calling it like this:

value!.Should()!.BeNull(); // or value?.Should()?.BeNull(); 

but that just ... wrong

'simplest' fix would be to turn on nullable types in 'csproj' and fix all warnings that will popup ...

[scott-moxham]

@CzBuCHi your alternative:
value?.Should()?.BeNull();
gets rid of the warnings, it will not do what you want.
If value is null, then the assertion for null will never be executed because of the ?.

[CzBuCHi]

@scott-moxham oh .. i didnt noticed that one .... good catch ....
still think that both ! could be fixed by using nullable types in library:

-public static ObjectAssertions Should(this object actualValue) {
+public static ObjectAssertions Should(this object? actualValue) {
        return new ObjectAssertions(actualValue);
}

[0xced]

I regularly stumble on this when writing tests and would love to see @dennisdoomen's proposal to add [System.Diagnostics.CodeAnalysis.NotNull] to Should implemented in FluentAssertions. I also agree that in the end it's a unit test, and it'll fail. 🤷

Also, this post by Brad Wilson might be relevant. (It might also not be relevant, I haven't fully investigated.)

omg I have to add this: (I wish I was kidding)

dotnet_code_quality.CA1062.null_check_validation_methods = Xunit.Internal.Guard.ArgumentNotNull``1(``0,System.String)|Xunit.Internal.Guard.ArgumentNotNull``1(System.Nullable{``0},System.String)|Xunit.Internal.Guard.ArgumentNotNull``1(System.String,``0,System.String)|Xunit.Internal.Guard.ArgumentNotNullOrEmpty``1(``0,System.String)|Xunit.Internal.Guard.ArgumentNotNullOrEmpty``1(System.String,``0,System.String)

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1062#null-check-validation-methods

[bkoelman]

I'm having a hard time selling FA to my team because of this. It requires thousands of tests be sprinkled with ! suppressions, whereas the currently used Assert.NotNull doesn't require so. This is considered extremely annoying because the team aims to keep the use of warning suppressions in the codebase to a minimum.

From a technical perspective, adding NotNull to the Should() argument is lying. But from a practical point of view, in the eyes of FA consumers, it's the sane thing to do. The compiler doesn't flow the nullability from .Should().NotBeNull(), and I don't think that's going to change.

In another project with another team, we ended up adding dozens of methods like ShouldNotBeNull, ShouldHaveCount, ShouldContain etc, just to avoid the immense need for !. While it works, it kinda defeats the idea to "just use" FA and it's a pain to maintain. It requires all test projects to reference a shared test-utilities project that defines these. For newcomers this pattern is perceived as confusing: sometimes they need to use our methods and sometimes the FA built-in ones.

[cremor]

I agree that a "real" solution in FluentAssertions would be great, but what's so bad with disabling the CS8602 diagnostic in test projects?
If something is null you'll see it because a test is failing. And if it's after an Should().NotBeNull() call (which is the only thing FluentAssertions can improve) it will never be reached anyway.

[POnakS]

Basically the same as for your porduction code. Why bother when you can run the code and see that there is null reference there?

Bot jokes aside, you want this inspection in all your code, and especially in your tests - as your test code is not tested.

[CzBuCHi]

as your test code is not tested.

you can verify your tests with dotnet stryker ... or more precisely stryker will tell you what parts of your code you can change without breaking any tests (aka untested code)

[0xced]

I just submitted #2380 that will address this issue.

Here's a compelling example:

[Theory]
[InlineData(true)]
[InlineData(false)]
public void Test1(bool currentUser)
{
    IPrincipal principal = currentUser ? new WindowsPrincipal(WindowsIdentity.GetCurrent()) : new ClaimsPrincipal();
    IIdentity? identity = principal.Identity;
    
    identity.Should().NotBeOfType<GenericIdentity>();
    identity.IsAuthenticated.Should().BeFalse();
}

This test fails (as expected) with the following errors:

• currentUser=true

Expected identity.IsAuthenticated to be false, but found True.

• currentUser=false

Expected identity not to be System.Security.Principal.GenericIdentity, but found <null>.

But most importantly, the CS8602 warning is gone (Dereference of a possibly null reference). No more need to rely on Assert.NotNull to make the warning disappear. 🥳