Implement operator(s) '<, <=, >, >='

[CheloXL]

Describe the feature

As the title says, implement the comparison operators. This is to fix the CA1036 warning: AbcId should define operator(s) '<, <=, >, >=' since it implements IComparable.

[SteveDunn]

@CheloXL - thanks for the report.

[SteveDunn]

The trouble is, not all types that implement IComparable also have these operators. For instance, a Value Object of string reports this when the operators are added to the VO:

error CS0019: Operator '>' cannot be applied to operands of type 'string' and 'string'

So, Vogen must inspect the primitive type to see if it has these operators.

It's acheivable, but just wanted to make a note here for when it's implemented fully

[glen-84]

I see that the duplicate #292 was fixed, but I'm not able to view the changes. Did they not make it into a release?

[jeffward01]

@glen-84 - im not sure, I see this issue on mine as well

Vogen Version: 3.0.9

Note:
The value object in the screenshot is of type: [ValueObject<Guid>], please see assemblyinfo.cs below

Here is my assemblyinfo.cs

// Set the defaults for the project

[assembly:
    VogenDefaults(
        typeof(Guid),
        Conversions.EfCoreValueConverter | Conversions.NewtonsoftJson | Conversions.TypeConverter |
        Conversions.SystemTextJson,
        typeof(ValueObjectValidationException))]

---

Note:
Here I have generated the extension methods, and now the warning is gone:

[ValueObject]
// ReSharper disable once PartialTypeWithSinglePart
public readonly partial struct BloomEventCommentId
{
    public static bool operator <(
        BloomEventCommentId left,
        BloomEventCommentId right)
    {
        return left.CompareTo(right) < 0;
    }

    public static bool operator <=(
        BloomEventCommentId left,
        BloomEventCommentId right)
    {
        return left.CompareTo(right) <= 0;
    }

    public static bool operator >(
        BloomEventCommentId left,
        BloomEventCommentId right)
    {
        return left.CompareTo(right) > 0;
    }

    public static bool operator >=(
        BloomEventCommentId left,
        BloomEventCommentId right)
    {
        return left.CompareTo(right) >= 0;
    }
}

[glen-84]

@SteveDunn Any update on this one? I'm not able to apply <, <=, >, >= filtering to my VOs without this.

[SteveDunn]

Hi - there's multiple aspects to this:

