Changed every use of ASTImporter::Import to Import_New

[balazske]

• Changed some missing Import calls.

[martong]

lib/AST/ASTImporter.cpp

7797

Actually, this patch is not merely a Import -> Import_New substitution. As this line shows, the error will be propagated correctly. However, in the old version we return with a nullptr DC which may be referenced later. This is the reason, why https://reviews.llvm.org/D59692 breaks in the unittests and thus depends on this patch.

[martong]

Ping

[a_sidorin]

Hi Balazs,

The looks mostly good to me.

lib/AST/ASTImporter.cpp

3369–3370

There is the same deletion in D53757.

8598

Wow, we import types instead of just checking them for structural equivalence. That's OK to leave it in the patch as-is but looks pretty strange. Maybe this even deserves a FIXME.

unittests/AST/ASTImporterTest.cpp

160

Dead code?

[balazske]

lib/AST/ASTImporter.cpp

8598

The Import call was already here: "ToContext.hasSameType(Import(From), To))"
This is not a big thing, because the type already exists in ImportedTypes, so the Import call will return it.

[balazske]

• Rebase to current master.
• Fixed error handling in testImport.

[balazske]

Corrected rebase problems.

[martong]

Ping @shafik @a_sidorin

[a_sidorin]

LGTM.

lib/AST/ASTImporter.cpp

else
       return ToD.takeError();
 
   llvm::MutableArrayRef<NamedDecl *> CH = {NamedChain, D->getChainingSize()};
   IndirectFieldDecl *ToIndirectField;
   if (GetImportedOrCreateDecl(ToIndirectField, D, Importer.getToContext(), DC,
                               Loc, Name.getAsIdentifierInfo(), *TypeOrErr, CH))
     // FIXME here we leak `NamedChain` which is allocated before
     return ToIndirectField;
 
   ToIndirectField->setAccess(D->getAccess());
   ToIndirectField->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(ToIndirectField);
   return ToIndirectField;
 }
 
 ExpectedDecl ASTNodeImporter::VisitFriendDecl(FriendDecl *D) {
   // Import the major distinguishing characteristics of a declaration.
   DeclContext *DC, *LexicalDC;
else
     llvm::consumeError(To.takeError());
   return nullptr;
 }
 
 Expected<DeclContext *> ASTImporter::ImportContext(DeclContext *FromDC) {
   if (!FromDC)
     return FromDC;
 
-  auto *ToDC = cast_or_null<DeclContext>(Import(cast<Decl>(FromDC)));
-  if (!ToDC)
-    return nullptr;
+  ExpectedDecl ToDCOrErr = Import_New(cast<Decl>(FromDC));
+  if (!ToDCOrErr)
+    return ToDCOrErr.takeError();
+  auto *ToDC = cast<DeclContext>(*ToDCOrErr);
 
   // When we're using a record/enum/Objective-C class/protocol as a context, we
   // need it to have a definition.
   if (auto *ToRecord = dyn_cast<RecordDecl>(ToDC)) {
     auto *FromRecord = cast<RecordDecl>(FromDC);
     if (ToRecord->isCompleteDefinition()) {
       // Do nothing.
     } else if (FromRecord->isCompleteDefinition()) {
Decl *ASTImporter::MapImported(Decl *From, Decl *To) {
   // This mapping should be maintained only in this function. Therefore do not
   // check for additional consistency.
   ImportedFromDecls[To] = From;
   return To;
 }
 
 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() && ToContext.hasSameType(Import(From), To))
+  llvm::DenseMap<const Type *, const Type *>::iterator Pos =
+      ImportedTypes.find(From.getTypePtr());
+  if (Pos != ImportedTypes.end()) {
+    if (ExpectedType ToFromOrErr = Import_New(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);
 }

lib/AST/ExternalASTMerger.cpp

LookupSameContext(Source<TranslationUnitDecl *> SourceTU, const DeclContext *DC,
   Source<const DeclContext *> SourceParentDC =
       LookupSameContext(SourceTU, DC->getParent(), ReverseImporter);
   if (!SourceParentDC) {
     // If we couldn't find the parent DC in this TranslationUnit, give up.
     return nullptr;
   }
   auto *ND = cast<NamedDecl>(DC);
   DeclarationName Name = ND->getDeclName();
-  Source<DeclarationName> SourceName = ReverseImporter.Import(Name);
+  auto SourceNameOrErr = ReverseImporter.Import_New(Name);
+  if (!SourceNameOrErr) {
+    llvm::consumeError(SourceNameOrErr.takeError());
+    return nullptr;
+  }
+  Source<DeclarationName> SourceName = *SourceNameOrErr;
   DeclContext::lookup_result SearchResult =
       SourceParentDC.get()->lookup(SourceName.get());
   size_t SearchResultSize = SearchResult.size();
   if (SearchResultSize == 0 || SearchResultSize > 1) {
     // There are two cases here.  First, we might not find the name.
     // We might also find multiple copies, in which case we have no
     // guarantee that the one we wanted is the one we pick.  (E.g.,
     // if we have two specializations of the same template it is
if (Erase)
       OI = Origins.erase(OI);
     else
       ++OI;
   }
 }
 
 template <typename DeclTy>
 static bool importSpecializations(DeclTy *D, ASTImporter *Importer) {
-  for (auto *Spec : D->specializations())
-    if (!Importer->Import(Spec))
+  for (auto *Spec : D->specializations()) {
+    auto ImportedSpecOrError = Importer->Import_New(Spec);
+    if (!ImportedSpecOrError) {
+      llvm::consumeError(ImportedSpecOrError.takeError());
       return true;
+    }
+  }
   return false;
 }
 
 /// Imports specializations from template declarations that can be specialized.
 static bool importSpecializationsIfNeeded(Decl *D, ASTImporter *Importer) {
   if (!isa<TemplateDecl>(D))
     return false;
   if (auto *FunctionTD = dyn_cast<FunctionTemplateDecl>(D))
bool ExternalASTMerger::FindExternalVisibleDeclsByName(const DeclContext *DC,
   llvm::SmallVector<NamedDecl *, 1> Decls;
   llvm::SmallVector<Candidate, 4> Candidates;
 
   auto FilterFoundDecl = [&Candidates](const Candidate &C) {
    if (!HasDeclOfSameType(Candidates, C))
      Candidates.push_back(C);
   };
 
-  ForEachMatchingDC(DC, [&](ASTImporter &Forward, ASTImporter &Reverse,
-                            Source<const DeclContext *> SourceDC) -> bool {
-    DeclarationName FromName = Reverse.Import(Name);
-    DeclContextLookupResult Result = SourceDC.get()->lookup(FromName);
+  ForEachMatchingDC(DC,
+                    [&](ASTImporter &Forward, ASTImporter &Reverse,
+                      auto FromNameOrErr = Reverse.Import_New(Name);
+                      if (!FromNameOrErr) {
+                        llvm::consumeError(FromNameOrErr.takeError());
+                        return false;
+                      }
+                      DeclContextLookupResult Result =
+                          SourceDC.get()->lookup(*FromNameOrErr);
                       for (NamedDecl *FromD : Result) {
                         FilterFoundDecl(std::make_pair(FromD, &Forward));
                       }
                       return false;
                     });
 
   if (Candidates.empty())
     return false;
 
   Decls.reserve(Candidates.size());
   for (const Candidate &C : Candidates) {
     Decl *LookupRes = C.first.get();
     ASTImporter *Importer = C.second;
-    NamedDecl *ND = cast_or_null<NamedDecl>(Importer->Import(LookupRes));
+    auto NDOrErr = Importer->Import_New(LookupRes);
+    assert(NDOrErr);
+    (void)static_cast<bool>(NDOrErr);
+    NamedDecl *ND = cast_or_null<NamedDecl>(*NDOrErr);
     assert(ND);
     // If we don't import specialization, they are not available via lookup
     // because the lookup result is imported TemplateDecl and it does not
     // reference its specializations until they are imported explicitly.
     bool IsSpecImportFailed =
         importSpecializationsIfNeeded(LookupRes, Importer);
     assert(!IsSpecImportFailed);
     (void)IsSpecImportFailed;
     Decls.push_back(ND);
   }
   SetExternalVisibleDeclsForName(DC, Name, Decls);
   return true;
 }
 
 void ExternalASTMerger::FindExternalLexicalDecls(
     const DeclContext *DC, llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
     SmallVectorImpl<Decl *> &Result) {
   ForEachMatchingDC(DC, [&](ASTImporter &Forward, ASTImporter &Reverse,
                             Source<const DeclContext *> SourceDC) -> bool {
     for (const Decl *SourceDecl : SourceDC.get()->decls()) {
       if (IsKindWeWant(SourceDecl->getKind())) {
-        Decl *ImportedDecl = Forward.Import(const_cast<Decl *>(SourceDecl));
-        assert(!ImportedDecl || IsSameDC(ImportedDecl->getDeclContext(), DC));
-        (void)ImportedDecl;
+        auto ImportedDeclOrErr = Forward.Import_New(SourceDecl);
+        if (ImportedDeclOrErr)
+          assert(!(*ImportedDeclOrErr) ||
+                 IsSameDC((*ImportedDeclOrErr)->getDeclContext(), DC));
+        else
+          llvm::consumeError(ImportedDeclOrErr.takeError());
       }
     }
     return false;
   });
 }
 

lib/CrossTU/CrossTranslationUnit.cpp

llvm::Expected<ASTUnit *> CrossTranslationUnitContext::loadExternalAST(
   return Unit;
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
   assert(FD->hasBody() && "Functions to be imported should have body.");
 
   ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
-  auto *ToDecl =
-      cast_or_null<FunctionDecl>(Importer.Import(const_cast<FunctionDecl *>(FD)));
-  if (!ToDecl)
+  auto ToDeclOrError = Importer.Import_New(FD);
+  if (!ToDeclOrError) {
+    handleAllErrors(ToDeclOrError.takeError(),
+                    [&](const ImportError &IE) {
+                      switch (IE.Error) {
+                      case ImportError::NameConflict:
+                        // FIXME: Add statistic.
+                         break;
+                      case ImportError::UnsupportedConstruct:
+                        // FIXME: Add statistic.
+                        break;
+                      case ImportError::Unknown:
+                        llvm_unreachable("Unknown import error happened.");
+                        break;
+                      }
+                    });
     return llvm::make_error<IndexError>(index_error_code::failed_import);
-  assert(ToDecl->hasBody());
-  assert(FD->hasBody() && "Functions already imported should have body.");
+  }
+  auto *ToDecl = cast<FunctionDecl>(*ToDeclOrError);  
+  assert(ToDecl->hasBody() && "Imported function should have body.");
   ++NumGetCTUSuccess;
+
   return ToDecl;
 }
 
 void CrossTranslationUnitContext::lazyInitLookupTable(
     TranslationUnitDecl *ToTU) {
   if (!LookupTable)
     LookupTable = llvm::make_unique<ASTImporterLookupTable>(*ToTU);
 }

lib/Frontend/ASTMerge.cpp

for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
     TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
     for (auto *D : TU->decls()) {
       // Don't re-import __va_list_tag, __builtin_va_list.
       if (const auto *ND = dyn_cast<NamedDecl>(D))
         if (IdentifierInfo *II = ND->getIdentifier())
           if (II->isStr("__va_list_tag") || II->isStr("__builtin_va_list"))
             continue;
 
-      Decl *ToD = Importer.Import(D);
+      llvm::Expected<Decl *> ToDOrError = Importer.Import_New(D);
 
-      if (ToD) {
-        DeclGroupRef DGR(ToD);
+      if (ToDOrError) {
+        DeclGroupRef DGR(*ToDOrError);
         CI.getASTConsumer().HandleTopLevelDecl(DGR);
+      } else {
+        llvm::consumeError(ToDOrError.takeError());
       }
     }
   }
 
   AdaptedAction->ExecuteAction();
   CI.getDiagnostics().getClient()->EndSourceFile();
 }
 

unittests/AST/ASTImporterTest.cpp

auto DefaultTestValuesForRunOptions = ::testing::Values(
     ArgVector{"-fms-compatibility"},
     ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
 
 // Base class for those tests which use the family of `testImport` functions.
 class TestImportBase : public CompilerOptionSpecificTest,
                        public ::testing::WithParamInterface<ArgVector> {
 
   template <typename NodeType>
-  NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter &Importer,
-                      NodeType Node) {
+  llvm::Expected<NodeType> importNode(ASTUnit *From, ASTUnit *To,
+                                      ASTImporter &Importer, NodeType Node) {
     ASTContext &ToCtx = To->getASTContext();
 
     // Add 'From' file to virtual file system so importer can 'find' it
     // while importing SourceLocations. It is safe to add same file multiple
     // times - it just isn't replaced.
     StringRef FromFileName = From->getMainFileName();
     createVirtualFileIfNeeded(To, FromFileName,
                               From->getBufferForFile(FromFileName));
 
-    auto Imported = Importer.Import(Node);
+    auto Imported = Importer.Import_New(Node);
 
+    if (Imported) {
       // This should dump source locations and assert if some source locations
       // were not imported.
       SmallString<1024> ImportChecker;
       llvm::raw_svector_ostream ToNothing(ImportChecker);
       ToCtx.getTranslationUnitDecl()->print(ToNothing);
 
       // This traverses the AST to catch certain bugs like poorly or not
       // implemented subtrees.
-    Imported->dump(ToNothing);
+      (*Imported)->dump(ToNothing);
+    }
 
     return Imported;
   }
 
   template <typename NodeType>
   testing::AssertionResult
   testImport(const std::string &FromCode, const ArgVector &FromArgs,
              const std::string &ToCode, const ArgVector &ToArgs,
if (!ToImport)
       return testing::AssertionFailure() << "Node type mismatch!";
 
     // Sanity check: the node being imported should match in the same way as
     // the result node.
     BindableMatcher<NodeType> WrapperMatcher(VerificationMatcher);
     EXPECT_TRUE(Verifier.match(ToImport, WrapperMatcher));
 
     auto Imported = importNode(FromAST.get(), ToAST.get(), Importer, ToImport);
-    if (!Imported)
-      return testing::AssertionFailure() << "Import failed, nullptr returned!";
-
+    if (!Imported) {
+      std::string ErrorText;
+      handleAllErrors(
+          Imported.takeError(),
+          [&ErrorText](const ImportError &Err) { ErrorText = Err.message(); });
+      return testing::AssertionFailure()
+             << "Import failed, error: \"" << ErrorText << "\"!";
+    }
 
-    return Verifier.match(Imported, WrapperMatcher);
+    return Verifier.match(*Imported, WrapperMatcher);
   }
 
   template <typename NodeType>
   testing::AssertionResult
   testImport(const std::string &FromCode, const ArgVector &FromArgs,
              const std::string &ToCode, const ArgVector &ToArgs,
              MatchVerifier<NodeType> &Verifier,
              const BindableMatcher<NodeType> &VerificationMatcher) {
for (const ImportAction &Action : ImportActions) {
             From->getFileManager(), false));
 
       // Find the declaration and import it.
       auto FoundDecl = match(Action.ImportPredicate.bind(DeclToImportID),
                              From->getASTContext());
       EXPECT_TRUE(FoundDecl.size() == 1);
       const Decl *ToImport = selectFirst<Decl>(DeclToImportID, FoundDecl);
       auto Imported = importNode(From, To, *ImporterRef, ToImport);
-      EXPECT_TRUE(Imported);
+      EXPECT_TRUE(static_cast<bool>(Imported));
+      if (!Imported)
+        llvm::consumeError(Imported.takeError());
     }
 
     // Find the declaration and import it.
     auto FoundDecl = match(FinalSelectPredicate.bind(DeclToVerifyID),
                            AllASTs[FileForFinalCheck]->getASTContext());
     EXPECT_TRUE(FoundDecl.size() == 1);
     const Decl *ToVerify = selectFirst<Decl>(DeclToVerifyID, FoundDecl);
     MatchVerifier<Decl> Verifier;
void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
       }
       assert(&ToAST->getASTContext() == &Importer->getToContext());
       createVirtualFileIfNeeded(ToAST, FileName, Code);
     }
 
     Decl *import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
                  Decl *FromDecl) {
       lazyInitImporter(LookupTable, ToAST);
-      return Importer->Import(FromDecl);
+      if (auto ImportedOrErr = Importer->Import_New(FromDecl))
+        return *ImportedOrErr;
+      else {
+        llvm::consumeError(ImportedOrErr.takeError());
+        return nullptr;
+      }
     }
 
     QualType import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
                     QualType FromType) {
       lazyInitImporter(LookupTable, ToAST);
-      return Importer->Import(FromType);
+      if (auto ImportedOrErr = Importer->Import_New(FromType))
+        return *ImportedOrErr;
+      else {
+        llvm::consumeError(ImportedOrErr.takeError());
+        return QualType{};
+      }
     }
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
   // moving of the stored objects because that would mean breaking the