llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) <details> <summary>Changes</summary> This patch reapplies 3a0309c5 "[Clang] Improve diagnostics for expansion length mismatch" and b8a1b698 "[Clang] fix missing initialization of original number of expansions". Both were reverted before the Clang 16 release due to a regression uncovered by GH58452. I believe this regression can now be avoided by my recent patch 5c55f966, which defers the overeager diagnostics until the `PackExpansionTypes` are actually expanded. Therefore, it should be safe to reapply these two patches, as they do help improve diagnostics for packs. Moreover, I also pulled in Richard's test cases for [PR16668](https://bugs.llvm.org/show_bug.cgi?id=16668#c2) which, unfortunately uncovers yet another rejects-valid case which I don't think urgent to fix at this point. However, part of this patch - specifically teaching pack-expansion functions to recognize `Subst*` packs - is helpful (and essential) to solve that bug. For now, that portion is blocked with `#if 0` in the test `clang/test/SemaTemplate/pack-deduction.cpp`. This reverts commit acecf68c and adopts the patch to the current `CheckParameterPacksForExpansion`, while also following the ongoing migration away from `PointerIntPairs::{is,get}` and their friends. Below is the original commit message, by @<!-- -->mizvekov who will be credited as the co-author. 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. When expanding undeclared function parameters, we should initialize the original number of expansions, if known, before trying to expand them, otherwise a length mismatch with an outer pack might not be diagnosed. Fixes #<!-- -->56094 --- Patch is 31.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121044.diff 10 Files Affected: - (modified) clang/include/clang/Sema/Sema.h (+5-3) - (modified) clang/include/clang/Sema/SemaInternal.h (+1-1) - (modified) clang/lib/AST/ExprCXX.cpp (+1-1) - (modified) clang/lib/Sema/SemaDeclCXX.cpp (+1-1) - (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+5-2) - (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+176-164) - (modified) clang/lib/Sema/TreeTransform.h (+1) - (modified) clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp (+50) - (modified) clang/test/SemaTemplate/cxx1z-fold-expressions.cpp (+1-1) - (modified) clang/test/SemaTemplate/pack-deduction.cpp (+44-7) ``````````diff diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9ac26d8728446..34f99b8adf467 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -232,9 +232,11 @@ 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 SubstNonTypeTemplateParmPackExpr *, const NamedDecl *>, + SourceLocation>; /// Describes whether we've seen any nullability information for the given /// file. diff --git a/clang/include/clang/Sema/SemaInternal.h b/clang/include/clang/Sema/SemaInternal.h index 95874077050a9..acf6c8146d70d 100644 --- a/clang/include/clang/Sema/SemaInternal.h +++ b/clang/include/clang/Sema/SemaInternal.h @@ -75,7 +75,7 @@ getDepthAndIndex(UnexpandedParameterPack UPP) { if (const auto *TTP = dyn_cast<const TemplateTypeParmType *>(UPP.first)) return std::make_pair(TTP->getDepth(), TTP->getIndex()); - return getDepthAndIndex(cast<NamedDecl *>(UPP.first)); + return getDepthAndIndex(cast<const NamedDecl *>(UPP.first)); } class TypoCorrectionConsumer : public VisibleDeclConsumer { diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp index c8d61e2cf3f26..afa3db950284b 100644 --- a/clang/lib/AST/ExprCXX.cpp +++ b/clang/lib/AST/ExprCXX.cpp @@ -1734,7 +1734,7 @@ PackIndexingExpr *PackIndexingExpr::Create( NamedDecl *PackIndexingExpr::getPackDecl() const { if (auto *D = dyn_cast<DeclRefExpr>(getPackIdExpression()); D) { NamedDecl *ND = dyn_cast<NamedDecl>(D->getDecl()); - assert(ND && "exected a named decl"); + assert(ND && "expected a named decl"); return ND; } assert(false && "invalid declaration kind in pack indexing expression"); diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 96aac7871db1e..756937f44e948 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -17589,7 +17589,7 @@ DeclResult Sema::ActOnTemplatedFriendTag( unsigned FriendDeclDepth = TempParamLists.front()->getDepth(); for (UnexpandedParameterPack &U : Unexpanded) { if (getDepthAndIndex(U).first >= FriendDeclDepth) { - auto *ND = dyn_cast<NamedDecl *>(U.first); + auto *ND = dyn_cast<const NamedDecl *>(U.first); if (!ND) ND = cast<const TemplateTypeParmType *>(U.first)->getDecl(); Diag(U.second, diag::friend_template_decl_malformed_pack_expansion) diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp index e6ec4a7178e81..6af842121ae79 100644 --- a/clang/lib/Sema/SemaTemplateDeduction.cpp +++ b/clang/lib/Sema/SemaTemplateDeduction.cpp @@ -874,8 +874,11 @@ class PackDeductionScope { SmallVector<UnexpandedParameterPack, 2> Unexpanded; S.collectUnexpandedParameterPacks(Pattern, Unexpanded); for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) { - unsigned Depth, Index; - std::tie(Depth, Index) = getDepthAndIndex(Unexpanded[I]); + UnexpandedParameterPack U = Unexpanded[I]; + if (isa<const SubstTemplateTypeParmPackType *, + const SubstNonTypeTemplateParmPackExpr *>(U.first)) + continue; + auto [Depth, Index] = getDepthAndIndex(U); if (Depth == Info.getDeducedDepth()) AddPack(Index); } diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp index d9256dbd07d7a..d6c71ecec13b6 100644 --- a/clang/lib/Sema/SemaTemplateVariadic.cpp +++ b/clang/lib/Sema/SemaTemplateVariadic.cpp @@ -38,7 +38,7 @@ class CollectUnexpandedParameterPacksVisitor unsigned DepthLimit = (unsigned)-1; #ifndef NDEBUG - bool ContainsIntermediatePacks = false; + bool ContainsIntermediatePacks = false; #endif void addUnexpanded(NamedDecl *ND, SourceLocation Loc = SourceLocation()) { @@ -97,6 +97,33 @@ class CollectUnexpandedParameterPacksVisitor return true; } + bool VisitSubstTemplateTypeParmPackTypeLoc( + SubstTemplateTypeParmPackTypeLoc TL) override { + Unexpanded.push_back({TL.getTypePtr(), TL.getNameLoc()}); +#ifdef NDEBUG + ContainsIntermediatePacks = true; +#endif + return true; + } + + bool VisitSubstTemplateTypeParmPackType( + SubstTemplateTypeParmPackType *T) override { + Unexpanded.push_back({T, SourceLocation()}); +#ifdef NDEBUG + ContainsIntermediatePacks = true; +#endif + return true; + } + + bool VisitSubstNonTypeTemplateParmPackExpr( + SubstNonTypeTemplateParmPackExpr *E) override { + Unexpanded.push_back({E, E->getParameterPackLocation()}); +#ifdef NDEBUG + ContainsIntermediatePacks = true; +#endif + return true; + } + /// Record occurrences of function and non-type template /// parameter packs in an expression. bool VisitDeclRefExpr(DeclRefExpr *E) override { @@ -315,24 +342,6 @@ class CollectUnexpandedParameterPacksVisitor return true; } - bool TraverseSubstNonTypeTemplateParmPackExpr( - SubstNonTypeTemplateParmPackExpr *) override { - ContainsIntermediatePacks = true; - return true; - } - - bool VisitSubstTemplateTypeParmPackType( - SubstTemplateTypeParmPackType *) override { - ContainsIntermediatePacks = true; - return true; - } - - bool VisitSubstTemplateTypeParmPackTypeLoc( - SubstTemplateTypeParmPackTypeLoc) override { - ContainsIntermediatePacks = true; - return true; - } - bool containsIntermediatePacks() const { return ContainsIntermediatePacks; } #endif }; @@ -374,7 +383,8 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc, auto *TTPD = dyn_cast<TemplateTypeParmDecl>(LocalPack); return TTPD && TTPD->getTypeForDecl() == TTPT; } - return declaresSameEntity(cast<NamedDecl *>(Pack.first), LocalPack); + return declaresSameEntity(cast<const NamedDecl *>(Pack.first), + LocalPack); }; if (llvm::any_of(CSI->LocalPacks, DeclaresThisPack)) ParamPackReferences.push_back(Pack); @@ -425,8 +435,8 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc, if (const TemplateTypeParmType *TTP = Unexpanded[I].first.dyn_cast<const TemplateTypeParmType *>()) Name = TTP->getIdentifier(); - else if (NamedDecl *ND = Unexpanded[I].first.dyn_cast<NamedDecl *>()) - Name = ND->getIdentifier(); + else + Name = cast<const NamedDecl *>(Unexpanded[I].first)->getIdentifier(); if (Name && NamesKnown.insert(Name).second) Names.push_back(Name); @@ -500,7 +510,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(dyn_cast<const NamedDecl *>(Parm.first))) UnexpandedParms.push_back(Parm); if (UnexpandedParms.empty()) return false; @@ -749,6 +759,19 @@ ExprResult Sema::CheckPackExpansion(Expr *Pattern, SourceLocation EllipsisLoc, PackExpansionExpr(Context.DependentTy, Pattern, EllipsisLoc, NumExpansions); } +static bool IsUnexpandedPackExpansion(const TemplateArgument &TA) { + if (!TA.isPackExpansion()) + return false; + + if (TA.getKind() == TemplateArgument::Type) + return !TA.getAsType()->getAs<PackExpansionType>()->getNumExpansions(); + + if (TA.getKind() == TemplateArgument::Expression) + return !cast<PackExpansionExpr>(TA.getAsExpr())->getNumExpansions(); + + return !TA.getNumTemplateExpansions(); +} + bool Sema::CheckParameterPacksForExpansion( SourceLocation EllipsisLoc, SourceRange PatternRange, ArrayRef<UnexpandedParameterPack> Unexpanded, @@ -756,125 +779,104 @@ bool Sema::CheckParameterPacksForExpansion( bool &RetainExpansion, std::optional<unsigned> &NumExpansions) { ShouldExpand = true; RetainExpansion = false; - std::pair<IdentifierInfo *, SourceLocation> FirstPack; - bool HaveFirstPack = false; - std::optional<unsigned> NumPartialExpansions; - SourceLocation PartiallySubstitutedPackLoc; - typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack; + std::pair<const IdentifierInfo *, SourceLocation> FirstPack; + std::optional<std::pair<unsigned, SourceLocation>> PartialExpansion; + std::optional<unsigned> CurNumExpansions, CurMaximumOfLeastExpansions; - for (UnexpandedParameterPack ParmPack : Unexpanded) { + for (auto [P, Loc] : Unexpanded) { // Compute the depth and index for this parameter pack. - unsigned Depth = 0, Index = 0; - IdentifierInfo *Name; - bool IsVarDeclPack = false; - FunctionParmPackExpr *BindingPack = nullptr; - - if (const TemplateTypeParmType *TTP = - ParmPack.first.dyn_cast<const TemplateTypeParmType *>()) { - Depth = TTP->getDepth(); - Index = TTP->getIndex(); - Name = TTP->getIdentifier(); - } else { - NamedDecl *ND = cast<NamedDecl *>(ParmPack.first); - if (isa<VarDecl>(ND)) - IsVarDeclPack = true; - else if (isa<BindingDecl>(ND)) { + std::optional<std::pair<unsigned, unsigned>> Pos; + unsigned NewPackSize, PendingPackExpansionSize = 0; + const auto *ND = dyn_cast_if_present<const NamedDecl *>(P); + if (ND) { + if (isa<VarDecl>(ND)) { + auto *DAP = dyn_cast<LocalInstantiationScope::DeclArgumentPack *>( + *CurrentInstantiationScope->findInstantiationOf(ND)); + if (!DAP) { + // We can't expand this function parameter pack, so we can't expand + // the pack expansion. + ShouldExpand = false; + continue; + } + NewPackSize = DAP->size(); + } else if (isa<BindingDecl>(ND)) { // Find the instantiated BindingDecl and check it for a resolved pack. - llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation = - CurrentInstantiationScope->findInstantiationOf(ND); + llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> + *Instantiation = CurrentInstantiationScope->findInstantiationOf(ND); Decl *B = cast<Decl *>(*Instantiation); Expr *BindingExpr = cast<BindingDecl>(B)->getBinding(); - BindingPack = cast_if_present<FunctionParmPackExpr>(BindingExpr); + auto *BindingPack = cast_if_present<FunctionParmPackExpr>(BindingExpr); if (!BindingPack) { ShouldExpand = false; continue; } + NewPackSize = BindingPack->getNumExpansions(); } else - std::tie(Depth, Index) = getDepthAndIndex(ND); - - Name = ND->getIdentifier(); + Pos = getDepthAndIndex(ND); + } else if (const auto *TTP = dyn_cast<const TemplateTypeParmType *>(P)) { + 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 = + dyn_cast<const SubstTemplateTypeParmPackType *>(P)) { + NewPackSize = STP->getNumArgs(); + PendingPackExpansionSize = llvm::count_if( + STP->getArgumentPack().getPackAsArray(), IsUnexpandedPackExpansion); + ND = STP->getReplacedParameter(); + } else { + const auto *SEP = cast<const SubstNonTypeTemplateParmPackExpr *>(P); + NewPackSize = SEP->getArgumentPack().pack_size(); + PendingPackExpansionSize = llvm::count_if( + SEP->getArgumentPack().getPackAsArray(), IsUnexpandedPackExpansion); + ND = SEP->getParameterPack(); } - // Determine the size of this argument pack. - unsigned NewPackSize, PendingPackExpansionSize = 0; - if (IsVarDeclPack) { - // Figure out whether we're instantiating to an argument pack or not. - llvm::PointerUnion<Decl *, DeclArgumentPack *> *Instantiation = - CurrentInstantiationScope->findInstantiationOf( - cast<NamedDecl *>(ParmPack.first)); - if (isa<DeclArgumentPack *>(*Instantiation)) { - // We could expand this function parameter pack. - NewPackSize = cast<DeclArgumentPack *>(*Instantiation)->size(); - } else { - // We can't expand this function parameter pack, so we can't expand - // the pack expansion. - ShouldExpand = false; - continue; - } - } else if (BindingPack) { - NewPackSize = BindingPack->getNumExpansions(); - } else { + 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() || - !TemplateArgs.hasTemplateArgument(Depth, Index)) { + if (Pos->first >= TemplateArgs.getNumLevels() || + !TemplateArgs.hasTemplateArgument(Pos->first, Pos->second)) { ShouldExpand = false; continue; } - // Determine the size of the argument pack. ArrayRef<TemplateArgument> Pack = - TemplateArgs(Depth, Index).getPackAsArray(); + TemplateArgs(Pos->first, Pos->second).getPackAsArray(); NewPackSize = Pack.size(); PendingPackExpansionSize = - llvm::count_if(Pack, [](const TemplateArgument &TA) { - if (!TA.isPackExpansion()) - return false; - - if (TA.getKind() == TemplateArgument::Type) - return !TA.getAsType() - ->castAs<PackExpansionType>() - ->getNumExpansions(); - - if (TA.getKind() == TemplateArgument::Expression) - return !cast<PackExpansionExpr>(TA.getAsExpr()) - ->getNumExpansions(); - - return !TA.getNumTemplateExpansions(); - }); - } - - // 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()) { - unsigned PartialDepth, PartialIndex; - std::tie(PartialDepth, PartialIndex) = getDepthAndIndex(PartialPack); - if (PartialDepth == Depth && PartialIndex == Index) { + llvm::count_if(Pack, IsUnexpandedPackExpansion); + // 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 (CurrentInstantiationScope) + if (const NamedDecl *PartialPack = + CurrentInstantiationScope->getPartiallySubstitutedPack(); + PartialPack && getDepthAndIndex(PartialPack) == *Pos) { RetainExpansion = true; // We don't actually know the new pack size yet. - NumPartialExpansions = NewPackSize; - PartiallySubstitutedPackLoc = ParmPack.second; + PartialExpansion = {NewPackSize, Loc}; continue; } - } } - if (!NumExpansions) { + unsigned LeastNewPackSize = NewPackSize - PendingPackExpansionSize; + if (PendingPackExpansionSize) + CurMaximumOfLeastExpansions = + CurMaximumOfLeastExpansions + ? std::max(LeastNewPackSize, *CurMaximumOfLeastExpansions) + : LeastNewPackSize; + + // FIXME: Workaround for Canonical TTP. + const IdentifierInfo *Name = ND ? ND->getIdentifier() : nullptr; + if (!CurNumExpansions) { // This is the first pack we've seen for which we have an argument. // Record it. - NumExpansions = NewPackSize; - FirstPack.first = Name; - FirstPack.second = ParmPack.second; - HaveFirstPack = true; - continue; - } - - if (NewPackSize != *NumExpansions) { + CurNumExpansions = NewPackSize; + FirstPack = {Name, Loc}; + } else if (NewPackSize != *CurNumExpansions) { // In some cases, we might be handling packs with unexpanded template // arguments. For example, this can occur when substituting into a type // alias declaration that uses its injected template parameters as @@ -888,31 +890,40 @@ bool Sema::CheckParameterPacksForExpansion( // Pack comes from another template parameter. 'S<int>' is first // instantiated, expanding the outer pack 'Outer' to <int>. The alias // declaration is accordingly substituted, leaving the template arguments - // as unexpanded - // '<Pack...>'. + // as unexpanded '<Pack...>'. // // Since we have no idea of the size of '<Pack...>' until its expansion, // we shouldn't assume its pack size for validation. However if we are // certain that there are extra arguments beyond unexpanded packs, in // which case the pack size is already larger than the previous expansion, // we can complain that before instantiation. - unsigned LeastNewPackSize = NewPackSize - PendingPackExpansionSize; - if (PendingPackExpansionSize && LeastNewPackSize <= *NumExpansions) { + if (PendingPackExpansionSize && LeastNewPackSize <= *CurNumExpansions) { ShouldExpand = false; continue; } // 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) - << FirstPack.first << Name << *NumExpansions - << (LeastNewPackSize != NewPackSize) << LeastNewPackSize - << SourceRange(FirstPack.second) << SourceRange(ParmPack.second); - else - Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel) - << Name << *NumExpansions << (LeastNewPackSize != NewPackSize) - << LeastNewPackSize << SourceRange(ParmPack.second); + Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict) + << FirstPack.first << Name << *CurNumExpansions + << (LeastNewPackSize != NewPackSize) << LeastNewPackSize + << SourceRange(FirstPack.second) << SourceRange(Loc); + return true; + } + } + + if (NumExpansions && CurNumExpansions && + *NumExpansions != *CurNumExpansions) { + if (CurMaximumOfLeastExpansions && + *CurMaximumOfLeastExpansions <= *NumExpansions) { + ShouldExpand = false; + } else { + Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel) + << FirstPack.first << *NumExpansions + << bool(CurMaximumOfLeastExpansions) + << (CurMaximumOfLeastExpansions ? *CurMaximumOfLeastExpansions + : *CurNumExpansions) + << SourceRange(FirstPack.second); return true; } } @@ -926,17 +937,18 @@ bool Sema::CheckParameterPacksForExpansion( // // ... a call to 'A<int, int>().f<int>' should expand the pack once and // retain an expansion. - if (NumPartialExpansions) { - if (NumExpansions && *NumExpansions < *NumPartialExpansions) { + if (PartialExpansion) { + if (CurNumExpansions && *CurNumExpansions < PartialExpansion->first) { NamedDecl *PartialPack = CurrentInstantiationScope->getPartiallySubstitutedPack(); Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_partial) - << PartialPack << *NumPartialExpansions << *NumExpansions - << SourceRange(PartiallySubstitutedPackLoc); + << PartialPack << PartialExpansion->first << *CurNumExpansions + << SourceRange(PartialExpansion->second); return true; } - - NumExpansions ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/121044 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
