From acecf68c8b7c3c625cfa00f00f8ddc8f15baae44 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Wed, 8 Mar 2023 12:12:33 -0800 Subject: [PATCH] Revert two patches to fix GH58452 regression GH58452 is a regression in the 16.0 release branch caused by both: b8a1b698afb2fc84819c7596090aabf4d826b436 and 3a0309c53674be56b5cfce038d78a0c2c6e2a98c This patch reverts both of those to make the 'valid' code stop diagnosing at the expense of crashes on invalid + unclear diagnostics. This patch also adds the tests from GH58452 to prevent any re-application from breaking this again. Revert "[clang] Improve diagnostics for expansion length mismatch" This reverts commit 3a0309c53674be56b5cfce038d78a0c2c6e2a98c. Revert "[clang] fix missing initialization of original number of expansions" This reverts commit b8a1b698afb2fc84819c7596090aabf4d826b436. Differential Revision: https://reviews.llvm.org/D145605 --- clang/include/clang/Sema/Sema.h | 8 +- clang/include/clang/Sema/SemaInternal.h | 4 +- clang/lib/Sema/SemaTemplateDeduction.cpp | 7 +- clang/lib/Sema/SemaTemplateVariadic.cpp | 234 ++++++++++----------- clang/lib/Sema/TreeTransform.h | 1 - .../test/CXX/temp/temp.decls/temp.variadic/p5.cpp | 72 +++---- clang/test/SemaTemplate/cxx1z-fold-expressions.cpp | 2 +- clang/test/SemaTemplate/pack-deduction.cpp | 6 +- 8 files changed, 158 insertions(+), 176 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index d134287..a492721 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -238,11 +238,9 @@ namespace threadSafety { // FIXME: No way to easily map from TemplateTypeParmTypes to // TemplateTypeParmDecls, so we have this horrible PointerUnion. -using UnexpandedParameterPack = std::pair< - llvm::PointerUnion< - const TemplateTypeParmType *, const SubstTemplateTypeParmPackType *, - const SubstNonTypeTemplateParmPackExpr *, const NamedDecl *>, - SourceLocation>; +typedef std::pair, + SourceLocation> + UnexpandedParameterPack; /// 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 4eca509..842eec0 100644 --- a/clang/include/clang/Sema/SemaInternal.h +++ b/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 getDepthAndIndex(const NamedDecl *ND) { +inline std::pair getDepthAndIndex(NamedDecl *ND) { if (const auto *TTP = dyn_cast(ND)) return std::make_pair(TTP->getDepth(), TTP->getIndex()); @@ -79,7 +79,7 @@ getDepthAndIndex(UnexpandedParameterPack UPP) { if (const auto *TTP = UPP.first.dyn_cast()) return std::make_pair(TTP->getDepth(), TTP->getIndex()); - return getDepthAndIndex(UPP.first.get()); + return getDepthAndIndex(UPP.first.get()); } class TypoCorrectionConsumer : public VisibleDeclConsumer { diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp index 9e48a2a..1fe2d3f 100644 --- a/clang/lib/Sema/SemaTemplateDeduction.cpp +++ b/clang/lib/Sema/SemaTemplateDeduction.cpp @@ -756,11 +756,8 @@ private: SmallVector Unexpanded; S.collectUnexpandedParameterPacks(Pattern, Unexpanded); for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) { - UnexpandedParameterPack U = Unexpanded[I]; - if (U.first.is() || - U.first.is()) - continue; - auto [Depth, Index] = getDepthAndIndex(U); + unsigned Depth, Index; + std::tie(Depth, Index) = getDepthAndIndex(Unexpanded[I]); if (Depth == Info.getDeducedDepth()) AddPack(Index); } diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp index 01a4356..86268b5 100644 --- a/clang/lib/Sema/SemaTemplateVariadic.cpp +++ b/clang/lib/Sema/SemaTemplateVariadic.cpp @@ -89,23 +89,6 @@ 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) { @@ -324,8 +307,7 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc, auto *TTPD = dyn_cast(LocalPack); return TTPD && TTPD->getTypeForDecl() == TTPT; } - return declaresSameEntity(Pack.first.get(), - LocalPack); + return declaresSameEntity(Pack.first.get(), LocalPack); }; if (llvm::any_of(LSI->LocalPacks, DeclaresThisPack)) LambdaParamPackReferences.push_back(Pack); @@ -377,7 +359,7 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc, = Unexpanded[I].first.dyn_cast()) Name = TTP->getIdentifier(); else - Name = Unexpanded[I].first.get()->getIdentifier(); + Name = Unexpanded[I].first.get()->getIdentifier(); if (Name && NamesKnown.insert(Name).second) Names.push_back(Name); @@ -440,7 +422,7 @@ bool Sema::DiagnoseUnexpandedParameterPackInRequiresExpr(RequiresExpr *RE) { llvm::SmallPtrSet ParmSet(Parms.begin(), Parms.end()); SmallVector UnexpandedParms; for (auto Parm : Unexpanded) - if (ParmSet.contains(Parm.first.dyn_cast())) + if (ParmSet.contains(Parm.first.dyn_cast())) UnexpandedParms.push_back(Parm); if (UnexpandedParms.empty()) return false; @@ -692,95 +674,109 @@ bool Sema::CheckParameterPacksForExpansion( bool &RetainExpansion, std::optional &NumExpansions) { ShouldExpand = true; RetainExpansion = false; - std::pair FirstPack; - std::optional> PartialExpansion; - std::optional CurNumExpansions; + std::pair FirstPack; + bool HaveFirstPack = false; + std::optional NumPartialExpansions; + SourceLocation PartiallySubstitutedPackLoc; - for (auto [P, Loc] : Unexpanded) { + for (UnexpandedParameterPack ParmPack : Unexpanded) { // Compute the depth and index for this parameter pack. - std::optional> Pos; + unsigned Depth = 0, Index = 0; + IdentifierInfo *Name; + bool IsVarDeclPack = false; + + if (const TemplateTypeParmType *TTP = + ParmPack.first.dyn_cast()) { + Depth = TTP->getDepth(); + Index = TTP->getIndex(); + Name = TTP->getIdentifier(); + } else { + NamedDecl *ND = ParmPack.first.get(); + if (isa(ND)) + IsVarDeclPack = true; + else + std::tie(Depth, Index) = getDepthAndIndex(ND); + + Name = ND->getIdentifier(); + } + + // Determine the size of this argument pack. unsigned NewPackSize; - const auto *ND = P.dyn_cast(); - if (ND && isa(ND)) { - const auto *DAP = - CurrentInstantiationScope->findInstantiationOf(ND) - ->dyn_cast(); - if (!DAP) { + if (IsVarDeclPack) { + // Figure out whether we're instantiating to an argument pack or not. + typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack; + + llvm::PointerUnion *Instantiation = + CurrentInstantiationScope->findInstantiationOf( + ParmPack.first.get()); + if (Instantiation->is()) { + // We could expand this function parameter pack. + NewPackSize = Instantiation->get()->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()) { - 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()) { - NewPackSize = STP->getNumArgs(); - ND = STP->getReplacedParameter(); } else { - const auto *SEP = P.get(); - 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 (Pos->first >= TemplateArgs.getNumLevels() || - !TemplateArgs.hasTemplateArgument(Pos->first, Pos->second)) { + if (Depth >= TemplateArgs.getNumLevels() || + !TemplateArgs.hasTemplateArgument(Depth, Index)) { ShouldExpand = false; continue; } + // Determine the size of the argument pack. - NewPackSize = TemplateArgs(Pos->first, Pos->second).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 (CurrentInstantiationScope) - if (const NamedDecl *PartialPack = - CurrentInstantiationScope->getPartiallySubstitutedPack(); - PartialPack && getDepthAndIndex(PartialPack) == *Pos) { + 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()) { + 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. - PartialExpansion = {NewPackSize, Loc}; + NumPartialExpansions = NewPackSize; + PartiallySubstitutedPackLoc = ParmPack.second; continue; } + } } - // FIXME: Workaround for Canonical TTP. - const IdentifierInfo *Name = ND ? ND->getIdentifier() : nullptr; - if (!CurNumExpansions) { + if (!NumExpansions) { // The is the first pack we've seen for which we have an argument. // Record it. - CurNumExpansions = NewPackSize; - FirstPack = {Name, Loc}; - } else if (NewPackSize != *CurNumExpansions) { + NumExpansions = NewPackSize; + FirstPack.first = Name; + FirstPack.second = ParmPack.second; + 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. - Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict) - << FirstPack.first << Name << *CurNumExpansions << NewPackSize - << SourceRange(FirstPack.second) << SourceRange(Loc); + if (HaveFirstPack) + Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict) + << FirstPack.first << Name << *NumExpansions << NewPackSize + << 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: // @@ -790,18 +786,17 @@ bool Sema::CheckParameterPacksForExpansion( // // ... a call to 'A().f' should expand the pack once and // retain an expansion. - if (PartialExpansion) { - if (CurNumExpansions && *CurNumExpansions < PartialExpansion->first) { + if (NumPartialExpansions) { + if (NumExpansions && *NumExpansions < *NumPartialExpansions) { NamedDecl *PartialPack = CurrentInstantiationScope->getPartiallySubstitutedPack(); Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_partial) - << PartialPack << PartialExpansion->first << *CurNumExpansions - << SourceRange(PartialExpansion->second); + << PartialPack << *NumPartialExpansions << *NumExpansions + << SourceRange(PartiallySubstitutedPackLoc); return true; } - NumExpansions = PartialExpansion->first; - } else { - NumExpansions = CurNumExpansions; + + NumExpansions = NumPartialExpansions; } return false; @@ -814,48 +809,47 @@ std::optional Sema::getNumArgumentsInExpansion( CollectUnexpandedParameterPacksVisitor(Unexpanded).TraverseType(Pattern); std::optional Result; - auto setResultSz = [&Result](unsigned Size) { - assert((!Result || *Result == Size) && "inconsistent pack sizes"); - Result = Size; - }; - auto setResultPos = [&](const std::pair &Pos) -> bool { - unsigned Depth = Pos.first, Index = Pos.second; - 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 true; - // Determine the size of the argument pack. - setResultSz(TemplateArgs(Depth, Index).pack_size()); - return false; - }; + for (unsigned I = 0, N = Unexpanded.size(); I != N; ++I) { + // Compute the depth and index for this parameter pack. + unsigned Depth; + unsigned Index; - for (auto [I, _] : Unexpanded) { - if (const auto *TTP = I.dyn_cast()) { - if (setResultPos({TTP->getDepth(), TTP->getIndex()})) - return std::nullopt; - } else if (const auto *STP = - I.dyn_cast()) { - setResultSz(STP->getNumArgs()); - } else if (const auto *SEP = - I.dyn_cast()) { - setResultSz(SEP->getArgumentPack().pack_size()); + if (const TemplateTypeParmType *TTP = + Unexpanded[I].first.dyn_cast()) { + Depth = TTP->getDepth(); + Index = TTP->getIndex(); } else { - const auto *ND = I.get(); - // Function parameter pack or init-capture pack. + NamedDecl *ND = Unexpanded[I].first.get(); if (isa(ND)) { - const auto *DAP = - CurrentInstantiationScope->findInstantiationOf(ND) - ->dyn_cast(); - if (!DAP) + // Function parameter pack or init-capture pack. + typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack; + + llvm::PointerUnion *Instantiation = + CurrentInstantiationScope->findInstantiationOf( + Unexpanded[I].first.get()); + if (Instantiation->is()) // The pattern refers to an unexpanded pack. We're not ready to expand // this pack yet. return std::nullopt; - setResultSz(DAP->size()); - } else if (setResultPos(getDepthAndIndex(ND))) { - return std::nullopt; + + unsigned Size = Instantiation->get()->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 std::nullopt; + + // Determine the size of the argument pack. + unsigned Size = TemplateArgs(Depth, Index).pack_size(); + assert((!Result || *Result == Size) && "inconsistent pack sizes"); + Result = Size; } return Result; diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index 44e4a14..c6a3af1 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -5897,7 +5897,6 @@ bool TreeTransform::TransformFunctionTypeParams( = dyn_cast(OldType)) { // We have a function parameter pack that may need to be expanded. QualType Pattern = Expansion->getPattern(); - NumExpansions = Expansion->getNumExpansions(); SmallVector Unexpanded; getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded); diff --git a/clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp b/clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp index 2d82a4f..7542186 100644 --- a/clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp +++ b/clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp @@ -473,51 +473,45 @@ int fn() { } } -namespace pr56094 { -template struct D { - template 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 D(B *); - // expected-note@-1 {{in instantiation of template type alias 'B' requested here}} +namespace GH58452 { +template struct A { + template using B = void(As...(Bs)); }; -using t1 = D::B; -// expected-note@-1 {{in instantiation of template class 'pr56094::D' requested here}} - -template struct F {}; -template struct G {}; -template struct E { - template using B = G...>; - // expected-error@-1 {{pack expansion contains parameter packs 'I' and 'U' that have different lengths (1 vs. 2)}} - template E(B *); - // expected-note@-1 {{in instantiation of template type alias 'B' requested here}} + +template struct C { + template using D = typename A::template B; }; -using t2 = E::B; -// expected-note@-1 {{in instantiation of template class 'pr56094::E' requested here}} -} // namespace pr56094 - -namespace GH56094 { -#if __cplusplus >= 201402L -template struct A; // expected-note {{template is declared here}} -template using B = char; -template int C{ A>{}... }; // expected-error {{implicit instantiation of undefined template}} -#endif -} // namespace GH56094 -namespace GH58679 { -#if __cplusplus >= 201402L -template constexpr int A = 1; +using t1 = C::template D; -template struct B; -template <> struct B<1> { using b1 = void; }; +template +using ConditionalRewrite = B; -template using C = char; +template +using SignatureType = int; -template int D{ B>>{}... }; +template +struct Type1 { + template + using Return = SignatureType...)>; -struct E { - template >::b1> E(E1); }; -template int F{ E(C{})... }; -#endif -} // namespace GH58679 +template +struct Type2 { + using T1 = Type1; + + template + using Return = typename T1::template Return; + +}; + +template +typename T::template Return InvokeMethod() { + return 3; +} + +int Function1() { + return InvokeMethod>(); +} +} diff --git a/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp b/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp index 98876ba..518eaf0 100644 --- a/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp +++ b/clang/test/SemaTemplate/cxx1z-fold-expressions.cpp @@ -97,7 +97,7 @@ namespace PR41845 { template struct Constant {}; template struct Sum { - template using type = Constant<((Is + Js) + ... + 0)>; // expected-error {{pack expansion contains parameter packs 'Is' and 'Js' that have different lengths (1 vs. 2)}} + template 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}} }; Sum<1>::type<1, 2> x; // expected-note {{instantiation of}} diff --git a/clang/test/SemaTemplate/pack-deduction.cpp b/clang/test/SemaTemplate/pack-deduction.cpp index f07bafd..e427098 100644 --- a/clang/test/SemaTemplate/pack-deduction.cpp +++ b/clang/test/SemaTemplate/pack-deduction.cpp @@ -134,14 +134,14 @@ namespace partial_full_mix { template struct tuple {}; template struct A { template static pair, tuple> f(pair ...p); - // expected-note@-1 {{[with U = ]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 3)}} + // expected-note@-1 {{[with U = ]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 3) from outer parameter packs}} // expected-note@-2 {{[with U = ]: pack expansion contains parameter pack 'U' that has a different length (at least 3 vs. 2) from outer parameter packs}} template static pair, tuple> g(pair ...p, ...); - // expected-note@-1 {{[with U = ]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 3)}} + // expected-note@-1 {{[with U = ]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 3) from outer parameter packs}} template static tuple h(tuple..., pair>); - // expected-note@-1 {{[with U = ]: pack expansion contains parameter packs 'T' and 'U' that have different lengths (2 vs. 1)}} + // expected-note@-1 {{[with U = ]: pack expansion contains parameter pack 'U' that has a different length (2 vs. 1) from outer parameter packs}} }; pair, tuple> k1 = A().f(pair(), pair()); -- 2.7.4