Can not define implicit cast operator

aeb-dev commented 1 year ago

Describe the bug

In the readme, it is stated that you can provide implicit operators manually. When I try to provide one I get a compilation error Duplicate user-defined conversion in type 'MyValue' - https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0557

The problem seems like you can not provide both implicit and explicit cast operator at the same time. However, Vogen automatically generates explicit cast operators.

Steps to reproduce

Add the following code to an empty project and compile.

[ValueObject<int>]
public partial struct MyValue
{
    public static implicit operator int(MyValue d) => 5;
}

Expected behaviour

I should be able to provide implicit cast operator

SteveDunn commented 1 year ago

Apologies @aeb-dev . I have only just realised that there is a limitation in C#: if you have an explicit cast operator, then you can't have an implicit cast operator.

I don't really know of a way around this apart from to specify some configuration to allow the user to chose which one they want.

But I'm sceptical of the usefulness of these operators. Most people want them so that they can do the same operations that they do on the primitives, which is fair enough, but those operations should have names, e.g. for a value object of int that represents a Score, then it would be more expressive to have score = Score.IncreaseBy(10) rather than score += 10

aeb-dev commented 1 year ago

Apologies @aeb-dev . I have only just realised that there is a limitation in C#: if you have an explicit cast operator, then you can't have an implicit cast operator.

No need for an apology. This is how software works. We are all in the same boat :)

I don't really know of a way around this apart from to specify some configuration to allow the user to chose which one they want.

I don't know how the code is generated but is it possible to add a check that if there is an same implicit operator generate implicit instead of explicit?

But I'm sceptical of the usefulness of these operators. Most people want them so that they can do the same operations that they do on the primitives, which is fair enough, but those operations should have names, e.g. for a value object of int that represents a Score, then it would be more expressive to have score = Score.IncreaseBy(10) rather than score += 10

I read your thoughts about this and 99% of the time I agree with you but for several edge cases not having it can be a real pain. As a user I would like to take that risk.

aeb-dev commented 1 year ago

Thank you very much for addressing the issue in such short notice

rbanks54 commented 1 month ago

Just revisiting this one as I have a scenario where the implicit cast would be very useful :)

In a Linq query across a collection I can do this without a problem

var x = list.Where(e => e.Id == 1).Single();

However, the same query in EFCore will fail with an InvalidCastException and a message of 'Invalid cast from 'System.Int32' to 'Domain.Id'

To fix this, an explicit cast is required

var x = context.Entities.Where(e => e.Id == (Id)id).Single();

or

var x = context.Entities.Where(e => e.Id == Id.From(id)).Single();

Unfortunately, I can't spot these problems at compile time as the Id and int types are equatable. The only way to prevent the runtime errors is to support the implicit cast.

For the concern about people not using the .From() method, we could mark the operator as Obsolete

    [Obsolete("Only for EF Core", true)]
    public static implicit operator X(int id) => X.From(id);

That said, I haven't tried this to see if it breaks EF or anything else. It's just an idea :)

Also, comment earlier in the thread around the += operator doesn't seem valid anymore as I get a compiler error when trying it.

For reference, the root cause is EF performing a Convert.ChangeType() call in the ValueConverter` base class. See: https://github.com/dotnet/efcore/blob/486047c7422ec04530ca66c7c55256fa89b0ffc2/src/EFCore/Storage/ValueConversion/ValueConverter%60.cs#L80-L96

If you need a repro for this I can give you one based on the Onion sample :)

SteveDunn commented 1 month ago

Hi @rbanks54, thanks very much for the report and the suggestion. But what a coincidence! I'm literally looking at this issue right now in another project. The solution there was to just pass in the strong type rather than compare with the primitive.

The solution here (to remove ambiguity with comparisons between the strong type and primitive) could be to remove the equals operators to the primitive:

[assembly: VogenDefaults(primitiveEqualityGeneration: PrimitiveEqualityGeneration.Omit)];

But, I've just realised that this conflicts with other generated code, in particular the generation of the IVogen<> interface. To demonstrate, this combination causes compilation errors as IVogen expects implementers to have equality to the primitive:

[assembly: VogenDefaults(
  primitiveEqualityGeneration: PrimitiveEqualityGeneration.Omit,
  staticAbstractsGeneration: StaticAbstractsGeneration.MostCommon | StaticAbstractsGeneration.InstanceMethodsAndProperties)]

... causes...

Error CS0535 : 'ToDoItemId' does not implement interface member 'IVogen<ToDoItemId, int>.operator ==(ToDoItemId, int)'

I've just added an issue to track this. It's a shame though, as equality to primitives is a much requested feature and it'd be a shame to have to turn it off just for the EFCore scenario. Perhaps an analyser could help here?

rbanks54 commented 1 month ago

Great timing, indeed!

The analyzer approach could work. It's better than the obsolete workaround :)

I'll move any future conversation over to the new issue.

SteveDunn / Vogen