[ASTImporter] Support functions with placeholder return types ...

[martong]

Summary

Support functions with placeholder return types even in cases when the type is
declared in the body of the function.
Example: auto f() { struct X{}; return X(); }

Currently, the import of such functions result in an infinite recursion.
The fix is that we import a simplified type for the function and once the FunctionDecl and the body is created, then we import and set the original type.

[a_sidorin]

Hello Gabor,
That's an interesting case, thank for fixing this!

clang/lib/AST/ASTImporter.cpp

3008

ASTImporter is not very const-friendly, but this function is pure, so I think it's better to const-qualify the parameters.

3020

Is it possible for getAs() to return nullptr at this point?

3174

If FromFPT is not used outside of the condition, we can move the initialization inside if().

clang/unittests/AST/ASTImporterTest.cpp

5609

Nit: a space before '{'.

5625

That's interesting. Have you tried '-frewrite-includes' for expanding inclusions only without macro expansion?

[martong]

clang/lib/AST/ASTImporter.cpp

3008

Ok, added the const qualifiers.

3020

No. So, I added an assertion to reflect this.

3174

Ok, I moved it inside the if.

clang/unittests/AST/ASTImporterTest.cpp

5625

I hadn't tried it before, but I've just done that now. Unfortunately, the AST produced with -E -frewrite-includes produces a type for the operator() that already has a canonical type: long (long) const

) ./bin/clang-check -p cxx_cmd_json ~w/llvm3/git/llvm-project/libcxx/src/filesystem/directory_iterator.cpp -ast-dump -ast-dump-filter "posix_utimes" | ag "CXXMethodDecl.*int_type"                                           --- COMMAND ---
  |           | |-CXXMethodDecl 0x3007ec0 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x30131f0 <col:18, line:400:3> line:396:18 implicit __invoke 'int_type (long)' static inline
  |           | |-CXXMethodDecl 0x2ddfaf0 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x2deae20 <col:18, line:400:3> line:396:18 implicit __invoke 'int_type (long)' static inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang-check -p cxx_cmd_json ~w/llvm3/git/llvm-project/libcxx/src/filesystem/directory_iterator.cpp -ast-dump -ast-dump-filter "posix_utimes" | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x2833020 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
  |           | |-CXXMethodDecl 0x287fa20 <col:30, line:400:3> line:396:18 used operator() 'int_type (long) const' inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang -x c++ directory_iterator.cpp.E.orig -fsyntax-only -Xclang -ast-dump -Xclang -ast-dump-filter -Xclang posix_utimes | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x31c6ad0 <col:30, line:400:3> line:396:18 used operator() 'long (long) const' inline
(venv) egbomrt@elx78373d5s|~/WORK/llvm3/build/release_assert
) ./bin/clang -x c++ directory_iterator.cpp.E.ri -fsyntax-only -Xclang -ast-dump -Xclang -ast-dump-filter -Xclang posix_utimes | ag "CXXMethodDecl.*operator"
  |           | |-CXXMethodDecl 0x25ad7f0 <col:30, line:400:3> line:396:18 used operator() 'long (long) const' inline

[martong]

• Address Alexei's comments

[martong]

Alexei, thanks for the assiduous review!

[a_sidorin]

LGTM, thanks!
Let's wait for Shafik's comments before committing this.

clang/lib/AST/ASTImporter.cpp

48

Do we use this new include?

[martong]

clang/unittests/AST/ASTImporterTest.cpp

5625

So, this means that the type information is transformed during the include expansion. That is very strange.
I think, I'll have an email to cfe-dev, maybe others have some insights on this.

[shafik]

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

[shafik]

Besides my comments about different scenarios this LGTM

clang/lib/AST/ASTImporter.cpp

3018

I added a comment with several examples of C++ code that I am not sure if this will catch or not. Since C++17 we allow types to be declared in several more places.

[martong]

In D70819#1777770, @shafik wrote:

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Thanks for these cases! I am going to write unit tests for these as well.

[martong]

In D70819#1781678, @martong wrote:

In D70819#1777770, @shafik wrote:

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Thanks for these cases! I am going to write unit tests for these as well.

