[NFC] Cleanup the overload of ASTImporter::import()

[steakhal]

Summary

This patch aims to address the comment of a previous review:
https://reviews.llvm.org/D109237#inline-1040678

The original problem was the following:

`T` is substituted by `clang::Type`

Expected<T *> import(T *From) {
  auto ToOrErr = Importer.Import(From);
  //             ^^^^^^^^^^^^^^^^^^^^^
  if (!ToOrErr)
    return ToOrErr.takeError();
  return cast_or_null<T>(*ToOrErr);
  //     ^^^^^^^^^^^^^^^^^^^^^^^^^
}

Importer.Import() operates on const Type *, thus returns const Type *.
Later, at the return statement, we will try to construct an Expected<Type*>
from a const Type *, which failed with a miserable error message.

In all other cases importer.Import() results in a non-const version,
so everything works out just fine, but for clang::types, we should
really return a const version.

So, in case of T is a subclass of clang::Type, it will return a
Exprected<const T*> instead.

[steakhal]

A minor style fixup.

[martong]

Awesome! Thanks!

clang/lib/AST/ASTImporter.cpp

1168

Probably we should have a shorter type alias for Expected<const Type*> to be aligned with the rest of the existing type aliases we already have.
I am thinking about something like this:

using ExpectedQualType = llvm::Expected<QualType>;
using ExpectedType = llvm::Expected<const Type*>;

[steakhal]

clang/lib/AST/ASTImporter.cpp

1168

I slightly disagree. Since this import aims to be used with the most specific derived type, we shouldn't expect too many uses of the raw Expected<const Type*> type to justify a type alias IMO.

For consistency though, I can see your point, but I'd like to hear what do the others think about this.
@shafik

[steakhal]

ping

[balazske]

clang/lib/AST/ASTImporter.cpp

1168

There is already ExpectedType, Expected<const Type *> could be ExpectedTypePtr.

[shafik]

clang/lib/AST/ASTImporter.cpp

1168

I feel that consistency aids readability and maintainability since it reduces surprises. If you see it done one way everywhere else then you wonder why is it different here or perhaps it creates confusion as to which way is the right way to do it for users contributing a patch.

[steakhal]

add type alias for ExpectedTypePtr

clang/lib/AST/ASTImporter.cpp

1168

Okay, fixed this.

[martong]

Great, thanks!

clang/lib/AST/ASTImporter.cpp

LLVM_NODISCARD Error importInto(ImportT *&To, ImportT *From) {
       if (ToOrErr)
         To = cast_or_null<ImportT>(*ToOrErr);
       return ToOrErr.takeError();
     }
 
     // Call the import function of ASTImporter for a baseclass of type `T` and
     // cast the return value to `T`.
     template <typename T>
