From f83ee1af597c5bdd474e4a9a1b99136758ef820b Mon Sep 17 00:00:00 2001 From: Fraser Crossman Date: Fri, 14 May 2021 21:56:52 +0100 Subject: [PATCH] [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 | 27 ++++++++----- tests/BUILD.bazel | 4 ++ tests/name_clash_test/invalid_test1.fbs | 5 +++ tests/name_clash_test/invalid_test2.fbs | 3 ++ tests/name_clash_test/valid_test1.fbs | 5 +++ tests/name_clash_test/valid_test2.fbs | 3 ++ tests/test.cpp | 51 +++++++++++++++++++++++++ 7 files changed, 88 insertions(+), 10 deletions(-) create mode 100644 tests/name_clash_test/invalid_test1.fbs create mode 100644 tests/name_clash_test/invalid_test2.fbs create mode 100644 tests/name_clash_test/valid_test1.fbs create mode 100644 tests/name_clash_test/valid_test2.fbs 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();