balazske created this revision.
Herald added subscribers: steakhal, martong, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: All.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This bug can cause that more import errors are generated than necessary
and many objects fail to import. Chance of an invalid AST after these
imports increases.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D122525
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5806,6 +5806,53 @@
EXPECT_FALSE(ImportedF);
}
+TEST_P(ErrorHandlingTest, DoNotInheritErrorFromNonDependentChild) {
+ // Declarations should not inherit an import error from a child object
+ // if the declaration has no direct dependence to such a child.
+ // For example a namespace should not get import error if one of the
+ // declarations inside it fails to import.
+ // There was a special case in error handling (when "import path circles" are
+ // encountered) when this property was not held. This case is provoked by the
+ // following code.
+ constexpr auto ToTUCode = R"(
+ namespace ns {
+ struct Err {
+ char A;
+ };
+ }
+ )";
+ constexpr auto FromTUCode = R"(
+ namespace ns {
+ struct A {
+ using U = struct Err;
+ };
+ }
+ namespace ns {
+ struct Err {}; // ODR violation
+ void f(A) {}
+ }
+ )";
+
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), hasDefinition()));
+ ASSERT_TRUE(FromA);
+ auto *ImportedA = Import(FromA, Lang_CXX11);
+ // 'A' can not be imported: ODR error at 'Err'
+ EXPECT_FALSE(ImportedA);
+ // When import of 'A' failed there was a "saved import path circle" that
+ // contained namespace 'ns' (A - U - Err - ns - f - A). This should not mean
+ // that every object in this path fails to import.
+
+ Decl *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("ns")));
+ EXPECT_TRUE(FromNS);
+ auto *ImportedNS = Import(FromNS, Lang_CXX11);
+ EXPECT_TRUE(ImportedNS);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
Decl *FromTU = getTuDecl(
R"(
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8776,8 +8776,24 @@
// Set the error for all nodes which have been created before we
// recognized the error.
- for (const auto &Path : SavedImportPaths[FromD])
+ for (const auto &Path : SavedImportPaths[FromD]) {
+ Decl *PrevFromDi = FromD;
for (Decl *FromDi : Path) {
+ // Begin and end of the path equals 'FromD', skip it.
+ if (FromDi == FromD)
+ continue;
+ // We should not set import error on a node that "consumes" the error
+ // during import of its children (failing import of the child does not
+ // cause failure of the parent). See ImportDeclContext and uses of
+ // 'consumeError'.
+ // The import path contains child nodes first.
+ // (Parent-child relationship is used here in sense of import
+ // dependency.)
+ if (!isa<TagDecl>(FromDi))
+ if (auto *FromDiDC = dyn_cast<DeclContext>(FromDi))
+ if (FromDiDC->containsDecl(PrevFromDi))
+ break; // Skip every following node in the path.
+ PrevFromDi = FromDi;
setImportDeclError(FromDi, ErrOut);
//FIXME Should we remove these Decls from ImportedDecls?
// Set the error for the mapped to Decl, which is in the "to" context.
@@ -8787,6 +8803,7 @@
// FIXME Should we remove these Decls from the LookupTable,
// and from ImportedFromDecls?
}
+ }
SavedImportPaths.erase(FromD);
// Do not return ToDOrErr, error was taken out of it.
Index: clang/unittests/AST/ASTImporterTest.cpp
===================================================================
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5806,6 +5806,53 @@
EXPECT_FALSE(ImportedF);
}
+TEST_P(ErrorHandlingTest, DoNotInheritErrorFromNonDependentChild) {
+ // Declarations should not inherit an import error from a child object
+ // if the declaration has no direct dependence to such a child.
+ // For example a namespace should not get import error if one of the
+ // declarations inside it fails to import.
+ // There was a special case in error handling (when "import path circles" are
+ // encountered) when this property was not held. This case is provoked by the
+ // following code.
+ constexpr auto ToTUCode = R"(
+ namespace ns {
+ struct Err {
+ char A;
+ };
+ }
+ )";
+ constexpr auto FromTUCode = R"(
+ namespace ns {
+ struct A {
+ using U = struct Err;
+ };
+ }
+ namespace ns {
+ struct Err {}; // ODR violation
+ void f(A) {}
+ }
+ )";
+
+ Decl *ToTU = getToTuDecl(ToTUCode, Lang_CXX11);
+ static_cast<void>(ToTU);
+ Decl *FromTU = getTuDecl(FromTUCode, Lang_CXX11);
+ auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("A"), hasDefinition()));
+ ASSERT_TRUE(FromA);
+ auto *ImportedA = Import(FromA, Lang_CXX11);
+ // 'A' can not be imported: ODR error at 'Err'
+ EXPECT_FALSE(ImportedA);
+ // When import of 'A' failed there was a "saved import path circle" that
+ // contained namespace 'ns' (A - U - Err - ns - f - A). This should not mean
+ // that every object in this path fails to import.
+
+ Decl *FromNS = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("ns")));
+ EXPECT_TRUE(FromNS);
+ auto *ImportedNS = Import(FromNS, Lang_CXX11);
+ EXPECT_TRUE(ImportedNS);
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
Decl *FromTU = getTuDecl(
R"(
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8776,8 +8776,24 @@
// Set the error for all nodes which have been created before we
// recognized the error.
- for (const auto &Path : SavedImportPaths[FromD])
+ for (const auto &Path : SavedImportPaths[FromD]) {
+ Decl *PrevFromDi = FromD;
for (Decl *FromDi : Path) {
+ // Begin and end of the path equals 'FromD', skip it.
+ if (FromDi == FromD)
+ continue;
+ // We should not set import error on a node that "consumes" the error
+ // during import of its children (failing import of the child does not
+ // cause failure of the parent). See ImportDeclContext and uses of
+ // 'consumeError'.
+ // The import path contains child nodes first.
+ // (Parent-child relationship is used here in sense of import
+ // dependency.)
+ if (!isa<TagDecl>(FromDi))
+ if (auto *FromDiDC = dyn_cast<DeclContext>(FromDi))
+ if (FromDiDC->containsDecl(PrevFromDi))
+ break; // Skip every following node in the path.
+ PrevFromDi = FromDi;
setImportDeclError(FromDi, ErrOut);
//FIXME Should we remove these Decls from ImportedDecls?
// Set the error for the mapped to Decl, which is in the "to" context.
@@ -8787,6 +8803,7 @@
// FIXME Should we remove these Decls from the LookupTable,
// and from ImportedFromDecls?
}
+ }
SavedImportPaths.erase(FromD);
// Do not return ToDOrErr, error was taken out of it.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
