[ASTImporter] Mark erroneous nodes in shared st

Summary:
Now we store the errors for the Decls in the "to" context too. For
that, however, we have to put these errors in a shared state (among all
the ASTImporter objects which handle the same "to" context but different
"from" contexts).

After a series of imports from different "from" TUs we have a "to" context
which may have erroneous nodes in it. (Remember, the AST is immutable so
there is no way to delete a node once we had created it and we realized
the error later.) All these erroneous nodes are marked in
ASTImporterSharedState::ImportErrors.  Clients of the ASTImporter may
use this as an input. E.g. the static analyzer engine may not try to
analyze a function if that is marked as erroneous (it can be queried via
ASTImporterSharedState::getImportDeclErrorIfAny()).

Reviewers: a_sidorin, a.sidorin, shafik

Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D62376

llvm-svn: 364785

3 files changed, 120 insertions, 38 deletions

diff --git a/clang/unittests/AST/ASTImporterFixtures.cpp b/clang/unittests/AST/ASTImporterFixtures.cpp
index b2b848f5e57..0be4e789375 100644
--- a/clang/unittests/AST/ASTImporterFixtures.cpp
+++ b/clang/unittests/AST/ASTImporterFixtures.cpp
@@ -14,7 +14,7 @@
 #include "ASTImporterFixtures.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Tooling/Tooling.h"
 
@@ -50,28 +50,31 @@ ASTImporterTestBase::TU::TU(StringRef Code, StringRef FileName, ArgVector Args,
   if (!Creator)
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
-                 bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporter(ToContext, ToFileManager, FromContext,
-                             FromFileManager, MinimalImport, LookupTable);
+                             FromFileManager, MinimalImport, SharedState);
     };
 }
 
 ASTImporterTestBase::TU::~TU() {}
 
 void ASTImporterTestBase::TU::lazyInitImporter(
-    ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
+    const std::shared_ptr<ASTImporterSharedState> &SharedState,
+    ASTUnit *ToAST) {
   assert(ToAST);
   if (!Importer)
     Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                            Unit->getASTContext(), Unit->getFileManager(), false,
-                           &LookupTable));
+                           SharedState));
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
 
-Decl *ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
-                                      ASTUnit *ToAST, Decl *FromDecl) {
-  lazyInitImporter(LookupTable, ToAST);
+Decl *ASTImporterTestBase::TU::import(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    Decl *FromDecl) {
+  lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromDecl))
     return *ImportedOrErr;
   else {
@@ -80,9 +83,10 @@ Decl *ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
   }
 }
 
-QualType ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
-                                         ASTUnit *ToAST, QualType FromType) {
-  lazyInitImporter(LookupTable, ToAST);
+QualType ASTImporterTestBase::TU::import(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    QualType FromType) {
+  lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromType))
     return *ImportedOrErr;
   else {
@@ -91,10 +95,10 @@ QualType ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
   }
 }
 
-void ASTImporterTestBase::lazyInitLookupTable(TranslationUnitDecl *ToTU) {
+void ASTImporterTestBase::lazyInitSharedState(TranslationUnitDecl *ToTU) {
   assert(ToTU);
-  if (!LookupTablePtr)
-    LookupTablePtr = llvm::make_unique<ASTImporterLookupTable>(*ToTU);
+  if (!SharedStatePtr)
+    SharedStatePtr = std::make_shared<ASTImporterSharedState>(*ToTU);
 }
 
 void ASTImporterTestBase::lazyInitToAST(Language ToLang, StringRef ToSrcCode,
@@ -107,7 +111,7 @@ void ASTImporterTestBase::lazyInitToAST(Language ToLang, StringRef ToSrcCode,
   // Build the AST from an empty file.
   ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, FileName);
   ToAST->enableSourceFileDiagnostics();
-  lazyInitLookupTable(ToAST->getASTContext().getTranslationUnitDecl());
+  lazyInitSharedState(ToAST->getASTContext().getTranslationUnitDecl());
 }
 
 ASTImporterTestBase::TU *ASTImporterTestBase::findFromTU(Decl *From) {
@@ -147,7 +151,7 @@ ASTImporterTestBase::getImportedDecl(StringRef FromSrcCode, Language FromLang,
   assert(FoundDecls.size() == 1);
 
   Decl *Imported =
-      FromTU.import(*LookupTablePtr, ToAST.get(), FoundDecls.front());
+      FromTU.import(SharedStatePtr, ToAST.get(), FoundDecls.front());
 
   assert(Imported);
   return std::make_tuple(*FoundDecls.begin(), Imported);
@@ -178,16 +182,17 @@ TranslationUnitDecl *ASTImporterTestBase::getToTuDecl(StringRef ToSrcCode,
 Decl *ASTImporterTestBase::Import(Decl *From, Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(From);
-  assert(LookupTablePtr);
-  return FromTU->import(*LookupTablePtr, ToAST.get(), From);
+  assert(SharedStatePtr);
+  Decl *To = FromTU->import(SharedStatePtr, ToAST.get(), From);
+  return To;
 }
 
 QualType ASTImporterTestBase::ImportType(QualType FromType, Decl *TUDecl,
                                          Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(TUDecl);
-  assert(LookupTablePtr);
-  return FromTU->import(*LookupTablePtr, ToAST.get(), FromType);
+  assert(SharedStatePtr);
+  return FromTU->import(SharedStatePtr, ToAST.get(), FromType);
 }
 
 ASTImporterTestBase::~ASTImporterTestBase() {
diff --git a/clang/unittests/AST/ASTImporterFixtures.h b/clang/unittests/AST/ASTImporterFixtures.h
index 4e666e37aa3..4b67a9435f1 100644
--- a/clang/unittests/AST/ASTImporterFixtures.h
+++ b/clang/unittests/AST/ASTImporterFixtures.h
@@ -17,8 +17,8 @@
 #include "gmock/gmock.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/Frontend/ASTUnit.h"
+#include "clang/AST/ASTImporterSharedState.h"
 
 #include "DeclMatcher.h"
 #include "Language.h"
@@ -26,7 +26,7 @@
 namespace clang {
 
 class ASTImporter;
-class ASTImporterLookupTable;
+class ASTImporterSharedState;
 class ASTUnit;
 
 namespace ast_matchers {
@@ -77,9 +77,9 @@ class ASTImporterTestBase : public CompilerOptionSpecificTest {
 
 public:
   /// Allocates an ASTImporter (or one of its subclasses).
-  typedef std::function<ASTImporter *(ASTContext &, FileManager &, ASTContext &,
-                                      FileManager &, bool,
-                                      ASTImporterLookupTable *)>
+  typedef std::function<ASTImporter *(
+      ASTContext &, FileManager &, ASTContext &, FileManager &, bool,
+      const std::shared_ptr<ASTImporterSharedState> &SharedState)>
       ImporterConstructor;
 
   // The lambda that constructs the ASTImporter we use in this test.
@@ -104,11 +104,13 @@ private:
        ImporterConstructor C = ImporterConstructor());
     ~TU();
 
-    void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST);
-    Decl *import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
-                 Decl *FromDecl);
-    QualType import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
-                    QualType FromType);
+    void
+    lazyInitImporter(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                     ASTUnit *ToAST);
+    Decl *import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                 ASTUnit *ToAST, Decl *FromDecl);
+    QualType import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                    ASTUnit *ToAST, QualType FromType);
   };
 
   // We may have several From contexts and related translation units. In each
@@ -120,13 +122,13 @@ private:
   // vector is expanding, with the list we won't have these issues.
   std::list<TU> FromTUs;
 
-  // Initialize the lookup table if not initialized already.
-  void lazyInitLookupTable(TranslationUnitDecl *ToTU);
+  // Initialize the shared state if not initialized already.
+  void lazyInitSharedState(TranslationUnitDecl *ToTU);
 
   void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName);
 
 protected:
-  std::unique_ptr<ASTImporterLookupTable> LookupTablePtr;
+  std::shared_ptr<ASTImporterSharedState> SharedStatePtr;
 
 public:
   // We may have several From context but only one To context.
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index d517d633c43..165946a669f 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -316,10 +316,11 @@ struct RedirectingImporterTest : ASTImporterOptionSpecificTestBase {
   RedirectingImporterTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
-                 bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new RedirectingImporter(ToContext, ToFileManager, FromContext,
                                      FromFileManager, MinimalImport,
-                                     LookupTable);
+                                     SharedState);
     };
   }
 };
@@ -2888,7 +2889,7 @@ private:
       CXXMethodDecl *Method =
           FirstDeclMatcher<CXXMethodDecl>().match(ToClass, MethodMatcher);
       ToClass->removeDecl(Method);
-      LookupTablePtr->remove(Method);
+      SharedStatePtr->getLookupTable()->remove(Method);
     }
 
     ASSERT_EQ(DeclCounter<CXXMethodDecl>().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@ struct ErrorHandlingTest : ASTImporterOptionSpecificTestBase {
   ErrorHandlingTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
-                 bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
                                            FromContext, FromFileManager,
-                                           MinimalImport, LookupTable);
+                                           MinimalImport, SharedState);
     };
   }
   // In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@ TEST_P(ErrorHandlingTest, ErrorIsNotPropagatedFromMemberToNamespace) {
   EXPECT_TRUE(ImportedOK);
 }
 
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+       ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+  // We already have a fwd decl.
+  TranslationUnitDecl *ToTU = getToTuDecl(
+      "class X;", Lang_CXX);
+  // Then we import a definition.
+  {
+    TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+        class X {
+          void f() { )") + ErroneousStmt + R"( }
+          void ok();
+        };
+        )",
+        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);
+  }
+  // ... but the node had been created.
+  auto *ToXDef = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+  // An error is set for "ToXDef" in the shared state.
+  Optional<ImportError> OptErr =
+      SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+  auto *ToXFwd = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition())));
+  // An error is NOT set for the fwd Decl of X in the shared state.
+  OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+  ASSERT_FALSE(OptErr);
+
+  // Try to import  X again but from another TU.
+  {
+    TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+        class X {
+          void f() { )") + ErroneousStmt + R"( }
+          void ok();
+        };
+        )",
+        Lang_CXX, "input1.cc");
+
+    auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+        FromTU, cxxRecordDecl(hasName("X")));
+    CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+    // If we did not save the errors for the "to" context then the below checks
+    // would fail, because the lookup finds the fwd Decl of the existing
+    // definition in the "to" context. We can reach the existing definition via
+    // the found fwd Decl. That existing definition is structurally equivalent
+    // (we check only the fields) with this one we want to import, so we return
+    // with the existing definition, which is erroneous (one method is missing).
+
+    // The import should fail.
+    EXPECT_FALSE(ImportedX);
+    ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+    Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+    // And an error is set for this new X in the "from" ctx.
+    ASSERT_TRUE(OptErr);
+    EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );