[Clang] Remove the wrong assumption when rebuilding SizeOfPackExprs for constraint normalization #115120
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or constraint normalization In 463a4f1, we assumed that all the template argument packs are of size 1 when normalizing a constraint expression because I mistakenly thought those packs were obtained from their injected template parameters. This was wrong because we might be checking constraints when instantiating a friend declaration within a class template specialization, where the parent class template is specialized with non-dependent template arguments. In that sense, we shouldn't assume any pack size nor expand anything in such a scenario. Moreover, there are no intermediate (substituted but unexpanded) AST nodes for template template parameters, so we have to special-case their transformations by looking into the instantiation scope instead of extracting anything from template arguments.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesIn 463a4f1, we assumed that all the template argument packs are of size 1 when normalizing a constraint expression because I mistakenly thought those packs were obtained from their injected template parameters. This was wrong because we might be checking constraints when instantiating a friend declaration within a class template specialization, where the parent class template is specialized with non-dependent template arguments. In that sense, we shouldn't assume any pack size nor expand anything in such a scenario. Moreover, there are no intermediate (substituted but unexpanded) AST nodes for template template parameters, so we have to special-case their transformations by looking into the instantiation scope instead of extracting anything from template arguments. Fixes #115098 Full diff: https://github.com/llvm/llvm-project/pull/115120.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index de0ec0128905ff..081873030a1a59 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -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);
@@ -1881,6 +1871,15 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
TemplateArgument Arg = TemplateArgs(TTP->getDepth(), TTP->getPosition());
if (TTP->isParameterPack()) {
+ // We might not have an index for pack expansion when normalizing
+ // constraint expressions. In that case, resort to instantiation scopes
+ // for the transformed declarations.
+ if (SemaRef.ArgumentPackSubstitutionIndex == -1 &&
+ SemaRef.CodeSynthesisContexts.back().Kind ==
+ Sema::CodeSynthesisContext::ConstraintNormalization) {
+ return SemaRef.FindInstantiatedDecl(Loc, cast<NamedDecl>(D),
+ TemplateArgs);
+ }
assert(Arg.getKind() == TemplateArgument::Pack &&
"Missing argument pack");
Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index dd518d283c83c8..7cb5cfc10b9a7b 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -703,6 +703,25 @@ C v;
} // namespace GH93099
+namespace GH115098 {
+
+template <typename... Ts> struct c {
+ template <typename T>
+ requires(sizeof...(Ts) > 0)
+ friend bool operator==(c, c);
+};
+
+template <typename... Ts> struct d {
+ template <typename T>
+ requires(sizeof...(Ts) > 0)
+ friend bool operator==(d, d);
+};
+
+template struct c<int>;
+template struct d<int, int>;
+
+} // namespace GH115098
+
namespace GH114685 {
template <typename T> struct ptr {
|
mizvekov
reviewed
Nov 6, 2024
mizvekov
approved these changes
Nov 7, 2024
Comment on lines
+1739
to
+1746
| if (SemaRef.CodeSynthesisContexts.back().Kind == | ||
| Sema::CodeSynthesisContext::ConstraintNormalization && | ||
| TransformedExpr->getPack() == E->getPack()) { | ||
| Decl *NewPack = | ||
| TransformDecl(E->getPackLoc(), TransformedExpr->getPack()); | ||
| if (!NewPack) | ||
| return ExprError(); | ||
| TransformedExpr->setPack(cast<NamedDecl>(NewPack)); |
There was a problem hiding this comment.
It looks like there is some dance here that could be avoided if we moved this into the inherited transform function in TreeTransform.h instead.
There was a problem hiding this comment.
(I was hesitant about whether we should do much special casing in TreeTransform. Putting it in Instantiator at least minimizes the blast radius. :)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/8230 Here is the relevant piece of the build log for the reference |
Groverkss
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Nov 15, 2024
…or constraint normalization (llvm#115120) In 463a4f1, we assumed that all the template argument packs are of size 1 when normalizing a constraint expression because I mistakenly thought those packs were obtained from their injected template parameters. This was wrong because we might be checking constraints when instantiating a friend declaration within a class template specialization, where the parent class template is specialized with non-dependent template arguments. In that sense, we shouldn't assume any pack size nor expand anything in such a scenario. Moreover, there are no intermediate (substituted but unexpanded) AST nodes for template template parameters, so we have to special-case their transformations by looking into the instantiation scope instead of extracting anything from template arguments. Fixes llvm#115098
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 463a4f1, we assumed that all the template argument packs are of size 1 when normalizing a constraint expression because I mistakenly thought those packs were obtained from their injected template parameters. This was wrong because we might be checking constraints when instantiating a friend declaration within a class template specialization, where the parent class template is specialized with non-dependent template arguments.
In that sense, we shouldn't assume any pack size nor expand anything in such a scenario. Moreover, there are no intermediate (substituted but unexpanded) AST nodes for template template parameters, so we have to special-case their transformations by looking into the instantiation scope instead of extracting anything from template arguments.
Fixes #115098