jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
497 stars 50 forks source link

Union does not support using the same type #411

Closed jdharmawan closed 10 months ago

jdharmawan commented 10 months ago

Flatsharp union don't support using the same type multiple times in a Union. This is incompatible with Google.Flatbuffers syntax. I feel that this issue needs to be addressed.

Schema used for testing:

enum Material : uint8 {
    Steel,
    Wood,
    Iron,
}

struct Sword {
    damage: int32;
    material: Material;
}

struct Halberd {
    damage: int32;
    tip_material: Material;
    pole_material: Material;
}

union Weapons {
    Claymore: Sword,
    Rapier: Sword,
    Halberd,
}

Error when trying to compile

// !! FLATSHARP CODE GENERATION FAILED. THIS FILE MAY CONTAIN INCOMPLETE OR INACCURATE DATA !!
/* Error: 
FlatSharp.Compiler.InvalidFbsFileException: Errors in FBS schema: 
FlatSharp unions may not contain duplicate types. Union = Weapon.Weapons
   at FlatSharp.Compiler.ErrorContext.ThrowIfHasErrors() in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\ErrorContext.cs:line 41
   at FlatSharp.Compiler.FlatSharpCompiler.CreateCSharp(List`1 bfbs, String inputHash, CompilerOptions options) in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 584
   at FlatSharp.Compiler.FlatSharpCompiler.RunCompiler(CompilerOptions options) in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 124
*/
jamescourtney commented 10 months ago

Thanks for reaching out. The behavior that you're observing is currently an intentional design choice in FlatSharp. The root of the reason for this choice is the way that FlatSharp defines Unions:

public struct Weapons : IFlatBufferUnion<Sword, Sword, Halberd>
{
    // Constructor for 'Claymore'
    public Weapons(Sword sword) { }

    // Constructor for 'Rapier'.
    public Weapons(Sword sword) { }

    public Weapons(Halberd halberd) { }
}

Unfortunately, this means that the C# compiler can't distinguish between the constructors for Claymore and Rapier. So, Flatsharp emits this error before the C# compiler can emit a more confusing error.

Now, there are some other approaches that might work, but they would be breaking changes. For example:

public struct Weapons : IFlatBufferUnion<Sword, Sword, Halberd>
{
    public static Weapons NewRapier(Sword s) { }
    public static Weapons NewClaymore(Sword s) { }
    public static Weapons NewHalberd(Halberd h) { }
}

I think this is worth revisiting the next time I do a major revision to FlatSharp, but I take breaking changes really seriously, so I want to be thoughtful about it.

In the meantime, Wouldn't it be logical for your Sword struct to have a type?


enum SwordKind : ubyte {
   Rapier = 0,
   Claymore = 1,
}

struct Sword {  
    damage: int32;
    material: Material;
    kind : SwordKind;
}
jdharmawan commented 10 months ago

Thank you for the support and clarifications.

I'll think of an alternative solution in the meantime.