S2955 FP with value types

SonarQube
,
1

Hi, I have a problem with rule S2955. We have a method like this:

    private bool AreEqual<T>(T expected, T actual)
    {
        if (expected == null && actual == null)
        {
            return true;
        }

        return expected != null && actual != null && expected.Equals(actual);
    }

This method is checking if both objects are the sames, if it’s reference type, then we need to check nulls, if it’s value type, then first if will be false all the time. So it works fine for both object types.
If we refactor this method to suggested solution it will looks like this:

    private bool AreEqual<T>(T expected, T actual)
    {
        if (EqualityComparer<T>.Default.Equals(expected, default(T)) &&
            EqualityComparer<T>.Default.Equals(actual, default(T)))
        {
            return true;
        }

        return EqualityComparer<T>.Default.Equals(expected, default(T)) is false && EqualityComparer<T>.Default.Equals(actual, default(T)) is false && expected.Equals(actual);
    }

But it doesn’t work the same, now if we compare two ints 0 and 0 it will return immediately from first if. It still works, but it will works differently. We have other examples of methods, when we need to check if generic parameter is not null and generic parameter can be reference or value type.
In general what do you suggest to do in scenario when we have method with generic parameter, which can be value and reference type and we need to check against null?
Best regards,
Lukasz.

2

Base on your original you could end up with this:

private bool AreEqual<T>(T expected, T actual)
    => expected is null
	    ? actual is null
	    : expected == actual;

But better, and even shorter, just like the suggestion:

private bool AreEqual<T>(T expected, T actual)
    => EqualityComparer<T>.Default.Equals(expected, actual);

Just my 2cts.

3

In first solution we still comparing T to null, so it will throw warning, since T can be reference or value type.

In second solution we will change the behavior, in this case it will work, but in general what to do if we need to check null reference on generic type, which can be value or reference type.

For example we have a method like this:

 public static T GetMandatoryValueOrFail<T>(this Context ctx, Expression<Func<Context, T>> expression)
where T : class
 {
     var compiledExpression = expression.Compile();
     T result = compiledExpression.Invoke(ctx);
     if (result == null) { throw new ArgumentNullException($"Value is null in path {expression}!"); }
     return result;
 }

Do you have any suggestions how to refactor this method to avoid S2955 warning?

4 
public static T GetMandatoryValueOrFail<T>(this Context ctx, Expression<Func<Context, T?>> expression)
where T : class
{
	var compiledExpression = expression.Compile();
	return compiledExpression.Invoke(ctx)
		?? throw new ArgumentNullException($"Value is null in path {expression}!");
}

So, the specified expression can return something that is null, that means that should match that.

I do not see why (now with nullable annotations) should behave different.

private bool AreEqual<T>(T? expected, T? actual)
    => EqualityComparer<T>.Default.Equals(expected, actual);

In fact, the contract of an EqualityComparer is exactly what the signature of your method is.

6

The first suggestion has a constraint that T needs to be a class, so I can’t use it for int? and int for example, but I need to.

The second suggestion will work differently, because if I use int as T, EqualityComparer.Default will be 0.

7

Hello @Lukasz_Granica and @Corniel!

I read through your comments and I think that the last suggestion from @Corniel was really valid:

bool AreEqual<T>(T? expected, T? actual) => 
    EqualityComparer<T>.Default.Equals(expected, actual);

It seems to me that the results are equal to your original method, both with value and reference types, I created this snippet to play a bit, feel free to use it.
Can you please clarify for which particular input this method will work differently?

9

AreEqual method will have the same result, but it works a liitle bit different. It’s visible in this snippet, I created a method than checking null and if not null then returns true. Both methods returns different results for arguments (0,0), please take a look.

10

I’m a bit confused.
I don’t think that the snippet you provided is doing what you expect.
The IsNullOld method should return the opposite bool, something like:

bool IsNullOld<T>(T expected, T actual) => expected == null && actual == null;

Regarding:

AreEqual method will have the same result, but it works a little bit different

If both have the same results for the same set of inputs, then why should you refrain from using the Equality comparer? IMO it’s cleaner, less prone to errors, and will avoid comparing value types with null, all in one bundle :smiley: