diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index bcbf476bb..f486e0788 100644 --- a/src/idl_parser.cpp +++ b/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), - new Type(BASE_TYPE_UNION, nullptr, enum_def)); + const auto qualified_name = + 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), - new Type(BASE_TYPE_STRUCT, struct_def, nullptr)); + const auto qualified_name = + 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), - &enum_def)) - return Error("enum already exists: " + enum_name); + const auto qualified_name = current_namespace_->GetFullyQualifiedName(name); + if (enums_.Add(qualified_name, &enum_def)) + 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; diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel index 856431c58..7b2f28477 100644 --- a/tests/BUILD.bazel +++ b/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", diff --git a/tests/name_clash_test/invalid_test1.fbs b/tests/name_clash_test/invalid_test1.fbs new file mode 100644 index 000000000..aa7213fab --- /dev/null +++ b/tests/name_clash_test/invalid_test1.fbs @@ -0,0 +1,5 @@ +include "invalid_test2.fbs"; + +namespace A; + +table X {} diff --git a/tests/name_clash_test/invalid_test2.fbs b/tests/name_clash_test/invalid_test2.fbs new file mode 100644 index 000000000..6d70e9c64 --- /dev/null +++ b/tests/name_clash_test/invalid_test2.fbs @@ -0,0 +1,3 @@ +namespace A; + +union X {} diff --git a/tests/name_clash_test/valid_test1.fbs b/tests/name_clash_test/valid_test1.fbs new file mode 100644 index 000000000..d61dea604 --- /dev/null +++ b/tests/name_clash_test/valid_test1.fbs @@ -0,0 +1,5 @@ +include "valid_test2.fbs"; + +namespace A; + +table X {} diff --git a/tests/name_clash_test/valid_test2.fbs b/tests/name_clash_test/valid_test2.fbs new file mode 100644 index 000000000..78edad6fe --- /dev/null +++ b/tests/name_clash_test/valid_test2.fbs @@ -0,0 +1,3 @@ +namespace B; + +union X {} diff --git a/tests/test.cpp b/tests/test.cpp index 2c0033979..f2d7207c5 100644 --- a/tests/test.cpp +++ b/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();