NUnit2041 false positives

[chm-tm]

Given test body

float x = 1;
C y = 1.1f;
Assert.That(y, Is.GreaterThan(x));
Assert.That(x, Is.LessThan(y));
object ox = x;
object oy = y;
Assert.That(oy, Is.GreaterThan(ox));

with

struct C : IComparable, IComparable<C> {
    float V { get; }
    public C(float v) => V = v;
    public static implicit operator C(float v) => new C(v);
    public int CompareTo(object obj) => V.CompareTo((float)obj));
    public int CompareTo(C c) => c.V.CompareTo(V);
}

All constraint arguments have false positive NUnit2041 errors.

[manfred-brands]

I have no access to a PC this week (holiday), can you check whether it also fails when C implements IComparable? I presume they all run in unit? Not sure if I want the last one to not complain? It happens to be comparable at runtime, bit of you assign something else to ox/oy it could fail.

[Dreamescaper]

As far as I remember, both those cases were known during implementation, but it was decided that they are rare, and chance of false-positive is very low. Apparently they're not.

@manfred-brands We have object as an exception in lots of analyzers exactly because of relatively high false-positive chance. Probably same could be done here.

[chm-tm]

@manfred-brands Yes it's in NUnit. I've updated (and corrected) the sample to let C also implement IComparable<C>.

[chm-tm]

I'd be OK to go with a #pragma warning disable here, since you should now what you're doing in these rare cases. I actually have had two false positives.

One is in the tests for a float-like struct like C. I corrected the C to float comparison by casting the float variable to C. This makes the test better understandable, since it now tests only the comparison. I can test the conversion in another place.

The other is at a place where I have an IList of values which are all of type float, or a custom float-like struct. I don't know which type it is at compile time. This was just the most convenient way to implement it. In this case, I can circumvent the complaint by declaring the variable corresponding with ox above as IComparable. I also feel that this is an improvement to the test. It's more readable if the constraint argument is IComparable at compile-time. After all, the constraint test does no magic with the two objects, but uses the IComparable interface to do its work.

Based on these occurrences, I'd vote for leaving the rule as it is, but please lower the severity to Warning. For both cases, I could improve them. But I wouldn't say these were defects, as I'd interpret the Error severity.

[manfred-brands]

I noticed that the code cannot handle type parameter constraints (I will fix that):

public static void GreaterOrEqual<T>(T value, T threshold, string? message = null)
    where T : notnull, IComparable<T>
{
    if (value.CompareTo(threshold) < 0)
    {
        Assert.That(value, Is.GreaterThanOrEqualTo(threshold), message);
    }
}

[manfred-brands]

@chm-tm Your first use case "Assert.That(y, Is.GreaterThan(x))" works in nunit (by chance), but is not in the spirit of IComparable which states:

Compares the current instance with another object of the same type

The interface definition even states that the implementation should throw an ArgumentException

obj is not the same type as this instance.

As x is not the same types as y this is explicitly flagged as wrong by the analyzer. One could say your implementation is too flexible. Note that this will not trigger the implicit operator as float matches object. It would be better if your classes would implement IComparable<float> instead of the object version.

The second use case "Assert.That(x, Is.LessThan(y))" fails when running in actual nunit. It eventually runs Single.CompareTo(Object) which fails with an ArgumentException because y is not a float.

It triggers this code in NunitComparer.Compare:

if (x is IComparable)
    return ((IComparable)x).CompareTo(y);

@Dreamescaper I am reluctant to have a fallback for object as the idea of the analyzers is to write better code and dropping down to object and assuming that the object implements IComparable is not right. By forcing you to declare the variable as IComparable you should get a compile time (or runtime cast exception).

[chm-tm]

Thanks for your investigation. After having read it, I'm OK with the analyzer and its severity regarding the use case "Assert.That(x, Is.LessThan(y))".

I'd also leave the current implementation in case of objects. Though I previously voted for lowering the severity to warning by default, the code is certainly more readable with an extra IComparable cast or declaration, if needed, than with a #pragma adornment. Splitting the analyzer into two in order to have different severity isn't worth the effort either.

It would be great to reflect some of the insights of this thread in the NUnit2041 documentation.

[Dreamescaper]

@manfred-brands Considering that this diagnostic has Error severity, I don't agree that its aim is to write better code - it is to block invalid code. And we have no ability to determine whether code is valid or not in case of object arguments.

[mikkelbu]

Sorry for the lack of activity, but I've been (too) busy the last month. On the question of whether we should report an error when comparing object arguments, then I agree with @Dreamescaper and think it makes sense to special case this situation, as we have no knowledge about the runtime behaviour.

I'll see if I can get some of the content of this issue into the NUnit2041 documentation.

[manfred-brands]

I actually just made time to address this, like @mikkelbu I was too busy.