[clang] [Clang][Concepts] Fix the constraint equivalence checking involving parameter packs (PR #102131)

[Younan Zhang via cfe-commits]

https://github.com/zyn0217 updated https://github.com/llvm/llvm-project/pull/102131

>From aa99ac433c9d383bfca732c19e5082a555f64c2d Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Tue, 6 Aug 2024 20:08:43 +0800
Subject: [PATCH 1/4] [Clang][Concepts] Fix the constraint equivalence checking
 for TemplateTypeParmTypes

---
 clang/lib/Sema/SemaConcept.cpp                | 11 +++++++--
 .../SemaTemplate/concepts-out-of-line-def.cpp | 23 +++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index d4c9d044985e34..14a67f35a8f9f8 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -972,8 +972,15 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   // equivalence.
   LocalInstantiationScope ScopeForParameters(S);
   if (auto *FD = DeclInfo.getDecl()->getAsFunction())
-    for (auto *PVD : FD->parameters())
-      ScopeForParameters.InstantiatedLocal(PVD, PVD);
+    for (auto *PVD : FD->parameters()) {
+      if (!PVD->isParameterPack()) {
+        ScopeForParameters.InstantiatedLocal(PVD, PVD);
+        continue;
+      }
+      // Parameter packs should expand to a size-of-1 argument.
+      ScopeForParameters.MakeInstantiatedLocalArgPack(PVD);
+      ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD);
+    }
 
   std::optional<Sema::CXXThisScopeRAII> ThisScope;
 
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 0142efcdc3ee86..333187b0d74ad6 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -599,3 +599,26 @@ template <class DerT>
 unsigned long DerivedCollection<DerTs...>::index() {}
 
 } // namespace GH72557
+
+namespace GH101735 {
+
+template <class, class>
+concept True = true;
+
+template <typename T>
+class A {
+  template <typename... Ts>
+  void method(Ts&... ts)
+    requires requires (T t) {
+      { t.method(static_cast<Ts &&>(ts)...) } -> True<void>;
+    };
+};
+
+template <typename T>
+template <typename... Ts>
+void A<T>::method(Ts&... ts)
+  requires requires (T t) {
+    { t.method(static_cast<Ts &&>(ts)...) } -> True<void>;
+  } {}
+
+}

>From 339ab57452ff2998f9b1d13a33d2ef1620a2a67c Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Sun, 25 Aug 2024 10:32:30 +0800
Subject: [PATCH 2/4] Clarify the reason & add a FIXME

---
 clang/lib/Sema/SemaConcept.cpp | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 14a67f35a8f9f8..b83e0f094579d2 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -977,7 +977,22 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
         ScopeForParameters.InstantiatedLocal(PVD, PVD);
         continue;
       }
-      // Parameter packs should expand to a size-of-1 argument.
+      // This is hacky: we're mapping the parameter pack to a size-of-1 argument
+      // to avoid building SubstTemplateTypeParmPackTypes for
+      // PackExpansionTypes. The SubstTemplateTypeParmPackType node would
+      // otherwise reference the AssociatedDecl of the template arguments, which
+      // is, in this case, the template declaration.
+      //
+      // However, as we're also calculating the redeclarations of the template,
+      // the canonical declarations thereof are actually themselves at the
+      // moment. So if we didn't expand these packs, we would end up with an
+      // incorrect profile difference because we will be profiling the
+      // canonical types!
+      //
+      // FIXME: Improve the "no-transform" machinery in FindInstantiatedDecl so
+      // that we can eliminate the Scope in the cases where the declarations are
+      // not necessarily instantiated. It would also benefit the noexcept
+      // specifier comparison.
       ScopeForParameters.MakeInstantiatedLocalArgPack(PVD);
       ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD);
     }

>From 811bfc9011adf48bb22cce3e0ece0f7539bd095c Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Mon, 26 Aug 2024 13:47:59 +0800
Subject: [PATCH 3/4] Apply Corentin's feedback

Co-authored-by: cor3ntin <corentinjabot at gmail.com>
---
 clang/lib/Sema/SemaConcept.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index b83e0f094579d2..6e31cc6f87f2c5 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -983,8 +983,8 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
       // otherwise reference the AssociatedDecl of the template arguments, which
       // is, in this case, the template declaration.
       //
-      // However, as we're also calculating the redeclarations of the template,
-      // the canonical declarations thereof are actually themselves at the
+      // However, as we are in the process of comparing potential re-declarations,
+      // the canonical declaration is the declaration itself at this point.
       // moment. So if we didn't expand these packs, we would end up with an
       // incorrect profile difference because we will be profiling the
       // canonical types!

>From 908804f9e2cc6b367b7d9e5dd2ac4fa5c7aa7be7 Mon Sep 17 00:00:00 2001
From: Younan Zhang <zyn7109 at gmail.com>
Date: Mon, 26 Aug 2024 13:50:10 +0800
Subject: [PATCH 4/4] Fix comments

---
 clang/lib/Sema/SemaConcept.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 6e31cc6f87f2c5..206bfb870905b5 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -983,9 +983,9 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
       // otherwise reference the AssociatedDecl of the template arguments, which
       // is, in this case, the template declaration.
       //
-      // However, as we are in the process of comparing potential re-declarations,
-      // the canonical declaration is the declaration itself at this point.
-      // moment. So if we didn't expand these packs, we would end up with an
+      // However, as we are in the process of comparing potential
+      // re-declarations, the canonical declaration is the declaration itself at
+      // this point. So if we didn't expand these packs, we would end up with an
       // incorrect profile difference because we will be profiling the
       // canonical types!
       //