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

Support for record hierarchies #12

Open
amis92 opened this issue Apr 9, 2017 · 10 comments
Open

Support for record hierarchies #12

amis92 opened this issue Apr 9, 2017 · 10 comments

Comments

@amis92
Copy link
Owner

@amis92 amis92 commented Apr 9, 2017

That'd require calling base class ctor with appropriate parameters.

We'd need some stub method probably to get a hint what parameters to pass, like:

private BaseClass BaseCtorCall() => new BaseClass(Property1, Property3);

That'd also require modifications to mutators, making them virtual for non-sealed classes and possibly more. That's just off the top of my head.

@amis92 amis92 added the enhancement label Apr 9, 2017
@amis92 amis92 added this to the Future milestone Apr 9, 2017
@amis92 amis92 added the API design label Aug 6, 2019
@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Aug 8, 2019

Noting down for future:

Let's start with the easiest scenario: closed hierarchy. So all base classes are abstract, and all leaf classes (non-abstract) are sealed. This gives us full knowledge about the hierarchy and allows for safe and well-implemented equalities, updates and base calls.

@amis92

This comment has been minimized.

@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Aug 8, 2019

Constructor is easy: parameter list is base.Properties.Concat(this.Properties), and pass base.Properties to base(…) call.

Builder is simply not included in abstract records, since they can't be instantiated. One could think about creating abstract Builder that'd then be inherited by leaf Builders, but it's for another discussion (optional QoL enhancement). After second thought, we can't have multiple Builder nested classes in a single leaf class. So if the BaseRecord has a Builder nested class, DerivedRecord couldn't declare Builder nested class again. This makes Builders only possible for Leaf records (sealed).

The most troublesome is the Update method for the core (ctor/with/builder) scenario.


Update needs to create a new instance with changed value. So the approach in the linked wham library is (given abstract BaseRecord and sealed, derived LeafRecord):

  • BaseRecord has a protected abstract BaseRecord UpdateBaseRecord(…); member
  • BaseRecord has a normal public BaseRecord Update(...) method which calls UpdateBaseRecord
  • LeafRecord has a public new LeafRecord Update(…) method that is implemented as today (instantiates the LeafRecord); new is required to hide BaseRecord.Update since it has a different return type.
  • LeafRecord has a protected sealed override BaseRecord UpdateBaseRecord(…) that calls the LeafRecord's own Update.
  • LeafRecord has all the With methods, and those that update BaseRecord's properties require new modifier as well, because they change the method's return type (all of BaseRecord's withs return BaseRecord, LeafRecord's - LeafRecord).

An example hierarchy like that in the linked repo is CostBaseCore and CostCore.

This hackaround is required because a method cannot be overridden with a different return type.


This approach also allows multiple abstract classes deriving one from another. An example hierarchy with multiple abstract classes in linked repo is SelectionEntryCore -> SelectionEntryBaseCore -> EntryBaseCore.

One difference from the simpler hierarchy is in SelectionEntryBaseCore, since it's both deriving and abstract:

  • it implements protected sealed override EntryBaseCore UpdateEntryBaseCore(…) that calls the regular Update
  • it also news the With methods for base record properties, same as LeafRecord would.
@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Aug 8, 2019

With that in mind, a design for the inheritance records could be written as follows.

