Allow more string comparisons

[manfred-brands]

Fixes #4898 for strings:

1. Types that can be implicit cast to string
2. Types that implement IEquatable

Fixes #4898 for types implementing both IEquatable and IConvertible. The latter path is only chosen if the actual type is neither a primitive nor decimal. Otherwise IEquatable.Equals is called.

[stevenaw]

I'm a little conflicted here myself but I'm tempted to say we fall back to a non-generic implementation here just in case there's something we're not considering. @OsirisTerje @jnm2 what are your thoughts?

[OsirisTerje]

I would say fallback to the non-generic. I believe we might be struggling with this over time, and then that fallback might "save the day".

[stevenaw]

Thanks @manfred-brands this looks great and my attempts to think up edge cases not covered have been futile. I think it looks pretty solid. I noticed you were asking for thoughts on how to handle the "last" case and thought it might be good to hear from @OsirisTerje or possibly @jnm2 if either are free

[smdn]

I used code from this branch to get my test suite running.
The problems I had experienced with NUnit 4.3.0 did not occur and all test cases worked as expected!

[OsirisTerje]

If all repros work with these changes, I am fine with them.

It also looks like we have got these cases into the test suite, which is awesome. #4902 also look like something we should add to the test suite here.

---

I do wonder though that we treat the expected and the actual different. The expected is being used as a parameter to the construction of the constraint, and that is when the implicit operator is being called. That applies to both classes and structs. The actual however, is only passed into the generic ApplyTo method, and thus the implicit operator is not being called. So we are comparing apples to bananas, which fails.

If we did have a class containing the actual value, with overloads matching what the constraint class have, the implicit operator would be called and we would be able to compare, comparing apples to apples as we should.

I don't mean that we should look into something like this, at least not now, but just for thoughts.

[moshekar]

I do wonder though that we treat the expected and the actual different. The expected is being used as a parameter to the construction of the constraint, and that is when the implicit operator is being called. That applies to both classes and structs. The actual however, is only passed into the generic ApplyTo method, and thus the implicit operator is not being called. So we are comparing apples to bananas, which fails.

If we did have a class containing the actual value, with overloads matching what the constraint class have, the implicit operator would be called and we would be able to compare, comparing apples to apples as we should.

I agree. If you can make ApplyTo to simply use the generic TActual, all conversion will happen automatically by the compiler and all the issues raised from 4.3.0 will be resolved.
I assume you wanted to allow more functionality for strings (ignore case, etc.) so you had to add the new overload. But just a thought, maybe you can use the new string features in the generic ApplyTo in case typeof(TExpected) == typeof(string).
I assume this will have all the benefits and also allow new functionality for specific types.

[OsirisTerje]

Suggest we push this out as a 4.3.1 when ready.

PS. I ran the 4.3.0 through some large solutions we have before the actual release, no faults detected. One day after release and things starts trickling in. Just show the importance of getting different people from different places and different organizations to check things out :-) Thanks @moshekar @smdn @alkampfergit

[manfred-brands]

@OsirisTerje

I do wonder though that we treat the expected and the actual different. The expected is being used as a parameter to the construction of the constraint, and that is when the implicit operator is being called. That applies to both classes and structs. The actual however, is only passed into the generic ApplyTo method, and thus the implicit operator is not being called. So we are comparing apples to bananas, which fails.

Yes. We have raised this before the decoupling of the TActual and IConstraint. Both are indepedently resolved by the compiler.

If we did have a class containing the actual value, with overloads matching what the constraint class have, the implicit operator would be called and we would be able to compare, comparing apples to apples as we should.

I tried adding an overload That(string actual, EqualStringConstraint constraint, ...), but that constraint is not picked because the generic TActual is a better fit which doesn't require an implicit conversion.

