diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h --- a/clang/include/clang/AST/ASTImporter.h +++ b/clang/include/clang/AST/ASTImporter.h @@ -517,12 +517,11 @@ /// /// \param NumDecls the number of conflicting declarations in \p Decls. /// - /// \returns the name that the newly-imported declaration should have. - virtual DeclarationName HandleNameConflict(DeclarationName Name, - DeclContext *DC, - unsigned IDNS, - NamedDecl **Decls, - unsigned NumDecls); + /// \returns the name that the newly-imported declaration should have. Or + /// an error if we can't handle the name conflict. + virtual Expected + HandleNameConflict(DeclarationName Name, DeclContext *DC, unsigned IDNS, + NamedDecl **Decls, unsigned NumDecls); /// Retrieve the context that AST nodes are being imported into. ASTContext &getToContext() const { return ToContext; } diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -80,6 +80,7 @@ using ExpectedExpr = llvm::Expected; using ExpectedDecl = llvm::Expected; using ExpectedSLoc = llvm::Expected; + using ExpectedName = llvm::Expected; std::string ImportError::toString() const { // FIXME: Improve error texts. @@ -2247,11 +2248,11 @@ } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, Decl::IDNS_Namespace, ConflictingDecls.data(), + ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -2355,21 +2356,19 @@ // already have a complete underlying type then return with that. if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType()) return Importer.MapImported(D, FoundTypedef); + // FIXME Handle redecl chain. When you do that make consistent changes + // in ASTImporterLookupTable too. + } else { + ConflictingDecls.push_back(FoundDecl); } - // FIXME Handle redecl chain. When you do that make consistent changes - // in ASTImporterLookupTable too. - break; } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -2442,11 +2441,10 @@ } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -2550,17 +2548,16 @@ continue; if (IsStructuralMatch(D, FoundEnum)) return Importer.MapImported(D, FoundEnum); + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(SearchName, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + SearchName, DC, IDNS, ConflictingDecls.data(), + ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -2685,17 +2682,16 @@ PrevDecl = FoundRecord->getMostRecentDecl(); break; } - } - - ConflictingDecls.push_back(FoundDecl); + ConflictingDecls.push_back(FoundDecl); + } // kind is RecordDecl } // for if (!ConflictingDecls.empty() && SearchName) { - Name = Importer.HandleNameConflict(SearchName, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + SearchName, DC, IDNS, ConflictingDecls.data(), + ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -2854,17 +2850,15 @@ if (auto *FoundEnumConstant = dyn_cast(FoundDecl)) { if (IsStructuralMatch(D, FoundEnumConstant)) return Importer.MapImported(D, FoundEnumConstant); + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -3102,17 +3096,15 @@ << Name << D->getType() << FoundFunction->getType(); Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here) << FoundFunction->getType(); + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -3758,17 +3750,15 @@ << Name << D->getType() << FoundVar->getType(); Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here) << FoundVar->getType(); + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, IDNS, - ConflictingDecls.data(), - ConflictingDecls.size()); - if (!Name) - return make_error(ImportError::NameConflict); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } } @@ -5106,19 +5096,17 @@ // see ASTTests test ImportExistingFriendClassTemplateDef. continue; } + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary, - ConflictingDecls.data(), - ConflictingDecls.size()); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(), + ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } - - if (!Name) - return make_error(ImportError::NameConflict); } CXXRecordDecl *FromTemplated = D->getTemplatedDecl(); @@ -5391,22 +5379,18 @@ FoundTemplate->getTemplatedDecl()); return Importer.MapImported(D, FoundTemplate); } + ConflictingDecls.push_back(FoundDecl); } - - ConflictingDecls.push_back(FoundDecl); } if (!ConflictingDecls.empty()) { - Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary, - ConflictingDecls.data(), - ConflictingDecls.size()); + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, Decl::IDNS_Ordinary, ConflictingDecls.data(), + ConflictingDecls.size()); + if (!NameOrErr) + return NameOrErr.takeError(); } - if (!Name) - // FIXME: Is it possible to get other error than name conflict? - // (Put this `if` into the previous `if`?) - return make_error(ImportError::NameConflict); - VarDecl *DTemplated = D->getTemplatedDecl(); // Import the type. @@ -8743,12 +8727,12 @@ return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data()); } -DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name, - DeclContext *DC, - unsigned IDNS, - NamedDecl **Decls, - unsigned NumDecls) { - return Name; +Expected ASTImporter::HandleNameConflict(DeclarationName Name, + DeclContext *DC, + unsigned IDNS, + NamedDecl **Decls, + unsigned NumDecls) { + return make_error(ImportError::NameConflict); } DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) { diff --git a/clang/test/Analysis/Inputs/ctu-other.c b/clang/test/Analysis/Inputs/ctu-other.c --- a/clang/test/Analysis/Inputs/ctu-other.c +++ b/clang/test/Analysis/Inputs/ctu-other.c @@ -12,11 +12,11 @@ } // Test enums. -enum B { x = 42, - l, - s }; +enum B { x2 = 42, + y2, + z2 }; int enumCheck(void) { - return x; + return x2; } // Test reporting an error in macro definition diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -1879,7 +1879,7 @@ auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl(); // We expect one (ODR) warning during the import. EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings()); - EXPECT_EQ(2u, + EXPECT_EQ(1u, DeclCounter().match(ToTU, recordDecl(hasName("X")))); } @@ -2432,6 +2432,62 @@ EXPECT_EQ(ToD1, ToD2); } +TEST_P(ImportFunctionTemplates, + ImportFunctionWhenThereIsAFunTemplateWithSameName) { + getToTuDecl( + R"( + template + void foo(T) {} + void foo(); + )", + Lang_CXX); + Decl *FromTU = getTuDecl("void foo();", Lang_CXX); + auto *FromD = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("foo"))); + auto *ImportedD = Import(FromD, Lang_CXX); + EXPECT_TRUE(ImportedD); +} + +TEST_P(ImportFunctionTemplates, + ImportConstructorWhenThereIsAFunTemplateWithSameName) { + auto Code = + R"( + struct Foo { + template + Foo(T) {} + Foo(); + }; + )"; + getToTuDecl(Code, Lang_CXX); + Decl *FromTU = getTuDecl(Code, Lang_CXX); + auto *FromD = + LastDeclMatcher().match(FromTU, cxxConstructorDecl()); + auto *ImportedD = Import(FromD, Lang_CXX); + EXPECT_TRUE(ImportedD); +} + +TEST_P(ImportFunctionTemplates, + ImportOperatorWhenThereIsAFunTemplateWithSameName) { + getToTuDecl( + R"( + template + void operator<(T,T) {} + struct X{}; + void operator<(X, X); + )", + Lang_CXX); + Decl *FromTU = getTuDecl( + R"( + struct X{}; + void operator<(X, X); + )", + Lang_CXX); + auto *FromD = LastDeclMatcher().match( + FromTU, functionDecl(hasOverloadedOperatorName("<"))); + auto *ImportedD = Import(FromD, Lang_CXX); + EXPECT_TRUE(ImportedD); +} + struct ImportFriendFunctions : ImportFunctions {}; TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) { @@ -5127,15 +5183,6 @@ } } -INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest, - DefaultTestValuesForRunOptions, ); - -INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest, - ::testing::Values(ArgVector()), ); - -INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain, - ::testing::Values(ArgVector()), ); - TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) { Decl *FromTU = getTuDecl( R"( @@ -5233,8 +5280,8 @@ // prototype (inside 'friend') for it comes first in the AST and is not // linked to the definition. EXPECT_EQ(ImportedDef, ToClassDef); -} - +} + struct LLDBLookupTest : ASTImporterOptionSpecificTestBase { LLDBLookupTest() { Creator = [](ASTContext &ToContext, FileManager &ToFileManager, @@ -5362,10 +5409,214 @@ EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate()); } +struct ConflictingDeclsTest : ASTImporterOptionSpecificTestBase {}; + +TEST_P(ConflictingDeclsTest, Typedef) { + getToTuDecl( + R"( + typedef int X; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + typedef double X; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, typedefNameDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, TypeAlias) { + getToTuDecl( + R"( + using X = int; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + using X = double; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, typedefNameDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, EnumDecl) { + getToTuDecl( + R"( + enum X { a, b }; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + enum X { a, b, c }; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, enumDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, EnumConstantDecl) { + getToTuDecl( + R"( + enum E { X = 0 }; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + enum E { X = 1 }; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, enumConstantDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, RecordDecl) { + getToTuDecl( + R"( + class X { int a; }; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + class X { int b; }; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, recordDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, VarDecl) { + getToTuDecl( + R"( + int X; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + double X; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, varDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, FunctionDecl) { + getToTuDecl( + R"( + void X(int); + )", + Lang_C); // C, no overloading! + Decl *FromTU = getTuDecl( + R"( + void X(double); + )", + Lang_C); + auto *FromX = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_C); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +TEST_P(ConflictingDeclsTest, ClassTemplateDecl) { + getToTuDecl( + R"( + template + struct X; + )", + Lang_CXX11); + Decl *FromTU = getTuDecl( + R"( + template + struct X; + )", + Lang_CXX11); + auto *FromX = FirstDeclMatcher().match( + FromTU, classTemplateDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX11); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +// FIXME We don't have proper structural equivalency check for VarTemplateDecl +TEST_P(ConflictingDeclsTest, DISABLED_VarTemplateDecl) { + const internal::VariadicDynCastAllOfMatcher + varTemplateDecl; + getToTuDecl( + R"( + template + constexpr T X; + )", + Lang_CXX14); + Decl *FromTU = getTuDecl( + R"( + template + constexpr int X = 0; + )", + Lang_CXX14); + auto *FromX = FirstDeclMatcher().match( + FromTU, varTemplateDecl(hasName("X"))); + auto *ImportedX = Import(FromX, Lang_CXX14); + EXPECT_FALSE(ImportedX); + ASTImporter *Importer = findFromTU(FromX)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromX); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins, ::testing::Values(ArgVector{"-target", "aarch64-linux-gnu"}), ); +INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest, + ::testing::Values(ArgVector()), ); + +INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain, + ::testing::Values(ArgVector()), ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest, DefaultTestValuesForRunOptions, ); @@ -5384,19 +5635,22 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterOptionSpecificTestBase, DefaultTestValuesForRunOptions, ); +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest, + DefaultTestValuesForRunOptions, ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests, RedirectingImporterTest, DefaultTestValuesForRunOptions, ); INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions, DefaultTestValuesForRunOptions, ); -INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates, +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates, DefaultTestValuesForRunOptions, ); -INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses, +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates, DefaultTestValuesForRunOptions, ); -INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates, +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses, DefaultTestValuesForRunOptions, ); INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctions, @@ -5418,6 +5672,8 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest, DefaultTestValuesForRunOptions, ); +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ConflictingDeclsTest, + DefaultTestValuesForRunOptions, ); } // end namespace ast_matchers } // end namespace clang