release/19.x: [Clang][Concepts] Fix the constraint equivalence checking involving parameter packs (#102131)

[llvmbot]

Backport e6974da

Requested by: @zyn0217

[llvmbot]

@zyn0217 What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (llvmbot)

Changes

Backport e6974da

Requested by: @zyn0217

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaConcept.cpp (+24-2)
• (modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+23)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index c34d32002b5ad7..244f6ef2f53faa 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -969,8 +969,30 @@ 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;
+      }
+      // 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 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!
+      //
+      // 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);
+    }
 
   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>;
+  } {}
+
+}

[tru]

Is this a major fix or regression fix?

[zyn0217]

Is this a major fix or regression fix?

It is a follow-up fix for the previous bug fix incorporated in 19. 04d20b1

But this isn't a regression since clang wasn't working properly in such cases, nor a major fix as I don't think it would affect much but rather ensure more correctness.

We have a report from @yuxuanchen1997 that some production codes rely on this to compile, so I think it merits a backport.

[tru]

Can this be reviewed @cor3ntin @mizvekov

[cor3ntin]

How about we deploy that in 19.0.2? It seems like the safest course of action at this point.

[tru]

Can someone approve this and I'll merge it after final.

[github-actions]

@zyn0217 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.