[clang][ASTImporter] Remove 'hasReturnTypeDeclaredInside'

[balazske]

The function hasReturnTypeDeclaredInside and a related visitor class was used to detect recursive import of function declarations. There is now a new way to detect the recursive import that works in all cases (unlike the previous method). The new cycle detector should find all cases of recursive import so the previous function is not needed.
The change is not entirely NFC because the sequence of the function import can change (and the new cycle detection is a different solution for the problem).

[llvmbot]

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

The function hasReturnTypeDeclaredInside and a related visitor class was used to detect recursive import of function declarations. There is now a new way to detect the recursive import that works in all cases (unlike the previous method). The new cycle detector should find all cases of recursive import so the previous function is not needed.
The change is not entirely NFC because the sequence of the function import can change (and the new cycle detection is a different solution for the problem).

---

Full diff: https://github.com/llvm/llvm-project/pull/169504.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+3-226)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c1441744c8578..f2e5215f09ece 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -748,13 +748,8 @@ namespace clang {
     Error ImportOverriddenMethods(CXXMethodDecl *ToMethod,
                                   CXXMethodDecl *FromMethod);
 
-    Expected<FunctionDecl *> FindFunctionTemplateSpecialization(
-        FunctionDecl *FromFD);
-
-    // Returns true if the given function has a placeholder return type and
-    // that type is declared inside the body of the function.
-    // E.g. auto f() { struct X{}; return X(); }
-    bool hasReturnTypeDeclaredInside(FunctionDecl *D);
+    Expected<FunctionDecl *>
+    FindFunctionTemplateSpecialization(FunctionDecl *FromFD);
   };
 
 template <typename InContainerTy>
@@ -3694,223 +3689,6 @@ Error ASTNodeImporter::ImportFunctionDeclBody(FunctionDecl *FromFD,
   return Error::success();
 }
 
