[clang] [Clang][Concepts] Normalize SizeOfPackExpr's pack declaration (PR #110238)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

<details>
<summary>Changes</summary>

SizeOfPackExpr has a pointer to the referenced pack declaration, which is left as-is during the transformation process.

The situation could be subtle when a friend class template declaration comes into play. The declaration per se would be instantiated into its parent declaration context, and consequently, the template parameter list would have a depth adjustment; however, as we don't evaluate constraints during instantiation, those constraints would still reference the original template parameters, which is fine for constraint evaluation because we have handled friend cases in the template argument collection.

However, things are different when we want to profile the constraint expression with dependent template arguments. The hash algorithm of SizeOfPackExpr takes its pack declaration as a factor, which is the original template parameter that might still have untransformed template depths after the constraint normalization.

This patch transforms the pack declaration when normalizing constraint expressions and pluses a fix in HandleFunctionTemplateDecl() where the associated declaration is incorrect for nested specifiers.

Note that the fix in HandleFunctionTemplateDecl(), as well as the handling logic for NestedNameSpecifier, would be removed once Krystian's refactoring patch lands. But I still want to incorporate it in the patch for the correction purpose, though it hasn't caused any problems so far - I just tripped over that in getFullyPackExpandedSize() when I tried to extract the transformed declarations from the TemplateArgument.

Fixes #<!-- -->93099 

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


5 Files Affected:

- (modified) clang/docs/ReleaseNotes.rst (+2) 
- (modified) clang/include/clang/AST/ExprCXX.h (+2) 
- (modified) clang/lib/Sema/SemaConcept.cpp (+9-5) 
- (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+27-1) 
- (modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+34) 


``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1fbcac807d0b30..831dab0657fcaf 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -444,6 +444,8 @@ Bug Fixes to C++ Support
 - Fixed an assertion failure in debug mode, and potential crashes in release mode, when
   diagnosing a failed cast caused indirectly by a failed implicit conversion to the type of the constructor parameter.
 - Fixed an assertion failure by adjusting integral to boolean vector conversions (#GH108326)
+- Fixed a bug in constraint expression comparison where the ``sizeof...`` expression was not handled properly
+  in certain friend declarations. (#GH93099)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 975bcdac5069b9..dfcf739ed1614f 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -4326,6 +4326,8 @@ class SizeOfPackExpr final
   /// Retrieve the parameter pack.
   NamedDecl *getPack() const { return Pack; }
 
+  void setPack(NamedDecl *NewPack) { Pack = NewPack; }
+
   /// Retrieve the length of the parameter pack.
   ///
   /// This routine may only be invoked when the expression is not
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 6a1b32598bb4a6..67fc603e9ce1d5 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -975,11 +975,14 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   // parameters that the surrounding function hasn't been instantiated yet. Note
   // this may happen while we're comparing two templates' constraint
   // equivalence.
-  LocalInstantiationScope ScopeForParameters(S);
-  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
+  std::optional<LocalInstantiationScope> ScopeForParameters;
+  if (const NamedDecl *ND = DeclInfo.getDecl();
+      ND && ND->isFunctionOrFunctionTemplate()) {
+    ScopeForParameters.emplace(S);
+    const FunctionDecl *FD = ND->getAsFunction();
     for (auto *PVD : FD->parameters()) {
       if (!PVD->isParameterPack()) {
-        ScopeForParameters.InstantiatedLocal(PVD, PVD);
+        ScopeForParameters->InstantiatedLocal(PVD, PVD);
         continue;
       }
       // This is hacky: we're mapping the parameter pack to a size-of-1 argument
@@ -998,9 +1001,10 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
       // 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);
+      ScopeForParameters->MakeInstantiatedLocalArgPack(PVD);
+      ScopeForParameters->InstantiatedLocalPackArg(PVD, PVD);
     }
+  }
 
   std::optional<Sema::CXXThisScopeRAII> ThisScope;
 
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fd51fa4afcacbf..20215d32539af9 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -371,7 +371,7 @@ Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD,
                   Specialization->getTemplateInstantiationArgs().asArray();
           }
           Result.addOuterTemplateArguments(
-              const_cast<FunctionTemplateDecl *>(FTD), Arguments,
+              TSTy->getTemplateName().getAsTemplateDecl(), Arguments,
               /*Final=*/false);
         }
       }
@@ -1722,6 +1722,32 @@ namespace {
       return inherited::TransformLambdaBody(E, Body);
     }
 
+    ExprResult TransformSizeOfPackExpr(SizeOfPackExpr *E) {
+      ExprResult Result = inherited::TransformSizeOfPackExpr(E);
+
+      if (SemaRef.CodeSynthesisContexts.back().Kind !=
+          Sema::CodeSynthesisContext::ConstraintNormalization)
+        return Result;
+
+      if (!Result.isUsable())
+        return Result;
+
+      SizeOfPackExpr *NewExpr = cast<SizeOfPackExpr>(Result.get());
+#ifndef NDEBUG
+      for (auto *Iter = TemplateArgs.begin(); Iter != TemplateArgs.end();
+           ++Iter)
+        for (const TemplateArgument &TA : Iter->Args)
+          assert(TA.getKind() != TemplateArgument::Pack || TA.pack_size() == 1);
+#endif
+      Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(SemaRef, 0);
+      Decl *NewDecl = TransformDecl(NewExpr->getPackLoc(), NewExpr->getPack());
+      if (!NewDecl)
+        return ExprError();
+
+      NewExpr->setPack(cast<NamedDecl>(NewDecl));
+      return NewExpr;
+    }
+
     ExprResult TransformRequiresExpr(RequiresExpr *E) {
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       ExprResult TransReq = inherited::TransformRequiresExpr(E);
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 5450d105a6f54a..8ca399a0f729a9 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -666,3 +666,37 @@ int foo() {
 }
 
 } // namespace eve
+
+namespace GH93099 {
+
+// Issues with sizeof...(expr)
+
+template <typename T = int> struct C {
+  template <int... N>
+    requires(sizeof...(N) > 0)
+  friend class NTTP;
+
+  template <class... Tp>
+    requires(sizeof...(Tp) > 0)
+  friend class TP;
+
+  template <template <typename> class... TTp>
+    requires(sizeof...(TTp) > 0)
+  friend class TTP;
+};
+
+template <int... N>
+  requires(sizeof...(N) > 0)
+class NTTP;
+
+template <class... Tp>
+  requires(sizeof...(Tp) > 0)
+class TP;
+
+template <template <typename> class... TTp>
+  requires(sizeof...(TTp) > 0)
+class TTP;
+
+C v;
+
+} // namespace GH93099

``````````

</details>


https://github.com/llvm/llvm-project/pull/110238