SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
887 stars 46 forks source link

Array value objects don't work #661

Closed aradalvand closed 2 months ago

aradalvand commented 3 months ago

Describe the bug

Array types don't work apparently:

image

Repro:

using Vogen;

Hash hash = Hash.From(new byte[] { 1, 2, 3 });

[ValueObject<byte[]>]
public readonly partial struct Hash;

Also, the README claims that "any type can be wrapped", which does not actually seem to be true.

image Repro:

using System.Collections.Immutable;
using Vogen;

[ValueObject<ImmutableArray<int>>]
public readonly partial struct Hash;

Am I missing something?

Steps to reproduce

Outlined above.

Expected behaviour

Should just work.

SteveDunn commented 3 months ago

Collections aren't supported because they don't exhibit value semantics. For instance, a Hash.From([1, 2, 3]) == Hash.From([1, 2, 3]) is false. Also, serialization probably wouldn't work for collections, e.g. when [ValueObject<IEnumerable<byte>>] and instantiating with `Hash.From([1,2,3]) (an array)

SteveDunn commented 3 months ago

An alternative is to use a "deep-equatable" array, and have that as the underlying type for a value object. Here is such an array implementation:

https://github.com/eiriktsarpalis/typeshape-csharp/blob/main/src/TypeShape.Roslyn/IncrementalTypes/ImmutableEquatableArray.cs

You can then do: [ValueObject<ImmutableEquatableArray<byte>>]

SteveDunn commented 3 months ago

Actually, that won't work either, as it delves into the hierarchy to see if it's a collection.

This works, kinda:

public class Hash<T> : IEquatable<T> where T : IEquatable<T>
{
    private readonly ImmutableEquatableArray<T> _items;
    public Hash(T[] items) => _items = items.ToImmutableEquatableArray();

    protected bool Equals(Hash<T> other) => _items.Equals(other._items);

    public bool Equals(T? other) => _items.Equals(other);

    public override bool Equals(object? obj) => _items.Equals(obj);

    public override int GetHashCode() => _items.GetHashCode();

    public static implicit operator Hash<T>(T[] items) => new(items);
}

[ValueObject<Hash<byte>>]
public readonly partial struct FileHash;

The only reason it doesn't currently work is that Vogen emits incorrect triple-slash comments when the underlying type is a generic - which I'll fix soon.

To be honest, I'm now re-thinking the whole thing of constraining the underlying type so that it can't be a collection.

Leave this with me, and I'll think it over again. Thanks for your input, and if you have any thoughts or suggestions on this topic, please let us know.

SteveDunn commented 2 months ago

Also, for array, I don't know what ToString would do by default...

aradalvand commented 2 months ago

Also, for array, I don't know what ToString would do by default...

I think for all collections you could just do something sensible like $"[{string.Join(", ", Value)}]"

SteveDunn commented 2 months ago

After careful consideration, I've left the restriction on ICollection, purely because it enables an easy way to mutate the data in the value object, which Vogen goes to considerable lengths (via the analyzers) to stop. However, you can achieve the same result by using an intermediary underlying type that wraps your collection. I've added an example test to demonstrate.

Thanks for reporting - please let me know if you need any further assistance or clarification

aradalvand commented 2 months ago

purely because it enables an easy way to mutate the data in the value object, which Vogen goes to considerable lengths (via the analyzers) to stop.

@SteveDunn Wouldn't it be feasible then to only allow immutable collection types (e.g. ImmutableArray<T>)?