[idl_parser] Check structs and enums do not clash in a namespace (#6562)

* [idl_parser] Check structs and enums do not clash in a namespace

Uses fully qualified names to check for clashes within a given namespace whether explicitly defined or in the global namespace.

* [idl_parser] Move type name clash check to ParseEnum and ParseDecl

Change point at which parsing error is returned to ensure error is caught more generally. This change means that the error is returned after parsing the entirety of the offending duplicate rather than at the start when parsing it's name.

* [idl_parser] Add single and multi file type name clash tests

Adds a selection of tests for valid single file schemas with types that have the same name but are in different namespaces.
Adds a test for an a valid schema that spans two files with two types that have the same name but are in different namespaces.
Adds a test for an an invalid schema that spans two files with two types that have the same name and are in the same namespace.

src/idl_parser.cpp

@@ -2407,14 +2407,18 @@ CheckedError Parser::ParseEnum(const bool is_union, EnumDef **dest) {
   }
 
   if (dest) *dest = enum_def;
-  types_.Add(current_namespace_->GetFullyQualifiedName(enum_def->name),
+  const auto qualified_name =
-             new Type(BASE_TYPE_UNION, nullptr, enum_def));
+      current_namespace_->GetFullyQualifiedName(enum_def->name);
+  if (types_.Add(qualified_name, new Type(BASE_TYPE_UNION, nullptr, enum_def)))
+    return Error("datatype already exists: " + qualified_name);
   return NoError();
 }
 
 CheckedError Parser::StartStruct(const std::string &name, StructDef **dest) {
   auto &struct_def = *LookupCreateStruct(name, true, true);
-  if (!struct_def.predecl) return Error("datatype already exists: " + name);
+  if (!struct_def.predecl)
+    return Error("datatype already exists: " +
+                 current_namespace_->GetFullyQualifiedName(name));
   struct_def.predecl = false;
   struct_def.name = name;
   struct_def.file = file_being_parsed_;
@@ -2594,8 +2598,11 @@ CheckedError Parser::ParseDecl() {
   ECHECK(CheckClash(fields, struct_def, "_byte_vector", BASE_TYPE_STRING));
   ECHECK(CheckClash(fields, struct_def, "ByteVector", BASE_TYPE_STRING));
   EXPECT('}');
-  types_.Add(current_namespace_->GetFullyQualifiedName(struct_def->name),
+  const auto qualified_name =
-             new Type(BASE_TYPE_STRUCT, struct_def, nullptr));
+      current_namespace_->GetFullyQualifiedName(struct_def->name);
+  if (types_.Add(qualified_name,
+                 new Type(BASE_TYPE_STRUCT, struct_def, nullptr)))
+    return Error("datatype already exists: " + qualified_name);
   return NoError();
 }
 
@@ -2747,17 +2754,17 @@ CheckedError Parser::ParseProtoDecl() {
   return NoError();
 }
 
-CheckedError Parser::StartEnum(const std::string &enum_name, bool is_union,
+CheckedError Parser::StartEnum(const std::string &name, bool is_union,
                                EnumDef **dest) {
   auto &enum_def = *new EnumDef();
-  enum_def.name = enum_name;
+  enum_def.name = name;
   enum_def.file = file_being_parsed_;
   enum_def.doc_comment = doc_comment_;
   enum_def.is_union = is_union;
   enum_def.defined_namespace = current_namespace_;
-  if (enums_.Add(current_namespace_->GetFullyQualifiedName(enum_name),
+  const auto qualified_name = current_namespace_->GetFullyQualifiedName(name);
-                 &enum_def))
+  if (enums_.Add(qualified_name, &enum_def))
-    return Error("enum already exists: " + enum_name);
+    return Error("enum already exists: " + qualified_name);
   enum_def.underlying_type.base_type =
       is_union ? BASE_TYPE_UTYPE : BASE_TYPE_INT;
   enum_def.underlying_type.enum_def = &enum_def;

tests/BUILD.bazel

@@ -43,6 +43,10 @@ cc_test(
         ":monsterdata_extra.json",
         ":monsterdata_test.golden",
         ":monsterdata_test.json",
+        ":name_clash_test/invalid_test1.fbs",
+        ":name_clash_test/invalid_test2.fbs",
+        ":name_clash_test/valid_test1.fbs",
+        ":name_clash_test/valid_test2.fbs",
         ":native_type_test.fbs",
         ":optional_scalars.fbs",
         ":prototest/imported.proto",

tests/name_clash_test/invalid_test1.fbs

@@ -0,0 +1,5 @@
+include "invalid_test2.fbs";
+
+namespace A;
+
+table X {}

tests/name_clash_test/invalid_test2.fbs

@@ -0,0 +1,3 @@
+namespace A;
+
+union X {}

tests/name_clash_test/valid_test1.fbs

@@ -0,0 +1,5 @@
+include "valid_test2.fbs";
+
+namespace A;
+
+table X {}

tests/name_clash_test/valid_test2.fbs

@@ -0,0 +1,3 @@
+namespace B;
+
+union X {}

tests/test.cpp

@@ -1667,6 +1667,12 @@ void ErrorTest() {
   TestError("enum X:byte { Y, Y }", "value already");
   TestError("enum X:byte { Y=2, Z=2 }", "unique");
   TestError("table X { Y:int; } table X {", "datatype already");
+  TestError("table X { } union X { }", "datatype already");
+  TestError("union X { } table X { }", "datatype already");
+  TestError("namespace A; table X { } namespace A; union X { }",
+            "datatype already");
+  TestError("namespace A; union X { } namespace A; table X { }",
+            "datatype already");
   TestError("struct X (force_align: 7) { Y:int; }", "force_align");
   TestError("struct X {}", "size 0");
   TestError("{}", "no root");
@@ -2515,6 +2521,49 @@ void ParseUnionTest() {
           true);
 }
 
+void ValidSameNameDifferentNamespaceTest() {
+  // Duplicate table names in different namespaces must be parsable
+  TEST_ASSERT(flatbuffers::Parser().Parse(
+      "namespace A; table X {} namespace B; table X {}"));
+  // Duplicate union names in different namespaces must be parsable
+  TEST_ASSERT(flatbuffers::Parser().Parse(
+      "namespace A; union X {} namespace B; union X {}"));
+  // Clashing table and union names in different namespaces must be parsable
+  TEST_ASSERT(flatbuffers::Parser().Parse(
+      "namespace A; table X {} namespace B; union X {}"));
+  TEST_ASSERT(flatbuffers::Parser().Parse(
+      "namespace A; union X {} namespace B; table X {}"));
+}
+
+void MultiFileNameClashTest() {
+  const auto name_clash_path =
+      flatbuffers::ConCatPathFileName(test_data_path, "name_clash_test");
+  const char *include_directories[] = { name_clash_path.c_str() };
+
+  // Load valid 2 file Flatbuffer schema
+  const auto valid_path =
+      flatbuffers::ConCatPathFileName(name_clash_path, "valid_test1.fbs");
+  std::string valid_schema;
+  TEST_ASSERT(flatbuffers::LoadFile(valid_path.c_str(), false, &valid_schema));
+  // Clashing table and union names in different namespaces must be parsable
+  TEST_ASSERT(
+      flatbuffers::Parser().Parse(valid_schema.c_str(), include_directories));
+
+  flatbuffers::Parser p;
+  TEST_ASSERT(p.Parse(valid_schema.c_str(), include_directories));
+
+  // Load invalid 2 file Flatbuffer schema
+  const auto invalid_path =
+      flatbuffers::ConCatPathFileName(name_clash_path, "invalid_test1.fbs");
+  std::string invalid_schema;
+  TEST_ASSERT(
+      flatbuffers::LoadFile(invalid_path.c_str(), false, &invalid_schema));
+  // Clashing table and union names in same namespace must fail to parse
+  TEST_EQ(
+      flatbuffers::Parser().Parse(invalid_schema.c_str(), include_directories),
+      false);
+}
+
 void InvalidNestedFlatbufferTest() {
   // First, load and parse FlatBuffer schema (.fbs)
   std::string schemafile;
@@ -3980,6 +4029,8 @@ int FlatBufferTests() {
   InvalidUTF8Test();
   UnknownFieldsTest();
   ParseUnionTest();
+  ValidSameNameDifferentNamespaceTest();
+  MultiFileNameClashTest();
   InvalidNestedFlatbufferTest();
   ConformTest();
   ParseProtoBufAsciiTest();