So I checked and wrote a test case for all of these. All of them passed!

clang/lib/AST/ASTImporter.cpp

48

Thanks for catching this! I used clangd this time and that added this include, I am not sure why, anyway I set up clangd to do not add includes.

clang/lib/AST/ASTImporter.cpp

 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/ExceptionSpecificationType.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
Error ImportArrayChecked(const InContainerTy &InContainer, OIter Obegin) {
       return ImportArrayChecked(InContainer.begin(), InContainer.end(), Obegin);
     }
 
     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 hasAutoReturnTypeDeclaredInside(FunctionDecl *D);
   };
 
 template <typename InContainerTy>
 Error ASTNodeImporter::ImportTemplateArgumentListInfo(
     SourceLocation FromLAngleLoc, SourceLocation FromRAngleLoc,
     const InContainerTy &Container, TemplateArgumentListInfo &Result) {
   auto ToLAngleLocOrErr = import(FromLAngleLoc);
   if (!ToLAngleLocOrErr)

 //----------------------------------------------------------------------------
 // Import Declarations
 //----------------------------------------------------------------------------
 Error ASTNodeImporter::ImportDeclParts(
     NamedDecl *D, DeclContext *&DC, DeclContext *&LexicalDC,
     DeclarationName &Name, NamedDecl *&ToD, SourceLocation &Loc) {
   // Check if RecordDecl is in FunctionDecl parameters to avoid infinite loop.
   // example: int struct_in_proto(struct data_t{int a;int b;} *d);
+  // FIXME: We could support these constructs by importing a different type of
+  // this parameter and by importing the original type of the parameter only
+  // after the FunctionDecl is created. See
+  // VisitFunctionDecl::UsedDifferentProtoType.
   DeclContext *OrigDC = D->getDeclContext();
   FunctionDecl *FunDecl;
   if (isa<RecordDecl>(D) && (FunDecl = dyn_cast<FunctionDecl>(OrigDC)) &&
       FunDecl->hasBody()) {
     auto getLeafPointeeType = [](const Type *T) {
       while (T->isPointerType() || T->isArrayType()) {
         T = T->getPointeeOrArrayElementType();
       }
if (Stmt *FromBody = FromFD->getBody()) {
     if (ExpectedStmt ToBodyOrErr = import(FromBody))
       ToFD->setBody(*ToBodyOrErr);
     else
       return ToBodyOrErr.takeError();
   }
   return Error::success();
 }
 
+// Returns true if the given D has a DeclContext up to the TranslationUnitDecl
+// which is equal to the given DC.
+bool isAncestorDeclContextOf(DeclContext *DC, Decl *D) {
+  DeclContext *DCi = D->getDeclContext();
+  while (DCi != D->getTranslationUnitDecl()) {
+    if (DCi == DC)
+      return true;
+    DCi = DCi->getParent();
+  }
+  return false;
+}
+
+bool ASTNodeImporter::hasAutoReturnTypeDeclaredInside(FunctionDecl *D) {
+  QualType FromTy = D->getType();
+  const FunctionProtoType *FromFPT = FromTy->getAs<FunctionProtoType>();
+  if (AutoType *AutoT = FromFPT->getReturnType()->getContainedAutoType()) {
+    QualType DeducedT = AutoT->getDeducedType();
+    if (const RecordType *RecordT =
+            DeducedT.isNull() ? nullptr : dyn_cast<RecordType>(DeducedT)) {
+      RecordDecl *RD = RecordT->getDecl();
+      assert(RD);
+      if (isAncestorDeclContextOf(D, RD)) {
+        assert(RD->getLexicalDeclContext() == RD->getDeclContext());
+        return true;
+      }
+    }
+  }
+  if (const TypedefType *TypedefT =
+          dyn_cast<TypedefType>(FromFPT->getReturnType())) {
+    TypedefNameDecl *TD = TypedefT->getDecl();
+    assert(TD);
+    if (isAncestorDeclContextOf(D, TD)) {
+      assert(TD->getLexicalDeclContext() == TD->getDeclContext());
+      return true;
+    }
+  }
+  return false;
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   SmallVector<Decl *, 2> Redecls = getCanonicalForwardRedeclChain(D);
   auto RedeclIt = Redecls.begin();
   // Import the first part of the decl chain. I.e. import all previous
   // declarations starting from the canonical decl.
   for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt) {
     ExpectedDecl ToRedeclOrErr = import(*RedeclIt);
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   }
 
   DeclarationNameInfo NameInfo(Name, Loc);
   // Import additional name location/type info.
   if (Error Err = ImportDeclarationNameLoc(D->getNameInfo(), NameInfo))
     return std::move(Err);
 
   QualType FromTy = D->getType();
-  bool usedDifferentExceptionSpec = false;
-
-  if (const auto *FromFPT = D->getType()->getAs<FunctionProtoType>()) {
+  // Set to true if we do not import the type of the function as is. There are
+  // cases when the original type would result in an infinite recursion during
+  // the import. To avoid an infinite recursion when importing, we create the
+  // FunctionDecl with a simplified function type and update it only after the
+  // relevant AST nodes are already imported.
+  bool UsedDifferentProtoType = false;
+  const auto *FromFPT = FromTy->getAs<FunctionProtoType>();
+  if (FromFPT) {
+    QualType FromReturnTy = FromFPT->getReturnType();
+    // Functions with auto return type may define a struct inside their body
+    // and the return type could refer to that struct.
+    // 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 and update it afterwards.
+    // with a simplified function type.
     if (FromEPI.ExceptionSpec.SourceDecl ||
         FromEPI.ExceptionSpec.SourceTemplate ||
         FromEPI.ExceptionSpec.NoexceptExpr) {
       FunctionProtoType::ExtProtoInfo DefaultEPI;
-      FromTy = Importer.getFromContext().getFunctionType(
-          FromFPT->getReturnType(), FromFPT->getParamTypes(), DefaultEPI);
-      usedDifferentExceptionSpec = true;
+      FromEPI = DefaultEPI;
+      UsedDifferentProtoType = true;
     }
+    FromTy = Importer.getFromContext().getFunctionType(
+        FromReturnTy, FromFPT->getParamTypes(), FromEPI);
   }
 
   QualType T;
   TypeSourceInfo *TInfo;
   SourceLocation ToInnerLocStart, ToEndLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   if (auto Imp = importSeq(
       FromTy, D->getTypeSourceInfo(), D->getInnerLocStart(),
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   if (TInfo) {
     if (auto ProtoLoc =
         TInfo->getTypeLoc().IgnoreParens().getAs<FunctionProtoTypeLoc>()) {
       for (unsigned I = 0, N = Parameters.size(); I != N; ++I)
         ProtoLoc.setParam(I, Parameters[I]);
     }
   }
 
-  if (usedDifferentExceptionSpec) {
-    // Update FunctionProtoType::ExtProtoInfo.
-    if (ExpectedType TyOrErr = import(D->getType()))
-      ToFunction->setType(*TyOrErr);
-    else
-      return TyOrErr.takeError();
-  }
-
   // Import the describing template function, if any.
   if (FromFT) {
     auto ToFTOrErr = import(FromFT);
     if (!ToFTOrErr)
       return ToFTOrErr.takeError();
   }
 
   // Import Ctor initializers.
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   if (D->doesThisDeclarationHaveABody()) {
     Error Err = ImportFunctionDeclBody(D, ToFunction);
 
     if (Err)
       return std::move(Err);
   }
 
+  // Import and set the original type in case we used another type.
+  if (UsedDifferentProtoType) {
+    if (ExpectedType TyOrErr = import(D->getType()))
+      ToFunction->setType(*TyOrErr);
+    else
+      return TyOrErr.takeError();
+  }
+
   // FIXME: Other bits to merge?
 
   // If it is a template, import all related things.
   if (Error Err = ImportTemplateInformation(D, ToFunction))
     return std::move(Err);
 
   bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend);
 

clang/unittests/AST/ASTImporterTest.cpp

 //===- unittest/AST/ASTImporterTest.cpp - AST node import test ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Tests for the correct import of AST nodes from one AST context to another.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/StringMap.h"
 
 #include "clang/AST/DeclContextInternals.h"
+#include "gtest/gtest.h"
 
 #include "ASTImporterFixtures.h"
 #include "MatchVerifier.h"
 
 namespace clang {
 namespace ast_matchers {
 
 using internal::Matcher;
TEST_P(ASTImporterOptionSpecificTestBase, ImportDefaultConstructibleLambdas) {
   FunctionDecl *ToF = Import(FromF, Lang_CXX2a);
   EXPECT_TRUE(ToF);
   TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We have two lambda classes after the import.
   EXPECT_EQ(DeclCounter<CXXRecordDecl>().match(ToTU, cxxRecordDecl(isLambda())),
             2u);
 }
 
+struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto X = [](long l){
+        using int_type = long;
+        auto dur = 13;
+        return static_cast<int_type>(dur);
+      };
+      )",
+      Lang_CXX14, "input0.cc");
+  CXXMethodDecl *From =
+      FirstDeclMatcher<CXXMethodDecl>().match(FromTU, cxxMethodDecl());
+
+  // Explicitly set the return type of the lambda's operator() to the TypeAlias.
+  // Normally the return type would be the built-in 'long' type. However, there
+  // are cases when Clang does not use the canonical type and the TypeAlias is
+  // used. I could not create such an AST from regular source code, it requires
+  // some special state in the preprocessor. I've found such an AST when Clang
+  // parsed libcxx/src/filesystem/directory_iterator.cpp, but could not reduce
+  // that with creduce, because after preprocessing, the AST no longer
+  // contained the TypeAlias as a return type of the lambda.
+  ASTContext &Ctx = From->getASTContext();
+  TypeAliasDecl *FromTA =
+      FirstDeclMatcher<TypeAliasDecl>().match(FromTU, typeAliasDecl());
+  QualType TT = Ctx.getTypedefType(FromTA);
+  const FunctionProtoType *FPT = cast<FunctionProtoType>(From->getType());
+  QualType NewFunType =
+      Ctx.getFunctionType(TT, FPT->getParamTypes(), FPT->getExtProtoInfo());
+  From->setType(NewFunType);
+
+  CXXMethodDecl *To = Import(From, Lang_CXX14);
+  EXPECT_TRUE(To);
+  EXPECT_TRUE(isa<TypedefType>(To->getReturnType()));
+}
+
+TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredInside) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto foo() {
+        struct X {};
+        return X();
+      }
+      )",
+      Lang_CXX14, "input0.cc");
+  FunctionDecl *From =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+
+  FunctionDecl *To = Import(From, Lang_CXX14);
+  EXPECT_TRUE(To);
+  EXPECT_TRUE(isa<AutoType>(To->getReturnType()));
+}
+
+TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredInside2) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto foo() {
+        struct X {};
+        return X();
+      }
+      )",
+      Lang_CXX14, "input0.cc");
+  FunctionDecl *From =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+
+  // This time import the type directly.
+  QualType ToT = ImportType(From->getType(), From, Lang_CXX14);
+  const FunctionProtoType *FPT = cast<FunctionProtoType>(ToT);
+  EXPECT_TRUE(isa<AutoType>(FPT->getReturnType()));
+}
+
+TEST_P(ImportAutoFunctions, ReturnWithTypedefToStructDeclaredInside) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto foo() {
+        struct X {};
+        using Y = X;
+        return Y();
+      }
+      )",
+      Lang_CXX14, "input0.cc");
+  FunctionDecl *From =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+
+  FunctionDecl *To = Import(From, Lang_CXX14);
+  EXPECT_TRUE(To);
+  EXPECT_TRUE(isa<AutoType>(To->getReturnType()));
+}
+
+TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredNestedInside) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      auto foo() {
+        struct X { struct Y{}; };
+        return X::Y();
+      }
+      )",
+      Lang_CXX14, "input0.cc");
+  FunctionDecl *From =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+
+  FunctionDecl *To = Import(From, Lang_CXX14);
+  EXPECT_TRUE(To);
+  EXPECT_TRUE(isa<AutoType>(To->getReturnType()));
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, RedirectingImporterTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportAutoFunctions,
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportClasses,
                         DefaultTestValuesForRunOptions, );