[ASTImporter] Changed use of Import to Import_New in ASTImporter.

[balazske]

The Import functions are replaced by Import_New and error handling is added. This is done in the implementation of the Import_New functions itself, and in importInto. (Code of old Import functions is moved to the Import_New and updated to use Import_New.) Some other functions in ASTImporter are still using the Import, these are to be changed later.

[balazske]

Reformatted code.

[a_sidorin]

Hi Balasz,

As I guess, the next step is to convert all Importcalls to Import_New and then rename Import_New into Import. If so, why aren't we able to just change the behaviour of Import methods?
Also, there are some comments inline (review is not fully completed, I'll continue review after we agree if this patch is needed).

include/clang/AST/ASTImporter.h

215–221

an import error?

221

these versions

lib/AST/ASTImporter.cpp

7966

We can use Import_New if we change this code, like it is done below.

8008

We can use -> here. Same in some cases below.

8009

If the true branch is enclosed with braces, the else branch should be enclosed as well.

8017–8018

This line exceeds 80-char limit.

[balazske]

If we change signature of Import now, other parts of the code (in clang and LLDB) would not compile (without changing to use the new kind of Import). If there is a Import_New the old code can still compile and can be changed later.

lib/AST/ASTImporter.cpp

7966

For IdentifierInfo there is no Import_New because this is the one kind of import that can not fail, it remains the same Import as before (does not return Expected).

[balazske]

• Small style corrections.

[balazske]

include/clang/AST/ASTImporter.h

215–221

I do not know what is the correct, but a specific import error is returned, that occurred during the import.

221

If the other comment is changed I can change this too, but alone this is not worth it. This is anyway a temporary comment, and if the first Import there is removed the second can not compile anyway (and the Import_New will be the Import later).

[a_sidorin]

LGTM. Thank you for addressing my questions!

[martong]

In D53818#1307321, @a_sidorin wrote:

LGTM. Thank you for addressing my questions!

Hi Alexei,

Could you please also take a look on that patch which this one depends on? https://reviews.llvm.org/D53751
I think the changes of that patch are included in this one too. Thank you.

[balazske]

Rebase to newest master.

[davide]

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too).
To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on.
I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance.
I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

[martong]

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance. I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

@davide I am terribly sorry about this issue. I just applied this patch on my Linux box, and it did not failed those tests which failed on the green lab buildbots. Could you please refer to the Linux buildbots which failed? Seems like there is a significant difference between the testsuites on Linux and MacOS. Our primary target is Linux and we do run the LLDB tests before we accept any commit into our fork and only then we continue with upstreaming to Phabricator.

Our platform in our CI is Linux only, but we are in the middle of getting a hosted MacOS to support our CI. I do hope that in the near future there will be no such issues. In the meantime, is there a way for us to execute a green lab MacOS build bot with a specific patch before a commit? If that is not possible, then we can just promise that we will monitor your buildbots and we will do the revert.

[davide]

In D53818#1312133, @martong wrote:

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance. I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

@davide I am terribly sorry about this issue. I just applied this patch on my Linux box, and it did not failed those tests which failed on the green lab buildbots. Could you please refer to the Linux buildbots which failed? Seems like there is a significant difference between the testsuites on Linux and MacOS. Our primary target is Linux and we do run the LLDB tests before we accept any commit into our fork and only then we continue with upstreaming to Phabricator.