We need to know the following about a given record:

  • is it abstract / sealed
  • does it have a base type (if yes, it has to be a Record and we need record descriptor for the base type - this descriptor's Entries include all Entries including those inherited)

This suggests extending RecordDescriptor with the following:

public bool IsSealed { get; }
public bool IsAbstract { get; }
public RecordDescriptor BaseDescriptor { get; }

Note: the actual design may differ. For example, maybe instead of a base descriptor we have a bool HasBaseRecord and Entries include all inherited entries that just know whether they're inherited or not.


Then the following rules apply ([abstract/sealed] partial class ThisRecord [ : BaseRecord ] {...}):

  • if IsAbstract (we're "inheritable", abstract partial class ThisRecord):
    • no Builder, ToBuilder members are generated
    • constructor should be protected as anything else makes little to no sense
    • protected abstract UpdateThisRecord method is generated
    • the generated Update method calls the abstract method above instead of directly instantiating the object
  • if BaseDescriptor != null (we're "inherited", partial class ThisRecord : BaseRecord)
    • IsAbstract || IsSealed, otherwise error
    • constructor
      • has parameters of BaseDescriptor.Entries followed by it's own Entries
      • calls base(…) with BaseDescriptor.Entries, the rest is assigned as normal
    • implement protected sealed override BaseRecord UpdateBaseRecord(…) with a call to Update
    • re-implement public new ThisRecord WithProperty(…) withers with a new modifier for BaseDescriptor.Entries

Those rules should suffice for handling all three expected scenarios:

  • abstract partial class BaseRecord {…}
  • abstract partial class MiddleRecord : BaseRecord {…}
  • sealed partial class LeafRecord : MiddleRecord {…}
@bboyle1234

This comment has been minimized.

Copy link

@bboyle1234 bboyle1234 commented Sep 22, 2019

The "new" keyword can help out making it all a bit easier.

// Not an abstract base class
public partial class BaseClass {
    public string BaseProperty1 { get; }
    public BaseClass Update(string baseProperty1) => null;
    public class Builder {
    }
}

public class NextClass  : BaseClass {
    public string NextClassProperty { get; }
    public NextClass Update(string baseProperty1, string nextClassProperty) => null;
    // new keyword gives us our builder back
    public new class Builder {
    }
}

// Rare case, an inheriting class without any extra properties
public class NextClassWithoutMoreProperties : BaseClass {
    // new keywork here helps out with the Update method
    public new NextClassWithoutMoreProperties Update(string baseProperty1) => null;
    public new class Builder {
    }
}
@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Sep 23, 2019

@bboyle1234

BaseClass @base = new NextClass("a", "b");
var baseBuilder = @base.ToBuilder();

The ToBuilder method will have to be newed as well because the return type changes, and as such virtual dispatch doesn't work, and as a result the implementation from BaseClass is called, and the BaseClass.Builder is instantiated and the information about "b" is lost.

NextClass.Builder has to inherit from BaseClass.Builder, and the ToBuilder will have to implement the same double-dispatch as with the Update. And the same will have to be done with ToImmutable on the Builder.

So, in full, the Builders and Update, skipping Withs, would look like that:

    public class A
    {
        public int Foo { get; }

        // generated
        public A(int foo) => Foo = foo;
        public A Update(int foo) => UpdateA(foo);
        protected virtual A UpdateA(int foo) => new A(foo);
        public Builder ToBuilder() => ToBuilderA();
        protected virtual Builder ToBuilderA() => new Builder { Foo = Foo };

        public class Builder
        {
            public int Foo { get; set; }

            public A ToImmutable() => ToImmutableA();

            protected virtual A ToImmutableA() => new A(Foo);
        }
    }

    public class B : A
    {
        public string Bar { get; }

        // generated
        public B(int foo, string bar) : base(foo) => Bar = bar;
        public B Update(int foo, string bar) => UpdateB(foo, bar);
        protected virtual B UpdateB(int foo, string bar) => new B(foo, bar);
        protected sealed override A UpdateA(int foo) => UpdateB(foo, Bar);
        public new Builder ToBuilder() => ToBuilderB();
        protected sealed override A.Builder ToBuilderA() => ToBuilderB();
        protected virtual Builder ToBuilderB() => new Builder { Foo = Foo, Bar = Bar };

        public new class Builder : A.Builder
        {
            public string Bar { get; set; }
            public new B ToImmutable() => ToImmutableB();
            protected sealed override A ToImmutableA() => ToImmutableB();
            protected virtual B ToImmutableB() => new B(Foo, Bar);
        }
    }

This looks like it could work. But it looks damn complex.


Unbound (not abstract+sealed only) inheritance makes it impossible to safely implement equality though. And it's more complicated. I'd prefer to start with the closed hierarchy approach, as most actual hierarchies should be doable as such, and only after that's solid and working, consider addition of support for open hierarchies.

@bboyle1234

This comment has been minimized.

Copy link

@bboyle1234 bboyle1234 commented Sep 26, 2019

Well done.
I would choose not to support inheritance.
I have hundreds of classes inheriting from something like this:

[Record]
public partial class Event { 
  public Guid Id { get; }
  public TimeStamp At { get; }
}

I was prepared to create an interface instead, and have all inheriting event objects implement that interface, whilst I would manually copy-paste those two property fields into each one.

public interface IEvent { 
  Guid Id  { get; }
  TimeStamp At { get; }
}

Having the inheritance feature is tempting, but so far all the implementations I can think of would not have great performance.

@bboyle1234

This comment has been minimized.

Copy link

@bboyle1234 bboyle1234 commented Sep 26, 2019

oooo now I'm thinking, why not autogen the interface properties!!!

public interface IEvent { 
  Guid Id { get; }
  TimeStamp At { get; }
} 

[Record(GenerateInterfaces(typeof(IEvent)))] // The IEvent fields will be auto-genned!!! This will compile just fine.
public partial class SomeEvent : IEvent { 
  string EventData { get; }
}

If custom behaviour is needed based on interface properties, those behaviours could be implemented as extension methods for the interface, replacing the need for any "base class" methods to be inherited.
And this has perfectly-fast performance too.

@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Sep 26, 2019

In your scenario, why would this not be enough?

public interface IEvent
{
    Guid Id { get; }
    TimeStamp At { get; }
}

[Record]
public abstract partial class EventBase : IEvent
{
    public Guid Id { get; }
    public TimeStamp At { get; }
}

This is compatible with the initial proposal, and makes the base class sufficient for use with derived events?

@amis92

This comment has been minimized.

Copy link
Owner Author

@amis92 amis92 commented Sep 26, 2019

why not autogen the interface properties

Well, I'd really like to keep requiring all the properties being user-defined. I could imagine generating an interface from a record, like

[Record(GeneratedInterfaceName = "IEvent")]
public abstract partial class EventBase
{
    public Guid Id { get; }
    public TimeStamp At { get; }
}
// generates
public interface IEvent
{
    Guid Id { get; }
    TimeStamp At { get; }
}
partial class EventBase : IEvent
{
    // ...
}

But not the other way around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.