[ASTImporter] Mark erroneous nodes in from ctx

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.

Reviewers: a_sidorin, a.sidorin, shafik

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

Tags: #clang

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

llvm-svn: 364771

Author: Gabor Marton

clang/include/clang/AST/ASTImporter.h

@@ -87,6 +87,127 @@ class TypeSourceInfo;
     using ImportedCXXBaseSpecifierMap =
         llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
 
+    // 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<Decl *, 32>;
+
+      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<VecTy::const_reverse_iterator>;
+      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<Decl *, int, 32> Aux;
+    };
+
   private:
 
     /// Pointer to the import specific lookup table, which may be shared
@@ -96,6 +217,17 @@ class TypeSourceInfo;
     /// 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;
 

clang/lib/AST/ASTImporter.cpp

@@ -7842,6 +7842,10 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   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 @@ Expected<Decl *> ASTImporter::Import(Decl *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;
   }
 
@@ -7889,16 +7897,20 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
       // 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<ImportError>(ErrOut);
   }
@@ -7921,6 +7933,7 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   Imported(FromD, ToD);
 
   updateFlags(FromD, ToD);
+  SavedImportPaths[FromD].clear();
   return ToDOrErr;
 }
 
@@ -8641,9 +8654,10 @@ ASTImporter::getImportDeclErrorIfAny(Decl *FromD) const {
 }
 
 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,