I have no Linux bots access, this fails on macos (looks on http://lab.llvm.org:8080/green/). Although now it's too late because the logs are gone.

Our platform in our CI is Linux only, but we are in the middle of getting a hosted MacOS to support our CI. I do hope that in the near future there will be no such issues. In the meantime, is there a way for us to execute a green lab MacOS build bot with a specific patch before a commit? If that is not possible, then we can just promise that we will monitor your buildbots and we will do the revert.

I'm not aware of a way of pre-commit testing this hosted on llvm.org. I generally run on my machine and check the bots output.

[balazske]

Reopening to fix failing lldb tests.

[balazske]

Rebase.

[balazske]

• Small style corrections.

[balazske]

• Small style corrections.

include/clang/AST/ASTImporter.h

public:
 
     /// \brief Import the given object, returns the result.
     ///
     /// \param To Import the object into this variable.
     /// \param From Object to import.
     /// \return Error information (success or error).
     template <typename ImportT>
     LLVM_NODISCARD llvm::Error importInto(ImportT &To, const ImportT &From) {
-      To = Import(From);
-      if (From && !To)
-          return llvm::make_error<ImportError>();
-      return llvm::Error::success();
-      // FIXME: this should be the final code
-      //auto ToOrErr = Import(From);
-      //if (ToOrErr)
-      //  To = *ToOrErr;
-      //return ToOrErr.takeError();
+      auto ToOrErr = Import_New(From);
+      if (ToOrErr)
+        To = *ToOrErr;
+      return ToOrErr.takeError();
     }
 
     /// Import the given type from the "from" context into the "to"
     /// context. A null type is imported as a null type (no error).
     ///
     /// \returns The equivalent type in the "to" context, or the import error.
     llvm::Expected<QualType> Import_New(QualType FromT);
     // FIXME: Remove this version.
public:
     /// error.
     llvm::Expected<Attr *> Import_New(const Attr *FromAttr);
     // FIXME: Remove this version.
     Attr *Import(const Attr *FromAttr);
 
     /// Import the given declaration from the "from" context into the
     /// "to" context.
     ///
     /// \returns The equivalent declaration in the "to" context, or the import
     /// error.
     llvm::Expected<Decl *> Import_New(Decl *FromD);
     llvm::Expected<Decl *> Import_New(const Decl *FromD) {
       return Import_New(const_cast<Decl *>(FromD));
     }
     // FIXME: Remove this version.
     Decl *Import(Decl *FromD);
     Decl *Import(const Decl *FromD) {
       return Import(const_cast<Decl *>(FromD));
     }
 
     /// Return the copy of the given declaration in the "to" context if
     /// it has already been imported from the "from" context.  Otherwise return
     /// nullptr.

lib/AST/ASTImporter.cpp

namespace clang {
 
   void updateFlags(const Decl *From, Decl *To) {
     // Check if some flags or attrs are new in 'From' and copy into 'To'.
     // FIXME: Other flags or attrs?
     if (From->isUsed(false) && !To->isUsed(false))
       To->setIsUsed();
   }
 
-  // FIXME: Temporary until every import returns Expected.
-  template <>
-  LLVM_NODISCARD Error
-  ASTImporter::importInto(SourceLocation &To, const SourceLocation &From) {
-    To = Import(From);
-    if (From.isValid() && To.isInvalid())
-        return llvm::make_error<ImportError>();
-    return Error::success();
-  }
-  // FIXME: Temporary until every import returns Expected.
-  template <>
-  LLVM_NODISCARD Error
-  ASTImporter::importInto(QualType &To, const QualType &From) {
-    To = Import(From);
-    if (!From.isNull() && To.isNull())
-        return llvm::make_error<ImportError>();
-    return Error::success();
-  }
-
   class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
                           public DeclVisitor<ASTNodeImporter, ExpectedDecl>,
                           public StmtVisitor<ASTNodeImporter, ExpectedStmt> {
     ASTImporter &Importer;
 
     // Use this instead of Importer.importInto .
     template <typename ImportT>
     LLVM_NODISCARD Error importInto(ImportT &To, const ImportT &From) {
: LookupTable(LookupTable), ToContext(ToContext), FromContext(FromContext),
       Minimal(MinimalImport) {
 
   ImportedDecls[FromContext.getTranslationUnitDecl()] =
       ToContext.getTranslationUnitDecl();
 }
 
 ASTImporter::~ASTImporter() = default;
 
-Expected<QualType> ASTImporter::Import_New(QualType FromT) {
-  QualType ToT = Import(FromT);
-  if (ToT.isNull() && !FromT.isNull())
-    return make_error<ImportError>();
-  return ToT;
-}
-
 Optional<unsigned> ASTImporter::getFieldIndex(Decl *F) {
   assert(F && (isa<FieldDecl>(*F) || isa<IndirectFieldDecl>(*F)) &&
       "Try to get field index for non-field.");
 
   auto *Owner = dyn_cast<RecordDecl>(F->getDeclContext());
   if (!Owner)
     return None;
 

 }
 
 void ASTImporter::AddToLookupTable(Decl *ToD) {
   if (LookupTable)
     if (auto *ToND = dyn_cast<NamedDecl>(ToD))
       LookupTable->add(ToND);
 }
 
-QualType ASTImporter::Import(QualType FromT) {
+Expected<QualType> ASTImporter::Import_New(QualType FromT) {
   if (FromT.isNull())
-    return {};
+    return QualType{};
 
   const Type *FromTy = FromT.getTypePtr();
 
   // Check whether we've already imported this type.
   llvm::DenseMap<const Type *, const Type *>::iterator Pos
     = ImportedTypes.find(FromTy);
   if (Pos != ImportedTypes.end())
     return ToContext.getQualifiedType(Pos->second, FromT.getLocalQualifiers());
 
   // Import the type
   ASTNodeImporter Importer(*this);
   ExpectedType ToTOrErr = Importer.Visit(FromTy);
-  if (!ToTOrErr) {
-    llvm::consumeError(ToTOrErr.takeError());
-    return {};
-  }
+  if (!ToTOrErr)
+    return ToTOrErr.takeError();
 
   // Record the imported type.
   ImportedTypes[FromTy] = (*ToTOrErr).getTypePtr();
 
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
+QualType ASTImporter::Import(QualType From) {
+  llvm::Expected<QualType> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 Expected<TypeSourceInfo *> ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
-  TypeSourceInfo *ToTSI = Import(FromTSI);
-  if (!ToTSI && FromTSI)
-    return llvm::make_error<ImportError>();
-  return ToTSI;
-}
-TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
     return FromTSI;
 
   // FIXME: For now we just create a "trivial" type source info based
   // on the type and a single location. Implement a real version of this.
-  QualType T = Import(FromTSI->getType());
-  if (T.isNull())
-    return nullptr;
+  ExpectedType TOrErr = Import_New(FromTSI->getType());
+  if (!TOrErr)
+    return TOrErr.takeError();
+  ExpectedSLoc BeginLocOrErr = Import_New(FromTSI->getTypeLoc().getBeginLoc());
+  if (!BeginLocOrErr)
+    return BeginLocOrErr.takeError();
 
-  return ToContext.getTrivialTypeSourceInfo(
-      T, Import(FromTSI->getTypeLoc().getBeginLoc()));
+  return ToContext.getTrivialTypeSourceInfo(*TOrErr, *BeginLocOrErr);
+}
+TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *From) {
+  llvm::Expected<TypeSourceInfo *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
 }
 
 Expected<Attr *> ASTImporter::Import_New(const Attr *FromAttr) {
-  return Import(FromAttr);
-}
-Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
-  // NOTE: Import of SourceRange may fail.
-  ToAttr->setRange(Import(FromAttr->getRange()));
+  if (auto ToRangeOrErr = Import_New(FromAttr->getRange()))
+    ToAttr->setRange(*ToRangeOrErr);
+  else
+    return ToRangeOrErr.takeError();
+
   return ToAttr;
 }
+Attr *ASTImporter::Import(const Attr *From) {
+  llvm::Expected<Attr *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
+}
 
 Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
   auto Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end())
     return Pos->second;
   else
     return nullptr;
 }
 
 TranslationUnitDecl *ASTImporter::GetFromTU(Decl *ToD) {
   auto FromDPos = ImportedFromDecls.find(ToD);
   if (FromDPos == ImportedFromDecls.end())
     return nullptr;
   return FromDPos->second->getTranslationUnitDecl();
 }
 
 Expected<Decl *> ASTImporter::Import_New(Decl *FromD) {
-  Decl *ToD = Import(FromD);
-  if (!ToD && FromD)
-    return llvm::make_error<ImportError>();
-  return ToD;
-}
-Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
     return nullptr;
 
   ASTNodeImporter Importer(*this);
 
   // 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 type.
+  // Import the declaration.
   ExpectedDecl ToDOrErr = Importer.Visit(FromD);
-  if (!ToDOrErr) {
-    llvm::consumeError(ToDOrErr.takeError());
-    return nullptr;
-  }
+  if (!ToDOrErr)
+    return ToDOrErr;
   ToD = *ToDOrErr;
 
   // 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 ToD;
+  return ToDOrErr;
+}
+Decl *ASTImporter::Import(Decl *From) {
+  llvm::Expected<Decl *> To = Import_New(From);
+  if (To)
+    return *To;
+  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)
if (ToProto->getDefinition()) {
       CompleteDecl(ToProto);
     }
   }
 
   return ToDC;
 }
 
 Expected<Expr *> ASTImporter::Import_New(Expr *FromE) {
-  Expr *ToE = Import(FromE);
-  if (!ToE && FromE)
-    return llvm::make_error<ImportError>();
-  return ToE;
+  if (ExpectedStmt ToSOrErr = Import_New(cast_or_null<Stmt>(FromE)))
+    return cast_or_null<Expr>(*ToSOrErr);
+  else
+    return ToSOrErr.takeError();
 }
