Union definition skipped with a cyclic include [Go, Go 1.17, Linux, v2.0.0]

[capnspacehook]

I think I found a bug when two flatbuffer schema files include each other. Minimal reproducible example:

test1.fbs:

include "test2.fbs";

namespace flattest;

table Table1 {
    foo:Union1;
}

table Table2 {}

test2.fbs:

include "test1.fbs";

namespace flattest;

union Union1 {
    Table3
}

table Table3 {
    bar:Table2;
}

I ran flatc -g test1.fbs test2.fbs and it printed this error message:

error: /tmp/test2.fbs:12: 0: error: type referenced but not defined (check namespace): Union1, originally at: test1.fbs:6

It seems because both files include each other, somehow the definition for Union is not correctly parsed? Very strange.

If I don't include test1.fbs from test2.fbs, flatc generates code fine.

modified test2.fbs:

namespace flattest;

union Union1 {
    Table3
}

table Table3 {}

I assume this isn't specific to generating Go code but I haven't tried generating for other languages.

[dbaileychess]

You shouldn't be able to even have cycles in your include statements.

I think the real issue is that the include logic doesn't throw an error saying "Cycle detected in include statements" or something to that nature.

[capnspacehook]

I agree, at the very least the error message should be improved, it is very confusing. I specifically looked to see in the flatbuffer docs if include cycles are allowed or not, and it didn't seem to say either way. So if include cycles aren't allowed, that should be documented.

[aardappel]

I'm afraid we currently allow cycles, because we simply skip any include that has already been included (much like what you get in C/C++ with #ifdefs, but automatically). That may be necessary in some cases, even for just DAGs of includes.

It does have the downside that it is hard to know the ordering.

We could introduce an error specifically for cycles, by first checking if the current requested include file is on the "stack" of current includes. That would not preclude DAG use cases. I wonder though how many people we'd break.

[dbaileychess]

I see. Though the user could always refactor their schemas to not include any cycles if there is a reason. In the above example, Table2 and Union1 should be put into their own file and the two test.fbs reference the common file.

[dbaileychess]

I think we are leaning towards making it an error if a cycle is detected in schema includes. This may break some existing users, but they can always refactor their schemas to avoid cycles.

[github-actions[bot]]

This issue is stale because it has been open 6 months with no activity. Please comment or this will be closed in 14 days.

[github-actions[bot]]

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.