Compare memberless records by value, others by members

[BrunoJuchli]

Background and motivation

Some of our records don't have properties, some do.
We like to use member-comparison by default, mainly because this enables much improved error messages. However, for records without members, we get the error

No members were found for comparison. Please specify some members to include in the comparison or choose a more meaningful assertion.

We'd like to be able to configure member-less records to be compared by value, without having to specify this for every single memberless record type.

API Proposal

public abstract class SelfReferenceEquivalencyAssertionOptions<TSelf> : // this already exists
{
    public TSelf ComparingRecordsByMembers(
        bool onlyRecordsWithMembers);
}

The existing ComparingRecordsByMembers is extended by an overload with a bool parameter indicating whether all records should be compared by members or only the ones which actually do have members.

For the implementation, I suggest to replace the backing bool? with a 3-state enum which is nullable, thus the following four states:

• a) unconfigured
• b) force comparison by members
• c) force comparison by value
• d) force comparison by members only for records with members (by value for the others)

Backwards compatibility:

• The existing method ComparingRecordsByValue() would set it to c) force comparison by value
• ComparingRecordsByMembers() would set it to b) force comparison by members
• ComparingRecordsByMembers(false) would set it to b) force comparison by members
• ComparingRecordsByMembers(true) would set it to - d) force comparison by members only for records with members (by value for the others)

---

Alternatively, there could be a single ComparingRecordsByMembers method with default argument value:

public TSelf ComparingRecordsByMembers(
        bool onlyRecordsWithMembers = false);

That would be backwards compatible as long as users recompile after updating.

API Usage

AssertionOptions.AssertEquivalencyUsing(options => options.
    ComparingRecordsByMembers(
        onlyRecordsWithMembers: true));

Alternative Designs

public enum EqualityConfig
{
    Equals,
    Members
}

public abstract class SelfReferenceEquivalencyAssertionOptions<TSelf> : // this already exists
{
    public TSelf CompareBy(
        Func<Type, EqualityConfig?> predicate);
}

and usage:

AssertionOptions.AssertEquivalencyUsing(options => options.
    CompareBy(
        type => type...);

Note: this is similar to what was proposed in PR #1383 from discussion #1374).
However, I reckon that we may want to have 3 config options:

• leave the decision up to FluentAssertions (default)
• choose "by members" comparison for a type
• choose "by value" (Equals) comparison for a type

Having a single list of the delegates gives a simple way to prioritize in case multiple delegates give a different result.

Also, in contrast to the ComparingRecordsByMembers(bool) API, this prevents the (possible) misconception that "has members" is in sync with how FluentAssertions detects members.
This is an advantage and disadvantage at the same time:

• makes it easier to implement and maintain
• but moves the burden to the user: for example, in case he adds a custom IMemberSelector, he would have to sync this functionality himself.

Risks

• there's a question of whether the "does this type have any members" should be synchronized with definition of member detection (fields, properties - public / non-public...), and if it's not, if it would cause confusion.
  • as far as i can understand, currently the detection of members is done at a later stage in the pipeline, only when it's needed.
• enabling the option would cost some performance
• overloading an existing method with a new parameter with default argument value restricts backwards compatibility by requiring users to recompile after updating the library. I think this also poses a problem in scenarios with assembly binding redirects, where compilations happens with a different version then the runtime uses.

Are you willing to help with a proof-of-concept (as PR in that or a separate repo) first and as pull-request later on?

Yes, please assign this issue to me.

[ITaluone]

Hmm.. I would rather try to build a CompareBy... method for every case instead of using bool parameter which in certain cases are overlapping (example 2 and 3 above, which does the same)

[BrunoJuchli]

Hmm.. I would rather try to build a CompareBy... method for every case instead of using bool parameter which in certain cases are overlapping (example 2 and 3 above, which does the same)

Can you make a concrete suggestion?

Do you mean something like:

CompareAllRecordsByMembers();
CompareRecordsByMembersButWhenTheyDontHaveMembersCompareByValue();
CompareAllRecordsByValue();

?

[dennisdoomen]

I must be missing something, but what is there to compare between member-less records?

[BrunoJuchli]

Hi @dennisdoomen

I must be missing something, but what is there to compare between member-less records?

---

Their type. That's why I'd like to have value semantic for them.
But I guess the interesting question is not how to compare them, but why would we have member-less records in a data-only object graph (no behavior)?.
The answer to that is: we use discriminated unions. So the first relevant information is in which case the discriminated union is. The second relevant information may be the data the case contains - but sometimes the case is all the data there is - hence I've got this object without any members.

Long version:

Let me start with a simplified example. This shows why we actually have member-less records in our object graph: it's about representing state/data via discriminated unions.

public record class FeatureLevel
{
    public abstract T Match<T>(
         Func<Basic, T> basic,
         Func<Professional, T> professional);

    public record class Basic : FeatureLevel
    {
         public override T Match<T>(
             Func<Basic, T> basic,
             Func<Professional, T> professional)
         {
             return basic(this);
         }
    }
    
    public record class Professional : FeatureLevel
    {
         public required int LicenseCount { get; init; }
        
         public override T Match<T>(
             Func<Basic, T> basic,
             Func<Professional, T> professional)
         {
             return professional(this);
         }
    }
}    

So usage would be:

FeatureLevel result = testee.ComputeFeatureLevel(...);

result.Should().BeEquivalentTo(
    new FeatureLevel.Professional
    {
        LicenseCount = 7
    });

which could result in an error similar to:

expected result to be
FeatureLevel.Professional { LicenseCount = 7 }
but found
FeatureLevel.Professional { LicenseCount = 3 }

or

expected result to be
FeatureLevel.Professional { LicenseCount = 7 }
but found
FeatureLevel.Basic

Now this example is not very interesting yet, because I could use BeEquivalentTo(...) when expecting a record with members (FeatureLevel.Professional). but just switch to Be(..) when expecting a member-less record FeatureLevel.Basic.

However, in real life we commonly have records which are more complex (= bigger tree of objects), so both, records with and without members, exist in the same tree. So this is what we do:

BeEquivalentTo(
    ....,
    options => options
         .CompareByValue<TypeWithoutMembers>()
         .CompareByValue<OtherTypeWithoutMembers>()
         .CompareByValue<YetAnotherTypeWithoutMembers>());

That's quite cumbersome and error prone. It's often a few rounds of trial and error until I've specified all the ones I need. And then, If ever I remove a without-members type property from a record, I should remove its .CompareByValue<..> as well. So it creates additional maintenance burden.
If we were able to configure assertions to configure records by members, but records without members by value - per default - we could basically get rid of all the CompareByValue<T> calls.

Sidenote: now that there is records, mostly the benefit of Should().BeEquivalent() for us is not that it provides equality comparison where there is no viable Equals() implementation, but that when inequality is detected, we get a nice message stating what exactly is inequal. That makes it valuable and preferable over Should().Be(..).
Actually, in the roughly 10 years I've been using FluentAssertions, the detailed message in BeEquivalent(..) telling me exactly what's different was and is the compelling feature. The second one would be collection comparison ;-)