-Expr *ASTImporter::Import(Expr *FromE) {
-  if (!FromE)
-    return nullptr;
-
-  return cast_or_null<Expr>(Import(cast<Stmt>(FromE)));
+Expr *ASTImporter::Import(Expr *From) {
+  llvm::Expected<Expr *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
 }
 
 Expected<Stmt *> ASTImporter::Import_New(Stmt *FromS) {
-  Stmt *ToS = Import(FromS);
-  if (!ToS && FromS)
-    return llvm::make_error<ImportError>();
-  return ToS;
-}
-Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
     return nullptr;
 
-  // Check whether we've already imported this declaration.
+  // Check whether we've already imported this statement.
   llvm::DenseMap<Stmt *, Stmt *>::iterator Pos = ImportedStmts.find(FromS);
   if (Pos != ImportedStmts.end())
     return Pos->second;
 
   // Import the statement.
   ASTNodeImporter Importer(*this);
   ExpectedStmt ToSOrErr = Importer.Visit(FromS);
-  if (!ToSOrErr) {
-    llvm::consumeError(ToSOrErr.takeError());
-    return nullptr;
-  }
+  if (!ToSOrErr)
+    return ToSOrErr;
 
   if (auto *ToE = dyn_cast<Expr>(*ToSOrErr)) {
     auto *FromE = cast<Expr>(FromS);
     // Copy ExprBitfields, which may not be handled in Expr subclasses
     // constructors.
     ToE->setValueKind(FromE->getValueKind());
     ToE->setObjectKind(FromE->getObjectKind());
     ToE->setTypeDependent(FromE->isTypeDependent());
     ToE->setValueDependent(FromE->isValueDependent());
     ToE->setInstantiationDependent(FromE->isInstantiationDependent());
     ToE->setContainsUnexpandedParameterPack(
         FromE->containsUnexpandedParameterPack());
   }
 
-  // Record the imported declaration.
+  // Record the imported statement object.
   ImportedStmts[FromS] = *ToSOrErr;
-  return *ToSOrErr;
+  return ToSOrErr;
+}
+Stmt *ASTImporter::Import(Stmt *From) {
+  llvm::Expected<Stmt *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
 }
 
 Expected<NestedNameSpecifier *>
 ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
-  NestedNameSpecifier *ToNNS = Import(FromNNS);
-  if (!ToNNS && FromNNS)
-    return llvm::make_error<ImportError>();
-  return ToNNS;
-}
-NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
     return nullptr;
 
-  NestedNameSpecifier *prefix = Import(FromNNS->getPrefix());
+  NestedNameSpecifier *Prefix;
+  if (Error Err = importInto(Prefix, FromNNS->getPrefix()))
+    return std::move(Err);
 
   switch (FromNNS->getKind()) {
   case NestedNameSpecifier::Identifier:
-    if (IdentifierInfo *II = Import(FromNNS->getAsIdentifier())) {
-      return NestedNameSpecifier::Create(ToContext, prefix, II);
-    }
-    return nullptr;
+    assert(FromNNS->getAsIdentifier() && "NNS should contain identifier.");
+    return NestedNameSpecifier::Create(ToContext, Prefix,
+                                       Import(FromNNS->getAsIdentifier()));
 
   case NestedNameSpecifier::Namespace:
-    if (auto *NS =
-            cast_or_null<NamespaceDecl>(Import(FromNNS->getAsNamespace()))) {
-      return NestedNameSpecifier::Create(ToContext, prefix, NS);
-    }
-    return nullptr;
+    if (ExpectedDecl NSOrErr = Import_New(FromNNS->getAsNamespace())) {
+      return NestedNameSpecifier::Create(ToContext, Prefix,
+                                         cast<NamespaceDecl>(*NSOrErr));
+    } else
+      return NSOrErr.takeError();
 
   case NestedNameSpecifier::NamespaceAlias:
-    if (auto *NSAD =
-          cast_or_null<NamespaceAliasDecl>(Import(FromNNS->getAsNamespaceAlias()))) {
-      return NestedNameSpecifier::Create(ToContext, prefix, NSAD);
-    }
-    return nullptr;
+    if (ExpectedDecl NSADOrErr = Import_New(FromNNS->getAsNamespaceAlias()))
+      return NestedNameSpecifier::Create(ToContext, Prefix,
+                                         cast<NamespaceAliasDecl>(*NSADOrErr));
+    else
+      return NSADOrErr.takeError();
 
   case NestedNameSpecifier::Global:
     return NestedNameSpecifier::GlobalSpecifier(ToContext);
 
   case NestedNameSpecifier::Super:
-    if (auto *RD =
-            cast_or_null<CXXRecordDecl>(Import(FromNNS->getAsRecordDecl()))) {
-      return NestedNameSpecifier::SuperSpecifier(ToContext, RD);
-    }
-    return nullptr;
+    if (ExpectedDecl RDOrErr = Import_New(FromNNS->getAsRecordDecl()))
+      return NestedNameSpecifier::SuperSpecifier(ToContext,
+                                                 cast<CXXRecordDecl>(*RDOrErr));
+    else
+      return RDOrErr.takeError();
 
   case NestedNameSpecifier::TypeSpec:
-  case NestedNameSpecifier::TypeSpecWithTemplate: {
-      QualType T = Import(QualType(FromNNS->getAsType(), 0u));
-      if (!T.isNull()) {
-        bool bTemplate = FromNNS->getKind() ==
-                         NestedNameSpecifier::TypeSpecWithTemplate;
-        return NestedNameSpecifier::Create(ToContext, prefix,
-                                           bTemplate, T.getTypePtr());
-      }
-    }
-      return nullptr;
-  }
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+    if (Expected<QualType> TyOrErr =
+            Import_New(QualType(FromNNS->getAsType(), 0u))) {
+      bool TSTemplate =
+          FromNNS->getKind() == NestedNameSpecifier::TypeSpecWithTemplate;
+      return NestedNameSpecifier::Create(ToContext, Prefix, TSTemplate,
+                                         TyOrErr->getTypePtr());
+    } else {
+      return TyOrErr.takeError();
 
   llvm_unreachable("Invalid nested name specifier kind");
 }
+NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *From) {
+  llvm::Expected<NestedNameSpecifier *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
+}
 
 Expected<NestedNameSpecifierLoc>
 ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
-  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
-  return ToNNS;
-}
-NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector<NestedNameSpecifierLoc , 8> NestedNames;
   NestedNameSpecifierLoc NNS = FromNNS;
 
   // Push each of the nested-name-specifiers's onto a stack for
   // serialization in reverse order.
   while (NNS) {
     NestedNames.push_back(NNS);
     NNS = NNS.getPrefix();
   }
 
   NestedNameSpecifierLocBuilder Builder;
 
   while (!NestedNames.empty()) {
     NNS = NestedNames.pop_back_val();
-    NestedNameSpecifier *Spec = Import(NNS.getNestedNameSpecifier());
-    if (!Spec)
-      return NestedNameSpecifierLoc();
+    NestedNameSpecifier *Spec;
+    if (Error Err = importInto(Spec, NNS.getNestedNameSpecifier()))
+      return std::move(Err);
 
     NestedNameSpecifier::SpecifierKind Kind = Spec->getKind();
+
+    SourceLocation ToLocalBeginLoc, ToLocalEndLoc;
+    if (Kind != NestedNameSpecifier::Super) {
+      if (Error Err = importInto(ToLocalBeginLoc, NNS.getLocalBeginLoc()))
+        return std::move(Err);
+
+      if (Kind != NestedNameSpecifier::Global)
+        if (Error Err = importInto(ToLocalEndLoc, NNS.getLocalEndLoc()))
+          return std::move(Err);
+    }
+
     switch (Kind) {
     case NestedNameSpecifier::Identifier:
-      Builder.Extend(getToContext(),
-                     Spec->getAsIdentifier(),
-                     Import(NNS.getLocalBeginLoc()),
-                     Import(NNS.getLocalEndLoc()));
+      Builder.Extend(getToContext(), Spec->getAsIdentifier(), ToLocalBeginLoc,
+                     ToLocalEndLoc);
       break;
 
     case NestedNameSpecifier::Namespace:
-      Builder.Extend(getToContext(),
-                     Spec->getAsNamespace(),
-                     Import(NNS.getLocalBeginLoc()),
-                     Import(NNS.getLocalEndLoc()));
+      Builder.Extend(getToContext(), Spec->getAsNamespace(), ToLocalBeginLoc,
+                     ToLocalEndLoc);
       break;
 
     case NestedNameSpecifier::NamespaceAlias:
-      Builder.Extend(getToContext(),
-                     Spec->getAsNamespaceAlias(),
-                     Import(NNS.getLocalBeginLoc()),
-                     Import(NNS.getLocalEndLoc()));
+      Builder.Extend(getToContext(), Spec->getAsNamespaceAlias(),
+                     ToLocalBeginLoc, ToLocalEndLoc);
       break;
 
     case NestedNameSpecifier::TypeSpec:
     case NestedNameSpecifier::TypeSpecWithTemplate: {
       TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
             QualType(Spec->getAsType(), 0));
-      Builder.Extend(getToContext(),
-                     Import(NNS.getLocalBeginLoc()),
-                     TSI->getTypeLoc(),
-                     Import(NNS.getLocalEndLoc()));
+      Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
+                     ToLocalEndLoc);
       break;
     }
 
     case NestedNameSpecifier::Global:
-      Builder.MakeGlobal(getToContext(), Import(NNS.getLocalBeginLoc()));
+      Builder.MakeGlobal(getToContext(), ToLocalBeginLoc);
       break;
 
     case NestedNameSpecifier::Super: {
-      SourceRange ToRange = Import(NNS.getSourceRange());
-      Builder.MakeSuper(getToContext(),
-                        Spec->getAsRecordDecl(),
-                        ToRange.getBegin(),
-                        ToRange.getEnd());
+      auto ToSourceRangeOrErr = Import_New(NNS.getSourceRange());
+      if (!ToSourceRangeOrErr)
+        return ToSourceRangeOrErr.takeError();
+
+      Builder.MakeSuper(getToContext(), Spec->getAsRecordDecl(),
+                        ToSourceRangeOrErr->getBegin(),
+                        ToSourceRangeOrErr->getEnd());
     }
   }
   }
 
   return Builder.getWithLocInContext(getToContext());
 }
+NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc From) {
+  llvm::Expected<NestedNameSpecifierLoc> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 Expected<TemplateName> ASTImporter::Import_New(TemplateName From) {
-  TemplateName To = Import(From);
-  if (To.isNull() && !From.isNull())
-    return llvm::make_error<ImportError>();
-  return To;
-}
-TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
-    if (auto *ToTemplate =
-            cast_or_null<TemplateDecl>(Import(From.getAsTemplateDecl())))
-      return TemplateName(ToTemplate);
-
-    return {};
+    if (ExpectedDecl ToTemplateOrErr = Import_New(From.getAsTemplateDecl()))
+      return TemplateName(cast<TemplateDecl>(*ToTemplateOrErr));
+    else
+      return ToTemplateOrErr.takeError();
 
   case TemplateName::OverloadedTemplate: {
     OverloadedTemplateStorage *FromStorage = From.getAsOverloadedTemplate();
     UnresolvedSet<2> ToTemplates;
     for (auto *I : *FromStorage) {
-      if (auto *To = cast_or_null<NamedDecl>(Import(I)))
-        ToTemplates.addDecl(To);
+      if (auto ToOrErr = Import_New(I))
+        ToTemplates.addDecl(cast<NamedDecl>(*ToOrErr));
       else
-        return {};
+        return ToOrErr.takeError();
     }
     return ToContext.getOverloadedTemplateName(ToTemplates.begin(),
                                                ToTemplates.end());
   }
 
   case TemplateName::QualifiedTemplate: {
     QualifiedTemplateName *QTN = From.getAsQualifiedTemplateName();
-    NestedNameSpecifier *Qualifier = Import(QTN->getQualifier());
-    if (!Qualifier)
-      return {};
-
-    if (auto *ToTemplate =
-            cast_or_null<TemplateDecl>(Import(From.getAsTemplateDecl())))
-      return ToContext.getQualifiedTemplateName(Qualifier,
-                                                QTN->hasTemplateKeyword(),
-                                                ToTemplate);
-
-    return {};
+    auto QualifierOrErr = Import_New(QTN->getQualifier());
+    if (!QualifierOrErr)
+      return QualifierOrErr.takeError();
+
+    if (ExpectedDecl ToTemplateOrErr = Import_New(From.getAsTemplateDecl()))
+      return ToContext.getQualifiedTemplateName(
+          *QualifierOrErr, QTN->hasTemplateKeyword(),
+          cast<TemplateDecl>(*ToTemplateOrErr));
+    else
+      return ToTemplateOrErr.takeError();
   }
 
   case TemplateName::DependentTemplate: {
     DependentTemplateName *DTN = From.getAsDependentTemplateName();
-    NestedNameSpecifier *Qualifier = Import(DTN->getQualifier());
-    if (!Qualifier)
-      return {};
+    auto QualifierOrErr = Import_New(DTN->getQualifier());
+    if (!QualifierOrErr)
+      return QualifierOrErr.takeError();
 
     if (DTN->isIdentifier()) {
-      return ToContext.getDependentTemplateName(Qualifier,
+      return ToContext.getDependentTemplateName(*QualifierOrErr,
                                                 Import(DTN->getIdentifier()));
     }
 
-    return ToContext.getDependentTemplateName(Qualifier, DTN->getOperator());
+    return ToContext.getDependentTemplateName(*QualifierOrErr,
+                                              DTN->getOperator());
   }
 
   case TemplateName::SubstTemplateTemplateParm: {
-    SubstTemplateTemplateParmStorage *subst
-      = From.getAsSubstTemplateTemplateParm();
-    auto *param =
-        cast_or_null<TemplateTemplateParmDecl>(Import(subst->getParameter()));
-    if (!param)
-      return {};
-
-    TemplateName replacement = Import(subst->getReplacement());
-    if (replacement.isNull())
-      return {};
+    SubstTemplateTemplateParmStorage *Subst =
+        From.getAsSubstTemplateTemplateParm();
+    ExpectedDecl ParamOrErr = Import_New(Subst->getParameter());
+    if (!ParamOrErr)
+      return ParamOrErr.takeError();
+
+    auto ReplacementOrErr = Import_New(Subst->getReplacement());
+    if (!ReplacementOrErr)
+      return ReplacementOrErr.takeError();
 
-    return ToContext.getSubstTemplateTemplateParm(param, replacement);
+    return ToContext.getSubstTemplateTemplateParm(
+        cast<TemplateTemplateParmDecl>(*ParamOrErr), *ReplacementOrErr);
   }
 
   case TemplateName::SubstTemplateTemplateParmPack: {
     SubstTemplateTemplateParmPackStorage *SubstPack
       = From.getAsSubstTemplateTemplateParmPack();
-    auto *Param =
-        cast_or_null<TemplateTemplateParmDecl>(
-            Import(SubstPack->getParameterPack()));
-    if (!Param)
-      return {};
+    ExpectedDecl ParamOrErr = Import_New(SubstPack->getParameterPack());
+    if (!ParamOrErr)
+      return ParamOrErr.takeError();
 
     ASTNodeImporter Importer(*this);
-    Expected<TemplateArgument> ArgPack
-      = Importer.ImportTemplateArgument(SubstPack->getArgumentPack());
-    if (!ArgPack) {
-      llvm::consumeError(ArgPack.takeError());
-      return {};
-    }
+    auto ArgPackOrErr =
+        Importer.ImportTemplateArgument(SubstPack->getArgumentPack());
+    if (!ArgPackOrErr)
+      return ArgPackOrErr.takeError();
 
-    return ToContext.getSubstTemplateTemplateParmPack(Param, *ArgPack);
+    return ToContext.getSubstTemplateTemplateParmPack(
+        cast<TemplateTemplateParmDecl>(*ParamOrErr), *ArgPackOrErr);
   }
   }
 
   llvm_unreachable("Invalid template name kind");
 }
+TemplateName ASTImporter::Import(TemplateName From) {
+  llvm::Expected<TemplateName> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 Expected<SourceLocation> ASTImporter::Import_New(SourceLocation FromLoc) {
-  SourceLocation ToLoc = Import(FromLoc);
-  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
-    return llvm::make_error<ImportError>();
-  return ToLoc;
-}
-SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
-    return {};
+    return SourceLocation{};
 
   SourceManager &FromSM = FromContext.getSourceManager();
   bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair<FileID, unsigned> Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
-  if (ToFileID.isInvalid())
-    return {};
+  Expected<FileID> ToFileIDOrErr = Import_New(Decomposed.first, IsBuiltin);
+  if (!ToFileIDOrErr)
+    return ToFileIDOrErr.takeError();
   SourceManager &ToSM = ToContext.getSourceManager();
-  return ToSM.getComposedLoc(ToFileID, Decomposed.second);
+  return ToSM.getComposedLoc(*ToFileIDOrErr, Decomposed.second);
+}
+SourceLocation ASTImporter::Import(SourceLocation From) {
+  llvm::Expected<SourceLocation> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
 }
 
 Expected<SourceRange> ASTImporter::Import_New(SourceRange FromRange) {
-  SourceRange ToRange = Import(FromRange);
-  return ToRange;
+  SourceLocation ToBegin, ToEnd;
+  if (Error Err = importInto(ToBegin, FromRange.getBegin()))
+    return std::move(Err);
+  if (Error Err = importInto(ToEnd, FromRange.getEnd()))
+    return std::move(Err);
+
+  return SourceRange(ToBegin, ToEnd);
 }
