jamescourtney / FlatSharp

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

Generated defaults fail to handle enums with negative int & large ulong #372

Closed kwsch closed 1 year ago

kwsch commented 1 year ago

Repro fbs (w/ latest 7.1.0 compiler & runtime NuGet pkg):

namespace Test;

enum IntEnum : int { Neg1 = -1 }
enum UlongEnum : ulong { Bit63Set = 0xCBF29CE484222645 }

table DummyObject {
  S32:IntEnum = Neg1;
  U64:UlongEnum = Bit63Set;
}

Generated output:

// !! FLATSHARP CODE GENERATION FAILED. THIS FILE MAY CONTAIN INCOMPLETE OR INACCURATE DATA !!
/* Error: 
FlatSharp.FlatSharpCompilationException: FlatSharp failed to generate proper C# for your schema.

FlatSharp compilation error: (82,53): error CS0119: 'IntEnum' is a type, which is not valid in the given context, Context = "Test.IntEnum"
FlatSharp compilation error: (82,52): error CS0075: To cast a negative value, you must enclose the value in parentheses., Context = "(Test.IntEnum) - 1"
FlatSharp compilation error: (82,53): error CS0119: 'IntEnum' is a type, which is not valid in the given context, Context = "Test.IntEnum"
FlatSharp compilation error: (35,20): error CS0031: Constant value '-3750763034362894779' cannot be converted to a 'ulong', Context = "-3750763034362894779"
FlatSharp compilation error: (55,25): error CS0119: 'IntEnum' is a type, which is not valid in the given context, Context = "Test.IntEnum"
FlatSharp compilation error: (55,24): error CS0075: To cast a negative value, you must enclose the value in parentheses., Context = "(Test.IntEnum) - 1"
FlatSharp compilation error: (55,25): error CS0119: 'IntEnum' is a type, which is not valid in the given context, Context = "Test.IntEnum"

   at FlatSharp.CodeGen.RoslynSerializerGenerator.ThrowOnEmitFailure(EmitResult result, SyntaxTree syntaxTree, Func`1 cSharpFactory) in D:\a\FlatSharp\FlatSharp\src\FlatSharp\Serialization\RoslynSerializerGenerator.cs:line 356
   at FlatSharp.CodeGen.RoslynSerializerGenerator.CompileAssembly(String sourceCode, Boolean enableAppDomainIntercept, Assembly[] additionalReferences) in D:\a\FlatSharp\FlatSharp\src\FlatSharp\Serialization\RoslynSerializerGenerator.cs:line 229
   at FlatSharp.Compiler.FlatSharpCompiler.<>c__DisplayClass17_1.<CreateCSharp>b__3() in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 561
   at FlatSharp.Compiler.FlatSharpCompiler.Instrument[T](String step, CompilerOptions opts, Func`1 callback) in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 647
   at FlatSharp.Compiler.FlatSharpCompiler.CreateCSharp(List`1 bfbs, String inputHash, CompilerOptions options) in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 0
   at FlatSharp.Compiler.FlatSharpCompiler.RunCompiler(CompilerOptions options) in D:\a\FlatSharp\FlatSharp\src\FlatSharp.Compiler\FlatSharpCompiler.cs:line 118
*/

Expected behavior: Compiler would be able to generate its code with a negative default enum value, and high-ulong enum value. Same behavior exists for signed byte type.

Changing the default value to 0 generates the code as expected. Removing the enum and using the primitive directly (int/ulong) & default value works as expected too.

jamescourtney commented 1 year ago

Thanks for raising this! The -1 issue is definitely a bug that I'll work on getting fixed soon. Real Work is quite busy right now, but it should be a small fix. I do have negative-value enum tests, but it seems that there are no tests that cover the combination of enum default + negative value.

The error with the large ulong value was, unfortunately, not fixable the last time I checked. The reason for this is that FlatSharp depends upon flatc (the Google compiler) to parse the FBS files. Unfortunately, flatc only exposes a signed 64 bit value for the enum default. I did check on this at one point and my conclusion was that default values over long.MaxValue were not possible. However, it has been awhile and I will confirm that this remains the case.

jamescourtney commented 1 year ago

373 addresses this issue as best I can. The included tests also show the limits of flatc. I would love to support defaults values for ulongs. If you want to raise this issue in google/FlatBuffers, I'll be delighted to consume the fixed version.