[clang] Improve diagnostics for expansion length mismatch

[mizvekov]

Summary

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 <[e-mail]>

[mizvekov]

.

[mizvekov]

.

[mizvekov]

rebase

[erichkeane]

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

[mizvekov]

In D128095#3712343, @erichkeane wrote:

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

Done.

I split this up into two patches:

• D131802: Introduces the test and a simple fix.
• This one which improves the analyzer to give better diagnostics.

[shafik]

clang/lib/Sema/SemaTemplateDeduction.cpp

744

Structured bindings maybe?

[mizvekov]

clang/lib/Sema/SemaTemplateDeduction.cpp

744

Sure, they weren't an option back when this patch was first submitted :)

[mizvekov]

ping

[aaron.ballman]

clang/lib/Sema/SemaTemplateVariadic.cpp

107–110

Do we need to handle FunctionParmPackExpr as well?

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

107–110

Right, that and SubstTemplateTemplateParmPack are the two missing cases we could handle here, and perhaps that would allow us to get rid of that 'from outer parameter packs' diagnostics completely.

Would you prefer to handle everything in one patch?

[erichkeane]

just a pair of minor changes I'd like to see, otherwise this LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp

107–110

I think I'm alright doing those in a followup patch.

710

Can you please make this an else if at the level higher? The 'else' after this ends up being so small that it gets lost trying to figure out what is going on here.

So:

} else if (const auto *VD = dyn_cast<VarDecl>(ParmPack.first.get<const NamedDecl *>()))

842

same comment here, see if the 'vardecl' logic can be moved up a level in the 'if' tree.

[aaron.ballman]

clang/lib/Sema/SemaTemplateVariadic.cpp

107–110

I'm also fine handling it in a follow-up if the changes are involved.

[erichkeane]

clang/lib/Sema/SemaTemplateVariadic.cpp

847

This pattern with the init + condition is a little awkward here... any reason to not just use the cast around the 'ND" and just use the VD in the use below? or is there a good reason to split it up like this?

Same with the above case.

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

847

No very strong reason than that just from my different perception this looked fine :)

Minor advantage that we don't make the variable a VarDecl pointer if we don't need to access it as such.

But no strong preference here, I can have another look tomorrow.

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

847

I played a little bit with this change.

I think one thing that makes that pattern of adding separate ND and VD a bit confusing is that at a glance it almost looks like these are different cases in the PointerUnion variant. You have to do a double take to see that, since the nested cast is a bit subtle. This can become even subtler as we add more cases in the next patch.

Or we could stop using PointerUnion on the next patch, since it's so unergonomic with so many variants.

Anyway, I did some other refactorings and I think in general this will be much clearer to read on my next push.

With this refactoring, on this part here that problem goes away since we make this section produce a NamedDecl.

On the second part, below, I tidied up the code so much that I think that nested else isn't almost invisible anymore, since the other blocks become about the same size.

Otherwise, let me know what you think.

I also added to this first part here a FIXME comment describing a pre-existing problem where if we get a canonical TTP, the diagnostics fail to point to the relevant parameter since we won't have it's identifier.

Will try to add a repro and fix for that on the next patch.

[erichkeane]

Happy enough, LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp

847

Thanks for spending time on this... the nested conditionals on the var-decl was hiding 90%+ of what was going on in the branch, and at least this top one is BETTER enough than before. I too hate the pointer union (note BTW, that _4_ is the maximum number of elements thanks to 32 bit platforms, that'll burn you :)).

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

847

Thanks, true about the maximum size :)

Some of the cases could be unified as we have two types and two expressions.

clang/include/clang/Sema/Sema.h

 
 namespace threadSafety {
   class BeforeSet;
   void threadSafetyCleanup(BeforeSet* Cache);
 }
 
 // FIXME: No way to easily map from TemplateTypeParmTypes to
 // TemplateTypeParmDecls, so we have this horrible PointerUnion.
-typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType*, NamedDecl*>,
-                  SourceLocation> UnexpandedParameterPack;
+using UnexpandedParameterPack =
+    std::pair<llvm::PointerUnion<const TemplateTypeParmType *,
+                                 const SubstTemplateTypeParmPackType *,
+                                 const NamedDecl *>,
+              SourceLocation>;
 
 /// Describes whether we've seen any nullability information for the given
 /// file.
 struct FileNullability {
   /// The first pointer declarator (of any pointer kind) in the file that does
   /// not have a corresponding nullability annotation.
   SourceLocation PointerLoc;
 

clang/include/clang/Sema/SemaInternal.h

inline InheritableAttr *getDLLAttr(Decl *D) {
   if (auto *Import = D->getAttr<DLLImportAttr>())
     return Import;
   if (auto *Export = D->getAttr<DLLExportAttr>())
     return Export;
   return nullptr;
 }
 
 /// 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());
 
   if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(ND))
     return std::make_pair(NTTP->getDepth(), NTTP->getIndex());
 
   const auto *TTP = cast<TemplateTemplateParmDecl>(ND);
   return std::make_pair(TTP->getDepth(), TTP->getIndex());
 }
 
 /// Retrieve the depth and index of an unexpanded parameter pack.
 inline std::pair<unsigned, unsigned>
 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 {
   typedef SmallVector<TypoCorrection, 1> TypoResultList;
   typedef llvm::StringMap<TypoResultList> TypoResultsMap;
   typedef std::map<unsigned, TypoResultsMap> TypoEditDistanceMap;
 
 public:

clang/lib/Sema/SemaTemplateDeduction.cpp

auto AddPack = [&](unsigned Index) {
       // FIXME: Also collect the unexpanded packs in any type and template
       // parameter packs that are pack expansions.
     };
 
     auto Collect = [&](TemplateArgument Pattern) {
       SmallVector<UnexpandedParameterPack, 2> Unexpanded;
       S.collectUnexpandedParameterPacks(Pattern, Unexpanded);
       for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) {
+        UnexpandedParameterPack U = Unexpanded[I];
+        if (U.first.is<const SubstTemplateTypeParmPackType *>())
+          continue;
         unsigned Depth, Index;
-        std::tie(Depth, Index) = getDepthAndIndex(Unexpanded[I]);
+        std::tie(Depth, Index) = getDepthAndIndex(U);
         if (Depth == Info.getDeducedDepth())
           AddPack(Index);
       }
     };
 
     // Look for unexpanded packs in the pattern.
     Collect(Pattern);
     assert(!Packs.empty() && "Pack expansion without unexpanded packs?");

clang/lib/Sema/SemaTemplateVariadic.cpp

void addUnexpanded(NamedDecl *ND, SourceLocation Loc = SourceLocation()) {
         auto *FTD = FD ? FD->getDescribedFunctionTemplate() : nullptr;
         if (FTD && FTD->getTemplateParameters()->getDepth() >= DepthLimit)
           return;
       } else if (getDepthAndIndex(ND).first >= DepthLimit)
         return;
 
       Unexpanded.push_back({ND, Loc});
     }