-// Returns true if the given D has a DeclContext up to the TranslationUnitDecl
-// which is equal to the given DC, or D is equal to DC.
-static bool isAncestorDeclContextOf(const DeclContext *DC, const Decl *D) {
-  const DeclContext *DCi = dyn_cast<DeclContext>(D);
-  if (!DCi)
-    DCi = D->getDeclContext();
-  assert(DCi && "Declaration should have a context");
-  while (DCi != D->getTranslationUnitDecl()) {
-    if (DCi == DC)
-      return true;
-    DCi = DCi->getParent();
-  }
-  return false;
-}
-
-// Check if there is a declaration that has 'DC' as parent context and is
-// referenced from statement 'S' or one of its children. The search is done in
-// BFS order through children of 'S'.
-static bool isAncestorDeclContextOf(const DeclContext *DC, const Stmt *S) {
-  SmallVector<const Stmt *> ToProcess;
-  ToProcess.push_back(S);
-  while (!ToProcess.empty()) {
-    const Stmt *CurrentS = ToProcess.pop_back_val();
-    ToProcess.append(CurrentS->child_begin(), CurrentS->child_end());
-    if (const auto *DeclRef = dyn_cast<DeclRefExpr>(CurrentS)) {
-      if (const Decl *D = DeclRef->getDecl())
-        if (isAncestorDeclContextOf(DC, D))
-          return true;
-    } else if (const auto *E =
-                   dyn_cast_or_null<SubstNonTypeTemplateParmExpr>(CurrentS)) {
-      if (const Decl *D = E->getAssociatedDecl())
-        if (isAncestorDeclContextOf(DC, D))
-          return true;
-    }
-  }
-  return false;
-}
-
-namespace {
-/// Check if a type has any reference to a declaration that is inside the body
-/// of a function.
-/// The \c CheckType(QualType) function should be used to determine
-/// this property.
-///
-/// The type visitor visits one type object only (not recursive).
-/// To find all referenced declarations we must discover all type objects until
-/// the canonical type is reached (walk over typedef and similar objects). This
-/// is done by loop over all "sugar" type objects. For every such type we must
-/// check all declarations that are referenced from it. For this check the
-/// visitor is used. In the visit functions all referenced declarations except
-/// the one that follows in the sugar chain (if any) must be checked. For this
-/// check the same visitor is re-used (it has no state-dependent data).
-///
-/// The visit functions have 3 possible return values:
-///  - True, found a declaration inside \c ParentDC.
-///  - False, found declarations only outside \c ParentDC and it is not possible
-///    to find more declarations (the "sugar" chain does not continue).
-///  - Empty optional value, found no declarations or only outside \c ParentDC,
-///    but it is possible to find more declarations in the type "sugar" chain.
-/// The loop over the "sugar" types can be implemented by using type visit
-/// functions only (call \c CheckType with the desugared type). With the current
-/// solution no visit function is needed if the type has only a desugared type
-/// as data.
-class IsTypeDeclaredInsideVisitor
-    : public TypeVisitor<IsTypeDeclaredInsideVisitor, std::optional<bool>> {
-public:
-  IsTypeDeclaredInsideVisitor(const FunctionDecl *ParentDC)
-      : ParentDC(ParentDC) {}
-
-  bool CheckType(QualType T) {
-    // Check the chain of "sugar" types.
-    // The "sugar" types are typedef or similar types that have the same
-    // canonical type.
-    if (std::optional<bool> Res = Visit(T.getTypePtr()))
-      return *Res;
-    QualType DsT =
-        T.getSingleStepDesugaredType(ParentDC->getParentASTContext());
-    while (DsT != T) {
-      if (std::optional<bool> Res = Visit(DsT.getTypePtr()))
-        return *Res;
-      T = DsT;
-      DsT = T.getSingleStepDesugaredType(ParentDC->getParentASTContext());
-    }
-    return false;
-  }
-
-  std::optional<bool> VisitTagType(const TagType *T) {
-    if (auto *Spec = dyn_cast<ClassTemplateSpecializationDecl>(T->getDecl()))
-      for (const auto &Arg : Spec->getTemplateArgs().asArray())
-        if (checkTemplateArgument(Arg))
-          return true;
-    return isAncestorDeclContextOf(ParentDC, T->getDecl());
-  }
-
-  std::optional<bool> VisitPointerType(const PointerType *T) {
-    return CheckType(T->getPointeeType());
-  }
-
-  std::optional<bool> VisitReferenceType(const ReferenceType *T) {
-    return CheckType(T->getPointeeTypeAsWritten());
-  }
-
-  std::optional<bool> VisitTypedefType(const TypedefType *T) {
-    return isAncestorDeclContextOf(ParentDC, T->getDecl());
-  }
-
-  std::optional<bool> VisitUsingType(const UsingType *T) {
-    return isAncestorDeclContextOf(ParentDC, T->getDecl());
-  }
-
-  std::optional<bool>
-  VisitTemplateSpecializationType(const TemplateSpecializationType *T) {
-    for (const auto &Arg : T->template_arguments())
-      if (checkTemplateArgument(Arg))
-        return true;
-    // This type is a "sugar" to a record type, it can have a desugared type.
-    return {};
-  }
-
-  std::optional<bool> VisitUnaryTransformType(const UnaryTransformType *T) {
-    return CheckType(T->getBaseType());
-  }
-
-  std::optional<bool>
-  VisitSubstTemplateTypeParmType(const SubstTemplateTypeParmType *T) {
-    // The "associated declaration" can be the same as ParentDC.
-    if (isAncestorDeclContextOf(ParentDC, T->getAssociatedDecl()))
-      return true;
-    return {};
-  }
-
-  std::optional<bool> VisitConstantArrayType(const ConstantArrayType *T) {
-    if (T->getSizeExpr() && isAncestorDeclContextOf(ParentDC, T->getSizeExpr()))
-      return true;
-
-    return CheckType(T->getElementType());
-  }
-
-  std::optional<bool> VisitVariableArrayType(const VariableArrayType *T) {
-    llvm_unreachable(
-        "Variable array should not occur in deduced return type of a function");
-  }
-
-  std::optional<bool> VisitIncompleteArrayType(const IncompleteArrayType *T) {
-    llvm_unreachable("Incomplete array should not occur in deduced return type "
-                     "of a function");
-  }
-
-  std::optional<bool> VisitDependentArrayType(const IncompleteArrayType *T) {
-    llvm_unreachable("Dependent array should not occur in deduced return type "
-                     "of a function");
-  }
-
-private:
-  const DeclContext *const ParentDC;
-
-  bool checkTemplateArgument(const TemplateArgument &Arg) {
-    switch (Arg.getKind()) {
-    case TemplateArgument::Null:
-      return false;
-    case TemplateArgument::Integral:
-      return CheckType(Arg.getIntegralType());
-    case TemplateArgument::Type:
-      return CheckType(Arg.getAsType());
-    case TemplateArgument::Expression:
-      return isAncestorDeclContextOf(ParentDC, Arg.getAsExpr());
-    case TemplateArgument::Declaration:
-      // FIXME: The declaration in this case is not allowed to be in a function?
-      return isAncestorDeclContextOf(ParentDC, Arg.getAsDecl());
-    case TemplateArgument::NullPtr:
-      // FIXME: The type is not allowed to be in the function?
-      return CheckType(Arg.getNullPtrType());
-    case TemplateArgument::StructuralValue:
-      return CheckType(Arg.getStructuralValueType());
-    case TemplateArgument::Pack:
-      for (const auto &PackArg : Arg.getPackAsArray())
-        if (checkTemplateArgument(PackArg))
-          return true;
-      return false;
-    case TemplateArgument::Template:
-      // Templates can not be defined locally in functions.
-      // A template passed as argument can be not in ParentDC.
-      return false;
-    case TemplateArgument::TemplateExpansion:
-      // Templates can not be defined locally in functions.
-      // A template passed as argument can be not in ParentDC.
-      return false;
-    }
-    llvm_unreachable("Unknown TemplateArgument::ArgKind enum");
-  };
-};
-} // namespace
-
-/// This function checks if the given function has a return type that contains
-/// a reference (in any way) to a declaration inside the same function.
-bool ASTNodeImporter::hasReturnTypeDeclaredInside(FunctionDecl *D) {
-  QualType FromTy = D->getType();
-  const auto *FromFPT = FromTy->getAs<FunctionProtoType>();
-  assert(FromFPT && "Must be called on FunctionProtoType");
-
-  auto IsCXX11Lambda = [&]() {
-    if (Importer.FromContext.getLangOpts().CPlusPlus14) // C++14 or later
-      return false;
-
-    return isLambdaMethod(D);
-  };
-
-  QualType RetT = FromFPT->getReturnType();
-  if (isa<AutoType>(RetT.getTypePtr()) || IsCXX11Lambda()) {
-    FunctionDecl *Def = D->getDefinition();
-    IsTypeDeclaredInsideVisitor Visitor(Def ? Def : D);
-    return Visitor.CheckType(RetT);
-  }
-
-  return false;
-}
-
 ExplicitSpecifier
 ASTNodeImporter::importExplicitSpecifier(Error &Err, ExplicitSpecifier ESpec) {
   Expr *ExplicitExpr = ESpec.getExpr();
@@ -4060,8 +3838,7 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
     // with a simplified return type.
     // Reuse this approach for auto return types declared as typenames from
     // template params, tracked in FindFunctionDeclImportCycle.
-    if (hasReturnTypeDeclaredInside(D) ||
-        Importer.FindFunctionDeclImportCycle.isCycle(D)) {
+    if (Importer.FindFunctionDeclImportCycle.isCycle(D)) {
       FromReturnTy = Importer.getFromContext().VoidTy;
       UsedDifferentProtoType = true;
     }