[ASTImporter] Fix infinite recurse on return type declared inside body (#68775)

[danix800]

Lambda without trailing auto could have return type declared inside the body too.

Fixes #68775

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ding Fei (danix800)

Changes

Lambda without trailing auto could has return type declared inside the body too.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+6-10)
• (modified) clang/unittests/AST/ASTImporterTest.cpp (+16)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 6aaa34c55ce307..88883bbd6ebaf1 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -695,7 +695,7 @@ namespace clang {
     // 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 hasAutoReturnTypeDeclaredInside(FunctionDecl *D);
+    bool hasReturnTypeDeclaredInside(FunctionDecl *D);
   };
 
 template <typename InContainerTy>
@@ -3649,19 +3649,15 @@ class IsTypeDeclaredInsideVisitor
 
 /// This function checks if the function has 'auto' return type that contains
 /// a reference (in any way) to a declaration inside the same function.
-bool ASTNodeImporter::hasAutoReturnTypeDeclaredInside(FunctionDecl *D) {
+bool ASTNodeImporter::hasReturnTypeDeclaredInside(FunctionDecl *D) {
   QualType FromTy = D->getType();
   const auto *FromFPT = FromTy->getAs<FunctionProtoType>();
   assert(FromFPT && "Must be called on FunctionProtoType");
 
   QualType RetT = FromFPT->getReturnType();
-  if (isa<AutoType>(RetT.getTypePtr())) {
-    FunctionDecl *Def = D->getDefinition();
-    IsTypeDeclaredInsideVisitor Visitor(Def ? Def : D);
-    return Visitor.CheckType(RetT);
-  }
-
-  return false;
+  FunctionDecl *Def = D->getDefinition();
+  IsTypeDeclaredInsideVisitor Visitor(Def ? Def : D);
+  return Visitor.CheckType(RetT);
 }
 
 ExplicitSpecifier
@@ -3811,7 +3807,7 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
     // E.g.: auto foo() { struct X{}; return X(); }
     // To avoid an infinite recursion when importing, create the FunctionDecl
     // with a simplified return type.
-    if (hasAutoReturnTypeDeclaredInside(D)) {
+    if (hasReturnTypeDeclaredInside(D)) {
       FromReturnTy = Importer.getFromContext().VoidTy;
       UsedDifferentProtoType = true;
     }
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index acc596fef87b76..3fac5514319c24 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -6721,6 +6721,22 @@ TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   EXPECT_FALSE(FromL->isDependentLambda());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, LambdaReturnTypeDeclaredInside) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"(
+      void foo() {
+        (void) []() {
+          struct X {};
+          return X();
+        };
+      }
+      )",
+      Lang_CXX11, "", Lang_CXX11, "foo");
+  auto *ToLambda = FirstDeclMatcher<LambdaExpr>().match(To, lambdaExpr());
+  EXPECT_TRUE(ToLambda);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionParam) {
   Decl *FromTU = getTuDecl(
       R"(

[mizvekov]

I think it would be a good idea to double check this for performance regressions, since this case will recurse into the function every time with this patch.
Though I don't know if there is a better way to test it than to merge it and wait for it to be picked up by google.

Another idea to avoid any potential regressions, is to mark the return type as deduced anyway, with the distinction that 'auto' was not written.

[danix800]

I think it would be a good idea to double check this for performance regressions, since this case will recurse into the function every time with this patch. Though I don't know if there is a better way to test it than to merge it and wait for it to be picked up by google.

For large TU it's worth considering for performance. The previous commit relaxed the TypeVisitor checking to all FunctionDecl. I reverted back to the original auto impl, plus an extra condition that if it's a lambda definition, see 3f1d714

Another idea to avoid any potential regressions, is to mark the return type as deduced anyway, with the distinction that 'auto' was not written.

I tested this idea, might be like this:

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 712958594473..988efcbef3a3 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -3680,24 +3680,10 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
     // E.g.: auto foo() { struct X{}; return X(); }
     // To avoid an infinite recursion when importing, create the FunctionDecl
     // with a simplified return type.
-    if (hasAutoReturnTypeDeclaredInside(D)) {
-      FromReturnTy = Importer.getFromContext().VoidTy;
-      UsedDifferentProtoType = true;
-    }
-    FunctionProtoType::ExtProtoInfo FromEPI = FromFPT->getExtProtoInfo();
-    // FunctionProtoType::ExtProtoInfo's ExceptionSpecDecl can point to the
-    // FunctionDecl that we are importing the FunctionProtoType for.
-    // To avoid an infinite recursion when importing, create the FunctionDecl
-    // with a simplified function type.
-    if (FromEPI.ExceptionSpec.SourceDecl ||
-        FromEPI.ExceptionSpec.SourceTemplate ||
-        FromEPI.ExceptionSpec.NoexceptExpr) {
-      FunctionProtoType::ExtProtoInfo DefaultEPI;
-      FromEPI = DefaultEPI;
-      UsedDifferentProtoType = true;
-    }
+    FunctionProtoType::ExtProtoInfo DefaultEPI;
+    UsedDifferentProtoType = true;
     FromTy = Importer.getFromContext().getFunctionType(
-        FromReturnTy, FromFPT->getParamTypes(), FromEPI);
+        Importer.getFromContext().VoidTy, FromFPT->getParamTypes(), DefaultEPI);
     FromTSI = Importer.getFromContext().getTrivialTypeSourceInfo(
         FromTy, D->getBeginLoc());
   }

but this will break two testcases from check-clang-astmerge-exprs-cpp:

Failed Tests (2):
  Clang :: ASTMerge/class-template/test.cpp
  Clang :: ASTMerge/exprs-cpp/test.cpp

[mizvekov]

I tested this idea, might be like this:

I meant something else. This patch makes it so the delayed function type importation happens unconditionally.
I was aware that change would cause breakages.

What I meant is that, we have a special sugar type node that marks deduction, this AutoType, and we apply it everywhere else, except this lambda case you exposed. I was going to suggest we do the same for these lambdas. That would make things more consistent.

[mizvekov]

LGTM, Thanks!