[clang][ASTImporter] Improved handling of functions with auto return type.

[balazske]

Summary

Avoid a crash if a function is imported that has auto return type that
references to a template with an expression-type of argument that
references into the function's body.
Fixes issue #56047

[martong]

Thanks, nice work!

clang/lib/AST/ASTImporter.cpp

3237

3237

The first call of getParents will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on protobuf and bitcoin with our internal CI infra)

3251–3254

Should this be handled in a switch rather, perhaps with an llvm_unreachable at the default case? Just to make sure that no "kind" is left out.

3269–3273

Is it possible that T is both a RecordType and a TemplateSpecializationType at the same time? From the hierarchy this seems impossible (?)

clang/unittests/AST/ASTImporterTest.cpp

6323

Nice test!

[balazske]

Small code NFC improvements.

[balazske]

clang/lib/AST/ASTImporter.cpp

3269–3273

The type dump shows that a RecordType is contained within the TemplateSpecializationType and it looks like that the get function handles this case. When the else is added the check of TemplateSpecializationType is skipped and the template argument expression X is not found (only integer constant 1).

TemplateSpecializationType 0x23fd8c0 'Tmpl<X>' sugar Tmpl
|-TemplateArgument expr
| `-ConstantExpr 0x23fd798 'int'
|   |-value: Int 1
|   `-ImplicitCastExpr 0x23fd780 'int' <LValueToRValue>
|     `-DeclRefExpr 0x23fd760 'const int' lvalue Var 0x23fd648 'X' 'const int' non_odr_use_constant
`-RecordType 0x23fd8a0 'struct Tmpl<1>'
  `-ClassTemplateSpecialization 0x23fd7b8 'Tmpl'

[martong]

clang/lib/AST/ASTImporter.cpp

3269–3273

Okay. I've dug deeper to understand how that is working. So, in the TemplateSpecializationType 'Tmpl<X>' is actually a "sugar" to the RecordType 'struct Tmpl<1>'.
And getAs<RecordType> will return with the desugared type:

// If this is a typedef for the type, strip the typedef off without
// losing all typedef information.
return cast<T>(getUnqualifiedDesugaredType());

Thus, to answer my own question, it is possible that T is both a RecordType and a TemplateSpecializationType at the same time!

[martong]

LGTM

clang/lib/AST/ASTImporter.cpp

3237

The first call of getParents will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on protobuf and bitcoin with our internal CI infra)

I've seen the measurement results, they look good.

We have errors in the CTU analysis of 'qtbase' and 'contour' even with the base line. The duration for the remaining 3 (xerces, protobuf, bincoin) looks good.

clang/lib/AST/ASTImporter.cpp

 //===- ASTImporter.cpp - Importing ASTs from other Contexts ---------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ASTImporter class which imports AST nodes from one
 //  context into another context.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclAccessPair.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclGroup.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/LambdaCapture.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/OperationKinds.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #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/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"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <memory>
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.
+// 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 = D->getDeclContext();
+  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;
 }
 
+// Returns true if the statement S has a parent declaration that has a
+// DeclContext that is inside (or equal to) DC. In a specific use case if DC is
+// a FunctionDecl, check if statement S resides in the body of the function.
+static bool isAncestorDeclContextOf(const DeclContext *DC, const Stmt *S) {
+  ParentMapContext &ParentC = DC->getParentASTContext().getParentMapContext();
+  DynTypedNodeList P = ParentC.getParents(*S);
+  while (!P.empty()) {
+    if (const Decl *PD = P.begin()->get<Decl>())
+      return isAncestorDeclContextOf(DC, PD);
+    P = ParentC.getParents(*P.begin());
+  }
+  return false;
+}
+
 static bool hasTypeDeclaredInsideFunction(QualType T, const FunctionDecl *FD) {
   if (T.isNull())
     return false;
+
+  auto CheckTemplateArgument = [FD](const TemplateArgument &Arg) {
+    if (Arg.getKind() == TemplateArgument::Type)
+      return hasTypeDeclaredInsideFunction(Arg.getAsType(), FD);
+    if (Arg.getKind() == TemplateArgument::Expression)
+      return isAncestorDeclContextOf(FD, Arg.getAsExpr());
+    return false;
+  };
+
   if (const auto *RecordT = T->getAs<RecordType>()) {
     const RecordDecl *RD = RecordT->getDecl();
     assert(RD);
     if (isAncestorDeclContextOf(FD, RD)) {
       assert(RD->getLexicalDeclContext() == RD->getDeclContext());
       return true;
     }
     if (const auto *RDTempl = dyn_cast<ClassTemplateSpecializationDecl>(RD))
-      return llvm::count_if(RDTempl->getTemplateArgs().asArray(),
-                            [FD](const TemplateArgument &Arg) {
-                              return hasTypeDeclaredInsideFunction(
-                                  Arg.getAsType(), FD);
-                            });
+      if (llvm::count_if(RDTempl->getTemplateArgs().asArray(),
+                         CheckTemplateArgument))
+        return true;
+    // Note: It is possible that T can be get as both a RecordType and a
+    // TemplateSpecializationType.
+  }
+  if (const auto *TST = T->getAs<TemplateSpecializationType>()) {
+    return llvm::count_if(TST->template_arguments(), CheckTemplateArgument);
   }
   return false;
 }
 
 bool ASTNodeImporter::hasAutoReturnTypeDeclaredInside(FunctionDecl *D) {
   QualType FromTy = D->getType();
   const auto *FromFPT = FromTy->getAs<FunctionProtoType>();
   assert(FromFPT && "Must be called on FunctionProtoType");

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase,
   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, ReturnWithTemplateWithIntegerArgDeclaredInside) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      template<int> struct Tmpl {};
+      auto foo() {
+        constexpr int X = 1;
+        return Tmpl<X>();
+      }
+      )",
+      Lang_CXX14, "input0.cc");
+  FunctionDecl *From = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("foo")));
+
+  FunctionDecl *To = Import(From, Lang_CXX14);
+  EXPECT_TRUE(To);
+  EXPECT_TRUE(isa<AutoType>(To->getReturnType()));
+}
+
 TEST_P(ImportAutoFunctions, ReturnWithTemplateWithStructDeclaredInside1) {
   Decl *FromTU = getTuDecl(
       R"(
       template<class> struct Tmpl {};
       auto foo() {
         struct X {};
         return Tmpl<X>();
       }