[Clang][Sema] Revisit the fix for the lambda within a type alias template decl #89934

zyn0217 · 2024-04-24T14:19:43Z

In the last patch #82310, we used template depths to tell if such alias decls contain lambdas, which is wrong because the lambda can also appear as a part of the default argument, and that would make getTemplateInstantiationArgs provide extra template arguments in undesired contexts. This leads to issue #89853.

Moreover, our approach for #82104 was sadly wrong. We tried to teach DeduceReturnType to consider alias template arguments; however, giving these arguments in the context where they should have been substituted in a TransformCallExpr call is never correct.

This patch addresses such problems by using a RecursiveASTVisitor to check if the lambda is contained by an alias Decl, as well as twiddling the lambda dependencies - we should also build a dependent lambda expression if the surrounding alias template arguments were dependent.

Fixes #89853
Fixes #102760
Fixes #105885

zyn0217 · 2024-04-24T14:37:29Z

I didn't add a release note because the previous patch has not been released yet.

13steinj · 2024-04-24T17:09:41Z

lambda can also appear as a part of the default argument, and that would make getTemplateInstantiationArgs provide extra template arguments in undesired contexts. This leads to issue #89853.

FYI in my issue, even if the lambda didn't appear as part of the default argument, I was still able to trigger the bug. Noting in case this fix only resolves the case of lambdas in default template arguments, and not passed in explicitly.

e.g.

static constexpr auto not_default_now = []<const char c> {
    (void) static_cast<char>(c);
};

template<auto Pred>
using broken = decltype(Pred.template operator()<'\0'>());

broken<not_default_now>* boom;

zyn0217 · 2024-04-25T02:03:34Z

FYI in my issue, even if the lambda didn't appear as part of the default argument, I was still able to trigger the bug. Noting in case this fix only resolves the case of lambdas in default template arguments, and not passed in explicitly.

e.g.
static constexpr auto not_default_now = []<const char c> {
    (void) static_cast<char>(c);
};

template<auto Pred>
using broken = decltype(Pred.template operator()<'\0'>());

broken<not_default_now>* boom;

Yeah, this patch fixes that as well. To clarify, the issue was caused by having extra template arguments in the context where we don't actually expect them. For example, the problem arose from the argument for Pred when it was substituted before we exercise DeduceReturnType for the lambda not_default_now. This patch now avoids that fault and only offers complete arguments relative to the primary template while checking constraints - this matches what our constraint checking expects.

zyn0217 · 2024-04-25T02:50:05Z

clang/test/SemaTemplate/alias-template-with-lambdas.cpp

+// FIXME: This still crashes because we can't extract template arguments T and U
+// outside of the instantiation context of T16.
+#if 0
+template <typename T, typename... U>
+using T16 = decltype([](auto Param) requires (sizeof(Param) != 1 && sizeof...(U) > 0) {
+  return Value<T, U...> + sizeof(Param);
+});
+static_assert(T16<int, char, float>()(42) == 2 + sizeof(42));
+#endif


I have to admit we still have lots of edge cases that our current approach (i.e. by extracting template arguments from the type alias instantiation context) doesn't cover. However, I think we can still land this patch (because it addresses the regressions and covers a number of cases) and probably come up with another solution later.

Perhaps, I think one promising approach is to change the TypeAliasTemplateDecl to a kind of DeclContext and thereby we can invent something e.g. TypeAliasTemplateSpecializationDecl so that they can be handled like what we do for member function templates.

Maybe, or something along those lines.
(Or maybe we need to push some sort of state when parsing / transforming an alias and have a more generic way to get at template parameter)

