[clang] [Clang] Remove the wrong assumption when rebuilding SizeOfPackExprs for constraint normalization (PR #115120)

[Younan Zhang via cfe-commits]

================
@@ -1736,23 +1736,13 @@ namespace {
                                      SourceLocation RParenLoc,
                                      std::optional<unsigned> Length,
                                      ArrayRef<TemplateArgument> PartialArgs) {
-      if (SemaRef.CodeSynthesisContexts.back().Kind !=
-          Sema::CodeSynthesisContext::ConstraintNormalization)
-        return inherited::RebuildSizeOfPackExpr(OperatorLoc, Pack, PackLoc,
-                                                RParenLoc, Length, PartialArgs);
-
-#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, /*NewSubstitutionIndex=*/0);
-      Decl *NewPack = TransformDecl(PackLoc, Pack);
-      if (!NewPack)
-        return ExprError();
-
+      Decl *NewPack = Pack;
+      if (SemaRef.CodeSynthesisContexts.back().Kind ==
+          Sema::CodeSynthesisContext::ConstraintNormalization) {
+        NewPack = TransformDecl(PackLoc, Pack);
+        if (!NewPack)
+          return ExprError();
+      }
       return inherited::RebuildSizeOfPackExpr(OperatorLoc,
                                               cast<NamedDecl>(NewPack), PackLoc,
                                               RParenLoc, Length, PartialArgs);
----------------
zyn0217 wrote:

> Also, is the ConstraintNormalization check necessary for correctness, or is this done for performance reasons?

I think it's for both.

The default version of `sizeof...` transformation doesn't handle the Pack declaration unless `TryExpandParameterPacks` decides that there wouldn't be a pack expansion, which isn't the case for the normalization, where we (almost always? we `getTemplateInstantiationArgs()`'ed these template arguments with `ForConstraintInstantiation=true`) have a corresponding template argument pack for template parameters.

However, I think it's *never wrong* to transform the declarations as we transform/expand other parts of such expressions. AFAICT, in our codebase, there's no reliance on the declaration being untransformed. That said, doing that might partway hurt performance, given that we have preserved the untransformed declaration for such a long time. So I added a context check to avoid performance issues. Moreover, it's hard to tell if other out-of-tree customers are relying on the untransformed declarations, so a conservative strategy is to apply it only for normalizations.

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