So thank you @dennisdoomen and @jnyrup for this great library! And especially the continued maintenance and support of it :-)

[dennisdoomen]

It's a bit of a niche feature, but I would be fine with somebody contributing an option like ComparingMemberlessRecordsByValue. What do you think @jnyrup ?

[jnyrup]

This problem is not unique to records but memberless types in general, e.g. #2391.
The general question is how to compare memberless objects?

• Structural equivalency implies that it's equivalent to all other empty types. (Which is what we disallow currently)
• Value semantics rarely makes sense for types that doesn't override Equals.

I see how using value semantics on memberless records makes sense, since synthesized Equals will be a check if the types are exactly the same, which often seems what users want to compare.

I'm wondering if there is a more general applicable approach that could also work for types without overridden Equals.

[dennisdoomen]

Maybe something like ComparingMemberlessTypesByValue?

[BrunoJuchli]

@dennisdoomen @jnyrup

So the options are as follows, correct?

• using expected.Equals(actual):
  • CompareMemberlessTypesByValue (on all memberless types)
  • CompareMemberlessRecordsByValue (on memberless records only)
• using actual.GetType() == expected.GetType()
  • CompareMemberlessTypesByType (on all memberelss type)
  • CompareMemberlessRecordsByType (on memberless records only)

Out of these I'm rather opinionated on CompareMemberlessTypesByValue: I think it's a bad option, because BeEquivalentTo() means we don't have to implement Equals() ourselves, and this would violate this principle.

Furthermore, I can make a small case as that it should only affect records:

Structural equivalency implies that it's equivalent to all other empty types. (Which is what we disallow currently)

This behavior helps preventing "false positive" tests in the following cases:
-a) the type does have meaning, so it's not just about structural equivalency
-b) I (accidentally?) didn't "publish" my members on the type (private)

For a) it doesn't matter whether the involved types are records or not.
I think though, that for b) it does: in the case of a records, it's very likely that it features a generated .Equals() implementation which would consider these private members. The opposite is true for non-record classes.

=> The likelihood of false positives with records is smaller.

Counterargument would be, that problem b) also occurs when part of your members are private, and some are not. So there's no reliable fix for it - so it could create more confusion than it's worth?

---

Anyway, I'm sure you guys have better insight into this, so in case you both can agree on a variant as acceptable for a PR, I'd happily do that one.

[dennisdoomen]

I that case, CompareMemberlessTypesByType makes more sense to me, since that's what your intention is.

[BrunoJuchli]

Ok, so we'd have

• ComparingRecordsByValue()
• ComparingRecordsByMembers()
• CompareMemberlessTypesByType()

How should the implementation respect combinations of these? Should it (not) matter in which sequence these are called?
For example

.ComparingRecordsByMembers()
.CompareMemberlessTypesByType()

=> Memberless records compared by type
=> Records with members compared by members

vs

.CompareMemberlessTypesByType()
.ComparingRecordsByMembers()

=> Memberless non records compared by type, but
=> Memberless records compared by member?

or should memberless records still be compared by type?

[dennisdoomen]

It doesn't matter. If the type is memberless, it's compared by type. But now I'm in doubt again whether CompareMemberlessTypesByValue isn't the better option. Also considering #1860.