-SourceRange ASTImporter::Import(SourceRange FromRange) {
-  return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
+SourceRange ASTImporter::Import(SourceRange From) {
+  llvm::Expected<SourceRange> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
 }
 
 Expected<FileID> ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
-  FileID ToID = Import(FromID, IsBuiltin);
-  if (ToID.isInvalid() && FromID.isValid())
-    return llvm::make_error<ImportError>();
-  return ToID;
-}
-FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap<FileID, FileID>::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
     return Pos->second;
 
   SourceManager &FromSM = FromContext.getSourceManager();
   SourceManager &ToSM = ToContext.getSourceManager();
   const SrcMgr::SLocEntry &FromSLoc = FromSM.getSLocEntry(FromID);
 
   // Map the FromID to the "to" source manager.
   FileID ToID;
   if (FromSLoc.isExpansion()) {
     const SrcMgr::ExpansionInfo &FromEx = FromSLoc.getExpansion();
-    SourceLocation ToSpLoc = Import(FromEx.getSpellingLoc());
-    SourceLocation ToExLocS = Import(FromEx.getExpansionLocStart());
+    ExpectedSLoc ToSpLoc = Import_New(FromEx.getSpellingLoc());
+    if (!ToSpLoc)
+      return ToSpLoc.takeError();
+    ExpectedSLoc ToExLocS = Import_New(FromEx.getExpansionLocStart());
+    if (!ToExLocS)
+      return ToExLocS.takeError();
     unsigned TokenLen = FromSM.getFileIDSize(FromID);
     SourceLocation MLoc;
     if (FromEx.isMacroArgExpansion()) {
-      MLoc = ToSM.createMacroArgExpansionLoc(ToSpLoc, ToExLocS, TokenLen);
+      MLoc = ToSM.createMacroArgExpansionLoc(*ToSpLoc, *ToExLocS, TokenLen);
     } else {
-      SourceLocation ToExLocE = Import(FromEx.getExpansionLocEnd());
-      MLoc = ToSM.createExpansionLoc(ToSpLoc, ToExLocS, ToExLocE, TokenLen,
+      if (ExpectedSLoc ToExLocE = Import_New(FromEx.getExpansionLocEnd()))
+        MLoc = ToSM.createExpansionLoc(*ToSpLoc, *ToExLocS, *ToExLocE, TokenLen,
                                        FromEx.isExpansionTokenRange());
+      else
+        return ToExLocE.takeError();
     }
     ToID = ToSM.getFileID(MLoc);
   } else {
     const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
 
     if (!IsBuiltin) {
       // Include location of this file.
-      SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
+      ExpectedSLoc ToIncludeLoc =
+          Import_New(FromSLoc.getFile().getIncludeLoc());
+      if (!ToIncludeLoc)
+        return ToIncludeLoc.takeError();
 
       if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
         // FIXME: We probably want to use getVirtualFile(), so we don't hit the
         // disk again
         // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
         // than mmap the files several times.
         const FileEntry *Entry =
             ToFileManager.getFile(Cache->OrigEntry->getName());
         // FIXME: The filename may be a virtual name that does probably not
         // point to a valid file and we get no Entry here. In this case try with
         // the memory buffer below.
         if (Entry)
-          ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+          ToID = ToSM.createFileID(Entry, *ToIncludeLoc,
                                    FromSLoc.getFile().getFileCharacteristic());
       }
     }
 
     if (ToID.isInvalid() || IsBuiltin) {
       // FIXME: We want to re-use the existing MemoryBuffer!
       bool Invalid = true;
       const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
           FromContext.getDiagnostics(), FromSM, SourceLocation{}, &Invalid);
       if (!FromBuf || Invalid)
-        return {};
+        // FIXME: Use a new error kind?
+        return llvm::make_error<ImportError>(ImportError::Unknown);
 
       std::unique_ptr<llvm::MemoryBuffer> ToBuf =
           llvm::MemoryBuffer::getMemBufferCopy(FromBuf->getBuffer(),
                                                FromBuf->getBufferIdentifier());
       ToID = ToSM.createFileID(std::move(ToBuf),
                                FromSLoc.getFile().getFileCharacteristic());
     }
   }
 
   assert(ToID.isValid() && "Unexpected invalid fileID was created.");
 
   ImportedFileIDs[FromID] = ToID;
   return ToID;
 }
