[clang] Improve diagnostics for expansion length mismatch

When checking parameter packs for expansion, instead of basing the diagnostic for
length mismatch for outer parameters only on the known number of expansions,
we should also analyze SubstTemplateTypeParmPackType and SubstNonTypeTemplateParmPackExpr
for unexpanded packs, so we can emit a diagnostic pointing to a concrete
outer parameter.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Differential Revision: https://reviews.llvm.org/D128095

clang/docs/ReleaseNotes.rst

@@ -118,6 +118,8 @@ Improvements to Clang's diagnostics
 - Correctly diagnose a future keyword if it exist as a keyword in the higher
   language version and specifies in which version it will be a keyword. This
   supports both c and c++ language.
+- When diagnosing multi-level pack expansions of mismatched lengths, Clang will
+  now, in most cases, be able to point to the relevant outer parameter.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/include/clang/Sema/Sema.h

@@ -238,8 +238,11 @@ namespace threadSafety {
 
 // FIXME: No way to easily map from TemplateTypeParmTypes to
 // TemplateTypeParmDecls, so we have this horrible PointerUnion.
-typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType*, NamedDecl*>,
+using UnexpandedParameterPack = std::pair<
-                  SourceLocation> UnexpandedParameterPack;
+    llvm::PointerUnion<
+        const TemplateTypeParmType *, const SubstTemplateTypeParmPackType *,
+        const SubstNonTypeTemplateParmPackExpr *, const NamedDecl *>,
+    SourceLocation>;
 
 /// Describes whether we've seen any nullability information for the given
 /// file.

clang/include/clang/Sema/SemaInternal.h

@@ -62,7 +62,7 @@ inline InheritableAttr *getDLLAttr(Decl *D) {
 }
 
 /// Retrieve the depth and index of a template parameter.
-inline std::pair<unsigned, unsigned> getDepthAndIndex(NamedDecl *ND) {
+inline std::pair<unsigned, unsigned> getDepthAndIndex(const NamedDecl *ND) {
   if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(ND))
     return std::make_pair(TTP->getDepth(), TTP->getIndex());
 
@@ -79,7 +79,7 @@ getDepthAndIndex(UnexpandedParameterPack UPP) {
   if (const auto *TTP = UPP.first.dyn_cast<const TemplateTypeParmType *>())
     return std::make_pair(TTP->getDepth(), TTP->getIndex());
 
-  return getDepthAndIndex(UPP.first.get<NamedDecl *>());
+  return getDepthAndIndex(UPP.first.get<const NamedDecl *>());
 }
 
 class TypoCorrectionConsumer : public VisibleDeclConsumer {

clang/lib/Sema/SemaTemplateDeduction.cpp

@@ -738,8 +738,11 @@ private:
       SmallVector<UnexpandedParameterPack, 2> Unexpanded;
       S.collectUnexpandedParameterPacks(Pattern, Unexpanded);
       for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) {
-        unsigned Depth, Index;
+        UnexpandedParameterPack U = Unexpanded[I];
-        std::tie(Depth, Index) = getDepthAndIndex(Unexpanded[I]);
+        if (U.first.is<const SubstTemplateTypeParmPackType *>() ||
+            U.first.is<const SubstNonTypeTemplateParmPackExpr *>())
+          continue;
+        auto [Depth, Index] = getDepthAndIndex(U);
         if (Depth == Info.getDeducedDepth())
           AddPack(Index);
       }

clang/lib/Sema/SemaTemplateVariadic.cpp

@@ -88,6 +88,23 @@ namespace {
       return true;
     }
 
+    bool
+    VisitSubstTemplateTypeParmPackTypeLoc(SubstTemplateTypeParmPackTypeLoc TL) {
+      Unexpanded.push_back({TL.getTypePtr(), TL.getNameLoc()});
+      return true;
+    }
+
+    bool VisitSubstTemplateTypeParmPackType(SubstTemplateTypeParmPackType *T) {
+      Unexpanded.push_back({T, SourceLocation()});
+      return true;
+    }
+
+    bool
+    VisitSubstNonTypeTemplateParmPackExpr(SubstNonTypeTemplateParmPackExpr *E) {
+      Unexpanded.push_back({E, E->getParameterPackLocation()});
+      return true;
+    }
+
     /// Record occurrences of function and non-type template
     /// parameter packs in an expression.
     bool VisitDeclRefExpr(DeclRefExpr *E) {
@@ -306,7 +323,8 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc,
           auto *TTPD = dyn_cast<TemplateTypeParmDecl>(LocalPack);
           return TTPD && TTPD->getTypeForDecl() == TTPT;
         }
-        return declaresSameEntity(Pack.first.get<NamedDecl *>(), LocalPack);
+        return declaresSameEntity(Pack.first.get<const NamedDecl *>(),
+                                  LocalPack);
       };
       if (llvm::any_of(LSI->LocalPacks, DeclaresThisPack))
         LambdaParamPackReferences.push_back(Pack);
@@ -358,7 +376,7 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc,
           = Unexpanded[I].first.dyn_cast<const TemplateTypeParmType *>())
       Name = TTP->getIdentifier();
     else
