[ASTImporter] Store import errors for Decls

[martong]

Summary

We add a new member (ImportedFromDecls) which is a mapping from the already-imported
declarations in the "from" context to the error status of the import of
that declaration. This map contains only the declarations that were not
correctly imported. The same declaration may or may not be included in
ImportedDecls. This map is updated continuously during imports and never
cleared (like ImportedDecls). In Import(Decl*) we use this mapping, so
if there was a previous failed import we return with the existing error.

We add/remove from the Lookuptable in consistency with ImportedFromDecls.
When we map a decl in the 'to' context to something in the 'from'
context then and only then we add it to the lookup table. When we
remove a mapping then and only then we remove it from the lookup table.

This patch is the first in a series of patches whose aim is to further
strengthen the error handling in ASTImporter.

[a_sidorin]

Hi Gabor,
The idea looks fine to me, but I have some questions inline.

clang/lib/AST/ASTImporter.cpp

5109

Is this FIXME obsolete now?

7823

I see the enabled test case, but does it cover the logic in this block?

7851

Is it possible to get this error more than once?

[martong]

• Test error values are set for AST nodes

[martong]

clang/lib/AST/ASTImporter.cpp

5109

That is not obsolete. We should call HandleNameConflict as we do in other visit functions. But it is not trivial how to do that, there are open questions which I cannot address in this PR:

• Should we pass D->getDeclName or rather the name of the imported ClassTemplateDecl or should we import the name here (again)?
• Should we pass the template args to HandleNameConflict?

7823

Right, that is not strictly related. I have added relevant unit test cases for the error handling. This branch is handled in ErrorHappensAfterCreatingTheNodeButBeforeLinkingThatToTheAST test.

7851

Yes, that can happen in cyclic imports like: ClassTemplateDecl -> TemplatedDecl -> ClassTemplateDecl.

[martong]

• Remove unrelated change

[martong]

• Remove formatv b/c it can't handle braces in code

[martong]

• Remove unused include and using

[a_sidorin]

clang/unittests/AST/ASTImporterTest.cpp

4697 ↗

