Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static Abstract Members Support #1 #511

Closed
Aragas opened this issue Oct 28, 2023 · 8 comments
Closed

Static Abstract Members Support #1 #511

Aragas opened this issue Oct 28, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@Aragas
Copy link

Aragas commented Oct 28, 2023

Add support for static abstract members. From what I've seen, we can add at least these members:

public interface IVogen<TSelf, TValueObject>
    where TSelf : IVogen<TSelf, TValueObject>
    where TValueObject : notnull
{
    static abstract explicit operator TSelf(TValueObject value);
    static abstract explicit operator TValueObject(TSelf value);

    static abstract bool operator ==(TSelf left, TSelf right);
    static abstract bool operator !=(TSelf left, TSelf right);

    static abstract bool operator ==(TSelf left, TValueObject right);
    static abstract bool operator !=(TSelf left, TValueObject right);

    static abstract bool operator ==(TValueObject left, TSelf right);
    static abstract bool operator !=(TValueObject left, TSelf right);

    static abstract TSelf From(TValueObject value);
}

This will require the language version check and the runtime check, as I understand

The main usecase I see is user-defined extensions that don't need direct support from Vogen.
As an example, I'm currently using it to define a generic EF Core Comparer

// We need to expose the IsInitialized value unfortunately
public interface IHasIsInitialized<in TSelf>
{
    static abstract bool IsInitialized(TSelf instance);
}

[ValueObject<string>]
public readonly partial struct ApplicationRole : IVogen<ApplicationRole, string>, IHasIsInitialized<ApplicationRole>
{
    public static bool IsInitialized(ApplicationRole instance) => instance._isInitialized;
}

public class VogenValueComparer<TVogen, TValueObject> : ValueComparer<TVogen>
    where TVogen : struct, IVogen<TVogen, TValueObject>, IHasIsInitialized<TVogen>
    where TValueObject : notnull
{
    private static int GetHashCodeInternal(TVogen instance) => TVogen.IsInitialized(instance) ? instance.GetHashCode() : 0;
    private static bool EqualsInternal(TVogen left, TVogen right) => left == right;

    public VogenValueComparer() : base((left, right) => EqualsInternal(left, right), vogen => GetHashCodeInternal(vogen)) { }

    public override int GetHashCode(TVogen instance) => GetHashCodeInternal(instance);
    public override bool Equals(TVogen left, TVogen right) => EqualsInternal(left, right);
}
@Aragas Aragas added the enhancement New feature or request label Oct 28, 2023
@Aragas Aragas changed the title Static Abstract Members Support Static Abstract Members Support #1 Oct 28, 2023
@SteveDunn
Copy link
Owner

An interesting idea - thanks Aragas! I'll look into this shortly.

@SteveDunn
Copy link
Owner

It is tricky to implement this, especially for the casting operators, because they are not guaranteed to be the same for every value object. e.g., one VO can specify to have them generated, and another one can say to omit them.
We could assume, and maybe have an analyzer that enforces, that if someone wants static abstract interfaces generated, then they must at least have explicit casting selected.

The equality operators look to be no problem though, as they are universal.

@SteveDunn
Copy link
Owner

SteveDunn commented May 15, 2024

@Aragas , this is now implemented in a branch. I'm working on a sample, and thought your value comparer would be ideal.

On the subject of EF Core, Vogen generates this code:

        public class EfCoreValueComparer : global::Microsoft.EntityFrameworkCore.ChangeTracking.ValueComparer<SomeId>
        {
            public EfCoreValueComparer() : base(
                (left, right) => DoCompare(left, right), 
                instance => instance._isInitialized ? instance._value.GetHashCode() : 0) 
            { 
            }
                
            static bool DoCompare(SomeId left, SomeId right)
            {
                // if both null, then they're equal
                if (left is null) return right is null;
                
                // if only right is null, then they're not equal
                if (right is null) return false;
                
                // if they're both the same reference, then they're equal
                if (ReferenceEquals(left, right)) return true;
                
                // if neither are initialized, then they're equal
                if(!left._isInitialized && !right._isInitialized) return true;
                
                return left._isInitialized && right._isInitialized && left._value.Equals(right._value);            
            }                
        }

Also generated is this extension method:

            public static class __EfCoreDateTimeOffsetVoEfCoreExtensions 
            {
                public static global::Microsoft.EntityFrameworkCore.Metadata.Builders.PropertyBuilder<EfCoreDateTimeOffsetVo> HasVogenConversion(this global::Microsoft.EntityFrameworkCore.Metadata.Builders.PropertyBuilder<EfCoreDateTimeOffsetVo> propertyBuilder) =>
                    propertyBuilder.HasConversion<EfCoreDateTimeOffsetVo.EfCoreValueConverter, EfCoreDateTimeOffsetVo.EfCoreValueComparer>();
            }

So, in your code, you have this:

            modelBuilder
                .Entity<EfCoreTestEntity>(builder =>
                {
                    builder
                        .Property(x => x.Id)
                        .HasVogenConversion()

                        // use the above instead to register the converter and comparer
                        // .HasConversion(new EfCoreDateTimeOffsetVo.EfCoreValueConverter())
                        .ValueGeneratedNever();
                });

I was struggling to see how I could fit this example in, so instead I went with the scenario of a 'unique ID factory', where it just needs to know that it's a value object of type int.

What do you think? Might you be able to help by adding another scenario, perhaps another EF core scenario where the generated stuff isn't needed and just uses the code you have above?

@Aragas
Copy link
Author

Aragas commented May 21, 2024

@SteveDunn I would like to extended the ValueComparer a bit with #598
If possible, the Copy() could be also part of the IVogen<,> interface, as the IsInitialized() already is

If speaking about EF Core scenarios, I'll try to take a look at it, especially with the snapshot functionality

@Aragas
Copy link
Author

Aragas commented May 21, 2024

Another point with IsInitialized() - right now it's an optional feature.
The current SAM(Static Abstract Members) implementation will include it into the main IVogen<> interface if specified by a config, which enforces every Vogen type to have it implemented.
Since not every type can/might have it, maybe it would make sense to create a separate interface IVogenHasIsInitialized<> which is automatically inherited if the method is not omited

@Aragas
Copy link
Author

Aragas commented May 21, 2024

And if Copy() will also be implemented as an optional feature, the same would apply to it

@Aragas
Copy link
Author

Aragas commented May 22, 2024

I also believe that I might have been wrong with the notnull constraint, since theoretically null values are permitted in reference types, we should remove it to avoid confusion

where TPrimitive : notnull

SteveDunn added a commit that referenced this issue May 26, 2024
The non-null constraint on TPrimitive, found in the IVogen interface in the WriteStaticAbstracts.cs file, has been removed based on comment: #511 (comment)
@SteveDunn
Copy link
Owner

Released in 4.0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants