[ASTImporter] Mark erroneous nodes in from ctx

[martong]

Summary

During import of a specific Decl D, it may happen that some AST nodes
had already been created before we recognize an error. In this case we
signal back the error to the caller, but the "to" context remains
polluted with those nodes which had been created. Ideally, those nodes
should not had been created, but that time we did not know about the
error, the error happened later. Since the AST is immutable (most of
the cases we can't remove existing nodes) we choose to mark these nodes
as erroneous.
Here are the steps of the algorithm:

1. We keep track of the nodes which we visit during the import of D: See

ImportPathTy.

2. If a Decl is already imported and it is already on the import path

(we have a cycle) then we copy/store the relevant part of the import
path. We store these cycles for each Decl.

3. When we recognize an error during the import of D then we set up this

error to all Decls in the stored cycles for D and we clear the stored
cycles.

[a_sidorin]

Hi Gabor,
Could you please provide any test for the import itself?

clang/lib/AST/ASTImporter.cpp

8693

const?

[martong]

• Add test ErrorPropagatesThroughImportCycles
• Change existing test to new behaviour

[martong]

Alexei,

Thank you for the review, I have added a test which demonstrates the changes.
By tracking the import paths and cycles we implement a very strict error handling mechanism, but this seems to be the way to avoid reaching any faulty AST nodes for the ASTImporter clients.

[a_sidorin]

Thanks!

[martong]

Alexei,

I think I still owe you some explanation about this patch.
I do consider this patch as one of the most intricate patches regarding ASTImporter.
I'd like to answer the following questions in this comment:
What is an ImportPath and why do we need to track it?
What does it mean if we have a cycle in the import path?
How do we use these cycles during the error handling?

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.

I hope this explanation makes it even cleaner than the tests and thanks again for the review!

[balazske]

clang/lib/AST/ASTImporter.cpp

7840

It is possible to use the make_scope_exit instead, this results in less written code.

[martong]

• Remove the macro and use std::string.op+
• We may set up an error twice in case of a cycle, thus remove the assert

[balazske]

clang/lib/AST/ASTImporter.cpp

8628

We should not remove this assert? (Or check if the error is the same, otherwise keep the first or last error? Normally if we set an error multiple times it should be the same error otherwise something is wrong with the import.)

[martong]

• Add back an assertion in setImportDeclError(), remove the condition in Import()

[martong]

clang/lib/AST/ASTImporter.cpp

8628

Well, yes that is a good point. I was thinking about a theoretical scenario where we could set two different Errors for the same node, but failed to fabricate one. So I added an assert that they have the same kind,

[martong]

• Use make_scope_exit
• Make hasCycleAtBack const

clang/lib/AST/ASTImporter.cpp

7840

Thanks! Changed to use that.

8693

Thanks! I made to be const.

[martong]

clang/lib/AST/ASTImporter.cpp

7874–7875

We may set up an error multiple times now, but the error should be the same, this is handled in setImportDeclError.

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

4915–4916

I see now that this test is really not required, the previous test checks the almost same thing in case of class B and NS (but more test is always better).

[martong]

clang/unittests/AST/ASTImporterTest.cpp

4915–4916

Actually, this test belongs to the previous patch , not to this one.

[a_sidorin]

Hello Gabor,
Thank you for the explanation. I got the idea of this patch anyway, but it will be definitely useful for anyone digging into the code. Should we make it a comment for ImportPathTy?

clang/lib/AST/ASTImporter.cpp

8675

I think these definitions are small enough to move them into the class definitions.

[martong]

Thank you for the explanation. I got the idea of this patch anyway, but it will be definitely useful for anyone digging into the code. Should we make it a comment for ImportPathTy?

Ok, I have added the explanation to ImportPathTy as a class comment.

clang/lib/AST/ASTImporter.cpp

8675

Ok, I have made them in-class defined.

[martong]

• Move ImportPathTy's funcitons in-class, add class comment

[martong]

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.

[martong]

Jenkins is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29809/

clang/include/clang/AST/ASTImporter.h

 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/Error.h"
 #include <utility>
 
 namespace clang {
 
 class ASTContext;
 class Attr;
 class ASTImporterLookupTable;
class TypeSourceInfo;
   /// merging AST nodes where appropriate.
   class ASTImporter {
     friend class ASTNodeImporter;
   public:
     using NonEquivalentDeclSet = llvm::DenseSet<std::pair<Decl *, Decl *>>;
     using ImportedCXXBaseSpecifierMap =
         llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
 
+    class ImportPathTy {
+    public:
+      using VecTy = llvm::SmallVector<Decl *, 32>;
+
+      void push(Decl *D);
+      void pop();
+      Decl *top() const;
+
+      /// Returns true if the last element can be found earlier in the path.
+      bool hasCycleAtBack();
+      using Cycle = llvm::iterator_range<VecTy::const_reverse_iterator>;
+      Cycle getCycleAtBack() const;
+      /// Returns the copy of the cycle.
+      VecTy copyCycleAtBack() const;
+
+    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<Decl *, int, 32> Aux;
+    };
+
   private:
 
     /// Pointer to the import specific lookup table, which may be shared
     /// amongst several ASTImporter objects.
     /// This is an externally managed resource (and should exist during the
     /// lifetime of the ASTImporter object)
     /// 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<ImportPathTy::VecTy, 32>;
+    using SavedImportPathsTy =
+        llvm::SmallDenseMap<Decl *, SavedImportPathsForOneDecl, 32>;
+    SavedImportPathsTy SavedImportPaths;
+
     /// The contexts we're importing to and from.
     ASTContext &ToContext, &FromContext;
 
     /// The file managers we're importing to and from.
     FileManager &ToFileManager, &FromFileManager;
 
     /// Whether to perform a minimal import.
     bool Minimal;

clang/lib/AST/ASTImporter.cpp

 
 TranslationUnitDecl *ASTImporter::GetFromTU(Decl *ToD) {
   auto FromDPos = ImportedFromDecls.find(ToD);
   if (FromDPos == ImportedFromDecls.end())
     return nullptr;
   return FromDPos->second->getTranslationUnitDecl();
 }
 
+// RAII class to maintain the import path.
+class ImportPathBuilder {
+  ASTImporter::ImportPathTy &Path;
+
+public:
+  ImportPathBuilder(ASTImporter::ImportPathTy &Path, Decl *FromD) : Path(Path) {
+    Path.push(FromD);
+  };
+  ~ImportPathBuilder() { Path.pop(); }
+};
+
 Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   if (!FromD)
     return nullptr;
 
+  // Push FromD to the stack, and remove that when we return.
+  ImportPathBuilder PathRAII(ImportPath, FromD);
 
   // Check whether there was a previous failed import.
   // If yes return the existing error.
   if (auto Error = getImportDeclErrorIfAny(FromD))
     return make_error<ImportError>(*Error);
 
   // Check whether we've already imported this declaration.
   Decl *ToD = GetAlreadyImportedOrNull(FromD);
   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;
   }
 
   // Import the declaration.
   ExpectedDecl ToDOrErr = ImportImpl(FromD);
   if (!ToDOrErr) {
     // Failed to import.
 
if (Pos != ImportedDecls.end()) {
         if (LookupTable)
           if (auto *ToND = dyn_cast<NamedDecl>(ToD))
             LookupTable->remove(ToND);
         ImportedFromDecls.erase(PosF);
       }
 
       // FIXME: AST may contain remaining references to the failed object.
     }
 
     if (!getImportDeclErrorIfAny(FromD)) {
       // Error encountered for the first time.
       // After takeError the error is not usable any more 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<ImportError>(ErrOut);
     }
     return ToDOrErr;
   }
 
   ToD = *ToDOrErr;
 
   // FIXME: Handle the "already imported with error" case. We can get here
   // nullptr only if GetImportedOrCreateDecl returned nullptr (after a
   // previously failed create was requested).
   // Later GetImportedOrCreateDecl can be updated to return the error.
   if (!ToD) {
     auto Err = getImportDeclErrorIfAny(FromD);
     assert(Err);
     return make_error<ImportError>(*Err);
   }
 
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
   // Notify subclasses.
   Imported(FromD, ToD);
 
   updateFlags(FromD, ToD);
+  SavedImportPaths[FromD].clear();
   return ToDOrErr;
 }
 
 Expected<DeclContext *> ASTImporter::ImportContext(DeclContext *FromDC) {
   if (!FromDC)
     return FromDC;
 
   ExpectedDecl ToDCOrErr = Import(cast<Decl>(FromDC));
ASTImporter::getImportDeclErrorIfAny(Decl *FromD) const {
   auto Pos = ImportDeclErrors.find(FromD);
   if (Pos != ImportDeclErrors.end())
     return Pos->second;
   else
     return Optional<ImportError>();
 }
 
 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;
 }
 
 bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To,
                                            bool Complain) {
   llvm::DenseMap<const Type *, const Type *>::iterator Pos =
       ImportedTypes.find(From.getTypePtr());
   if (Pos != ImportedTypes.end()) {
     if (ExpectedType ToFromOrErr = Import(From)) {
       if (ToContext.hasSameType(*ToFromOrErr, To))
         return true;
     } else {
       llvm::consumeError(ToFromOrErr.takeError());
     }
   }
 
   StructuralEquivalenceContext Ctx(FromContext, ToContext, NonEquivalentDecls,
                                    getStructuralEquivalenceKind(*this), false,
                                    Complain);
   return Ctx.IsEquivalent(From, To);
 }