-    Expected<T *> import(T *From) {
+    auto import(T *From)
+        -> std::conditional_t<std::is_base_of<Type, T>::value,
+                              Expected<const T *>, Expected<T *>> {
       auto ToOrErr = Importer.Import(From);
       if (!ToOrErr)
         return ToOrErr.takeError();
       return cast_or_null<T>(*ToOrErr);
     }
 
     template <typename T>
-    Expected<T *> import(const T *From) {
+    auto import(const T *From) {
       return import(const_cast<T *>(From));
     }
 
     // Call the import function of ASTImporter for type `T`.
     template <typename T>
     Expected<T> import(const T &From) {
       return Importer.Import(From);
     }

 
 ExpectedType
 ASTNodeImporter::VisitMemberPointerType(const MemberPointerType *T) {
   // FIXME: Check for C++ support in "to" context.
   ExpectedType ToPointeeTypeOrErr = import(T->getPointeeType());
   if (!ToPointeeTypeOrErr)
     return ToPointeeTypeOrErr.takeError();
 
-  ExpectedType ClassTypeOrErr = import(QualType(T->getClass(), 0));
+  Expected<const Type *> ClassTypeOrErr = import(T->getClass());
   if (!ClassTypeOrErr)
     return ClassTypeOrErr.takeError();
 
-  return Importer.getToContext().getMemberPointerType(
-      *ToPointeeTypeOrErr, (*ClassTypeOrErr).getTypePtr());
+  return Importer.getToContext().getMemberPointerType(*ToPointeeTypeOrErr,
+                                                      *ClassTypeOrErr);
 }
 
 ExpectedType
 ASTNodeImporter::VisitConstantArrayType(const ConstantArrayType *T) {
   Error Err = Error::success();
   auto ToElementType = importChecked(Err, T->getElementType());
   auto ToSizeExpr = importChecked(Err, T->getSizeExpr());
   if (Err)
if (!ToDeclOrErr)
     return ToDeclOrErr.takeError();
 
   return Importer.getToContext().getTemplateTypeParmType(
       T->getDepth(), T->getIndex(), T->isParameterPack(), *ToDeclOrErr);
 }
 
 ExpectedType ASTNodeImporter::VisitSubstTemplateTypeParmType(
     const SubstTemplateTypeParmType *T) {
-  ExpectedType ReplacedOrErr = import(QualType(T->getReplacedParameter(), 0));
+  Expected<const TemplateTypeParmType *> ReplacedOrErr =
+      import(T->getReplacedParameter());
   if (!ReplacedOrErr)
     return ReplacedOrErr.takeError();
-  const TemplateTypeParmType *Replaced =
-      cast<TemplateTypeParmType>((*ReplacedOrErr).getTypePtr());
 
   ExpectedType ToReplacementTypeOrErr = import(T->getReplacementType());
   if (!ToReplacementTypeOrErr)
     return ToReplacementTypeOrErr.takeError();
 
   return Importer.getToContext().getSubstTemplateTypeParmType(
-        Replaced, (*ToReplacementTypeOrErr).getCanonicalType());
+      *ReplacedOrErr, (*ToReplacementTypeOrErr).getCanonicalType());
 }
 
 ExpectedType ASTNodeImporter::VisitSubstTemplateTypeParmPackType(
     const SubstTemplateTypeParmPackType *T) {
-  ExpectedType ReplacedOrErr = import(QualType(T->getReplacedParameter(), 0));
+  Expected<const TemplateTypeParmType *> ReplacedOrErr =
+      import(T->getReplacedParameter());
   if (!ReplacedOrErr)
     return ReplacedOrErr.takeError();
-  const TemplateTypeParmType *Replaced =
-      cast<TemplateTypeParmType>(ReplacedOrErr->getTypePtr());
 
   Expected<TemplateArgument> ToArgumentPack = import(T->getArgumentPack());
   if (!ToArgumentPack)
     return ToArgumentPack.takeError();
 
   return Importer.getToContext().getSubstTemplateTypeParmPackType(
-      Replaced, *ToArgumentPack);
+      *ReplacedOrErr, *ToArgumentPack);
 }
 
 ExpectedType ASTNodeImporter::VisitTemplateSpecializationType(
                                        const TemplateSpecializationType *T) {
   auto ToTemplateOrErr = import(T->getTemplateName());
   if (!ToTemplateOrErr)
     return ToTemplateOrErr.takeError();
 
   SmallVector<TemplateArgument, 2> ToTemplateArgs;
   if (Error Err = ImportTemplateArguments(
       T->getArgs(), T->getNumArgs(), ToTemplateArgs))
     return std::move(Err);
 
   QualType ToCanonType;
-  if (!QualType(T, 0).isCanonical()) {
+  if (!T->isCanonicalUnqualified()) {
     QualType FromCanonType
       = Importer.getFromContext().getCanonicalType(QualType(T, 0));
     if (ExpectedType TyOrErr = import(FromCanonType))
       ToCanonType = *TyOrErr;
     else
       return TyOrErr.takeError();
   }
   return Importer.getToContext().getTemplateSpecializationType(*ToTemplateOrErr,
if (!FromT)
     return FromT;
 
   // Check whether we've already imported this type.
   llvm::DenseMap<const Type *, const Type *>::iterator Pos =
       ImportedTypes.find(FromT);
   if (Pos != ImportedTypes.end())
     return Pos->second;
 
-  // Import the type
+  // Import the type.
   ASTNodeImporter Importer(*this);
   ExpectedType ToTOrErr = Importer.Visit(FromT);
   if (!ToTOrErr)
     return ToTOrErr.takeError();
 
   // Record the imported type.
   ImportedTypes[FromT] = ToTOrErr->getTypePtr();
 
case NestedNameSpecifier::Super:
     if (ExpectedDecl RDOrErr = Import(FromNNS->getAsRecordDecl()))
       return NestedNameSpecifier::SuperSpecifier(ToContext,
                                                  cast<CXXRecordDecl>(*RDOrErr));
     else
       return RDOrErr.takeError();
 
   case NestedNameSpecifier::TypeSpec:
   case NestedNameSpecifier::TypeSpecWithTemplate:
-    if (Expected<QualType> TyOrErr =
-            Import(QualType(FromNNS->getAsType(), 0u))) {
+    if (Expected<const Type *> TyOrErr = Import(FromNNS->getAsType())) {
       bool TSTemplate =
           FromNNS->getKind() == NestedNameSpecifier::TypeSpecWithTemplate;
       return NestedNameSpecifier::Create(ToContext, Prefix, TSTemplate,
-                                         TyOrErr->getTypePtr());
+                                         *TyOrErr);
     } else {
       return TyOrErr.takeError();
     }
   }
 
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
if (FromValue.getLValueBase()) {
           if (Err)
             return std::move(Err);
           Base = APValue::LValueBase(cast<ValueDecl>(ImpDecl),
                                      FromValue.getLValueBase().getCallIndex(),
                                      FromValue.getLValueBase().getVersion());
         }
       } else {
         FromElemTy = FromValue.getLValueBase().getTypeInfoType();
-        QualType ImpTypeInfo = importChecked(
-            Err,
-            QualType(FromValue.getLValueBase().get<TypeInfoLValue>().getType(),
-                     0));
+        const Type *ImpTypeInfo = importChecked(
+            Err, FromValue.getLValueBase().get<TypeInfoLValue>().getType());
         QualType ImpType =
             importChecked(Err, FromValue.getLValueBase().getTypeInfoType());
         if (Err)
           return std::move(Err);
-        Base = APValue::LValueBase::getTypeInfo(
-            TypeInfoLValue(ImpTypeInfo.getTypePtr()), ImpType);
+        Base = APValue::LValueBase::getTypeInfo(TypeInfoLValue(ImpTypeInfo),
+                                                ImpType);
       }
     }
     CharUnits Offset = FromValue.getLValueOffset();
     unsigned PathLength = FromValue.getLValuePath().size();
     Result.MakeLValue();
     if (FromValue.hasLValuePath()) {
       MutableArrayRef<APValue::LValuePathEntry> ToPath = Result.setLValueUninit(
           Base, Offset, PathLength, FromValue.isLValueOnePastTheEnd(),