Index: cfe/trunk/include/clang/AST/ASTImporter.h =================================================================== --- cfe/trunk/include/clang/AST/ASTImporter.h +++ cfe/trunk/include/clang/AST/ASTImporter.h @@ -87,6 +87,127 @@ using ImportedCXXBaseSpecifierMap = llvm::DenseMap; + // An ImportPath is the list of the AST nodes which we visit during an + // Import call. + // If node `A` depends on node `B` then the path contains an `A`->`B` edge. + // From the call stack of the import functions we can read the very same + // path. + // + // Now imagine the following AST, where the `->` represents dependency in + // therms of the import. + // ``` + // A->B->C->D + // `->E + // ``` + // We would like to import A. + // The import behaves like a DFS, so we will visit the nodes in this order: + // ABCDE. + // During the visitation we will have the following ImportPaths: + // ``` + // A + // AB + // ABC + // ABCD + // ABC + // AB + // ABE + // AB + // A + // ``` + // If during the visit of E there is an error then we set an error for E, + // then as the call stack shrinks for B, then for A: + // ``` + // A + // AB + // ABC + // ABCD + // ABC + // AB + // ABE // Error! Set an error to E + // AB // Set an error to B + // A // Set an error to A + // ``` + // However, during the import we could import C and D without any error and + // they are independent from A,B and E. + // We must not set up an error for C and D. + // So, at the end of the import we have an entry in `ImportDeclErrors` for + // A,B,E but not for C,D. + // + // Now what happens if there is a cycle in the import path? + // Let's consider this AST: + // ``` + // A->B->C->A + // `->E + // ``` + // During the visitation we will have the below ImportPaths and if during + // the visit of E there is an error then we will set up an error for E,B,A. + // But what's up with C? + // ``` + // A + // AB + // ABC + // ABCA + // ABC + // AB + // ABE // Error! Set an error to E + // AB // Set an error to B + // A // Set an error to A + // ``` + // This time we know that both B and C are dependent on A. + // This means we must set up an error for C too. + // As the call stack reverses back we get to A and we must set up an error + // to all nodes which depend on A (this includes C). + // But C is no longer on the import path, it just had been previously. + // Such situation can happen only if during the visitation we had a cycle. + // If we didn't have any cycle, then the normal way of passing an Error + // object through the call stack could handle the situation. + // This is why we must track cycles during the import process for each + // visited declaration. + class ImportPathTy { + public: + using VecTy = llvm::SmallVector; + + void push(Decl *D) { + Nodes.push_back(D); + ++Aux[D]; + } + + void pop() { + if (Nodes.empty()) + return; + --Aux[Nodes.back()]; + Nodes.pop_back(); + } + + /// Returns true if the last element can be found earlier in the path. + bool hasCycleAtBack() const { + auto Pos = Aux.find(Nodes.back()); + return Pos != Aux.end() && Pos->second > 1; + } + + using Cycle = llvm::iterator_range; + Cycle getCycleAtBack() const { + assert(Nodes.size() >= 2); + return Cycle(Nodes.rbegin(), + std::find(Nodes.rbegin() + 1, Nodes.rend(), Nodes.back()) + + 1); + } + + /// Returns the copy of the cycle. + VecTy copyCycleAtBack() const { + auto R = getCycleAtBack(); + return VecTy(R.begin(), R.end()); + } + + private: + // All the nodes of the path. + VecTy Nodes; + // Auxiliary container to be able to answer "Do we have a cycle ending + // at last element?" as fast as possible. + // We count each Decl's occurrence over the path. + llvm::SmallDenseMap Aux; + }; + private: /// Pointer to the import specific lookup table, which may be shared @@ -96,6 +217,17 @@ /// If not set then the original C/C++ lookup is used. ASTImporterLookupTable *LookupTable = nullptr; + /// The path which we go through during the import of a given AST node. + ImportPathTy ImportPath; + /// Sometimes we have to save some part of an import path, so later we can + /// set up properties to the saved nodes. + /// We may have several of these import paths associated to one Decl. + using SavedImportPathsForOneDecl = + llvm::SmallVector; + using SavedImportPathsTy = + llvm::SmallDenseMap; + SavedImportPathsTy SavedImportPaths; + /// The contexts we're importing to and from. ASTContext &ToContext, &FromContext; Index: cfe/trunk/lib/AST/ASTImporter.cpp =================================================================== --- cfe/trunk/lib/AST/ASTImporter.cpp +++ cfe/trunk/lib/AST/ASTImporter.cpp @@ -7842,6 +7842,10 @@ if (!FromD) return nullptr; + // Push FromD to the stack, and remove that when we return. + ImportPath.push(FromD); + auto ImportPathBuilder = + llvm::make_scope_exit([this]() { ImportPath.pop(); }); // Check whether there was a previous failed import. // If yes return the existing error. @@ -7853,6 +7857,10 @@ if (ToD) { // If FromD has some updated flags after last import, apply it updateFlags(FromD, ToD); + // If we encounter a cycle during an import then we save the relevant part + // of the import path associated to the Decl. + if (ImportPath.hasCycleAtBack()) + SavedImportPaths[FromD].push_back(ImportPath.copyCycleAtBack()); return ToD; } @@ -7889,16 +7897,20 @@ // FIXME: AST may contain remaining references to the failed object. } - // Error encountered for the first time. - assert(!getImportDeclErrorIfAny(FromD) && - "Import error already set for Decl."); - - // After takeError the error is not usable any more in ToDOrErr. + // After takeError the error is not usable anymore in ToDOrErr. // Get a copy of the error object (any more simple solution for this?). ImportError ErrOut; handleAllErrors(ToDOrErr.takeError(), [&ErrOut](const ImportError &E) { ErrOut = E; }); setImportDeclError(FromD, ErrOut); + + // Set the error for all nodes which have been created before we + // recognized the error. + for (const auto &Path : SavedImportPaths[FromD]) + for (Decl *Di : Path) + setImportDeclError(Di, ErrOut); + SavedImportPaths[FromD].clear(); + // Do not return ToDOrErr, error was taken out of it. return make_error(ErrOut); } @@ -7921,6 +7933,7 @@ Imported(FromD, ToD); updateFlags(FromD, ToD); + SavedImportPaths[FromD].clear(); return ToDOrErr; } @@ -8641,9 +8654,10 @@ } void ASTImporter::setImportDeclError(Decl *From, ImportError Error) { - assert(ImportDeclErrors.find(From) == ImportDeclErrors.end() && - "Setting import error allowed only once for a Decl."); - ImportDeclErrors[From] = Error; + auto InsertRes = ImportDeclErrors.insert({From, Error}); + // Either we set the error for the first time, or we already had set one and + // now we want to set the same error. + assert(InsertRes.second || InsertRes.first->second.Error == Error.Error); } bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To, Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp =================================================================== --- cfe/trunk/unittests/AST/ASTImporterTest.cpp +++ cfe/trunk/unittests/AST/ASTImporterTest.cpp @@ -358,6 +358,106 @@ EXPECT_EQ(0U, count); } +struct ImportPath : ASTImporterOptionSpecificTestBase { + Decl *FromTU; + FunctionDecl *D0, *D1, *D2; + ImportPath() { + FromTU = getTuDecl("void f(); void f(); void f();", Lang_CXX); + auto Pattern = functionDecl(hasName("f")); + D0 = FirstDeclMatcher().match(FromTU, Pattern); + D2 = LastDeclMatcher().match(FromTU, Pattern); + D1 = D2->getPreviousDecl(); + } +}; + +TEST_P(ImportPath, Push) { + ASTImporter::ImportPathTy path; + path.push(D0); + EXPECT_FALSE(path.hasCycleAtBack()); +} + +TEST_P(ImportPath, SmallCycle) { + ASTImporter::ImportPathTy path; + path.push(D0); + path.push(D0); + EXPECT_TRUE(path.hasCycleAtBack()); + path.pop(); + EXPECT_FALSE(path.hasCycleAtBack()); + path.push(D0); + EXPECT_TRUE(path.hasCycleAtBack()); +} + +TEST_P(ImportPath, GetSmallCycle) { + ASTImporter::ImportPathTy path; + path.push(D0); + path.push(D0); + EXPECT_TRUE(path.hasCycleAtBack()); + std::array Res; + int i = 0; + for (Decl *Di : path.getCycleAtBack()) { + Res[i++] = Di; + } + ASSERT_EQ(i, 2); + EXPECT_EQ(Res[0], D0); + EXPECT_EQ(Res[1], D0); +} + +TEST_P(ImportPath, GetCycle) { + ASTImporter::ImportPathTy path; + path.push(D0); + path.push(D1); + path.push(D2); + path.push(D0); + EXPECT_TRUE(path.hasCycleAtBack()); + std::array Res; + int i = 0; + for (Decl *Di : path.getCycleAtBack()) { + Res[i++] = Di; + } + ASSERT_EQ(i, 4); + EXPECT_EQ(Res[0], D0); + EXPECT_EQ(Res[1], D2); + EXPECT_EQ(Res[2], D1); + EXPECT_EQ(Res[3], D0); +} + +TEST_P(ImportPath, CycleAfterCycle) { + ASTImporter::ImportPathTy path; + path.push(D0); + path.push(D1); + path.push(D0); + path.push(D1); + path.push(D2); + path.push(D0); + EXPECT_TRUE(path.hasCycleAtBack()); + std::array Res; + int i = 0; + for (Decl *Di : path.getCycleAtBack()) { + Res[i++] = Di; + } + ASSERT_EQ(i, 4); + EXPECT_EQ(Res[0], D0); + EXPECT_EQ(Res[1], D2); + EXPECT_EQ(Res[2], D1); + EXPECT_EQ(Res[3], D0); + + path.pop(); + path.pop(); + path.pop(); + EXPECT_TRUE(path.hasCycleAtBack()); + i = 0; + for (Decl *Di : path.getCycleAtBack()) { + Res[i++] = Di; + } + ASSERT_EQ(i, 3); + EXPECT_EQ(Res[0], D0); + EXPECT_EQ(Res[1], D1); + EXPECT_EQ(Res[2], D0); + + path.pop(); + EXPECT_FALSE(path.hasCycleAtBack()); +} + TEST_P(ImportExpr, ImportStringLiteral) { MatchVerifier Verifier; testImport( @@ -4547,12 +4647,6 @@ EXPECT_EQ(*Res.begin(), A); } -INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest, - ::testing::Values(ArgVector()), ); - -INSTANTIATE_TEST_CASE_P( - ParameterizedTests, CanonicalRedeclChain, - ::testing::Values(ArgVector()),); // FIXME This test is disabled currently, upcoming patches will make it // possible to enable. @@ -4770,19 +4864,99 @@ CXXMethodDecl *ImportedF = Import(FromF, Lang_CXX); EXPECT_FALSE(ImportedF); - // There is no error set for ok(). + // There is an error set for the other member too. auto *FromOK = FirstDeclMatcher().match( FromTU, cxxMethodDecl(hasName("ok"))); OptErr = Importer->getImportDeclErrorIfAny(FromOK); - EXPECT_FALSE(OptErr); - // And we should be able to import. + EXPECT_TRUE(OptErr); + // Cannot import the other member. CXXMethodDecl *ImportedOK = Import(FromOK, Lang_CXX); - EXPECT_TRUE(ImportedOK); + EXPECT_FALSE(ImportedOK); +} - // Unwary clients may access X even if the error is set, so, at least make - // sure the class is set to be complete. - CXXRecordDecl *ToX = cast(ImportedOK->getDeclContext()); - EXPECT_TRUE(ToX->isCompleteDefinition()); +// Check that an error propagates to the dependent AST nodes. +// In the below code it means that an error in X should propagate to A. +// And even to F since the containing A is erroneous. +// And to all AST nodes which we visit during the import process which finally +// ends up in a failure (in the error() function). +TEST_P(ErrorHandlingTest, ErrorPropagatesThroughImportCycles) { + Decl *FromTU = getTuDecl( + std::string(R"( + namespace NS { + class A { + template class F {}; + class X { + template friend class F; + void error() { )") + ErroneousStmt + R"( } + }; + }; + + class B {}; + } // NS + )", + Lang_CXX, "input0.cc"); + + auto *FromFRD = FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("F"), isDefinition())); + auto *FromA = FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("A"), isDefinition())); + auto *FromB = FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("B"), isDefinition())); + auto *FromNS = FirstDeclMatcher().match( + FromTU, namespaceDecl(hasName("NS"))); + + // Start by importing the templated CXXRecordDecl of F. + // Import fails for that. + EXPECT_FALSE(Import(FromFRD, Lang_CXX)); + // Import fails for A. + EXPECT_FALSE(Import(FromA, Lang_CXX)); + // But we should be able to import the independent B. + EXPECT_TRUE(Import(FromB, Lang_CXX)); + // And the namespace. + EXPECT_TRUE(Import(FromNS, Lang_CXX)); + + // An error is set to the templated CXXRecordDecl of F. + ASTImporter *Importer = findFromTU(FromFRD)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromFRD); + EXPECT_TRUE(OptErr); + + // An error is set to A. + OptErr = Importer->getImportDeclErrorIfAny(FromA); + EXPECT_TRUE(OptErr); + + // There is no error set to B. + OptErr = Importer->getImportDeclErrorIfAny(FromB); + EXPECT_FALSE(OptErr); + + // There is no error set to NS. + OptErr = Importer->getImportDeclErrorIfAny(FromNS); + EXPECT_FALSE(OptErr); + + // Check some of those decls whose ancestor is X, they all should have an + // error set if we visited them during an import process which finally failed. + // These decls are part of a cycle in an ImportPath. + // There would not be any error set for these decls if we hadn't follow the + // ImportPaths and the cycles. + OptErr = Importer->getImportDeclErrorIfAny( + FirstDeclMatcher().match( + FromTU, classTemplateDecl(hasName("F")))); + // An error is set to the 'F' ClassTemplateDecl. + EXPECT_TRUE(OptErr); + // An error is set to the FriendDecl. + OptErr = Importer->getImportDeclErrorIfAny( + FirstDeclMatcher().match( + FromTU, friendDecl())); + EXPECT_TRUE(OptErr); + // An error is set to the implicit class of A. + OptErr = + Importer->getImportDeclErrorIfAny(FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("A"), isImplicit()))); + EXPECT_TRUE(OptErr); + // An error is set to the implicit class of X. + OptErr = + Importer->getImportDeclErrorIfAny(FirstDeclMatcher().match( + FromTU, cxxRecordDecl(hasName("X"), isImplicit()))); + EXPECT_TRUE(OptErr); } TEST_P(ErrorHandlingTest, ErrorIsNotPropagatedFromMemberToNamespace) { @@ -4828,9 +5002,18 @@ 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()), ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest, DefaultTestValuesForRunOptions, ); +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath, + ::testing::Values(ArgVector()), ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr, DefaultTestValuesForRunOptions, );