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

Record struct union support #101

Open
domn1995 opened this issue Jan 6, 2023 · 8 comments
Open

Record struct union support #101

domn1995 opened this issue Jan 6, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@domn1995
Copy link
Owner

domn1995 commented Jan 6, 2023

We should support declaring unions as record structs for consumers that want value semantics and reduced memory allocations. For example:

using Dunet;

[Union]
partial record struct Option<T>
{
    partial record struct Some(T Value);
    partial record struct None;
}

Mixing and matching structs and classes should not be allowed.

Design note: Since using structs we cannot use abstract and inheritance to resolve the union type. We'll have to use a private discriminant like an enum to support it and switch on it when matching.

@domn1995
Copy link
Owner Author

domn1995 commented Jan 6, 2023

@domn1995
Copy link
Owner Author

domn1995 commented Jan 6, 2023

Can we make the record struct readonly?

@domn1995 domn1995 added the enhancement New feature or request label Jan 6, 2023
@iamim
Copy link

iamim commented Feb 3, 2023

I checked some examples and here are my 2 cents.

I remember reading a discussion somewhere in F# GitHub issues or RFCs the struct DU implementation. It mentioned that Rust implemented it in a smarter way by storing less data and reusing fields. Unfortunately, I can't find the discussion now and I forgot how it's properly called.

Here's what I mean. Consider your Shape example. The size of Shape is ShapeType + 1 double for Circle + 2 double for Rectangle + 2 double for Triangle + padding. It makes the struct rather large and it looses a lot of it's benefits. It gets worse as you add more shape cases.

Instead you could store just 2 private double fields (_d1 and _d2) in Shape + a ShapeType field. When matching you would choose how to marshal the stored memory (2 doubles) depending on the tag value: for Circle only _d1 is used, for Rectangle _d1 is interpreted as Height and _d2 is encoded as Width, etc.

For reference types you could store an object and then cast it to a specific class.

Obviously, it gets much more difficult with generics and you may want to fallback to the way you implemented it so far or ban generics for struct unions. To be honest it's not the strongest point of C# type inference anyways. It's rather tiring to type Result<int, Exception>. part over and over again :) At the same time it's the simple unions (Option, Result) are the best suited for struct (?).

Also important to notice, I'm not that proficient in lower level stuff. I would definitely double check my claims :)

@NotAsea
Copy link

NotAsea commented Jan 14, 2024

i thought about it and you can support struct just fine, both readonly struct or readonly record struct, but now instead of nested case directly in union struct, you store their value of each case, with this you can support bot primitive, generic and object just find, example

[Union]
public partial readonly record struct Shape<T, T1, T2>
{
    public static partial Shape Rectangle (T value) // note that that case type now use Base union directly rather than struct
    public static partial Shape Circle(T1 value, T2 value)
}
//got generated as
partial readonly record struct Shape<T, T1, T2>
{
    private enum ShapeField: byte //adjust base on number of case
    {
        Rectangle, Circle
    }
    private readonly T value1; // Rectangle value
    private readonly T1 value2;  // both value of Circle
    private readonly T2 value3;
    private ShapeField  _flag;  // store flag
    
    private Shape(ShapeField flag ,T value = default!, T1 value1 = default!, T value2 = default) // private constructor, prevent other to construct
    public static partial Shape Rectangle(T vale) => new Shape(ShapeField.Rectangle, value ); // create case 
  // ...
}

//helper class
public static class ShapeStatic
{
   public static Shape<T,T1,T2> Rectangle<T,T1,T2>(T value) => Shape<T,T1,T2>.Rectangle(value);
}

@domn1995
Copy link
Owner Author

@NotAsea 100% agree. I have a prototype with almost that exact implementation. The only issue I'm trying to solve is preserving the current method of instantiating a union variant with a simple new Union.Variant() call or coming up with a better method. Feel free to explore it some more and share your findings.

@NotAsea
Copy link

NotAsea commented Jan 14, 2024

well with my code above you already can Shape.Circle(value) as it's a static method of struct, we can introduce to user to add

using static Dunet.Helper.Shape // namspace of generated ShapeStatic
 
var shape = Retangle(123); // Rectangle is static method

, but i thought other problem that it seem not align with your record side, as users can accidentally type struct in case of union

@domn1995
Copy link
Owner Author

domn1995 commented Jan 14, 2024

The problem I was running into is that the static method collides with the variant type, since, at least with the current implementation, the type is declared within the union struct.

// User writes this.
[Union]
public partial record struct Shape
{ 
  public partial record struct Circle(double Radius);
}

// Generator creates this. 
// But the `Circle()` method collides with the variant declaration above.
public partial record struct Shape
{
  public static Shape Circle(double radius) => ...;
}

@NotAsea
Copy link

NotAsea commented Jan 14, 2024

The problem is if you still decide embed struct variant inside base struct union, you will cause your struct container very large and defeat its benefit compare to record class (heck even make it slower if value is very big), and of course increase complexity too. My current thought is using static method as variant directly and generator can retrieve all their value and just generate necessity backing field and a simple flag to switch, it even has benefit as if variant value is same type as value of other variant we can reuse backing field of another variant thus reduce memory need . (but loose ability to use switch statement/expression)

Back to our problem, my solution is generate an additional static class for our union and instruct user to using static that helper class so they can just var shape = Circle(3.4); that only clean solution i can think of (and checking your example you already have its answer)
anyway for the code of the second point

public partial readonly struct Shape<T,T1,T2>
{
// sniff
}

public static class Shape 
{ 
    public static Shape<T, T1, T2> Circle<T, T1, T2>(T1 value1, T2 value1) => new Shape.Circle(value1, value2);
}

// user code
using static Shape
 var shape = Circle(1,3);

note that this only work if our struct union is generic

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

3 participants