[ASTImporter] Added Import functions for transition to new API.

[balazske]

Summary

These Import_New functions should be used in the ASTImporter,
and the old Import functions should not be used. Later the
Import_New should be renamed to Import again and the old Import
functions must be removed. But this can happen only after LLDB
was updated to use the new Import interface.

This commit is only about introducing the new Import_New
functions. These are not implemented now, only calling the old
Import ones.

[balazske]

• Corrected Import_New(const Attr *)

[balazske]

• Added missing Import_New with Selector and DeclarationName.

[shafik]

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

[martong]

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.
Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

Hi @shafik ,

I completely understand your concern. We modify ASTImporter in order to make cross translation unit (CTU) analysis working on C++ projects. During these modifications we carefully try to keep LLDB functionality intact.

This patch is one of the last patches of a refactor in ASTImporter about using Error and Expected<T> as return types. We need this in CTU analysis because

• we want to distinguish between different kind of errors
• internally in ASTImporter we'd like to enforce the checking whether there has been any error during the import of any subexpressions

After this patch our next patch would rename these Import_New functions to Import and the old Import function implementations returning with a raw pointer would be deleted. This indeed would effect LLDB, thus we are going to create an LLDB patch too. We already have that LLDB change in our fork (https://github.com/Ericsson/lldb/commit/7085d20) and soon we will put that in Phabricator too.

Now, my concern is that waiting for the approve from @rsmith could take several months since he is usually very overwhelmed. Unfortunately, we have several other changes which depend on this change, so this is a blocker for us. Also, I think that @a_sidorin has the greatest experience in ASTImporter code. Would it be okay for you to accept this patch once Alexei approved it?

[a_sidorin]

Hi Balazs,
The changes look mostly fine to me. I think they can be accepted after some minor stylish fixes.

Hi Shafik,

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

As I understand, these changes are done exactly in order to perform a seamless transition into a new error-checking API. This was discussed in both cfe-dev and lldb-dev: https://lists.llvm.org/pipermail/cfe-dev/2018-April/057771.html.
It looks like the patches for the final transition will be ready soon. @balazske, @martong Am I correct?

Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

Review from @rsmith is extremely appreciated; however, me and @spyffe have reviewed most ASTImporter-related patches in the last two years. ASTImporter itself doesn't have any use in clang frontend. It was an LLDB bridge only until CSA started to use it too.

include/clang/AST/ASTImporter.h

244

Please split this line and the changed line below.

lib/AST/ASTImporter.cpp

7882

Looks like this line needs to be split too.

8254

... and this one too.

[balazske]

• Split long lines.

[balazske]

• Split long lines (ASTImporter.cpp).

[a_sidorin]

LGTM, thanks!

[balazske]

• Corrected Import_New(const Attr *)
• Added missing Import_New with Selector and DeclarationName.
• Split long lines.
• Split long lines (ASTImporter.cpp).

Rebase to newest master.

[aaron.ballman]

In D53751#1301551, @shafik wrote:

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

When is the renaming and removal of the old API expected take place? Days? Weeks? Months?

[balazske]

It is really "ugly" thing to have a Import_New and a Import for the same functionality. Without these a single large patch would have been needed for clang and another smaller one for LLDB that must be committed "at the same time". Now 1 dependent patch is under review. Then 1 patch is to do for clang and one for lldb (these can contain the removal of Import_New too). It mainly depends on the speed of the reviews how fast the update happens.

[martong]

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

[shafik]

In D53751#1311037, @martong wrote:

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

I believe an alternate approach would have been to change one function at a time and do this change across. These would have been smaller changes and easier to review. This would have prevented the need for an intermediate name which causes churn in the commit history.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

[aaron.ballman]

In D53751#1311037, @martong wrote:

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

Thank you for the explanation -- I guess I would have preferred to go with the suggestion from @shafik to have done this one API at a time, rather than as an entire set of APIs. However, given that the code is already in, I don't think it would be worth the churn to revert and go that route.

include/clang/AST/ASTImporter.h

LLVM_NODISCARD llvm::Error importInto(ImportT &To, const ImportT &From) {
       // FIXME: this should be the final code
       //auto ToOrErr = Import(From);
       //if (ToOrErr)
       //  To = *ToOrErr;
       //return ToOrErr.takeError();
     }
 
     /// Import the given type from the "from" context into the "to"
-    /// context.
+    /// context. A null type is imported as a null type (no error).
     ///
-    /// \returns the equivalent type in the "to" context, or a NULL type if
-    /// an error occurred.
+    /// \returns The equivalent type in the "to" context, or the import error.
+    llvm::Expected<QualType> Import_New(QualType FromT);
+    // FIXME: Remove this version.
     QualType Import(QualType FromT);
 
     /// Import the given type source information from the
     /// "from" context into the "to" context.
     ///
-    /// \returns the equivalent type source information in the "to"
-    /// context, or NULL if an error occurred.
+    /// \returns The equivalent type source information in the "to"
+    /// context, or the import error.
+    llvm::Expected<TypeSourceInfo *> Import_New(TypeSourceInfo *FromTSI);
+    // FIXME: Remove this version.
     TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
 
     /// Import the given attribute from the "from" context into the
     /// "to" context.
     ///
-    /// \returns the equivalent attribute in the "to" context.
+    /// \returns The equivalent attribute in the "to" context, or the import
+    /// 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 a NULL type
-    /// if an error occurred.
+    /// \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
     /// NULL.
     Decl *GetAlreadyImportedOrNull(Decl *FromD);
 
     /// Import the given declaration context from the "from"
     /// AST context into the "to" AST context.
     ///
     /// \returns the equivalent declaration context in the "to"
     /// context, or error value.
     llvm::Expected<DeclContext *> ImportContext(DeclContext *FromDC);
 
     /// Import the given expression from the "from" context into the
     /// "to" context.
     ///
-    /// \returns the equivalent expression in the "to" context, or NULL if
-    /// an error occurred.
+    /// \returns The equivalent expression in the "to" context, or the import
+    /// error.
+    llvm::Expected<Expr *> Import_New(Expr *FromE);
+    // FIXME: Remove this version.
     Expr *Import(Expr *FromE);
 
     /// Import the given statement from the "from" context into the
     /// "to" context.
     ///
-    /// \returns the equivalent statement in the "to" context, or NULL if
-    /// an error occurred.
+    /// \returns The equivalent statement in the "to" context, or the import
+    /// error.
+    llvm::Expected<Stmt *> Import_New(Stmt *FromS);
+    // FIXME: Remove this version.
     Stmt *Import(Stmt *FromS);
 
     /// Import the given nested-name-specifier from the "from"
     /// context into the "to" context.
     ///
-    /// \returns the equivalent nested-name-specifier in the "to"
-    /// context, or NULL if an error occurred.
+    /// \returns The equivalent nested-name-specifier in the "to"
+    /// context, or the import error.
+    llvm::Expected<NestedNameSpecifier *>
+    Import_New(NestedNameSpecifier *FromNNS);
+    // FIXME: Remove this version.
     NestedNameSpecifier *Import(NestedNameSpecifier *FromNNS);
 
-    /// Import the given nested-name-specifier from the "from"
+    /// Import the given nested-name-specifier-loc from the "from"
     /// context into the "to" context.
     ///
-    /// \returns the equivalent nested-name-specifier in the "to"
-    /// context.
+    /// \returns The equivalent nested-name-specifier-loc in the "to"
+    /// context, or the import error.
+    llvm::Expected<NestedNameSpecifierLoc>
+    Import_New(NestedNameSpecifierLoc FromNNS);
+    // FIXME: Remove this version.
     NestedNameSpecifierLoc Import(NestedNameSpecifierLoc FromNNS);
 
-    /// Import the goven template name from the "from" context into the
-    /// "to" context.
+    /// Import the given template name from the "from" context into the
+    /// "to" context, or the import error.
+    llvm::Expected<TemplateName> Import_New(TemplateName From);
+    // FIXME: Remove this version.
     TemplateName Import(TemplateName From);
 
     /// Import the given source location from the "from" context into
     /// the "to" context.
     ///
-    /// \returns the equivalent source location in the "to" context, or an
-    /// invalid source location if an error occurred.
+    /// \returns The equivalent source location in the "to" context, or the
+    /// import error.
+    llvm::Expected<SourceLocation> Import_New(SourceLocation FromLoc);
+    // FIXME: Remove this version.
     SourceLocation Import(SourceLocation FromLoc);
 
     /// Import the given source range from the "from" context into
     /// the "to" context.
     ///
-    /// \returns the equivalent source range in the "to" context, or an
-    /// invalid source location if an error occurred.
+    /// \returns The equivalent source range in the "to" context, or the import
+    /// error.
+    llvm::Expected<SourceRange> Import_New(SourceRange FromRange);
+    // FIXME: Remove this version.
     SourceRange Import(SourceRange FromRange);
 
     /// Import the given declaration name from the "from"
     /// context into the "to" context.
     ///
-    /// \returns the equivalent declaration name in the "to" context,
-    /// or an empty declaration name if an error occurred.
+    /// \returns The equivalent declaration name in the "to" context, or the
+    /// import error.
+    llvm::Expected<DeclarationName> Import_New(DeclarationName FromName);
+    // FIXME: Remove this version.
     DeclarationName Import(DeclarationName FromName);
 
     /// Import the given identifier from the "from" context
     /// into the "to" context.
     ///
-    /// \returns the equivalent identifier in the "to" context. Note: It
+    /// \returns The equivalent identifier in the "to" context. Note: It
     /// returns nullptr only if the FromId was nullptr.
     IdentifierInfo *Import(const IdentifierInfo *FromId);
 
     /// Import the given Objective-C selector from the "from"
     /// context into the "to" context.
     ///
-    /// \returns the equivalent selector in the "to" context.
+    /// \returns The equivalent selector in the "to" context, or the import
+    /// error.
+    llvm::Expected<Selector> Import_New(Selector FromSel);
+    // FIXME: Remove this version.
     Selector Import(Selector FromSel);
 
     /// Import the given file ID from the "from" context into the
     /// "to" context.
     ///
-    /// \returns the equivalent file ID in the source manager of the "to"
-    /// context.
+    /// \returns The equivalent file ID in the source manager of the "to"
+    /// context, or the import error.
+    llvm::Expected<FileID> Import_New(FileID);
+    // FIXME: Remove this version.
     FileID Import(FileID);
 
     /// Import the given C++ constructor initializer from the "from"
     /// context into the "to" context.
     ///
-    /// \returns the equivalent initializer in the "to" context.
+    /// \returns The equivalent initializer in the "to" context, or the import
+    /// error.
+    llvm::Expected<CXXCtorInitializer *>
+    Import_New(CXXCtorInitializer *FromInit);
+    // FIXME: Remove this version.
     CXXCtorInitializer *Import(CXXCtorInitializer *FromInit);
 
     /// Import the given CXXBaseSpecifier from the "from" context into
     /// the "to" context.
     ///
-    /// \returns the equivalent CXXBaseSpecifier in the source manager of the
-    /// "to" context.
+    /// \returns The equivalent CXXBaseSpecifier in the source manager of the
+    /// "to" context, or the import error.
+    llvm::Expected<CXXBaseSpecifier *>
+    Import_New(const CXXBaseSpecifier *FromSpec);
+    // FIXME: Remove this version.
     CXXBaseSpecifier *Import(const CXXBaseSpecifier *FromSpec);
 
     /// Import the definition of the given declaration, including all of
     /// the declarations it contains.
     LLVM_NODISCARD llvm::Error ImportDefinition_New(Decl *From);
 
     // FIXME: Compatibility function.
     // Usages of this should be changed to ImportDefinition_New.

lib/AST/ASTImporter.cpp

: ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),
       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;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
     return {};
 
   const Type *FromTy = FromT.getTypePtr();
 
   // Check whether we've already imported this type.
   llvm::DenseMap<const Type *, const Type *>::iterator Pos
QualType ASTImporter::Import(QualType FromT) {
   }
 
   // Record the imported type.
   ImportedTypes[FromTy] = (*ToTOrErr).getTypePtr();
 
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+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;
 
   return ToContext.getTrivialTypeSourceInfo(
       T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+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()));
   return ToAttr;
 }
 
 Decl *ASTImporter::GetAlreadyImportedOrNull(Decl *FromD) {
   llvm::DenseMap<Decl *, Decl *>::iterator Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end()) {
     Decl *ToD = Pos->second;
     // FIXME: move this call to ImportDeclParts().
     if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD))
       llvm::consumeError(std::move(Err));
     return ToD;
   } else {
     return nullptr;
   }
 }
 
+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 (auto *ToRecord = dyn_cast<RecordDecl>(ToDC)) {
     } else {
       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;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
     return nullptr;
 
   return cast_or_null<Expr>(Import(cast<Stmt>(FromE)));
 }
 
+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.
   llvm::DenseMap<Stmt *, Stmt *>::iterator Pos = ImportedStmts.find(FromS);
   if (Pos != ImportedStmts.end())
     return Pos->second;
ToE->setContainsUnexpandedParameterPack(
         FromE->containsUnexpandedParameterPack());
   }
 
   // Record the imported declaration.
   ImportedStmts[FromS] = *ToSOrErr;
   return *ToSOrErr;
 }
 