(On Diff #206020)

#undef ERRONEOUSSTMT?

[balazske]

clang/lib/AST/ASTImporter.cpp

7851

I do not understand this completely, in this branch setImportDeclError is called so at next time we can not go into this branch again (for the same FromD). (We can get the error and not go into this branch more than once but get no error and go into the branch only once.)

clang/unittests/AST/ASTImporterTest.cpp

4697 ↗

(On Diff #206020)

Maybe we should not use a macro for this, specially not define the macro inside the struct block. For the current tests it is sufficient to make a std::string that contains something like void f() { asm(""); }.

[martong]

clang/lib/AST/ASTImporter.cpp

7851

Yes, I missed that.
So, perhaps we can remove the condition if (!getImportDeclErrorIfAny(FromD)) ?

clang/unittests/AST/ASTImporterTest.cpp

4697 ↗

(On Diff #206020)

My first attempt was to use std::string for the errouneous Stmt and then to use llvm::formatv to easily concatenate the test code examples.
However, it tunred out that formatv actively forbids the use of the braces in the source string.
So I decided to use a macro because that way we can build up the test code most easily.
What is the problem by defining the macro in a struct block? There are no blocks for the preprocessor, just directives. I'd like to indicate that the macro is used only by the test.

[balazske]

clang/lib/AST/ASTImporter.cpp

7851

I think yes: If !ToDOrErr is true we have an error that was newly created in ImportImpl. But I am not totally sure, and the ImportImpl can be overridden, so it is better to not remove the condition. Or make an assert out of it: assert(!getImportDeclErrorIfAny(FromD) && "Import error already set for decl.").

clang/unittests/AST/ASTImporterTest.cpp

4697 ↗

(On Diff #206020)

The problem was that it looks like that macro is an internal thing (to the struct) but in reality it is not. Probably std::string::append can be used if possible.

[martong]

• Assert that we set the error only once
• Remove the macro and use std::string.op+

[martong]

clang/lib/AST/ASTImporter.cpp

7851

Ok, I changed to have an assertion.

clang/unittests/AST/ASTImporterTest.cpp

4697 ↗

(On Diff #206020)

Ok, I removed the macro and use rather std::string.operator+()

[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]

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

clang/include/clang/AST/ASTImporter.h

private:
     /// Mapping from the already-imported types in the "from" context
     /// to the corresponding types in the "to" context.
     llvm::DenseMap<const Type *, const Type *> ImportedTypes;
 
     /// Mapping from the already-imported declarations in the "from"
     /// context to the corresponding declarations in the "to" context.
     llvm::DenseMap<Decl *, Decl *> ImportedDecls;
 
+    /// Mapping from the already-imported declarations in the "from"
+    /// context to the error status of the import of that declaration.
+    /// This map contains only the declarations that were not correctly
+    /// imported. The same declaration may or may not be included in
+    /// ImportedDecls. This map is updated continuously during imports and never
+    /// cleared (like ImportedDecls).
+    llvm::DenseMap<Decl *, ImportError> ImportDeclErrors;
+
     /// Mapping from the already-imported declarations in the "to"
     /// context to the corresponding declarations in the "from" context.
     llvm::DenseMap<Decl *, Decl *> ImportedFromDecls;
 
     /// Mapping from the already-imported statements in the "from"
     /// context to the corresponding statements in the "to" context.
     llvm::DenseMap<Stmt *, Stmt *> ImportedStmts;
 
public:
 
     /// Subclasses can override this function to observe all of the \c From ->
     /// \c To declaration mappings as they are imported.
     virtual void Imported(Decl *From, Decl *To) {}
 
     void RegisterImportedDecl(Decl *FromD, Decl *ToD);
 
     /// Store and assign the imported declaration to its counterpart.
+    /// It may happen that several decls from the 'from' context are mapped to
+    /// the same decl in the 'to' context.
     Decl *MapImported(Decl *From, Decl *To);
 
     /// Called by StructuralEquivalenceContext.  If a RecordDecl is
     /// being compared to another RecordDecl as part of import, completing the
     /// other RecordDecl may trigger importation of the first RecordDecl. This
     /// happens especially for anonymous structs.  If the original of the second
     /// RecordDecl can be found, we can complete it without the need for
     /// importation, eliminating this loop.
     virtual Decl *GetOriginalDecl(Decl *To) { return nullptr; }
 
+    /// Return if import of the given declaration has failed and if yes
+    /// the kind of the problem. This gives the first error encountered with
+    /// the node.
+    llvm::Optional<ImportError> getImportDeclErrorIfAny(Decl *FromD) const;
+
+    /// Mark (newly) imported declaration with error.
+    void setImportDeclError(Decl *From, ImportError Error);
+
     /// Determine whether the given types are structurally
     /// equivalent.
     bool IsStructurallyEquivalent(QualType From, QualType To,
                                   bool Complain = true);
 
     /// Determine the index of a field in its parent record.
     /// F should be a field (or indirect field) declaration.
     /// \returns The index of the field in its parent context (starting from 0).
     /// On error `None` is returned (parent context is non-record).
     static llvm::Optional<unsigned> getFieldIndex(Decl *F);
 
   };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_ASTIMPORTER_H

clang/lib/AST/ASTImporter.cpp

// Wrapper for an overload set.
     }
     // Use this version if a special create function must be
     // used, e.g. CXXRecordDecl::CreateLambda .
     template <typename ToDeclT, typename CreateFunT, typename FromDeclT,
               typename... Args>
     LLVM_NODISCARD bool
     GetImportedOrCreateSpecialDecl(ToDeclT *&ToD, CreateFunT CreateFun,
                                    FromDeclT *FromD, Args &&... args) {
-      // FIXME: This code is needed later.
-      //if (Importer.getImportDeclErrorIfAny(FromD)) {
-      //  ToD = nullptr;
-      //  return true; // Already imported but with error.
-      //}
+      if (Importer.getImportDeclErrorIfAny(FromD)) {
+        ToD = nullptr;
+        return true; // Already imported but with error.
+      }
       ToD = cast_or_null<ToDeclT>(Importer.GetAlreadyImportedOrNull(FromD));
       if (ToD)
         return true; // Already imported.
       ToD = CreateFun(std::forward<Args>(args)...);
       // Keep track of imported Decls.
       Importer.RegisterImportedDecl(FromD, ToD);
       InitializeImportedDecl(FromD, ToD);
       return false; // A new Decl is created.
if (IsStructuralMatch(D, PrevDecl)) {
         // TODO Import instantiated default arguments.
         // TODO Import instantiated exception specifications.
         //
         // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
         // what else could be fused during an AST merge.
         return PrevDecl;
       }
     } else { // ODR violation.
       // FIXME HandleNameConflict
-      return nullptr;
+      return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
   // Import the location of this declaration.
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
   ExpectedSLoc IdLocOrErr = import(D->getLocation());

 Expected<Decl *> ASTImporter::ImportImpl(Decl *FromD) {
   // Import the decl using ASTNodeImporter.
   ASTNodeImporter Importer(*this);
   return Importer.Visit(FromD);
 }
 
 void ASTImporter::RegisterImportedDecl(Decl *FromD, Decl *ToD) {
   MapImported(FromD, ToD);
-  AddToLookupTable(ToD);
 }
 
 Expected<QualType> ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
     return QualType{};
 
   const Type *FromTy = FromT.getTypePtr();
 
TranslationUnitDecl *ASTImporter::GetFromTU(Decl *ToD) {
   return FromDPos->second->getTranslationUnitDecl();
 }
 
 Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   if (!FromD)
     return nullptr;
 
 
+  // 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);
     return ToD;
   }
 
   // Import the declaration.
   ExpectedDecl ToDOrErr = ImportImpl(FromD);
-  if (!ToDOrErr)
+  if (!ToDOrErr) {
+    // Failed to import.
+
+    auto Pos = ImportedDecls.find(FromD);
+    if (Pos != ImportedDecls.end()) {
+      // Import failed after the object was created.
+      // Remove all references to it.
+      auto *ToD = Pos->second;
+      ImportedDecls.erase(Pos);
+
+      // ImportedDecls and ImportedFromDecls are not symmetric.  It may happen
+      // (e.g. with namespaces) that several decls from the 'from' context are
+      // mapped to the same decl in the 'to' context.  If we removed entries
+      // from the LookupTable here then we may end up removing them multiple
+      // times.
+
+      // The Lookuptable contains decls only which are in the 'to' context.
+      // Remove from the Lookuptable only if it is *imported* into the 'to'
+      // context (and do not remove it if it was added during the initial
+      // traverse of the 'to' context).
+      auto PosF = ImportedFromDecls.find(ToD);
+      if (PosF != ImportedFromDecls.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);
+      // Do not return ToDOrErr, error was taken out of it.
+      return make_error<ImportError>(ErrOut);
+    }
     return ToDOrErr;
+  }
+
   ToD = *ToDOrErr;
 
-  // FIXME Use getImportDeclErrorIfAny() here (and return with the error) once
-  // the error handling is finished in GetImportedOrCreateSpecialDecl().
+  // 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) {
-    return nullptr;
+    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?");
-
-  // Once the decl is connected to the existing declarations, i.e. when the
-  // redecl chain is properly set then we populate the lookup again.
-  // This way the primary context will be able to find all decls.
-  AddToLookupTable(ToD);
-
   // Notify subclasses.
   Imported(FromD, ToD);
 
   updateFlags(FromD, ToD);
   return ToDOrErr;
 }
 
 Expected<DeclContext *> ASTImporter::ImportContext(DeclContext *FromDC) {
Decl *ASTImporter::MapImported(Decl *From, Decl *To) {
   assert((Pos == ImportedDecls.end() || Pos->second == To) &&
       "Try to import an already imported Decl");
   if (Pos != ImportedDecls.end())
     return Pos->second;
   ImportedDecls[From] = To;
   // This mapping should be maintained only in this function. Therefore do not
   // check for additional consistency.
   ImportedFromDecls[To] = From;
+  AddToLookupTable(To);
   return To;
 }
 
+llvm::Optional<ImportError>
+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;

clang/test/ASTMerge/class-template-partial-spec/test.cpp

-// FIXME: Crashes after r357394
-// XFAIL: *
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/class-template-partial-spec1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/class-template-partial-spec2.cpp
 // RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(sizeof(**SingleSource.member) == sizeof(**SingleDest.member));
 static_assert(sizeof(SecondDoubleSource.member) == sizeof(SecondDoubleDest.member));
 static_assert(NumberSource.val == 42);
 static_assert(sizeof(Z0Source.member) == sizeof(char));