+FileID ASTImporter::Import(FileID From, bool IsBuiltin) {
+  llvm::Expected<FileID> To = Import_New(From, IsBuiltin);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 Expected<CXXCtorInitializer *>
 ASTImporter::Import_New(CXXCtorInitializer *From) {
-  CXXCtorInitializer *To = Import(From);
-  if (!To && From)
-    return llvm::make_error<ImportError>();
-  return To;
-}
-CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
-  Expr *ToExpr = Import(From->getInit());
-  if (!ToExpr && From->getInit())
-    return nullptr;
+  ExpectedExpr ToExprOrErr = Import_New(From->getInit());
+  if (!ToExprOrErr)
+    return ToExprOrErr.takeError();
+
+  auto LParenLocOrErr = Import_New(From->getLParenLoc());
+  if (!LParenLocOrErr)
+    return LParenLocOrErr.takeError();
+
+  auto RParenLocOrErr = Import_New(From->getRParenLoc());
+  if (!RParenLocOrErr)
+    return RParenLocOrErr.takeError();
 
   if (From->isBaseInitializer()) {
-    TypeSourceInfo *ToTInfo = Import(From->getTypeSourceInfo());
-    if (!ToTInfo && From->getTypeSourceInfo())
-      return nullptr;
+    auto ToTInfoOrErr = Import_New(From->getTypeSourceInfo());
+    if (!ToTInfoOrErr)
+      return ToTInfoOrErr.takeError();
+
+    SourceLocation EllipsisLoc;
+    if (From->isPackExpansion())
+      if (Error Err = importInto(EllipsisLoc, From->getEllipsisLoc()))
+        return std::move(Err);
 
     return new (ToContext) CXXCtorInitializer(
-        ToContext, ToTInfo, From->isBaseVirtual(), Import(From->getLParenLoc()),
-        ToExpr, Import(From->getRParenLoc()),
-        From->isPackExpansion() ? Import(From->getEllipsisLoc())
-                                : SourceLocation());
+        ToContext, *ToTInfoOrErr, From->isBaseVirtual(), *LParenLocOrErr,
+        *ToExprOrErr, *RParenLocOrErr, EllipsisLoc);
   } else if (From->isMemberInitializer()) {
-    auto *ToField = cast_or_null<FieldDecl>(Import(From->getMember()));
-    if (!ToField && From->getMember())
-      return nullptr;
+    ExpectedDecl ToFieldOrErr = Import_New(From->getMember());
+    if (!ToFieldOrErr)
+      return ToFieldOrErr.takeError();
+
+    auto MemberLocOrErr = Import_New(From->getMemberLocation());
+    if (!MemberLocOrErr)
+      return MemberLocOrErr.takeError();
 
     return new (ToContext) CXXCtorInitializer(
-        ToContext, ToField, Import(From->getMemberLocation()),
-        Import(From->getLParenLoc()), ToExpr, Import(From->getRParenLoc()));
+        ToContext, cast_or_null<FieldDecl>(*ToFieldOrErr), *MemberLocOrErr,
+        *LParenLocOrErr, *ToExprOrErr, *RParenLocOrErr);
   } else if (From->isIndirectMemberInitializer()) {
-    auto *ToIField = cast_or_null<IndirectFieldDecl>(
-        Import(From->getIndirectMember()));
-    if (!ToIField && From->getIndirectMember())
-      return nullptr;
+    ExpectedDecl ToIFieldOrErr = Import_New(From->getIndirectMember());
+    if (!ToIFieldOrErr)
+      return ToIFieldOrErr.takeError();
+
+    auto MemberLocOrErr = Import_New(From->getMemberLocation());
+    if (!MemberLocOrErr)
+      return MemberLocOrErr.takeError();
 
     return new (ToContext) CXXCtorInitializer(
-        ToContext, ToIField, Import(From->getMemberLocation()),
-        Import(From->getLParenLoc()), ToExpr, Import(From->getRParenLoc()));
+        ToContext, cast_or_null<IndirectFieldDecl>(*ToIFieldOrErr),
+        *MemberLocOrErr, *LParenLocOrErr, *ToExprOrErr, *RParenLocOrErr);
   } else if (From->isDelegatingInitializer()) {
-    TypeSourceInfo *ToTInfo = Import(From->getTypeSourceInfo());
-    if (!ToTInfo && From->getTypeSourceInfo())
-      return nullptr;
+    auto ToTInfoOrErr = Import_New(From->getTypeSourceInfo());
+    if (!ToTInfoOrErr)
+      return ToTInfoOrErr.takeError();
 
     return new (ToContext)
-        CXXCtorInitializer(ToContext, ToTInfo, Import(From->getLParenLoc()),
-                           ToExpr, Import(From->getRParenLoc()));
+        CXXCtorInitializer(ToContext, *ToTInfoOrErr, *LParenLocOrErr,
+                           *ToExprOrErr, *RParenLocOrErr);
   } else {
-    return nullptr;
+    // FIXME: assert?
+    return make_error<ImportError>();
   }
 }
+CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
+  llvm::Expected<CXXCtorInitializer *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
+}
 
 Expected<CXXBaseSpecifier *>
-ASTImporter::Import_New(const CXXBaseSpecifier *From) {
-  CXXBaseSpecifier *To = Import(From);
-  if (!To && From)
-    return llvm::make_error<ImportError>();
-  return To;
-}
-CXXBaseSpecifier *ASTImporter::Import(const CXXBaseSpecifier *BaseSpec) {
+ASTImporter::Import_New(const CXXBaseSpecifier *BaseSpec) {
   auto Pos = ImportedCXXBaseSpecifiers.find(BaseSpec);
   if (Pos != ImportedCXXBaseSpecifiers.end())
     return Pos->second;
 
+  Expected<SourceRange> ToSourceRange = Import_New(BaseSpec->getSourceRange());
+  if (!ToSourceRange)
+    return ToSourceRange.takeError();
+  Expected<TypeSourceInfo *> ToTSI = Import_New(BaseSpec->getTypeSourceInfo());
+  if (!ToTSI)
+    return ToTSI.takeError();
+  ExpectedSLoc ToEllipsisLoc = Import_New(BaseSpec->getEllipsisLoc());
+  if (!ToEllipsisLoc)
+    return ToEllipsisLoc.takeError();
   CXXBaseSpecifier *Imported = new (ToContext) CXXBaseSpecifier(
-        Import(BaseSpec->getSourceRange()),
-        BaseSpec->isVirtual(), BaseSpec->isBaseOfClass(),
-        BaseSpec->getAccessSpecifierAsWritten(),
-        Import(BaseSpec->getTypeSourceInfo()),
-        Import(BaseSpec->getEllipsisLoc()));
+      *ToSourceRange, BaseSpec->isVirtual(), BaseSpec->isBaseOfClass(),
+      BaseSpec->getAccessSpecifierAsWritten(), *ToTSI, *ToEllipsisLoc);
   ImportedCXXBaseSpecifiers[BaseSpec] = Imported;
   return Imported;
 }
+CXXBaseSpecifier *ASTImporter::Import(const CXXBaseSpecifier *From) {
+  llvm::Expected<CXXBaseSpecifier *> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return nullptr;
+}
 
 Error ASTImporter::ImportDefinition_New(Decl *From) {
   Decl *To = Import(From);
   if (!To)
     return llvm::make_error<ImportError>();
 
   if (auto *FromDC = cast<DeclContext>(From)) {
     ASTNodeImporter Importer(*this);

 }
 
 void ASTImporter::ImportDefinition(Decl *From) {
   Error Err = ImportDefinition_New(From);
   llvm::consumeError(std::move(Err));
 }
 
 Expected<DeclarationName> ASTImporter::Import_New(DeclarationName FromName) {
-  DeclarationName ToName = Import(FromName);
-  if (!ToName && FromName)
-    return llvm::make_error<ImportError>();
-  return ToName;
-}
-DeclarationName ASTImporter::Import(DeclarationName FromName) {
   if (!FromName)
-    return {};
+    return DeclarationName{};
 
   switch (FromName.getNameKind()) {
   case DeclarationName::Identifier:
-    return Import(FromName.getAsIdentifierInfo());
+    return DeclarationName(Import(FromName.getAsIdentifierInfo()));
 
   case DeclarationName::ObjCZeroArgSelector:
   case DeclarationName::ObjCOneArgSelector:
   case DeclarationName::ObjCMultiArgSelector:
-    return Import(FromName.getObjCSelector());
+    if (auto ToSelOrErr = Import_New(FromName.getObjCSelector()))
+      return DeclarationName(*ToSelOrErr);
+    else
+      return ToSelOrErr.takeError();
 
   case DeclarationName::CXXConstructorName: {
-    QualType T = Import(FromName.getCXXNameType());
-    if (T.isNull())
-      return {};
-
+    if (auto ToTyOrErr = Import_New(FromName.getCXXNameType()))
       return ToContext.DeclarationNames.getCXXConstructorName(
-                                               ToContext.getCanonicalType(T));
+          ToContext.getCanonicalType(*ToTyOrErr));
+    else
+      return ToTyOrErr.takeError();
   }
 
   case DeclarationName::CXXDestructorName: {
-    QualType T = Import(FromName.getCXXNameType());
-    if (T.isNull())
-      return {};
-
+    if (auto ToTyOrErr = Import_New(FromName.getCXXNameType()))
       return ToContext.DeclarationNames.getCXXDestructorName(
-                                               ToContext.getCanonicalType(T));
+          ToContext.getCanonicalType(*ToTyOrErr));
+    else
+      return ToTyOrErr.takeError();
   }
 
   case DeclarationName::CXXDeductionGuideName: {
-    auto *Template = cast_or_null<TemplateDecl>(
-        Import(FromName.getCXXDeductionGuideTemplate()));
-    if (!Template)
-      return {};
-    return ToContext.DeclarationNames.getCXXDeductionGuideName(Template);
+    if (auto ToTemplateOrErr =
+            Import_New(FromName.getCXXDeductionGuideTemplate()))
+      return ToContext.DeclarationNames.getCXXDeductionGuideName(
+          cast<TemplateDecl>(*ToTemplateOrErr));
+    else
+      return ToTemplateOrErr.takeError();
   }
 
   case DeclarationName::CXXConversionFunctionName: {
-    QualType T = Import(FromName.getCXXNameType());
-    if (T.isNull())
-      return {};
-
+    if (auto ToTyOrErr = Import_New(FromName.getCXXNameType()))
       return ToContext.DeclarationNames.getCXXConversionFunctionName(
-                                               ToContext.getCanonicalType(T));
+          ToContext.getCanonicalType(*ToTyOrErr));
+    else
+      return ToTyOrErr.takeError();
   }
 
   case DeclarationName::CXXOperatorName:
     return ToContext.DeclarationNames.getCXXOperatorName(
                                           FromName.getCXXOverloadedOperator());
 
   case DeclarationName::CXXLiteralOperatorName:
     return ToContext.DeclarationNames.getCXXLiteralOperatorName(
-                                   Import(FromName.getCXXLiteralIdentifier()));
 
   case DeclarationName::CXXUsingDirective:
     // FIXME: STATICS!
     return DeclarationName::getUsingDirectiveName();
   }
 
   llvm_unreachable("Invalid DeclarationName Kind!");
 }
+DeclarationName ASTImporter::Import(DeclarationName From) {
+  llvm::Expected<DeclarationName> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 IdentifierInfo *ASTImporter::Import(const IdentifierInfo *FromId) {
   if (!FromId)
     return nullptr;
 
   IdentifierInfo *ToId = &ToContext.Idents.get(FromId->getName());
 
   if (!ToId->getBuiltinID() && FromId->getBuiltinID())
     ToId->setBuiltinID(FromId->getBuiltinID());
 
   return ToId;
 }
 
 Expected<Selector> ASTImporter::Import_New(Selector FromSel) {
-  Selector ToSel = Import(FromSel);
-  if (ToSel.isNull() && !FromSel.isNull())
-    return llvm::make_error<ImportError>();
-  return ToSel;
-}
-Selector ASTImporter::Import(Selector FromSel) {
   if (FromSel.isNull())
-    return {};
+    return Selector{};
 
   SmallVector<IdentifierInfo *, 4> Idents;
   Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(0)));
   for (unsigned I = 1, N = FromSel.getNumArgs(); I < N; ++I)
     Idents.push_back(Import(FromSel.getIdentifierInfoForSlot(I)));
   return ToContext.Selectors.getSelector(FromSel.getNumArgs(), Idents.data());
 }
 
 DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
                                                 DeclContext *DC,
                                                 unsigned IDNS,
                                                 NamedDecl **Decls,
                                                 unsigned NumDecls) {
   return Name;
 }
+Selector ASTImporter::Import(Selector From) {
+  llvm::Expected<Selector> To = Import_New(From);
+  if (To)
+    return *To;
+  else
+    llvm::consumeError(To.takeError());
+  return {};
+}
 
 DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) {
   if (LastDiagFromFrom)
     ToContext.getDiagnostics().notePriorDiagnosticFrom(
       FromContext.getDiagnostics());
   LastDiagFromFrom = false;
   return ToContext.getDiagnostics().Report(Loc, DiagID);
 }