[clang][ASTImporter] Fix for importing functions with EST_Unevaluated prototype.

[balazske]

Summary

Fix for importing functions where the TypeSourceInfo is set and the
exception specification information contains reference to the function
declaration itself.

[steakhal]

Good job.

clang/unittests/AST/ASTImporterTest.cpp

6180

Perhaps, put here a FIXME to replace this with a real-world scenario.

6189

Please assert that the number of constructors is correct.
I would rather recommend doing so in the from case as well.

[shafik]

clang/unittests/AST/ASTImporterTest.cpp

6180

I think it is useful to have cases that we run into in practice as a regression test but it would be nice to see a more general test as well.

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

6180

What means a "more general test"? Code where the setTypeSourceInfo is not needed? I just could not find out why there is no TypeSourceInfo set. The involved code part looks like this (comes from Bitcoin):

void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) {
    // Each connection captures pwalletIn to ensure that each callback is
    // executed before pwalletIn is destroyed. For more details see #18338.
    g_signals.m_internals->Register(std::move(pwalletIn));
}

void RegisterValidationInterface(CValidationInterface* callbacks)
{
    // Create a shared_ptr with a no-op deleter - CValidationInterface lifecycle
    // is managed by the caller.
    RegisterSharedValidationInterface({callbacks, [](CValidationInterface*){}});
    //                                            ^ This is the lambda to be imported.
}

If the same code is used in the test and a simple CValidationInterface and std::shared_ptr with appropriate constructor is added there is still no TypeSourceInfo in the needed place.

[shafik]

clang/unittests/AST/ASTImporterTest.cpp

6180

Yeah, that is what I meant, a test that did not require the setTypeSourceInfo hand crafting.

[martong]

Thanks! The fix makes sense and looks good to me. Though, it would be nice if you could provide a test without the hand crafting of the setTypeSourceInfo, but I think that could be done in a follow-up patch.

clang/unittests/AST/ASTImporterTest.cpp

6180

On one hand, I think this is a through and precisely written test case and we had similar test cases before. On the other hand, I agree it is unfortunate that we have to change the AST in the test setup.
@balazske Perhaps creduce-ing the to be imported TU with the bitcoin case could provide a reduced case that does not require the hand crafting of the setTypeSourceInfo ?

[balazske]

Improved test, set of TypeSourceInfo is not needed any more.

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

6189

Here the goal is only to get a copy constructor, not to test if all constructors are imported. To get the constructor count relatively much additional code is needed (there is no function for it).

[steakhal]

I love it.

clang/unittests/AST/ASTImporterTest.cpp

6189

I see. Thanks.

[martong]

Thanks for updating the test case!

clang/lib/AST/ASTImporter.cpp

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();
+  TypeSourceInfo *FromTSI = D->getTypeSourceInfo();
   // 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.
+  // The type is related to TypeSourceInfo (it references the type), so we must
+  // do the same with TypeSourceInfo.
   bool UsedDifferentProtoType = false;
   if (const auto *FromFPT = FromTy->getAs<FunctionProtoType>()) {
     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 (FromEPI.ExceptionSpec.SourceDecl ||
         FromEPI.ExceptionSpec.SourceTemplate ||
         FromEPI.ExceptionSpec.NoexceptExpr) {
       FunctionProtoType::ExtProtoInfo DefaultEPI;
       FromEPI = DefaultEPI;
       UsedDifferentProtoType = true;
     }
     FromTy = Importer.getFromContext().getFunctionType(
         FromReturnTy, FromFPT->getParamTypes(), FromEPI);
+    FromTSI = Importer.getFromContext().getTrivialTypeSourceInfo(
+        FromTy, D->getBeginLoc());
   }
 
   Error Err = Error::success();
   auto T = importChecked(Err, FromTy);
-  auto TInfo = importChecked(Err, D->getTypeSourceInfo());
+  auto TInfo = importChecked(Err, FromTSI);
   auto ToInnerLocStart = importChecked(Err, D->getInnerLocStart());
   auto ToEndLoc = importChecked(Err, D->getEndLoc());
   auto ToQualifierLoc = importChecked(Err, D->getQualifierLoc());
   auto TrailingRequiresClause =
       importChecked(Err, D->getTrailingRequiresClause());
   if (Err)
     return std::move(Err);
 
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   }
 
   // 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();
+    if (Expected<TypeSourceInfo *> TSIOrErr = import(D->getTypeSourceInfo()))
+      ToFunction->setTypeSourceInfo(*TSIOrErr);
+    else
+      return TSIOrErr.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);
 

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportDefaultConstructibleLambdas) {
   FunctionDecl *ToF = Import(FromF, Lang_CXX20);
   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);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportFunctionDeclWithTypeSourceInfoWithSourceDecl) {
+  // This code results in a lambda with implicit constructor.
+  // The constructor's TypeSourceInfo points out the function prototype.
+  // This prototype has an EST_Unevaluated in its exception information and a
+  // SourceDecl that is the function declaration itself.
+  // The test verifies that AST import of such AST does not crash.
+  // (Here the function's TypeSourceInfo references the function itself.)
+  Decl *FromTU = getTuDecl(
+      R"(
+        template<typename T> void f(T) { auto X = [](){}; }
+        void g() { f(10); }
+        )",
+      Lang_CXX11, "input0.cc");
+
+  // Use LastDeclMatcher to find the LambdaExpr in the template specialization.
+  CXXRecordDecl *FromL = LastDeclMatcher<LambdaExpr>()
+                             .match(FromTU, lambdaExpr())
+                             ->getLambdaClass();
+
+  CXXConstructorDecl *FromCtor = *FromL->ctor_begin();
+  ASSERT_TRUE(FromCtor->isCopyConstructor());
+  ASSERT_TRUE(FromCtor->getTypeSourceInfo());
+  const auto *FromFPT = FromCtor->getType()->getAs<FunctionProtoType>();
+  ASSERT_TRUE(FromFPT);
+  EXPECT_EQ(FromCtor->getTypeSourceInfo()->getType().getTypePtr(), FromFPT);
+  FunctionProtoType::ExtProtoInfo FromEPI = FromFPT->getExtProtoInfo();
+  // If type is EST_Unevaluated, SourceDecl should be set to the parent Decl.
+  EXPECT_EQ(FromEPI.ExceptionSpec.Type, EST_Unevaluated);
+  EXPECT_EQ(FromEPI.ExceptionSpec.SourceDecl, FromCtor);
+
+  auto ToL = Import(FromL, Lang_CXX11);
+
+  // Check if the import was correct.
+  CXXConstructorDecl *ToCtor = *ToL->ctor_begin();
+  EXPECT_TRUE(ToCtor->getTypeSourceInfo());
+  const auto *ToFPT = ToCtor->getType()->getAs<FunctionProtoType>();
+  ASSERT_TRUE(ToFPT);
+  EXPECT_EQ(ToCtor->getTypeSourceInfo()->getType().getTypePtr(), ToFPT);
+  FunctionProtoType::ExtProtoInfo ToEPI = ToFPT->getExtProtoInfo();
+  EXPECT_EQ(ToEPI.ExceptionSpec.Type, EST_Unevaluated);
+  EXPECT_EQ(ToEPI.ExceptionSpec.SourceDecl, ToCtor);
+}
+
 struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
   Decl *FromTU = getTuDecl(
       R"(
       auto X = [](long l) {
         using int_type = long;
         auto dur = 13;