dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
632 stars 180 forks source link

bug with option --json-printer and vector of enums #182

Closed jobol closed 3 years ago

jobol commented 3 years ago

The command

flatcc --json-printer bug.fbs

crashes when processing the schema bug.fbs below

enum id : int {
    none = 0
}

table get_in {
    ids: [id];
}

The message it prints is:

flatcc: ...flatcc/src/compiler/codegen_c.h:102: scalar_type_prefix: Assertion `0' failed.
Aborted (core dumped)
mikkelfj commented 3 years ago

Is that on master from today, or ealier. I just pushed a change affecting enums in semantics.h

mikkelfj commented 3 years ago

https://github.com/dvidelabs/flatcc/commit/149f6d3f92ebdf0aeeba4a873113e2936d8a25df

mikkelfj commented 3 years ago

Happens both before and after that commit.

mikkelfj commented 3 years ago

Please check https://github.com/dvidelabs/flatcc/tree/issue182

mikkelfj commented 3 years ago

It appears that no one ever attempted to print vectors of enums to JSON? Certainly the symbol table was accessed incorrectly in this case, triggering an internal error.

jobol commented 3 years ago

the fix works also here. thank you maybe adding the case in monster_test.fbs is valuable

mikkelfj commented 3 years ago

Are you up for that?

mikkelfj commented 3 years ago

If so, please note that the main monster table attempts to keep in sync with flatc monster test, at least from time to time. Therefore other tests are added to other tables in schema.

jobol commented 3 years ago

no sorry adding just 3 lines seems suddenly very confuse.

In an experiment, added a thing like

table testjsonprinter1 {
   colors: [Color];
}

in monster_test.fbs and it worked, that crashed!

mikkelfj commented 3 years ago

I'm getting an error

monster_test.fbs:367:13: error: 'Color': unknown vector type reference used with table field: monster_test.fbs:367:4: 'colors'
output failed

If I add the testjsonprinter1 last in the monster_test file. This is probably a namespace issue. You get a crash? If I add the enum and the table in a separate file, it seems to run OK.

mikkelfj commented 3 years ago

If I add the following last in monster_test, it also appears to be working:

366 namespace MyGame.Example;
367
368 table testjsonprinter1 {
369    colors: [Color];
370 }
mikkelfj commented 3 years ago

However, running the tests with the above change results in a compile error:

test/json_test/generated/monster_test_json_printer.h:670:76: error: too many arguments to function call, expected 6, have 7
    flatcc_json_printer_int8_enum_vector_field(ctx, td, 0, "colors", 6, 0, MyGame_Example_Color_print_json_enum);
jobol commented 3 years ago

I have not compiled the generated code.... (postponed) trying that now....

mikkelfj commented 3 years ago

Just pushed a fix for the extra argument, looks like a copy paste bug.

jobol commented 3 years ago

I tested on my side and it works

mikkelfj commented 3 years ago

So how about tests?

jobol commented 3 years ago

I have a schema and I can parse pipe print json items. flatcc with your fixes works very well

mikkelfj commented 3 years ago

Does the printed output also work? If so, maybe we should just merge the fix without tests.

Ideally the json printer test should be updated, but that is a lot of work, and I have changed all the JSON tests on an unpublished branch.

mikkelfj commented 3 years ago

Rereading your comment, you mean that you can pipe printed output to the parser? If so, we are good.

jobol commented 3 years ago

I did the opposite, I used the parser to create a memory object that I printed then. A tiny main using generated headers. I just checked the buggy case. I am not about to push anything because I have nothing.

mikkelfj commented 3 years ago

Can you check that the printed output can be parsed, or at least is valid JSON?

jobol commented 3 years ago

Using schema

enum id : ushort {
    none = 0,
    led1 = 101,
    led2 = 102,
    led3 = 103,
    button1 = 401,
    button2 = 402,
    button3 = 403
}
table get_in {
    ids: [id];
}

I got:

$ ./a.out '{"ids":[101,"led1",0]}'
{"ids":["led1","led1","none"]}
$ ./a.out '{"ids":[20,400,401]}'
{"ids":[20,400,"button1"]}

What seems correct IMHO

mikkelfj commented 3 years ago

Yes that looks fine, I'll merge and close. Thanks for reporting and testing.

mikkelfj commented 3 years ago

Merge to master