This revision was automatically updated to reflect the committed changes.
Closed by commit rL364752: [ASTImporter] Propagate error from ImportDeclContext
(authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D63603?vs=206250&id=207274#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63603/new/
https://reviews.llvm.org/D63603
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===================================================================
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -57,6 +57,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
@@ -1631,16 +1632,32 @@
auto ToDCOrErr = Importer.ImportContext(FromDC);
return ToDCOrErr.takeError();
}
+
+ // We use strict error handling in case of records and enums, but not
+ // with e.g. namespaces.
+ //
+ // FIXME Clients of the ASTImporter should be able to choose an
+ // appropriate error handling strategy for their needs. For instance,
+ // they may not want to mark an entire namespace as erroneous merely
+ // because there is an ODR error with two typedefs. As another example,
+ // the client may allow EnumConstantDecls with same names but with
+ // different values in two distinct translation units.
+ bool AccumulateChildErrors = isa<TagDecl>(FromDC);
+
+ Error ChildErrors = Error::success();
llvm::SmallVector<Decl *, 8> ImportedDecls;
for (auto *From : FromDC->decls()) {
ExpectedDecl ImportedOrErr = import(From);
- if (!ImportedOrErr)
- // Ignore the error, continue with next Decl.
- // FIXME: Handle this case somehow better.
- consumeError(ImportedOrErr.takeError());
+ if (!ImportedOrErr) {
+ if (AccumulateChildErrors)
+ ChildErrors =
+ joinErrors(std::move(ChildErrors), ImportedOrErr.takeError());
+ else
+ consumeError(ImportedOrErr.takeError());
+ }
}
- return Error::success();
+ return ChildErrors;
}
Error ASTNodeImporter::ImportDeclContext(
@@ -1698,6 +1715,11 @@
}
To->startDefinition();
+ // Complete the definition even if error is returned.
+ // The RecordDecl may be already part of the AST so it is better to
+ // have it in complete state even if something is wrong with it.
+ auto DefinitionCompleter =
+ llvm::make_scope_exit([To]() { To->completeDefinition(); });
if (Error Err = setTypedefNameForAnonDecl(From, To, Importer))
return Err;
@@ -1822,7 +1844,6 @@
if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
return Err;
- To->completeDefinition();
return Error::success();
}
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===================================================================
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -4738,6 +4738,93 @@
EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
}
+// An error should be set for a class if we cannot import one member.
+TEST_P(ErrorHandlingTest, ErrorIsPropagatedFromMemberToClass) {
+ TranslationUnitDecl *FromTU = getTuDecl(
+ std::string(R"(
+ class X {
+ void f() { )") + ErroneousStmt + R"( } // This member has the error
+ // during import.
+ void ok(); // The error should not prevent importing this.
+ }; // An error will be set for X too.
+ )",
+ Lang_CXX);
+ auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("X")));
+ CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+ // An error is set for X.
+ EXPECT_FALSE(ImportedX);
+ ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+ // An error is set for f().
+ auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
+ FromTU, cxxMethodDecl(hasName("f")));
+ OptErr = Importer->getImportDeclErrorIfAny(FromF);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+ // And any subsequent import should fail.
+ CXXMethodDecl *ImportedF = Import(FromF, Lang_CXX);
+ EXPECT_FALSE(ImportedF);
+
+ // There is no error set for ok().
+ auto *FromOK = FirstDeclMatcher<CXXMethodDecl>().match(
+ FromTU, cxxMethodDecl(hasName("ok")));
+ OptErr = Importer->getImportDeclErrorIfAny(FromOK);
+ EXPECT_FALSE(OptErr);
+ // And we should be able to import.
+ CXXMethodDecl *ImportedOK = Import(FromOK, Lang_CXX);
+ EXPECT_TRUE(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<CXXRecordDecl>(ImportedOK->getDeclContext());
+ EXPECT_TRUE(ToX->isCompleteDefinition());
+}
+
+TEST_P(ErrorHandlingTest, ErrorIsNotPropagatedFromMemberToNamespace) {
+ TranslationUnitDecl *FromTU = getTuDecl(
+ std::string(R"(
+ namespace X {
+ void f() { )") + ErroneousStmt + R"( } // This member has the error
+ // during import.
+ void ok(); // The error should not prevent importing this.
+ }; // An error will be set for X too.
+ )",
+ Lang_CXX);
+ auto *FromX = FirstDeclMatcher<NamespaceDecl>().match(
+ FromTU, namespaceDecl(hasName("X")));
+ NamespaceDecl *ImportedX = Import(FromX, Lang_CXX);
+
+ // There is no error set for X.
+ EXPECT_TRUE(ImportedX);
+ ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+ Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+ ASSERT_FALSE(OptErr);
+
+ // An error is set for f().
+ auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasName("f")));
+ OptErr = Importer->getImportDeclErrorIfAny(FromF);
+ ASSERT_TRUE(OptErr);
+ EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+ // And any subsequent import should fail.
+ FunctionDecl *ImportedF = Import(FromF, Lang_CXX);
+ EXPECT_FALSE(ImportedF);
+
+ // There is no error set for ok().
+ auto *FromOK = FirstDeclMatcher<FunctionDecl>().match(
+ FromTU, functionDecl(hasName("ok")));
+ OptErr = Importer->getImportDeclErrorIfAny(FromOK);
+ EXPECT_FALSE(OptErr);
+ // And we should be able to import.
+ FunctionDecl *ImportedOK = Import(FromOK, Lang_CXX);
+ EXPECT_TRUE(ImportedOK);
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
DefaultTestValuesForRunOptions, );
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