-      Name = Unexpanded[I].first.get<NamedDecl *>()->getIdentifier();
+      Name = Unexpanded[I].first.get<const NamedDecl *>()->getIdentifier();
 
     if (Name && NamesKnown.insert(Name).second)
       Names.push_back(Name);
@@ -421,7 +439,7 @@ bool Sema::DiagnoseUnexpandedParameterPackInRequiresExpr(RequiresExpr *RE) {
   llvm::SmallPtrSet<NamedDecl*, 8> ParmSet(Parms.begin(), Parms.end());
   SmallVector<UnexpandedParameterPack, 2> UnexpandedParms;
   for (auto Parm : Unexpanded)
-    if (ParmSet.contains(Parm.first.dyn_cast<NamedDecl*>()))
+    if (ParmSet.contains(Parm.first.dyn_cast<const NamedDecl *>()))
       UnexpandedParms.push_back(Parm);
   if (UnexpandedParms.empty())
     return false;
@@ -672,109 +690,95 @@ bool Sema::CheckParameterPacksForExpansion(
     bool &RetainExpansion, Optional<unsigned> &NumExpansions) {
   ShouldExpand = true;
   RetainExpansion = false;
-  std::pair<IdentifierInfo *, SourceLocation> FirstPack;
+  std::pair<const IdentifierInfo *, SourceLocation> FirstPack;
-  bool HaveFirstPack = false;
+  Optional<std::pair<unsigned, SourceLocation>> PartialExpansion;
-  Optional<unsigned> NumPartialExpansions;
+  Optional<unsigned> CurNumExpansions;
-  SourceLocation PartiallySubstitutedPackLoc;
 
-  for (UnexpandedParameterPack ParmPack : Unexpanded) {
+  for (auto [P, Loc] : Unexpanded) {
     // Compute the depth and index for this parameter pack.
-    unsigned Depth = 0, Index = 0;
+    Optional<std::pair<unsigned, unsigned>> Pos;
-    IdentifierInfo *Name;
-    bool IsVarDeclPack = false;
-
-    if (const TemplateTypeParmType *TTP =
-            ParmPack.first.dyn_cast<const TemplateTypeParmType *>()) {
-      Depth = TTP->getDepth();
-      Index = TTP->getIndex();
-      Name = TTP->getIdentifier();
-    } else {
-      NamedDecl *ND = ParmPack.first.get<NamedDecl *>();
-      if (isa<VarDecl>(ND))
-        IsVarDeclPack = true;
-      else
-        std::tie(Depth, Index) = getDepthAndIndex(ND);
-
-      Name = ND->getIdentifier();
-    }
-
-    // Determine the size of this argument pack.
     unsigned NewPackSize;
-    if (IsVarDeclPack) {
+    const auto *ND = P.dyn_cast<const NamedDecl *>();
-      // Figure out whether we're instantiating to an argument pack or not.
+    if (ND && isa<VarDecl>(ND)) {
-      typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
+      const auto *DAP =
-
+          CurrentInstantiationScope->findInstantiationOf(ND)
-      llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation =
+              ->dyn_cast<LocalInstantiationScope::DeclArgumentPack *>();
-          CurrentInstantiationScope->findInstantiationOf(
+      if (!DAP) {
-              ParmPack.first.get<NamedDecl *>());
-      if (Instantiation->is<DeclArgumentPack *>()) {
-        // We could expand this function parameter pack.
-        NewPackSize = Instantiation->get<DeclArgumentPack *>()->size();
-      } else {
         // We can't expand this function parameter pack, so we can't expand
         // the pack expansion.
         ShouldExpand = false;
         continue;
       }
+      NewPackSize = DAP->size();
+    } else if (ND) {
+      Pos = getDepthAndIndex(ND);
+    } else if (const auto *TTP = P.dyn_cast<const TemplateTypeParmType *>()) {
+      Pos = {TTP->getDepth(), TTP->getIndex()};
+      ND = TTP->getDecl();
+      // FIXME: We either should have some fallback for canonical TTP, or
+      //        never have canonical TTP here.
+    } else if (const auto *STP =
+                   P.dyn_cast<const SubstTemplateTypeParmPackType *>()) {
+      NewPackSize = STP->getNumArgs();
+      ND = STP->getReplacedParameter()->getDecl();
     } else {
+      const auto *SEP = P.get<const SubstNonTypeTemplateParmPackExpr *>();
+      NewPackSize = SEP->getArgumentPack().pack_size();
+      ND = SEP->getParameterPack();
+    }
+
+    if (Pos) {
       // If we don't have a template argument at this depth/index, then we
       // cannot expand the pack expansion. Make a note of this, but we still
       // want to check any parameter packs we *do* have arguments for.
-      if (Depth >= TemplateArgs.getNumLevels() ||
+      if (Pos->first >= TemplateArgs.getNumLevels() ||
-          !TemplateArgs.hasTemplateArgument(Depth, Index)) {
+          !TemplateArgs.hasTemplateArgument(Pos->first, Pos->second)) {
         ShouldExpand = false;
         continue;
       }
-
       // Determine the size of the argument pack.
-      NewPackSize = TemplateArgs(Depth, Index).pack_size();
+      NewPackSize = TemplateArgs(Pos->first, Pos->second).pack_size();
-    }
+      // C++0x [temp.arg.explicit]p9:
-
+      //   Template argument deduction can extend the sequence of template
-    // C++0x [temp.arg.explicit]p9:
+      //   arguments corresponding to a template parameter pack, even when the
-    //   Template argument deduction can extend the sequence of template
+      //   sequence contains explicitly specified template arguments.
-    //   arguments corresponding to a template parameter pack, even when the
+      if (CurrentInstantiationScope)
-    //   sequence contains explicitly specified template arguments.
+        if (const NamedDecl *PartialPack =
-    if (!IsVarDeclPack && CurrentInstantiationScope) {
+                CurrentInstantiationScope->getPartiallySubstitutedPack();
-      if (NamedDecl *PartialPack
+            PartialPack && getDepthAndIndex(PartialPack) == *Pos) {
-                    = CurrentInstantiationScope->getPartiallySubstitutedPack()){
-        unsigned PartialDepth, PartialIndex;
-        std::tie(PartialDepth, PartialIndex) = getDepthAndIndex(PartialPack);
-        if (PartialDepth == Depth && PartialIndex == Index) {
           RetainExpansion = true;
           // We don't actually know the new pack size yet.
-          NumPartialExpansions = NewPackSize;
+          PartialExpansion = {NewPackSize, Loc};
-          PartiallySubstitutedPackLoc = ParmPack.second;
           continue;
         }
-      }
     }
 
-    if (!NumExpansions) {
+    // FIXME: Workaround for Canonical TTP.
+    const IdentifierInfo *Name = ND ? ND->getIdentifier() : nullptr;
+    if (!CurNumExpansions) {
       // The is the first pack we've seen for which we have an argument.
       // Record it.
-      NumExpansions = NewPackSize;
+      CurNumExpansions = NewPackSize;
-      FirstPack.first = Name;
+      FirstPack = {Name, Loc};
-      FirstPack.second = ParmPack.second;
+    } else if (NewPackSize != *CurNumExpansions) {
-      HaveFirstPack = true;
-      continue;
-    }
-
-    if (NewPackSize != *NumExpansions) {
       // C++0x [temp.variadic]p5:
       //   All of the parameter packs expanded by a pack expansion shall have
       //   the same number of arguments specified.
-      if (HaveFirstPack)
+      Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict)
-        Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict)
+          << FirstPack.first << Name << *CurNumExpansions << NewPackSize
-            << FirstPack.first << Name << *NumExpansions << NewPackSize
+          << SourceRange(FirstPack.second) << SourceRange(Loc);
-            << SourceRange(FirstPack.second) << SourceRange(ParmPack.second);
-      else
-        Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel)
-            << Name << *NumExpansions << NewPackSize
-            << SourceRange(ParmPack.second);
       return true;
     }
   }
 
+  if (NumExpansions && CurNumExpansions &&
+      *NumExpansions != *CurNumExpansions) {
+    Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel)
+        << FirstPack.first << *CurNumExpansions << *NumExpansions
+        << SourceRange(FirstPack.second);
+    return true;
+  }
+
   // If we're performing a partial expansion but we also have a full expansion,
   // expand to the number of common arguments. For example, given:
   //
@@ -784,17 +788,18 @@ bool Sema::CheckParameterPacksForExpansion(
   //
   // ... a call to 'A<int, int>().f<int>' should expand the pack once and
   // retain an expansion.
-  if (NumPartialExpansions) {
+  if (PartialExpansion) {
-    if (NumExpansions && *NumExpansions < *NumPartialExpansions) {
+    if (CurNumExpansions && *CurNumExpansions < PartialExpansion->first) {
       NamedDecl *PartialPack =
           CurrentInstantiationScope->getPartiallySubstitutedPack();
       Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_partial)
-        << PartialPack << *NumPartialExpansions << *NumExpansions
+          << PartialPack << PartialExpansion->first << *CurNumExpansions
-        << SourceRange(PartiallySubstitutedPackLoc);
+          << SourceRange(PartialExpansion->second);
       return true;
     }
-
+    NumExpansions = PartialExpansion->first;
-    NumExpansions = NumPartialExpansions;
+  } else {
+    NumExpansions = CurNumExpansions;
   }
 
   return false;
@@ -807,47 +812,48 @@ Optional<unsigned> Sema::getNumArgumentsInExpansion(QualType T,
   CollectUnexpandedParameterPacksVisitor(Unexpanded).TraverseType(Pattern);
 
   Optional<unsigned> Result;
-  for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) {
+  auto setResultSz = [&Result](unsigned Size) {
-    // Compute the depth and index for this parameter pack.
+    assert((!Result || *Result == Size) && "inconsistent pack sizes");
-    unsigned Depth;
+    Result = Size;
-    unsigned Index;
+  };
-
+  auto setResultPos = [&](const std::pair<unsigned, unsigned> &Pos) -> bool {
-    if (const TemplateTypeParmType *TTP
+    unsigned Depth = Pos.first, Index = Pos.second;
-          = Unexpanded[I].first.dyn_cast<const TemplateTypeParmType *>()) {
-      Depth = TTP->getDepth();
-      Index = TTP->getIndex();
-    } else {
-      NamedDecl *ND = Unexpanded[I].first.get<NamedDecl *>();
-      if (isa<VarDecl>(ND)) {
-        // Function parameter pack or init-capture pack.
-        typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
-
-        llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation
-          = CurrentInstantiationScope->findInstantiationOf(
-                                        Unexpanded[I].first.get<NamedDecl *>());
-        if (Instantiation->is<Decl*>())
-          // The pattern refers to an unexpanded pack. We're not ready to expand
-          // this pack yet.
-          return None;
-
-        unsigned Size = Instantiation->get<DeclArgumentPack *>()->size();
-        assert((!Result || *Result == Size) && "inconsistent pack sizes");
-        Result = Size;
-        continue;
-      }
-
-      std::tie(Depth, Index) = getDepthAndIndex(ND);
-    }
     if (Depth >= TemplateArgs.getNumLevels() ||
         !TemplateArgs.hasTemplateArgument(Depth, Index))
       // The pattern refers to an unknown template argument. We're not ready to
       // expand this pack yet.
-      return None;
+      return true;
-
     // Determine the size of the argument pack.
-    unsigned Size = TemplateArgs(Depth, Index).pack_size();
+    setResultSz(TemplateArgs(Depth, Index).pack_size());
-    assert((!Result || *Result == Size) && "inconsistent pack sizes");
+    return false;
-    Result = Size;
+  };
+
+  for (auto [I, _] : Unexpanded) {
+    if (const auto *TTP = I.dyn_cast<const TemplateTypeParmType *>()) {
+      if (setResultPos({TTP->getDepth(), TTP->getIndex()}))
+        return None;
+    } else if (const auto *STP =
+                   I.dyn_cast<const SubstTemplateTypeParmPackType *>()) {
+      setResultSz(STP->getNumArgs());
+    } else if (const auto *SEP =
+                   I.dyn_cast<const SubstNonTypeTemplateParmPackExpr *>()) {
+      setResultSz(SEP->getArgumentPack().pack_size());
+    } else {
+      const auto *ND = I.get<const NamedDecl *>();
+      // Function parameter pack or init-capture pack.
+      if (isa<VarDecl>(ND)) {
+        const auto *DAP =
+            CurrentInstantiationScope->findInstantiationOf(ND)
+                ->dyn_cast<LocalInstantiationScope::DeclArgumentPack *>();
+        if (!DAP)
+          // The pattern refers to an unexpanded pack. We're not ready to expand
+          // this pack yet.
+          return None;
+        setResultSz(DAP->size());
+      } else if (setResultPos(getDepthAndIndex(ND))) {
+        return None;
+      }
+    }
   }
 
   return Result;

clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp

@@ -473,7 +473,7 @@ int fn() {
 namespace pr56094 {
 template <typename... T> struct D {
   template <typename... U> using B = int(int (*...p)(T, U));
-  // expected-error@-1 {{pack expansion contains parameter pack 'U' that has a different length (1 vs. 2) from outer parameter packs}}
+  // expected-error@-1 {{pack expansion contains parameter packs 'T' and 'U' that have different lengths (1 vs. 2)}}
   template <typename U1, typename U2> D(B<U1, U2> *);
   // expected-note@-1 {{in instantiation of template type alias 'B' requested here}}
 };
@@ -484,7 +484,7 @@ template <bool...> struct F {};
 template <class...> struct G {};
 template <bool... I> struct E {
   template <bool... U> using B = G<F<I, U>...>;
-  // expected-error@-1 {{pack expansion contains parameter pack 'U' that has a different length (1 vs. 2) from outer parameter packs}}
+  // expected-error@-1 {{pack expansion contains parameter packs 'I' and 'U' that have different lengths (1 vs. 2)}}
   template <bool U1, bool U2> E(B<U1, U2> *);
   // expected-note@-1 {{in instantiation of template type alias 'B' requested here}}
 };

clang/test/SemaTemplate/cxx1z-fold-expressions.cpp

@@ -97,7 +97,7 @@ namespace PR41845 {
   template <int I> struct Constant {};
 
   template <int... Is> struct Sum {
-    template <int... Js> using type = Constant<((Is + Js) + ... + 0)>; // expected-error {{pack expansion contains parameter pack 'Js' that has a different length (1 vs. 2) from outer parameter packs}}
+    template <int... Js> using type = Constant<((Is + Js) + ... + 0)>; // expected-error {{pack expansion contains parameter packs 'Is' and 'Js' that have different lengths (1 vs. 2)}}
   };
 
   Sum<1>::type<1, 2> x; // expected-note {{instantiation of}}

clang/test/SemaTemplate/pack-deduction.cpp

@@ -134,14 +134,14 @@ namespace partial_full_mix {
   template<typename ...T> struct tuple {};
   template<typename ...T> struct A {
     template<typename ...U> static pair<tuple<T...>, tuple<U...>> f(pair<T, U> ...p);
-    // expected-note@-1 {{[with U = <char, double, long>]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 3) from outer parameter packs}}
+    // expected-note@-1 {{[with U = <char, double, long>]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 3)}}
     // expected-note@-2 {{[with U = <char, double, void>]: pack expansion contains parameter pack 'U' that has a different length (at least 3 vs. 2) from outer parameter packs}}
 
     template<typename ...U> static pair<tuple<T...>, tuple<U...>> g(pair<T, U> ...p, ...);
-    // expected-note@-1 {{[with U = <char, double, long>]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 3) from outer parameter packs}}
+    // expected-note@-1 {{[with U = <char, double, long>]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 3)}}
 
     template<typename ...U> static tuple<U...> h(tuple<pair<T, U>..., pair<int, int>>);
-    // expected-note@-1 {{[with U = <int[2]>]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 1) from outer parameter packs}}
+    // expected-note@-1 {{[with U = <int[2]>]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 1)}}
   };
 
   pair<tuple<int, float>, tuple<char, double>> k1 = A<int, float>().f<char>(pair<int, char>(), pair<float, double>());