+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());
 
   switch (FromNNS->getKind()) {
   case NestedNameSpecifier::Identifier:
case NestedNameSpecifier::TypeSpecWithTemplate: {
       }
     }
       return nullptr;
   }
 
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+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) {
case NestedNameSpecifier::Super: {
                         ToRange.getEnd());
     }
   }
   }
 
   return Builder.getWithLocInContext(getToContext());
 }
 
+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 {};
case TemplateName::SubstTemplateTemplateParmPack: {
 
     return ToContext.getSubstTemplateTemplateParmPack(Param, *ArgPack);
   }
   }
 
   llvm_unreachable("Invalid template name kind");
 }
 
+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 {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
 
   std::pair<FileID, unsigned> Decomposed = FromSM.getDecomposedLoc(FromLoc);
   FileID ToFileID = Import(Decomposed.first);
   if (ToFileID.isInvalid())
     return {};
   SourceManager &ToSM = ToContext.getSourceManager();
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected<SourceRange> ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected<FileID> ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+    return llvm::make_error<ImportError>();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   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);
if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
                                FromSLoc.getFile().getFileCharacteristic());
     }
   }
 
   ImportedFileIDs[FromID] = ToID;
   return ToID;
 }
 
+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;
 
   if (From->isBaseInitializer()) {
     TypeSourceInfo *ToTInfo = Import(From->getTypeSourceInfo());
     if (!ToTInfo && From->getTypeSourceInfo())
if (From->isBaseInitializer()) {
     return new (ToContext)
         CXXCtorInitializer(ToContext, ToTInfo, Import(From->getLParenLoc()),
                            ToExpr, Import(From->getRParenLoc()));
   } else {
     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) {
   auto Pos = ImportedCXXBaseSpecifiers.find(BaseSpec);
   if (Pos != ImportedCXXBaseSpecifiers.end())
     return Pos->second;
 
   CXXBaseSpecifier *Imported = new (ToContext) CXXBaseSpecifier(
         Import(BaseSpec->getSourceRange()),
         BaseSpec->isVirtual(), BaseSpec->isBaseOfClass(),
Error ASTImporter::ImportDefinition_New(Decl *From) {
   return Error::success();
 }
 
 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 {};
 
   switch (FromName.getNameKind()) {
   case DeclarationName::Identifier:
     return Import(FromName.getAsIdentifierInfo());
 
IdentifierInfo *ASTImporter::Import(const IdentifierInfo *FromId) {
   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 {};
 
   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)));