+    void addUnexpanded(const SubstTemplateTypeParmPackType *T,
+                       SourceLocation Loc = SourceLocation()) {
+      Unexpanded.push_back({T, Loc});
+    }
     void addUnexpanded(const TemplateTypeParmType *T,
                        SourceLocation Loc = SourceLocation()) {
       if (T->getDepth() < DepthLimit)
         Unexpanded.push_back({T, Loc});
     }
 
   public:
     explicit CollectUnexpandedParameterPacksVisitor(
public:
     /// Ideally, this routine would never be used.
     bool VisitTemplateTypeParmType(TemplateTypeParmType *T) {
       if (T->isParameterPack())
         addUnexpanded(T);
 
       return true;
     }
 
+    bool VisitSubstTemplateTypeParmPackType(SubstTemplateTypeParmPackType *T) {
+      addUnexpanded(T);
+      return true;
+    }
+
     /// Record occurrences of function and non-type template
     /// parameter packs in an expression.
     bool VisitDeclRefExpr(DeclRefExpr *E) {
       if (E->getDecl()->isParameterPack())
         addUnexpanded(E->getDecl(), E->getLocation());
 
       return true;
     }
 
     /// Record occurrences of template template parameter packs.
     bool TraverseTemplateName(TemplateName Template) {
       if (auto *TTP = dyn_cast_or_null<TemplateTemplateParmDecl>(
               Template.getAsTemplateDecl())) {
         if (TTP->isParameterPack())
           addUnexpanded(TTP);
       }
 
       return inherited::TraverseTemplateName(Template);
     }
Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc,
   SmallVector<UnexpandedParameterPack, 4> LambdaParamPackReferences;
   if (auto *LSI = getEnclosingLambda()) {
     for (auto &Pack : Unexpanded) {
       auto DeclaresThisPack = [&](NamedDecl *LocalPack) {
         if (auto *TTPT = Pack.first.dyn_cast<const TemplateTypeParmType *>()) {
           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);
     }
 
     if (LambdaParamPackReferences.empty()) {
       // Construct in lambda only references packs declared outside the lambda.
       // That's OK for now, but the lambda itself is considered to contain an
Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc,
   llvm::SmallPtrSet<IdentifierInfo *, 4> NamesKnown;
 
   for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) {
     IdentifierInfo *Name = nullptr;
     if (const TemplateTypeParmType *TTP
           = 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);
 
     if (Unexpanded[I].second.isValid())
       Locations.push_back(Unexpanded[I].second);
   }
 
bool Sema::DiagnoseUnexpandedParameterPackInRequiresExpr(RequiresExpr *RE) {
   assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
 
   // We only care about unexpanded references to the RequiresExpr's own
   // parameter packs.
   auto Parms = RE->getLocalParameters();
   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;
 
   return DiagnoseUnexpandedParameterPacks(RE->getBeginLoc(), UPPC_Requirement,
                                           UnexpandedParms);
 }
 
bool Sema::CheckParameterPacksForExpansion(
   bool HaveFirstPack = false;
   Optional<unsigned> NumPartialExpansions;
   SourceLocation PartiallySubstitutedPackLoc;
 
   for (UnexpandedParameterPack ParmPack : Unexpanded) {
     // Compute the depth and index for this parameter pack.
     unsigned Depth = 0, Index = 0;
     IdentifierInfo *Name;
-    bool IsVarDeclPack = false;
+    unsigned NewPackSize;
+    bool NotPack = true;
 
-    if (const TemplateTypeParmType *TTP =
+    if (const auto *TTP =
             ParmPack.first.dyn_cast<const TemplateTypeParmType *>()) {
       Depth = TTP->getDepth();
       Index = TTP->getIndex();
       Name = TTP->getIdentifier();
+    } else if (const auto *STP =
+                   ParmPack.first
+                       .dyn_cast<const SubstTemplateTypeParmPackType *>()) {
+      NewPackSize = STP->getNumArgs();
+      Name = STP->getIdentifier();
+      NotPack = false;
     } 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 = ParmPack.first.get<const NamedDecl *>();
+      if (isa<VarDecl>(ND)) {
         // Figure out whether we're instantiating to an argument pack or not.
-      typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
-
+        using DeclArgumentPack = LocalInstantiationScope::DeclArgumentPack;
         llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation =
             CurrentInstantiationScope->findInstantiationOf(
-              ParmPack.first.get<NamedDecl *>());
+                ParmPack.first.get<const 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;
         }
+        NotPack = false;
       } else {
+        std::tie(Depth, Index) = getDepthAndIndex(ND);
+      }
+      Name = ND->getIdentifier();
+    }
+
+    if (NotPack) {
       // 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() ||
           !TemplateArgs.hasTemplateArgument(Depth, Index)) {
         ShouldExpand = false;
         continue;
       }
-
       // Determine the size of the argument pack.
       NewPackSize = TemplateArgs(Depth, Index).pack_size();
-    }
-
       // C++0x [temp.arg.explicit]p9:
       //   Template argument deduction can extend the sequence of template
       //   arguments corresponding to a template parameter pack, even when the
       //   sequence contains explicitly specified template arguments.
-    if (!IsVarDeclPack && CurrentInstantiationScope) {
-      if (NamedDecl *PartialPack
-                    = CurrentInstantiationScope->getPartiallySubstitutedPack()){
+      if (CurrentInstantiationScope)
+        if (NamedDecl *PartialPack =
+                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;
             PartiallySubstitutedPackLoc = ParmPack.second;
             continue;
           }
         }
     }
 
     if (!NumExpansions) {
       // The is the first pack we've seen for which we have an argument.
       // Record it.
       NumExpansions = NewPackSize;
       FirstPack.first = Name;
       FirstPack.second = ParmPack.second;

 
 Optional<unsigned> Sema::getNumArgumentsInExpansion(QualType T,
                           const MultiLevelTemplateArgumentList &TemplateArgs) {
   QualType Pattern = cast<PackExpansionType>(T)->getPattern();
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded).TraverseType(Pattern);
 
   Optional<unsigned> Result;
+  auto setResult = [&Result](unsigned Size) {
+    assert((!Result || *Result == Size) && "inconsistent pack sizes");
+    Result = Size;
+  };
+
   for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) {
     // Compute the depth and index for this parameter pack.
     unsigned Depth;
     unsigned Index;
 
-    if (const TemplateTypeParmType *TTP
-          = Unexpanded[I].first.dyn_cast<const TemplateTypeParmType *>()) {
+    if (const auto *TTP =
+            Unexpanded[I].first.dyn_cast<const TemplateTypeParmType *>()) {
       Depth = TTP->getDepth();
       Index = TTP->getIndex();
+    } else if (const auto *STP =
+                   Unexpanded[I]
+                       .first
+                       .dyn_cast<const SubstTemplateTypeParmPackType *>()) {
+      setResult(STP->getNumArgs());
+      continue;
     } else {
-      NamedDecl *ND = Unexpanded[I].first.get<NamedDecl *>();
+      const auto *ND = Unexpanded[I].first.get<const 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 *>());
+        llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation =
+            CurrentInstantiationScope->findInstantiationOf(
+                Unexpanded[I].first.get<const 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;
+        setResult(Instantiation->get<DeclArgumentPack *>()->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;
 
     // Determine the size of the argument pack.
-    unsigned Size = TemplateArgs(Depth, Index).pack_size();
-    assert((!Result || *Result == Size) && "inconsistent pack sizes");
-    Result = Size;
+    setResult(TemplateArgs(Depth, Index).pack_size());
   }
 
   return Result;
 }
 
 bool Sema::containsUnexpandedParameterPacks(Declarator &D) {
   const DeclSpec &DS = D.getDeclSpec();
   switch (DS.getTypeSpecType()) {

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

 int fn() {
   FooAlias<> a;
   bar(a);
 
   FooAlias<int> b;
   bar(b);
 }
 }
+
+namespace pr56094 {
+template <typename... T> struct D {
+  template <typename... U> using B = int(int (*...p)(T, U));
+  // 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}}
+};
+using t1 = D<float>::B<int>;
+// expected-note@-1 {{in instantiation of template class 'pr56094::D<float>' requested here}}
+} // namespace pr56094