jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
511 stars 51 forks source link

Vector size in fbs file not compiling #79

Closed njannink closed 3 years ago

njannink commented 4 years ago

I have the following struct in my fbs schema file

struct Hash {
    sha256: [uint8:32];
}

When trying to compile this I get the following error:

Syntax error FBS file: Token=':', Msg='mismatched input ':' expecting ']'' Line='4:18'

According to the flatbuffer guide this is a valid schema. Is this a bug in the FlatSharp compiler?

jamescourtney commented 4 years ago

This is just something that isn't supported at the moment. The error messaging should be better in this case.

A separate question is what would it take for support to be added. The answer there is a bit more involved, since fixed-length vectors aren't really vectors at all, but just a syntax annotation. In terms of binary output, your schema is identical to:

struct Hash {
   sha256_0:uint8;
   sha256_1:uint8;
   sha256_2:uint8;
   ...
   sha256_31:uint8;
}

The problem is that FlatSharp has the expectation that there is only one way to serialize/parse a given C# type. So, if we're dealing with a type of byte[] from your example, that could be a "normal" vector or a "fixed-length" vector, and those have different binary representations, since the struct version is just syntax sugar.

I know this isn't very helpful. My best advice if you want to use the currently released version is to use a Table instead of a struct with a Memory vector for the hash, which will read the hash directly out of the input buffer without copying.

table SomeTableWithHash {
    Sha256:[ubyte] (VectorType:"memory");
}

If you're interested in trying to add support for this, I'm very open to contributions. There are two main ways I can think of it working right now:

njannink commented 4 years ago

For now I just use your suggestion of using a table with a hash

jamescourtney commented 3 years ago

After prototyping, my suggestion for IStructVector<T> won't work, because all struct vectors are going to be the same type, no matter what their actual length is, so we don't have a good way to distinguish between length X and length Y at codegen time.

I'm thinking now that this will need to be a compiler-only feature that injects some magic into the generated class to stitch it together, which is fine. It'll be a bit like gRPC -- something that's just not supported via the C# attribute-only approach.

njannink commented 3 years ago

Thanks a lot @jamescourtney! Keep up the good work

njannink commented 3 years ago

One thing I was wondering is. Is it really needed that these properties are public?

[FlatBufferItem(0)] public virtual byte __flatsharp__Hash_0 { get; set; }

Would be nicer if they are private or at least internal

jamescourtney commented 3 years ago

Protected is a great idea, and I really tried to make it work, but C# is pretty good at enforcing its access modifiers. I did add some attributes to the properties to hopefully hide them in intellisense (EditorBrowsableState).

jamescourtney commented 3 years ago

To expand on this, the real blocker comes in here:

[FlatBufferTable]
public class MyTable
{
    [FlatBufferItem(0)]
     protected virtual string SomeProperty { get; set; }
}

public class DerivedTable : MyTable
{
   // compiler error. We can only do this using the derived type
   public static string ExposeSomeProperty(MyTable table) => table.SomeProperty; 

   // works, but not useful :(
   public static string ExposeSomeProperty(DerivedTable derived) => derived.SomeProperty;
}
jamescourtney commented 3 years ago

Managed to get this working with protected properties! #120