Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang] Fix the instantiation of return type requirements in lambda bodies #76967

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

LYP951018
Copy link
Contributor

@LYP951018 LYP951018 commented Jan 4, 2024

Currently, due to the incomplete implementation of p0588r1, the instantiation of lambda expressions leads to the instantiation of the body. And EvaluateConstraints is false during the instantiation of the body, which causes crashes during the instantiation of the return type requirement:

template<typename T> concept doesnt_matter = true;

template<class T>
concept test = 
    []{
        return requires(T t) {
            { t } -> doesnt_matter; // crash
        };
    }();

static_assert(test<int>);

Although a complete implementation of p0588r1 can solve these crashes, it will take some time. Therefore, this pull request aims to fix these crashes first.

Fixes #63808
Fixes #64607
Fixes #64086

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 4, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-clang

Author: 刘雨培 (LYP951018)

Changes

Currently, due to the incomplete implementation of p0588r1, the instantiation of lambda expressions leads to the instantiation of the body. And EvaluateConstraints is false during the instantiation of the body, which causes crashes during the instantiation of the return type requirement:

template&lt;typename T&gt; concept doesnt_matter = true;

template&lt;class T&gt;
concept test = 
    []{
        return requires(T t) {
            { t } -&gt; doesnt_matter; // crash
        };
    }();

static_assert(test&lt;int&gt;);

Although a complete implementation of p0588r1 can solve these crashes, it will take some time. Therefore, this pull request aims to fix these crashes first.

Fixes #63808, #64607, #64086


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+8)
  • (modified) clang/test/SemaTemplate/concepts-lambda.cpp (+33)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 30cfe66703f5a9..1815e8d69332e2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -843,6 +843,9 @@ Bug Fixes to C++ Support
 - Fix crash when parsing nested requirement. Fixes:
   (`#73112 <https://github.com/llvm/llvm-project/issues/73112>`_)
 
+- Fixed a crash caused by using return type requirement in a lambda. Fixes:
+  (`#64607 <https://github.com/llvm/llvm-project/issues/64607>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 37e5b9cad08bc9..bbaead33a6efe1 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1479,6 +1479,14 @@ namespace {
       return Result;
     }
 
+    StmtResult TransformLambdaBody(LambdaExpr *E, Stmt *Body) {
+      bool Prev = EvaluateConstraints;
+      EvaluateConstraints = true;
+      StmtResult Stmt = inherited::TransformLambdaBody(E, Body);
+      EvaluateConstraints = Prev;
+      return Stmt;
+    }
+
     ExprResult TransformRequiresExpr(RequiresExpr *E) {
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       ExprResult TransReq = inherited::TransformRequiresExpr(E);
diff --git a/clang/test/SemaTemplate/concepts-lambda.cpp b/clang/test/SemaTemplate/concepts-lambda.cpp
index 8a184cbf4e9bc2..0a58137055a381 100644
--- a/clang/test/SemaTemplate/concepts-lambda.cpp
+++ b/clang/test/SemaTemplate/concepts-lambda.cpp
@@ -116,3 +116,36 @@ static_assert(E<int>);
 // expected-note@-11{{because 'Q.template operator()<float>()' would be invalid: no matching member function for call to 'operator()'}}
 }
 }
+
+namespace ReturnTypeConstraintInLambda {
+template <typename T>
+concept C1 = true;
+
+template <class T>
+concept test = [] {
+  return requires(T t) {
+    { t } -> C1;
+  };
+}();
+
+static_assert(test<int>);
+
+template <typename T>
+concept C2 = true;
+struct S1 {
+  int f1() { return 1; }
+};
+
+void foo() {
+  auto make_caller = []<auto member> {
+    return [](S1 *ps) {
+      if constexpr (requires {
+                      { (ps->*member)() } -> C2;
+                    })
+        ;
+    };
+  };
+
+  auto caller = make_caller.operator()<&S1::f1>();
+}
+} // namespace ReturnTypeConstraintInLambda

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a not-awful way of solving this until we stop instantiating the body of the lambda. Could you please comment to that effect in TransformLambdaBody?

Also would like @cor3ntin to take a look as well, else i'm ok with this.

clang/docs/ReleaseNotes.rst Show resolved Hide resolved
@erichkeane
Copy link
Collaborator

I'm still ok with it as-is, but would like @cor3ntin to take a run/do the approval.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I do hope that @erichkeane will be able to work on delayed instantiation though!

@LYP951018 LYP951018 merged commit e78a1f4 into llvm:main Jan 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
4 participants