Changed every use of ASTImporter::Import to Import_New

[balazske]

• Changed some missing Import calls.

[martong]

lib/AST/ASTImporter.cpp

7767 ↗

(On Diff #175864)

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

3440 ↗

(On Diff #175864)

There is the same deletion in D53757.

8550 ↗

(On Diff #175864)

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

146

Dead code?

[balazske]

lib/AST/ASTImporter.cpp

8550 ↗

(On Diff #175864)

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/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(
   } else {
     Unit = FnUnitCacheEntry->second;
   }
   return Unit;
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
-  ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
-  auto *ToDecl =
-      cast<FunctionDecl>(Importer.Import(const_cast<FunctionDecl *>(FD)));
-  assert(ToDecl->hasBody());
   assert(FD->hasBody() && "Functions already imported should have body.");
+  ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
+  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);
+  }
+  auto *ToDecl = cast<FunctionDecl>(*ToDeclOrError);  
+  assert(ToDecl->hasBody() && "Imported function should have body.");
   return ToDecl;
 }
 
 ASTImporter &
 CrossTranslationUnitContext::getOrCreateASTImporter(ASTContext &From) {
   auto I = ASTUnitImporterMap.find(From.getTranslationUnitDecl());
   if (I != ASTUnitImporterMap.end())
     return *I->second;

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

struct ParameterizedTestsFixture : ::testing::TestWithParam<ArgVector> {
   }
 
 };
 
 // Base class for those tests which use the family of `testImport` functions.
 class TestImportBase : public ParameterizedTestsFixture {
 
   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) {
+      llvm::Error Err = Imported.takeError();
+      return testing::AssertionFailure()
+             << "Import failed, error: \"" << Err << "\"!";
+      llvm::consumeError(std::move(Err));
+    }
 
-    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(ASTUnit *ToAST) {
             Unit->getASTContext(), Unit->getFileManager(), false));
       }
       assert(&ToAST->getASTContext() == &Importer->getToContext());
       createVirtualFileIfNeeded(ToAST, FileName, Code);
     }
 
     Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
       lazyInitImporter(ToAST);
-      return Importer->Import(FromDecl);
-     }
+      if (auto ImportedOrErr = Importer->Import_New(FromDecl))
+        return *ImportedOrErr;
+      else {
+        llvm::consumeError(ImportedOrErr.takeError());
+        return nullptr;
+      }
 
     QualType import(ASTUnit *ToAST, QualType FromType) {
       lazyInitImporter(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