It turns out to be an evaluation context that eventually becomes the lambda context declaration.
Given that Matheus is going to refactor these codes shortly in the future, and we have not been correctly compiling this case for a long time (https://gcc.godbolt.org/z/sbYT1Pa91), I think I can delegate it to @mizvekov, and hopefully, he could fix the test here.

This test case passes with my current WIP contextdecl patch.

By the way, Value needs to be constexpr in order for that static_assert to work.

llvmbot · 2024-04-25T02:51:11Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

In the last patch #82310, we used template depths to tell if such alias decls contain lambdas, which is wrong because the lambda can also appear as a part of the default argument, and that would make getTemplateInstantiationArgs provide extra template arguments in undesired contexts. This leads to issue #89853.

Moreover, our approach for #82104 was sadly wrong. We tried to teach DeduceReturnType to consider alias template arguments; however, giving these arguments in the context where they should have been substituted in a TransformCallExpr call is never correct.

This patch addresses such problems by using a RecursiveASTVisitor to check if the lambda is contained by an alias Decl, as well as twiddling the lambda dependencies - we should also build a dependent lambda expression if the surrounding alias template arguments were dependent.

Fixes #89853

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

2 Files Affected:

(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+38-35)
(modified) clang/test/SemaTemplate/alias-template-with-lambdas.cpp (+48-3)

diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 98d5c7cb3a8a80..8b4d841fab46cf 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprConcepts.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeVisitor.h"
@@ -87,12 +88,19 @@ struct Response {
 // than lambda classes.
 const FunctionDecl *
 getPrimaryTemplateOfGenericLambda(const FunctionDecl *LambdaCallOperator) {
+  if (!isLambdaCallOperator(LambdaCallOperator))
+    return LambdaCallOperator;
   while (true) {
     if (auto *FTD = dyn_cast_if_present<FunctionTemplateDecl>(
             LambdaCallOperator->getDescribedTemplate());
         FTD && FTD->getInstantiatedFromMemberTemplate()) {
       LambdaCallOperator =
           FTD->getInstantiatedFromMemberTemplate()->getTemplatedDecl();
+    } else if (LambdaCallOperator->getPrimaryTemplate()) {
+      // Cases where the lambda operator is instantiated in
+      // TemplateDeclInstantiator::VisitCXXMethodDecl.
+      LambdaCallOperator =
+          LambdaCallOperator->getPrimaryTemplate()->getTemplatedDecl();
     } else if (auto *Prev = cast<CXXMethodDecl>(LambdaCallOperator)
                                 ->getInstantiatedFromMemberFunction())
       LambdaCallOperator = Prev;
@@ -138,22 +146,28 @@ getEnclosingTypeAliasTemplateDecl(Sema &SemaRef) {
 // Check if we are currently inside of a lambda expression that is
 // surrounded by a using alias declaration. e.g.
 //   template <class> using type = decltype([](auto) { ^ }());
-// By checking if:
-//  1. The lambda expression and the using alias declaration share the
-//  same declaration context.
-//  2. They have the same template depth.
 // We have to do so since a TypeAliasTemplateDecl (or a TypeAliasDecl) is never
 // a DeclContext, nor does it have an associated specialization Decl from which
 // we could collect these template arguments.
 bool isLambdaEnclosedByTypeAliasDecl(
-    const FunctionDecl *PrimaryLambdaCallOperator,
+    const FunctionDecl *LambdaCallOperator,
     const TypeAliasTemplateDecl *PrimaryTypeAliasDecl) {
-  return cast<CXXRecordDecl>(PrimaryLambdaCallOperator->getDeclContext())
-                 ->getTemplateDepth() ==
-             PrimaryTypeAliasDecl->getTemplateDepth() &&
-         getLambdaAwareParentOfDeclContext(
-             const_cast<FunctionDecl *>(PrimaryLambdaCallOperator)) ==
-             PrimaryTypeAliasDecl->getDeclContext();
+  struct Visitor : RecursiveASTVisitor<Visitor> {
+    Visitor(const FunctionDecl *CallOperator) : CallOperator(CallOperator) {}
+    bool VisitLambdaExpr(const LambdaExpr *LE) {
+      // Return true to bail out of the traversal, implying the Decl contains
+      // the lambda.
+      return getPrimaryTemplateOfGenericLambda(LE->getCallOperator()) !=
+             CallOperator;
+    }
+    const FunctionDecl *CallOperator;
+  };
+
+  QualType Underlying =
+      PrimaryTypeAliasDecl->getTemplatedDecl()->getUnderlyingType();
+
+  return !Visitor(getPrimaryTemplateOfGenericLambda(LambdaCallOperator))
+              .TraverseType(Underlying);
 }
 
 // Add template arguments from a variable template instantiation.
@@ -283,23 +297,8 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function,
 
     // If this function is a generic lambda specialization, we are done.
     if (!ForConstraintInstantiation &&
-        isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function)) {
-      // TypeAliasTemplateDecls should be taken into account, e.g.
-      // when we're deducing the return type of a lambda.
-      //
-      // template <class> int Value = 0;
-      // template <class T>
-      // using T = decltype([]<int U = 0>() { return Value<T>; }());
-      //
-      if (auto TypeAlias = getEnclosingTypeAliasTemplateDecl(SemaRef)) {
-        if (isLambdaEnclosedByTypeAliasDecl(
-                /*PrimaryLambdaCallOperator=*/getPrimaryTemplateOfGenericLambda(
-                    Function),
-                /*PrimaryTypeAliasDecl=*/TypeAlias.PrimaryTypeAliasDecl))
-          return Response::UseNextDecl(Function);
-      }
+        isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function))
       return Response::Done();
-    }
 
   } else if (Function->getDescribedFunctionTemplate()) {
     assert(
@@ -411,10 +410,9 @@ Response HandleRecordDecl(Sema &SemaRef, const CXXRecordDecl *Rec,
     // Retrieve the template arguments for a using alias declaration.
     // This is necessary for constraint checking, since we always keep
     // constraints relative to the primary template.
-    if (auto TypeAlias = getEnclosingTypeAliasTemplateDecl(SemaRef)) {
-      const FunctionDecl *PrimaryLambdaCallOperator =
-          getPrimaryTemplateOfGenericLambda(Rec->getLambdaCallOperator());
-      if (isLambdaEnclosedByTypeAliasDecl(PrimaryLambdaCallOperator,
+    if (auto TypeAlias = getEnclosingTypeAliasTemplateDecl(SemaRef);
+        ForConstraintInstantiation && TypeAlias) {
+      if (isLambdaEnclosedByTypeAliasDecl(Rec->getLambdaCallOperator(),
                                           TypeAlias.PrimaryTypeAliasDecl)) {
         Result.addOuterTemplateArguments(TypeAlias.Template,
                                          TypeAlias.AssociatedTemplateArguments,
@@ -1670,12 +1668,17 @@ namespace {
 
     CXXRecordDecl::LambdaDependencyKind
     ComputeLambdaDependency(LambdaScopeInfo *LSI) {
-      auto &CCS = SemaRef.CodeSynthesisContexts.back();
-      if (CCS.Kind ==
-          Sema::CodeSynthesisContext::TypeAliasTemplateInstantiation) {
-        unsigned TypeAliasDeclDepth = CCS.Entity->getTemplateDepth();
+      if (auto TypeAlias =
+              TemplateInstArgsHelpers::getEnclosingTypeAliasTemplateDecl(
+                  getSema());
+          TypeAlias && TemplateInstArgsHelpers::isLambdaEnclosedByTypeAliasDecl(
+                           LSI->CallOperator, TypeAlias.PrimaryTypeAliasDecl)) {
+        unsigned TypeAliasDeclDepth = TypeAlias.Template->getTemplateDepth();
         if (TypeAliasDeclDepth >= TemplateArgs.getNumSubstitutedLevels())
           return CXXRecordDecl::LambdaDependencyKind::LDK_AlwaysDependent;
+        for (const TemplateArgument &TA : TypeAlias.AssociatedTemplateArguments)
+          if (TA.isDependent())
+            return CXXRecordDecl::LambdaDependencyKind::LDK_AlwaysDependent;
       }
       return inherited::ComputeLambdaDependency(LSI);
     }
diff --git a/clang/test/SemaTemplate/alias-template-with-lambdas.cpp b/clang/test/SemaTemplate/alias-template-with-lambdas.cpp
index ff94031e4d86f1..032ad2cf6a5ad2 100644
--- a/clang/test/SemaTemplate/alias-template-with-lambdas.cpp
+++ b/clang/test/SemaTemplate/alias-template-with-lambdas.cpp
@@ -91,15 +91,60 @@ void bar() {
 
 namespace GH82104 {
 
-template <typename, typename...> int Zero = 0;
+template <typename, typename... D> int Value = sizeof...(D);
 
-template <typename T, typename...U>
-using T14 = decltype([]<int V = 0>() { return Zero<T, U...>; }());
+template <typename T, typename... U>
+using T14 = decltype([]<int V = 0>(auto Param) {
+  return Value<T, U...> + V + (int)sizeof(Param);
+}("hello"));
 
 template <typename T> using T15 = T14<T, T>;
 
 static_assert(__is_same(T15<char>, int));
 
+// FIXME: This still crashes because we can't extract template arguments T and U
+// outside of the instantiation context of T16.
+#if 0
+template <typename T, typename... U>
+using T16 = decltype([](auto Param) requires (sizeof(Param) != 1 && sizeof...(U) > 0) {
+  return Value<T, U...> + sizeof(Param);
+});
+static_assert(T16<int, char, float>()(42) == 2 + sizeof(42));
+#endif
 } // namespace GH82104
 
+namespace GH89853 {
+
+template <typename = void>
+static constexpr auto innocuous = []<int m> { return m; };
+
+template <auto Pred = innocuous<>>
+using broken = decltype(Pred.template operator()<42>());
+
+broken<> *boom;
+
+template <auto Pred =
+              []<char c> {
+                (void)static_cast<char>(c);
+              }>
+using broken2 = decltype(Pred.template operator()<42>());
+
+broken2<> *boom2;
+
+template <auto Pred = []<char m> { return m; }>
+using broken3 = decltype(Pred.template operator()<42>());
+
+broken3<> *boom3;
+
+static constexpr auto non_default = []<char c>(True auto) {
+    (void) static_cast<char>(c);
+};
+
+template<True auto Pred>
+using broken4 = decltype(Pred.template operator()<42>(Pred));
+
+broken4<non_default>* boom4;
+
+}
+
 } // namespace lambda_calls

zyn0217 · 2024-05-08T01:48:04Z

Gently ping @cor3ntin

cor3ntin · 2024-08-26T04:50:04Z

I think this is reasonable. We are missing a changelog entry though

zyn0217 · 2024-08-26T05:16:09Z

We are missing a changelog entry though

No release entry because this was intended to be a fix for 19 regression and we now want to backport it :)

cor3ntin · 2024-08-26T10:45:08Z

can you Add the #105885 test?
Thanks

cor3ntin

LGTM (for the time being, I don't think this is workable long term)

mizvekov

I agree this is not the long term way forward, but it improves the current interim solution, so LGTM, minus nit.

mizvekov · 2024-08-26T18:13:19Z

clang/test/SemaTemplate/alias-template-with-lambdas.cpp

+// FIXME: This still crashes because we can't extract template arguments T and U
+// outside of the instantiation context of T16.
+#if 0
+template <typename T, typename... U>
+using T16 = decltype([](auto Param) requires (sizeof(Param) != 1 && sizeof...(U) > 0) {
+  return Value<T, U...> + sizeof(Param);
+});
+static_assert(T16<int, char, float>()(42) == 2 + sizeof(42));
+#endif


This test case passes with my current WIP contextdecl patch.

By the way, Value needs to be constexpr in order for that static_assert to work.

clang/test/SemaTemplate/alias-template-with-lambdas.cpp

zyn0217 · 2024-08-27T01:26:34Z

/cherry-pick b412ec5

…late decl (llvm#89934) In the last patch llvm#82310, we used template depths to tell if such alias decls contain lambdas, which is wrong because the lambda can also appear as a part of the default argument, and that would make `getTemplateInstantiationArgs` provide extra template arguments in undesired contexts. This leads to issue llvm#89853. Moreover, our approach for llvm#82104 was sadly wrong. We tried to teach `DeduceReturnType` to consider alias template arguments; however, giving these arguments in the context where they should have been substituted in a `TransformCallExpr` call is never correct. This patch addresses such problems by using a `RecursiveASTVisitor` to check if the lambda is contained by an alias `Decl`, as well as twiddling the lambda dependencies - we should also build a dependent lambda expression if the surrounding alias template arguments were dependent. Fixes llvm#89853 Fixes llvm#102760 Fixes llvm#105885 (cherry picked from commit b412ec5)

llvmbot · 2024-08-27T01:31:45Z

/pull-request #106166

…late decl (llvm#89934) In the last patch llvm#82310, we used template depths to tell if such alias decls contain lambdas, which is wrong because the lambda can also appear as a part of the default argument, and that would make `getTemplateInstantiationArgs` provide extra template arguments in undesired contexts. This leads to issue llvm#89853. Moreover, our approach for llvm#82104 was sadly wrong. We tried to teach `DeduceReturnType` to consider alias template arguments; however, giving these arguments in the context where they should have been substituted in a `TransformCallExpr` call is never correct. This patch addresses such problems by using a `RecursiveASTVisitor` to check if the lambda is contained by an alias `Decl`, as well as twiddling the lambda dependencies - we should also build a dependent lambda expression if the surrounding alias template arguments were dependent. Fixes llvm#89853 Fixes llvm#102760 Fixes llvm#105885 (cherry picked from commit b412ec5)

zyn0217 force-pushed the GH89853 branch from 670c4a4 to 92f8cfb Compare April 24, 2024 14:32

zyn0217 commented Apr 25, 2024

View reviewed changes

zyn0217 requested review from cor3ntin and erichkeane April 25, 2024 02:50

zyn0217 marked this pull request as ready for review April 25, 2024 02:50

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 25, 2024

[Clang][Sema] Revisit the lambda within a type alias template decl

9eb7a15

This was referenced Aug 20, 2024

FunctionDecl::getParamDecl: Assertion i < getNumParams() && "Illegal param #" failed #102760

Closed

Alias template with NTTP crashes if it has an unevaluated generic lambda taking auto... #105885

Closed

cor3ntin requested a review from mizvekov August 24, 2024 08:49

zyn0217 added 2 commits August 26, 2024 22:04

Merge branch 'main' into GH89853

68186d4

Add tests for GH105885

4462adb

zyn0217 force-pushed the GH89853 branch from c2342fe to 4462adb Compare August 26, 2024 14:19

Add tests for GH102760

e8a7c4d

cor3ntin approved these changes Aug 26, 2024

View reviewed changes

mizvekov approved these changes Aug 26, 2024

View reviewed changes

constexpr

2a5dc0a

zyn0217 merged commit b412ec5 into llvm:main Aug 27, 2024
3 of 5 checks passed

zyn0217 deleted the GH89853 branch August 27, 2024 01:26

zyn0217 added this to the LLVM 19.X Release milestone Aug 27, 2024

thewtex mentioned this pull request Feb 10, 2025

llvmorg 19.1.5 libcxxabi pthread lib name #126605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Revisit the fix for the lambda within a type alias template decl #89934

[Clang][Sema] Revisit the fix for the lambda within a type alias template decl #89934

zyn0217 commented Apr 24, 2024 •

edited

Loading

zyn0217 commented Apr 24, 2024

13steinj commented Apr 24, 2024

zyn0217 commented Apr 25, 2024

zyn0217 Apr 25, 2024

cor3ntin Aug 26, 2024

zyn0217 Aug 26, 2024

mizvekov Aug 26, 2024

llvmbot commented Apr 25, 2024

zyn0217 commented May 8, 2024

cor3ntin commented Aug 26, 2024

zyn0217 commented Aug 26, 2024

cor3ntin commented Aug 26, 2024

cor3ntin left a comment

mizvekov left a comment

mizvekov Aug 26, 2024

zyn0217 commented Aug 27, 2024

llvmbot commented Aug 27, 2024

[Clang][Sema] Revisit the fix for the lambda within a type alias template decl #89934

[Clang][Sema] Revisit the fix for the lambda within a type alias template decl #89934

Conversation

zyn0217 commented Apr 24, 2024 • edited Loading

zyn0217 commented Apr 24, 2024

13steinj commented Apr 24, 2024

zyn0217 commented Apr 25, 2024

zyn0217 Apr 25, 2024

Choose a reason for hiding this comment

cor3ntin Aug 26, 2024

Choose a reason for hiding this comment

zyn0217 Aug 26, 2024

Choose a reason for hiding this comment

mizvekov Aug 26, 2024

Choose a reason for hiding this comment

llvmbot commented Apr 25, 2024

zyn0217 commented May 8, 2024

cor3ntin commented Aug 26, 2024

zyn0217 commented Aug 26, 2024

cor3ntin commented Aug 26, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

mizvekov left a comment

Choose a reason for hiding this comment

mizvekov Aug 26, 2024

Choose a reason for hiding this comment

zyn0217 commented Aug 27, 2024

llvmbot commented Aug 27, 2024

zyn0217 commented Apr 24, 2024 •

edited

Loading