NUnitEqualityComparer.Default should be replaced with new NUnitEqualityComparer()

[jnm2]

In #2290, we made the constructor internal and introduced a Default static property which returns a new instance every time. I failed to notice this when reviewing that PR.

Edit: Thanks for the correction @mikkelbu. The Default property has been in NUnitEqualityComparer since v2.5.1!

I believe that static properties should return the same instance every time or at least semantically be able to return the same instance every time. If the returned instance is immutable, it should be a static property. If it's mutable, it must create a new instance every time and therefore should be a method. Rather than having a static method delegate to the default constructor, I'd prefer re-exposing the default constructor.

I would like to propose the following:

namespace NUnit.Framework.Constraints
{
    public class NUnitEqualityComparer
    {
+       public NUnitEqualityComparer();

+       [Obsolete("Deprecated. Use the default constructor instead.", error: true)]
        public static NUnitEqualityComparer Default { get; }
    }
}

Should we make the obsoletion a warning or error? I prefer error because it helpfully points people to the necessary refactor while flushing out all uses but I could go either way.

[mikkelbu]

I have no problem in removing/deprecating the static property, but I did not introduce it 😄 . It was in the framework from the beginning on github, so there are probably a lot of people using it. See e.g.

nunit/src/NUnitFramework/framework/Constraints/NUnitEqualityComparer.cs

Line 72 in de5b6e7

public static NUnitEqualityComparer Default

[jnm2]

@mikkelbu Ahh, thanks for setting me straight! I thought the changes happened at the same time but I only looked up the one. Let me update.

[CharliePoole]

@jnm2 I agree with this and I would also agree with making the entire class internal. However, it is a breaking change either way for any user who made use of the class.

This is in the category that I call "not part of our API" but which I believe you are calling part of the API because it's accessible. Maybe we should call it "accidentally exposed" instead. 😺

[CharliePoole]

However, since either change makes the property inaccessible to the user, we may as well delete the property and change our internal usage.

[jnm2]

It's not in an Internal namespace and it is currently an integral part of a real-world custom constraint. I'm not sure how I'd be able to implement the custom constraint without it. Unless I'm overlooking an API, NUnitEqualityComparer needs to be that public API we provide.

My proposal does not make the property inaccessible to the user. The problem with making it inaccessible or non-existent is that users either see a message saying they can't access it or a message saying it doesn't exist. What I'd prefer is for them to see the obsoletion message telling the how to fix. The lack of obsoletion message is exactly what sent me in circles when we made the constructor internal in #2290.

We could also make the obsoletion a warning rather than an error if we don't want to force change on people, but then if they ignore it and forget about it, there will be no helpful message telling them how to fix it when we really do remove it.

[CharliePoole]

@jnm2 I didn't realize you are both the user reporting the problem and implementer here. I was hoping this might not have been used.

To clarify, can you indicate why you need to use the class? Frankly, we have too little info on how extenders are using our "internal" classes.

BTW I'm using "internal" in quotes to mean "stuff we didn't intend for users to access." I recognize that we may not be able to put the toothpaste back on the tube but I think it's still an important distinction.

[jnm2]

The custom constraint implements HasEqualPropertyValues(expectedObj, "Property1", "Property2", ...). For each pair of properties I want to use NUnit's default equality comparison. I suppose could use new EqualConstraint(expectedPropertyValue).ApplyTo(actualPropertyValue).IsSuccess as an equality comparer though. It seems like moving parts that aren't 1:1 with the equality comparison semantics I need, but it would do the job.

Either way it's been around since 2.5.1, which is what made me think there's a good chance people use it.

It's getting confusing. We have things in the Internal namespace that we do consider and want to consider part of our public API, and I just learned now that we have things outside that namespace that we want to consider to not be public API. Now I have much less confidence in using that term because it's a matter of interpretation.

[CharliePoole]

Well, I have to say of course it's a matter of interpretation. 😸 Most things are! Historically, the team interpretation was known to the team as a whole. Of course, I'm the only one of that historical team left, which is why I'm trying to get it spelled out more clearly.

The particular class we're talking about got put into the constraint namespace because only constraints use it. Two of our own constraints use the Default property, while the rest use the default constructor. IIRC Default originally returned an immutable instant, until we made changes such that the comparer was no longer immutable. At that point we were forced to make it work just like new.

I think we need to spell out all these subtleties in order to deal with them. Historical fact 1: this class was not meant to be used by users or extenders. Fact 2: It is being used. One interpretation: it probably needs to be exposed in some way because users want to be able to write code that says... do this comparison in the standard NUnit way.

I'd like to start a general discussion of this but I need to catch up my mail first!

[jnm2]

I want to get this in the next release. If we don't have any other plan ready by then, I think we should restore the constructor.

For discussion:

Do we have any appetite for moving this class out of its current NUnit.Framework.Constraints namespace or changing its name, with or without leaving behind an obsoleted class pointing the way to the new location/name, before 3.10? I'm doubtful.

Do we have any appetite for changing the shape of the class's members before 3.10?

public class NUnitEqualityComparer
{
    public static NUnitEqualityComparer Default { get; }

    public bool CompareAsCollection { get; set; }

    public System.Collections.Generic.IList<EqualityAdapter> ExternalComparers { get; }

    public System.Collections.Generic.IList<NUnitEqualityComparer.FailurePoint> FailurePoints { get; }

    public bool IgnoreCase { get; set; }

    public bool WithSameOffset { get; set; }

    public bool AreEqual(object x, object y, ref Tolerance tolerance, bool topLevelComparison = true);

    public class FailurePoint
    {
        public bool ActualHasData;

        public object ActualValue;

        public bool ExpectedHasData;

        public object ExpectedValue;

        public long Position;

        public FailurePoint();
    }
}

Off the top of my head I'd like to obsolete Default, seal the classes, and try to make FailurePoint immutable and use properties and make the constructor internal. Perhaps try to make make AreEqual more intuitive? It's a moonshot, but I think comparers should be immutable and would try to make that happen too. Maybe AreEqual could return a complex type which encapsulates both the success/failure and the list of failure points.

Where do we want to draw the line, given the long history of the public NUnit.Framework.Constraints.NUnitEqualityComparer?

[CharliePoole]

Seems like a lot to bite off and it isn't even assigned to anyone.

[ChrisMaddock]

I agree with making Default a warning.

What's the motivation for moving the class? And why make the constructor internal - aren't we saying that we want this to be available, or not?

[jnm2]

@CharliePoole

Seems like a lot to bite off and it isn't even assigned to anyone.

I agree, but I stated my thoughts because you asked for a design discussion before we went ahead and made the NUnitEqualityComparer constructor public and deprecated the Default property. Like I said, I don't actually expect we'll want to do much before 3.10 and in that case I'd rather do what I proposed at the top.

@ChrisMaddock I have no motivation to move the class, but I was asking in case someone else has. My point being, if none of us is hoping to do anything drastic like renaming it or overhauling it, why not do what I suggested in the scope of my original proposal? =D

And why make the constructor internal

Sorry– I should have said "the FailurePoint constructor." On the principle of leaving as many doors open for ourselves as possible and not committing to a public API until asked.

[CharliePoole]

Sorry, my comments are generally to the last thing you wrote and I don't always know if the original proposal is relevant because of the way these discussions develop.

I'm afraid this sounded to me as well as if you were proposing moving the class.

To the point, I think a major revision is needed here and someone should eventually take it on, creating a plan and a series of PRs to implement, each one standing on its own. At least that's how I'd be most comfortable with the scope of change needed.