+
+void ASTImporter::ImportPathTy::push(Decl *D) {
+  Nodes.push_back(D);
+  ++Aux[D];
+}
+
+void ASTImporter::ImportPathTy::pop() {
+  if (Nodes.empty())
+    return;
+  --Aux[Nodes.back()];
+  Nodes.pop_back();
+}
+
+Decl *ASTImporter::ImportPathTy::top() const {
+  if (!Nodes.empty())
+    return Nodes.back();
+  return nullptr;
+}
+
+bool ASTImporter::ImportPathTy::hasCycleAtBack() {
+  return Aux[Nodes.back()] > 1;
+}
+
+ASTImporter::ImportPathTy::Cycle
+ASTImporter::ImportPathTy::getCycleAtBack() const {
+  assert(Nodes.size() >= 2);
+  return Cycle(Nodes.rbegin(),
+               std::find(Nodes.rbegin() + 1, Nodes.rend(), Nodes.back()) + 1);
+}
+
+ASTImporter::ImportPathTy::VecTy
+ASTImporter::ImportPathTy::copyCycleAtBack() const {
+  auto R = getCycleAtBack();
+  return VecTy(R.begin(), R.end());
+}

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(RedirectingImporterTest, InterceptIndirectImport) {
   // Make sure our ASTImporter prevented the importing of the decl.
   auto *ToTU = To->getTranslationUnitDecl();
   auto Pattern = functionDecl(hasName("shouldNotBeImported"));
   unsigned count =
       DeclCounterWithPredicate<CXXRecordDecl>().match(ToTU, Pattern);
   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<FunctionDecl>().match(FromTU, Pattern);
+    D2 = LastDeclMatcher<FunctionDecl>().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<Decl* ,2> 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<Decl* ,4> 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<Decl* ,4> 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<Decl> Verifier;
   testImport(
       "void declToImport() { (void)\"foo\"; }",
       Lang_CXX, "", Lang_CXX, Verifier,
       functionDecl(hasDescendant(
           stringLiteral(hasType(asString("const char [4]"))))));
   testImport(
TEST_P(ASTImporterLookupTableTest, LookupSearchesInTheWholeRedeclChain) {
   DeclarationName Name = A->getDeclName();
 
   ASTImporterLookupTable LT(*ToTU);
   auto Res = LT.lookup(N1, Name);
   ASSERT_EQ(Res.size(), 1u);
   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.
 TEST_P(ASTImporterOptionSpecificTestBase,
        DISABLED_RedeclChainShouldBeCorrectAmongstNamespaces) {
   Decl *FromTU = getTuDecl(
       R"(
       namespace NS {
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().
+  // There is an error set for the other member too.
   auto *FromOK = FirstDeclMatcher<CXXMethodDecl>().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);
+}
+
+// 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(
+      R"(
+      namespace NS {
+        class A {
+          template <int I> class F {};
+          class X {
+            template <int I> friend class F;
+            void error() { )" ERRONEOUSSTMT R"( }
+          };
+        };
+
+        class B {};
+      } // NS
+      )",
+      Lang_CXX, "input0.cc");
+
+  auto *FromFRD = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("F"), isDefinition()));
+  auto *FromA = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+  auto *FromB = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("B"), isDefinition()));
+  auto *FromNS = FirstDeclMatcher<NamespaceDecl>().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<ImportError> 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);
 
-  // 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());
+  // 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<ClassTemplateDecl>().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<FriendDecl>().match(
+          FromTU, friendDecl()));
+  EXPECT_TRUE(OptErr);
+  // An error is set to the implicit class of A.
+  OptErr =
+      Importer->getImportDeclErrorIfAny(FirstDeclMatcher<CXXRecordDecl>().match(
+          FromTU, cxxRecordDecl(hasName("A"), isImplicit())));
+  EXPECT_TRUE(OptErr);
+  // An error is set to the implicit class of X.
+  OptErr =
+      Importer->getImportDeclErrorIfAny(FirstDeclMatcher<CXXRecordDecl>().match(
+          FromTU, cxxRecordDecl(hasName("X"), isImplicit())));
+  EXPECT_TRUE(OptErr);
 }
 
 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, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportType,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportDecl,
                         DefaultTestValuesForRunOptions, );