If I also change That<TActual>(TActual actual into That(object? actual, I have more success.
Now both the Is.EqualTo and the Assert.That have both a string? and object? overload.
This allowed me to remove the code added in this PR to check if I can implicitly cast actual to string as the compiler does that.

However that is a binary breaking change: Removing a method.

It also broke 2 of our own NUnit tests.
One where an declared IList was passed in, but the value was initialized with an array.
When passed using generics, the type is an IList and has a Count property.
When passed as object, the actual type is found to be array which does not have a Count property.

It also triggered a possibly bug in the NUnit2026 rules of the analyzer complaining about non-matching types.

@moshekar

If you can make ApplyTo to simply use the generic TActual, all conversion will happen automatically by the compiler and all the issues raised from 4.3.0 will be resolved.

It won't. The way generics work is that an instance of type TActual only has the capabilities of the constraint but on the generic type. Inside a method using TActual the compiler no longer knows that it has an implicit cast to string.
Consider there is only one instance of this method compiled and it cannot do different things depending on the passed in type.

Neither can I hard-cast in case that triggers an operator.

Even though Convert.ChangeType compiles, it fails are runtime as it only support types that implement IConvertible.

I assume you wanted to allow more functionality for strings (ignore case, etc.) so you had to add the new overload. But just a thought, maybe you can use the new string features in the generic ApplyTo in case typeof(TExpected) == typeof(string).
I assume this will have all the benefits and also allow new functionality for specific types.

The functionality was already available on the generic overload, but that resulted in that function having modifiers that don't make sense. Such as:

Assert.That(2 + 2, Is.EqualTo(4).IgnoreCase);
Assert.That("NUnit", Is.EqualTo("nunit").Within(1e-3));

The extra overloads put into NUnit 4.3.0 means that both of those non-appropriate modifiers now give compile time errors.

[moshekar]

It won't. The way generics work is that an instance of type TActual only has the capabilities of the constraint but on the generic type. Inside a method using TActual the compiler no longer knows that it has an implicit cast to string.
Consider there is only one instance of this method compiled and it cannot do different things depending on the passed in type.

You are correct. I tried a simple method like that and there are cases the compiler can't infer the type. I incorrectly assumed the first argument type will be chosen but that's not the case and it tries to infer from both arguments.
So expected needs to be object?, like this simplified version:

public static bool IsEqualTo<TActual>(TActual? actual, object? expected)
{
	if (actual is null)
	{
		return expected is null;
	}

	if (expected is not null && actual is string str)
	{
		// add special string functionality here...
		return str.Equals(expected);
	}

	return actual.Equals(expected);
}

[manfred-brands]

@OsirisTerje I made the change for the fallback to EqualConstraint and upload any reported tests to the Issue4898 project.

There are actually two separate issues but combining them gives us a proper resolution:

1. EqualNumericConstraint could be called for types that cannot be handled by NUnit's Numerics.AreEqual.
   The reason is we cannot constrain the type to: double | float | long | int | ...
   The fix for this was to add a test on `Numerics.IsNumericType(t) and calls normal Equals otherwise.
   One alternative better fix, is to drop the Generic method and use 11 specific overloads (both in Is.cs and ConstaintExpression.cs) that way we know it will only be called for supported methods.
2. Types that have an implicit operator string. This unexpectedly causes the compiler to select the Is.EqualTo(string?) overload instead of the Is.EqualTo(object?) overload.
   The 'fix' for this was to also call the conversion operator on actual. This means that the values are compared as strings instead of as instances of the type. This is not ideal, it doesn't look we can change the overload behaviour in C#. Generics would be great, but even then C# doesn't allow two generic overloads with different constraints.
   Although asked in 2017. It is not likely to happen.

However, replacing the generic for Numerics with individual overloads, allows us to create an Is.EqualTo<T>(T? instance) for the fallback. Now RedisValue no longer calls the Is.EqualTo(string) overload and compares instances instead of strings.

I have updated the branch with the latest, which I think gives a better solution.
Note that I have removed the call to the implicit operator string.
Or do we want the following to pass, note that it fails in both 4.2.0 and 3.14.0 and we do not consider implicit casts in any other constraint,

        RedisValue redisResult = "42";

        Assert.That(redisResult, Is.EqualTo("42"));

[moshekar]

note that it fails in both 4.2.0 and 3.14.0 and we do not consider implicit casts in any other constraint,

        RedisValue redisResult = "42";

        Assert.That(redisResult, Is.EqualTo("42"));

Good catch :)
In this case I usually do Assert.That(redisValue.ToString(), Is.EqualTo("42")); to enofrce strings comparison, or just compare with a number 42.
But I think users expect Is.EqualTo to work similar to simple equality check. Like these two asserts pass:

RedisValue redisValue = "42";
Assert.That(redisValue.Equals("42"));
Assert.That(redisValue.Equals(42));

[OsirisTerje]

But I think users expect Is.EqualTo to work similar to simple equality check.

It's a good argument

note that it fails in both 4.2.0 and 3.14.0 and we do not consider implicit casts in any other constraint,

but since we haven't supported that earlier, we don't need to add this to this hotfix.

It could be raised as a separate enhancement issue though.

Comments @stevenaw @alkampfergit @smdn ?

[smdn]

@OsirisTerje

While I expected a simple comparison like the behaviour of Is.EqualTo, it is understandable that some people expect this kind of behaviour.
It is therefore difficult for me to decide how Is.EqualTo should behave.

Assert.That(redisResult, Is.EqualTo("42")); // expect to pass

Instead of using overloads or behavioural enhancements of Is.EqualTo, how about using analyser warnings?

Assert.That(redisResult, Is.EqualTo("42"));
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Warning NUnitXXX: 'Ambiguity comparison. Do you expect a comparison between `RedisValue` and `string`? Or expect implicit conversions?'

// If the intention is clear, no warning is given.
Assert.That(redisResult.ToString(), Is.EqualTo("42"));
Assert.That((string)redisResult, Is.EqualTo("42"));
Assert.That(redisResult, Is.EqualTo<string>("42"));
Assert.That(redisResult, Is.EqualTo<int>(42));

MyLong l1 = new(1);

Assert.That(l1, Is.EqualTo(1));
            ~~~~~~~~~~~~~~~~~ Warning NUnitXXX: 'Ambiguity comparison. Do you expect a comparison between `MyLong` and `int`? Or expect implicit conversions?'

// No warning is given
Assert.That(l1, Is.EqualTo(l1));
Assert.That(l1, Is.EqualTo<int>(1)); // performs implicit conversion (calls IConvertible.ToInt32() like with NUnit 4.3.0)

[smdn]

@manfred-brands

It seems that the changes in commit a12d6ab caused my test code to have build error NUnit2021.
This occurs in some of the test codes like following. (Not all, it seems.)
My codes can build successfully at 36b8b1d.

[Test]
public void Test()
{
  MyLong l0 = new(0);

  Assert.That(l0, Is.EqualTo((MyLong)0));
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error NUnit2021: The EqualTo constraint always fails as the actual and the expected value cannot be equal (https://github.com/nunit/nunit.analyzers/tree/master/documentation/NUnit2021.md)
}

I will report again when the situation can be clearly reproduced.

[manfred-brands]

@smdn

I will report again when the situation can be clearly reproduced.

I can reproduce it by adding an explicit cast operator to the MyLong class you previously supplied.

I suspect that the analyzer doesn't deal with the generic EqualTo<T> properly.

[moshekar]

a specific string? overload to enforce compile time check

@manfred-brands , after sleeping on it I got an idea how to forbid IgnoreCase during compile type while still managing the overload selection: If you wrap the string with a custom wrapper that itself has implicit conversion from string - it works :)

To do this, replace the string? overload with a custom StrictString overload, which is simply:

public record struct StrictString(string Value)
{
	public static implicit operator StrictString(string Value) => new(Value);
}

Results:

[manfred-brands]

@moshekar Thanks, you remined me that I needed the NUnitString class for similar reasons.

However, StrictString doesn't work because I now have the generic catch all method which is chosen in preference of any method requiring a conversion.

So with the current state of the branch only actual string values will cause this overload.

Do your RedisValues test pass as in my case the all fail:

[Test]
public void RedisValueString()
{
    // Arrange
    RedisValue value = "42";

    // Act & Assert
    using (Assert.EnterMultipleScope())
    {
        Assert.That(value, Is.EqualTo(value), "RedisValue should support comparisons with itself.");
        Assert.That(value, Is.EqualTo<int>(42), "Is.EqualTo<int>(42) -> EqualConstraint");
        Assert.That(value, Is.EqualTo(42), "Is.EqualTo(42) -> EqualNumericConstraint");
        Assert.That(value, Is.EqualTo("42"), "Is.EqualTo(\"42\") -> EqualStringConstraint");
    }
}

Fail with:

   Multiple failures or warnings in test:
  1)   Is.EqualTo<int>(42) -> EqualConstraint
Assert.That(value, Is.EqualTo<int>(42))
  Expected: 42 (Int32)
  But was:  42 (RedisValue)

  2)   Is.EqualTo(42) -> EqualNumericConstraint
Assert.That(value, Is.EqualTo(42))
  Expected: 42 (Int32)
  But was:  42 (RedisValue)

  3)   Is.EqualTo("42") -> EqualStringConstraint
Assert.That(value, Is.EqualTo("42"))
  Expected: "42"
  But was:  42

[moshekar]

Do your RedisValues test pass as in my case the all fail

Yes, in my asserts implementation all these pass:

RedisValue rv = 42;
Assert.That(rv, Is.EqualTo(rv)); // object? overload
Assert.That(rv, Is.EqualTo(42)); // object? overload
Assert.That(rv, Is.EqualTo("42")); // StrictString overload

rv = "42";
Assert.That(rv, Is.EqualTo(rv)); // object? overload
Assert.That(rv, Is.EqualTo(42)); // object? overload
Assert.That(rv, Is.EqualTo("42")); // StrictString overload

I didn't add a generic EqualTo. In my implementation, all non-strings goes to the object? overload which simply returns:

return Equals(actual, Expected);

and I just let the compiler/runtime do all their automatic conversions for me.
This simple way has all the benefits of automatic conversions without the hassle of dealing with converters, implicit operators, etc. And most important - it's 100% standard equality check, so no suprises.

[manfred-brands]

@moshekar The standard EqualConstraint in NUnit does way more than calling Equals but after excluding other possibilities, it does call expected.Equals(actual)

You would think that Equality is commutative, but if you changed your code to call Equals(Expected, actual),
which is the order NUnit calls it, you get different results for RedisValue

RedisValue is an exception to the rule with its Equal implementation, but it doesn't obey the rules.

Order matters when calling Equals

Assert.That(Equals(value, 42), Is.True, "RedisValue.Equals(42)");
Assert.That(Equals(42, value), Is.False, "42.Equals(RedisValue)");

Same for Assert.That, one fails the other passes:

Assert.That(value, Is.EqualTo(42), "value, Is.EqualTo(42) -> EqualNumericConstraint -> Fallback to EqualConstraint");
Assert.That(42, Is.EqualTo(value), "42, Is.EqualTo(value) -> EqualConstraint");

According to the documentation:

If the current instance is a value type, the Equals(Object) method tests for value equality. Value equality means the following:

The two objects are of the same type. a Byte object that has a value of 12 does not equal an Int32 object that has a value of 12, because the two objects have different run-time types.

Therefore RedisValue.Equals(42) should always return false.

[moshekar]

The standard EqualConstraint in NUnit does way more than calling Equals but after excluding other possibilities, it does call expected.Equals(actual)

Of course :) I just wanted to reproduce the new issues in a basic implementation to exclude other factors.

You would think that Equality is commutative, but if you changed your code to call Equals(Expected, actual),
which is the order NUnit calls it, you get different results for RedisValue

Since Equals is called on object it gets to the specific Equals(object? obj) method in RedisValue, then it calls TryParse to parse it to all supported types in RedisValue. So this specific Equals knows to compare two direct RedisValues or a RedisValue to a simple type.
You are right that the other way comparison fails for types mismtach:

Assert.That(42, Is.EqualTo(rv));
Assert.That("42", Is.EqualTo(rv));

But I don't see any problem with that as this is an expected behaviour.
The first argument is Actual and the second is Expected. Actual is the subject here and it compares itself to some expected value.
It knows what types it supports and it's its responsibility to do the comparison.
I don't expect an int to know how to compare itself to RedisValue.

[manfred-brands]

The first argument is Actual and the second is Expected. Actual is the subject here and it compares itself to some expected value.

You are saying: actual.Equals(expected) is more appropriate that expected.Equals(actual)?
Interesting, but that is outside the scope of this PR.

[manfred-brands]

@OsirisTerje @stevenaw Can we merge this PR and get a new version out?

The PR fixes both issues:

1. Values that have an implicit operator string no longer will call the EqualStringConstraint, but the standard EqualConstraint.
2. Non-numeric instances compared to numeric values will fall back to standard EqualConstraint instead of being Converted to int

[stevenaw]

@manfred-brands I had been thinking of asking the same 🙂 On my end, I'll do a final pass as a review within an hour or so.

[stevenaw]

Thanks @manfred-brands one question from me. It might be good to let @OsirisTerje review too as I feel I've been a bit more arms-length on this one

[stevenaw]

question: I understand why we added the overloads on Is.EqualTo(), but can you remind me why we needed to go back to struct here?

Related question about the need to make the constructor for this internal... Now that we've shipped a public constructor, is there a harm in letting people target it? I feel like the could also open some extensibility doors.

For example, a ComplexNumberEqualConstraint which internally composes two separate EqualNumericConstraint instances.

[manfred-brands]

why we needed to go back to struct here?

Unconstraint generics resulted in Possible null reference compile time error, see build

I could have used notnull, but the only supported numeric types are value types and maybe the compiler can do something more knowing that.

Related question about the need to make the constructor for this internal... Now that we've shipped a public constructor, is there a harm in letting people target it? I feel like the could also open some extensibility doors.

Initially I made it internal to prevent it being called with unsupported types.
As I have now added a Guard protecting it that is less of an issue, so if you prefer to have it public, that is fine by me.

[manfred-brands]

@stevenaw I added a commit restoring the EqualTo(object?) overload as removing it breaks binary compatibility for already compiled code (e.g. for some NUnitExtensions library). The extra EqualTo<T>(T) is still there to solve the implicit operator string issue for newly compiled code.

[moshekar]

The first argument is Actual and the second is Expected. Actual is the subject here and it compares itself to some expected value.

You are saying: actual.Equals(expected) is more appropriate that expected.Equals(actual)? Interesting, but that is outside the scope of this PR.

Isn't that why the parameters are named Actual (the result we check) and Expected (what value we expect it to be)?
Anyway, if you don't agree with this claim, there is a simple solution:
return Equals(actual, expected) || Equals(expected, actual);

Thank you @manfred-brands , @OsirisTerje and @stevenaw for all the work and cooperation on this issue.

[OsirisTerje]

@manfred-brands Agree, merge.

When you have merged I can start pushing out a 4.3.1 with this one.

[manfred-brands]

@OsirisTerje I cannot merge without someone approving the PR.

[OsirisTerje]

Approved

[stevenaw]

Thanks for contributing this fix, and the deep research into method binding to find the right approach @manfred-brands

[manfred-brands]

Thanks @stevenaw

[smdn]

Thank you for fixing this issue!

@manfred-brands @OsirisTerje
I tried the merged code and error NUnit2021 still occur. Is that okay?

If it is treated as outside the scope of this PR, that is fine.
I will wait for the analyser to be corrected.

[manfred-brands]

@smdn Could you create an issue for that in the nunit.analyzers project, I'll get that fixed.

[moshekar]

@OsirisTerje when the new fix will be available? The last version in nuget is 4.3.0.

UPDATE: Now it's updated. Thanks!

[mikkelbu]

@smdn We will release an new version of the analyzers today with @manfred-brands fix

[smdn]

@manfred-brands @mikkelbu @OsirisTerje
Sorry for the late reply.
NUnit 4.3.1 + NUnit.Analyzers 4.5.0 work fine for all my test cases.
Thank you for your accurate and quick work!