1. was to disable the CA warning which I think was the first fix that was added (CA1036 ClassName should define operator(s) since it implements IComparable #292 )
2. was then to implement the non-generic IComparable - which was recently added (it literally just implements int CompareTo(object other)) (Implement non-generic counterpart for IComparable #323)
3. implement any related operators if the underlying type has them (this one)

Now that 1. and 2. are implemented and released, nobody should be seeing any CA errors as far as I know.

I hope to get around to this one shortly, but I need to check my notes as I vaguely remember that it was quite a big deal to inspect operators on the underlying type in the source generator

[SteveDunn]

I've looked back through my notes and found this old issue: (#222)

It's non-trivial to inspect the operators on the underlying type, for instance, int, even though it has operators <, > etc., they are only defined in the resulting IL and so can't be inspected via Roslyn (as far as I know).

We could add some hints into the source generator to say 'if the type is int, then always add these operators'. I don't know how that would scale or if that would cause confusion with other types.

Another option, but this is a .NET 7+ feature, is to check if the underlying type implements IComparisonOperators:

But then we could risk adding more confusion by given the impression that Vogen fully wraps all operators, including:

• IDecrementOperators
• IEqualityOperators (which it currently does, but not via this interface)
• IIncrement/DecrementOperators
• IModulusOperators
• IMultiplyOperators etc.

[SteveDunn]

Perhaps a trade off, at least for now, it to have an explicit parameter on the ValueObjectAttribute, something like generateComparisonOperators.

Obviously, this will have to be on each Value Object and can't be defaulted globally (or we'd have all sorts of compilation errors where the underlying types don't themselves have comparison operators).

But for sorting, I think what we currently have looks sufficient: the generated CompareTo methods from IComparable just delegate to the underlying CompareTo methods if the underlying itself implements IComparable. The underlying's implementation of < > <= >= will then be used.

I suppose it boils down to what Vogen actually is. I define it as something that more 'strongly types' primitives, e.g. CustomerId instead of int. So using this definition, CustomerId does not have all of the functionality of int itself, aside from common concerns such as:

• [de]serialization
• equality (using the underlying's equality behaviour)
• sorting (using the underlying's IComparable behaviour)

[SteveDunn]

But I think the crux of this issue is this:

But I don't think the warning makes much sense for generated code. For instance, for the case of GUIDs, adding the operators doesn't change behaviour.

[glen-84]

We'd like to be able to use these operators to filter IDs within a certain range, for example.

A list of numeric types + the .NET 7-specific IComparisonOperators check (where possible) would be slightly better than the generateComparisonOperators option, as it wouldn't need to be defined for every VO. Having said that, it could be set in a custom attribute that derives from ValueObjectAttribute, which we have in our case (for IDs, at least).

[szarykott]

Another way to make the impact of this smaller is to solve #407

Some people do not need their Value Object to implement IComparable or its generic counterpart. Allowing them to opt out of this would make this issue a non-issue for them.

[lk-smit-ag]

Does ist make sense to disable the warning for these specifc types (like Guid) and implement the operators for the other types?

[SteveDunn]

Another way to make the impact of this smaller is to solve #407

Some people do not need their Value Object to implement IComparable or its generic counterpart. Allowing them to opt out of this would make this issue a non-issue for them.

Apologies @szarykott - #407 was merged to main in July but I forgot to do a release. I've just kicked off the release process for 3.0.21. It should be in NuGet in an hour or so. Would be great if you could try it out.

[lk-smit-ag]

Another way to make the impact of this smaller is to solve #407
Some people do not need their Value Object to implement IComparable or its generic counterpart. Allowing them to opt out of this would make this issue a non-issue for them.

Apologies @szarykott - #407 was merged to main in July but I forgot to do a release. I've just kicked off the release process for 3.0.21. It should be in NuGet in an hour or so. Would be great if you could try it out.

Since the update, the source generator seems to not work anymore.
It does not generate anything on my types. Can you have an eye on it?

Thanks!

[SteveDunn]

Oh no! Sorry about that. I'm not able to fix anything until next week. Perhaps clearing your intermediate files might help. Do you experience this in a new project?

[lk-smit-ag]

Oh my bad.. after cleaning up the intermediate files & restarting rider it worked.
But i cannot see the newly added operators <=, >=, <, >.

Is there any restriction for this?

My sample id looks like:

[ValueObject<int>]
[Instance("Default", 25)]
public readonly partial struct PageSize
{
    private static Validation Validate(int input)
    {
        return input > 0 ? Validation.Ok : Validation.Invalid($"{nameof(PageSize)} must be greater than zero.");
    }

    public static PageSize FromNullable(int? input) => input.HasValue ? From(input.Value) : Default;
}

[SteveDunn]

@lk-smit-ag - glad it's now working (you had me worried there!)

The operators aren't implemented; the implementation added merely allows types to omit the generation of IComparable (see #407 )

[lk-smit-ag]

@SteveDunn oh, my fault too!
After adding the Omit for comparison, it will be removed & sonarqube is happy with it! 👍

Thanks!

[prlcutting]

Forgive me if this is slightly off topic for this issue (#222 would have been a better fit I think, but that's closed)...

I'd like to make the case for implementing standard operators for the underlying type in the source generated value object, beyond just the equality and comparison operators.

In my use case, I'd like to create value objects for various double values, and perform mathematical operations with them, e.g., add, subtract, multiply, divide, etc. I can obviously write these myself by extending the source generated value object, but it's very repetitive, "boilerplatey" code, which would seem like the poster child for source generation:

public static bool operator <(
    Foo left,
    Foo right)
{
    return left.CompareTo(right) < 0;
}

public static bool operator <=(
    Foo left,
    Foo right)
{
    return left.CompareTo(right) <= 0;
}

public static bool operator >(
    Foo left,
    Foo right)
{
    return left.CompareTo(right) > 0;
}

public static bool operator >=(
    Foo left,
    Foo right)
{
    return left.CompareTo(right) >= 0;
}

public static Foo operator +(
    Foo left,
    Foo right)
{
    return From(left.Value + right.Value);
}

public static Foo operator -(
    Foo left,
    Foo right)
{
    return From(left.Value - right.Value);
}

public static Foo operator *(
    Foo left,
    Foo right)
{
    return From(left.Value * right.Value);
}

public static Foo operator /(
    Foo left,
    Foo right)
{
    return From(left.Value / right.Value);
}

// ...and more...

The temperature (Centigrade) and money (PaymentAmount) examples referenced in the wiki would be examples where I imagine it is highly likely that a consumer would want to perform math operations using operators.

I learned from reading this and other issues that there are differences in supported operators based on the underlying type, e.g., int vs. Guid vs. string, etc., but if these differences are accepted/expected for the underlying type, would that acceptance/expectation not extend to their corresponding value object wrappers? And I imagine that it would be relatively straightforward to conditionally source generate different sets of operators based on the underlying type.

The code that is source generated for the operators could be governed by configuration options in the attribute. This could take the form of "simple" grouped options, e.g., none, "match underlying type", etc., or they could be much more fine grained - perhaps an à la carte flags enumeration (with sensible defaults). Clearly those are half-baked thoughts that would require further analysis, but I'm sure there's a solution there.

I think without automatically source generated operators, a whole class of use cases for Vogen's value objects are out of reach. It would work well for identifiers and alike, but not for anything that requires math, which I suspect is a common need/use case. At least, not without significant repetitious and error prone manual code, which is what I assume Vogen is partially trying to avoid.

I'm new to Vogen, so please be gentle if I've missed something elementary. 😃 The library looks awesome by the way!

Thoughts?

[SteveDunn]

Hi @prlcutting—apologies for the very slow response—this thread is so long that I must've missed it originally. Thanks for the feedback, and I really appreciate that you like the library!

The way that I use value objects is to make each operator (< > = * / etc) an explicit operation. For instance, a Score type for a game: I could add a + operator, but what I really want to say is:

Increase the score by a certain number of points

... so I have an IncreaseBy(Points) method on it. This is clear to read and ensures that scores can't be decreased.

But I understand that some value objects are much closer in behaviour to the primitive that they wrap, and your suggestions would certainly help with this and avoid repetitious code.

I'll continue to think about how best to implement this. Perhaps it could be achieved by the new static abstracts in interfaces...

[viceroypenguin]

@SteveDunn would it make sense to offer a diagnostic suppressor which would turn off the CA1036 warning on [ValueType]s? It would be easy to implement - I'd be happy to do so this week. This would mean that developers could leave CA1036 turned on generally, but not have to worry about the fact that they're not implemented on marked types. This would side-step the issue of whether the comparison operators need to be implemented in the first place.

[SteveDunn]

Good idea @viceroypenguin . Thanks for the offer. There's a suppressor already that could be used as a reference point https://github.com/SteveDunn/Vogen/blob/main/src/Vogen/Suppressors/CA1822DecoratedMethodSuppressor.cs