Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][Sema] Retain the expanding index for unevaluated type constraints #109518

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Sep 21, 2024

(This continues the effort of #86265, fixing another piece of issue in constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint substitution. To that end, we avoided substituting type constraints by copying the constraint expression when instantiating a template. This, however, has left an issue in that for lambda's parameters, they can reference outer template packs that would be expanded in the process of an instantiation, where these parameters would make their way into the constraint evaluation, wherein we have no other way to expand them later in evaluation. For example,

template <class... Ts> void foo() {
   bar([](C<Ts> auto value) {}...);
}

The lambda references a pack Ts that should be expanded when instantiating foo(). The Ts along with the constraint expression would not be transformed until constraint evaluation, and at that point, we would have no chance to expand Ts anyhow.

This patch takes an approach that transforms Ts from an unexpanded TemplateTypeParmType into a SubstTemplateTypeParmType with the current pack substitution index, such that we could use that to expand the type during evaluation.

Fixes #101754

Copy link

github-actions bot commented Sep 21, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@zyn0217 zyn0217 marked this pull request as ready for review September 21, 2024 15:20
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 21, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 21, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

(This continues the effort of #86265, fixing another piece of issue in constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint substitution. To that end, we avoided substituting type constraints by copying the constraint expression when instantiating a template. This, however, has left an issue in that for lambda's parameters, they can reference outer template packs that would be expanded in the process of an instantiation, where these parameters would make their way into the constraint evaluation, wherein we have no other way to expand them later in evaluation. For example,

template &lt;class... Ts&gt; void foo() {
   bar([](C&lt;Ts&gt; auto value) {}...);
}

The lambda references a pack Ts that should be expanded when instantiating foo(). The Ts along with the constraint expression would not be transformed until constraint evaluation, and at that point, we would have no chance to expand Ts anyhow.

This patch takes an approach that transforms Ts from an unexpanded TemplateTypeParmType into a SubstTemplateTypeParmType with the current pack substitution index, such that we could use that to expand the type during evaluation.

Fixes #101754


Full diff: https://github.com/llvm/llvm-project/pull/109518.diff

6 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/Sema/Sema.h (+1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+4-2)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+124-3)
  • (modified) clang/lib/Sema/SemaType.cpp (+6-2)
  • (modified) clang/test/SemaCXX/fold_lambda_with_variadics.cpp (+54)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f4535db7356194..c069bb0b4fa597 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -407,6 +407,8 @@ Bug Fixes to C++ Support
 - Clang now uses the correct set of template argument lists when comparing the constraints of
   out-of-line definitions and member templates explicitly specialized for a given implicit instantiation of
   a class template. (#GH102320)
+- Fixed an issue in constraint evaluation, where type constraints on the lambda expression
+  containing outer unexpanded parameters were not correctly expanded. (#GH101754)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b86861ce7e8cfa..f711fe952d35e9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11252,6 +11252,7 @@ class Sema final : public SemaBase {
                             ConceptDecl *NamedConcept, NamedDecl *FoundDecl,
                             const TemplateArgumentListInfo *TemplateArgs,
                             TemplateTypeParmDecl *ConstrainedParameter,
+                            QualType ConstrainedType,
                             SourceLocation EllipsisLoc);
 
   bool AttachTypeConstraint(AutoTypeLoc TL,
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 16f4542d785715..bdfb0f4c95afb8 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1134,7 +1134,8 @@ bool Sema::BuildTypeConstraint(const CXXScopeSpec &SS,
       SS.isSet() ? SS.getWithLocInContext(Context) : NestedNameSpecifierLoc(),
       ConceptName, CD, /*FoundDecl=*/USD ? cast<NamedDecl>(USD) : CD,
       TypeConstr->LAngleLoc.isValid() ? &TemplateArgs : nullptr,
-      ConstrainedParameter, EllipsisLoc);
+      ConstrainedParameter, Context.getTypeDeclType(ConstrainedParameter),
+      EllipsisLoc);
 }
 
 template <typename ArgumentLocAppender>
@@ -1191,6 +1192,7 @@ bool Sema::AttachTypeConstraint(NestedNameSpecifierLoc NS,
                                 ConceptDecl *NamedConcept, NamedDecl *FoundDecl,
                                 const TemplateArgumentListInfo *TemplateArgs,
                                 TemplateTypeParmDecl *ConstrainedParameter,
+                                QualType ConstrainedType,
                                 SourceLocation EllipsisLoc) {
   // C++2a [temp.param]p4:
   //     [...] If Q is of the form C<A1, ..., An>, then let E' be
@@ -1199,7 +1201,7 @@ bool Sema::AttachTypeConstraint(NestedNameSpecifierLoc NS,
     TemplateArgs ? ASTTemplateArgumentListInfo::Create(Context,
                                                        *TemplateArgs) : nullptr;
 
-  QualType ParamAsArgument(ConstrainedParameter->getTypeForDecl(), 0);
+  QualType ParamAsArgument = ConstrainedType;
 
   ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint(
       *this, NS, NameInfo, NamedConcept, FoundDecl,
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 7481c700019dc8..34635ef3860803 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1646,6 +1646,21 @@ namespace {
                                            SubstTemplateTypeParmPackTypeLoc TL,
                                            bool SuppressObjCLifetime);
 
+    QualType
+    TransformSubstTemplateTypeParmType(TypeLocBuilder &TLB,
+                                       SubstTemplateTypeParmTypeLoc TL) {
+      if (SemaRef.CodeSynthesisContexts.back().Kind !=
+          Sema::CodeSynthesisContext::ConstraintSubstitution)
+        return inherited::TransformSubstTemplateTypeParmType(TLB, TL);
+
+      auto PackIndex = TL.getTypePtr()->getPackIndex();
+      std::optional<Sema::ArgumentPackSubstitutionIndexRAII> SubstIndex;
+      if (SemaRef.ArgumentPackSubstitutionIndex == -1 && PackIndex)
+        SubstIndex.emplace(SemaRef, *PackIndex);
+
+      return inherited::TransformSubstTemplateTypeParmType(TLB, TL);
+    }
+
     CXXRecordDecl::LambdaDependencyKind
     ComputeLambdaDependency(LambdaScopeInfo *LSI) {
       if (auto TypeAlias = getEnclosingTypeAliasTemplateDecl(getSema());
@@ -3056,6 +3071,58 @@ namespace {
 
 } // namespace
 
+namespace {
+
+struct ExpandPackedTypeConstraints
+    : TreeTransform<ExpandPackedTypeConstraints> {
+
+  using inherited = TreeTransform<ExpandPackedTypeConstraints>;
+
+  ExpandPackedTypeConstraints(Sema &SemaRef) : inherited(SemaRef) {}
+
+  using inherited::TransformTemplateTypeParmType;
+
+  QualType TransformTemplateTypeParmType(TypeLocBuilder &TLB,
+                                         TemplateTypeParmTypeLoc TL, bool) {
+    const TemplateTypeParmType *T = TL.getTypePtr();
+    if (!T->isParameterPack()) {
+      TemplateTypeParmTypeLoc NewTL =
+          TLB.push<TemplateTypeParmTypeLoc>(TL.getType());
+      NewTL.setNameLoc(TL.getNameLoc());
+      return TL.getType();
+    }
+
+    assert(SemaRef.ArgumentPackSubstitutionIndex != -1);
+
+    QualType Result = SemaRef.Context.getSubstTemplateTypeParmType(
+        TL.getType(), T->getDecl(), T->getIndex(),
+        SemaRef.ArgumentPackSubstitutionIndex);
+    SubstTemplateTypeParmTypeLoc NewTL =
+        TLB.push<SubstTemplateTypeParmTypeLoc>(Result);
+    NewTL.setNameLoc(TL.getNameLoc());
+    return Result;
+  }
+
+  QualType TransformSubstTemplateTypeParmType(TypeLocBuilder &TLB,
+                                              SubstTemplateTypeParmTypeLoc TL) {
+    const SubstTemplateTypeParmType *T = TL.getTypePtr();
+    if (T->getPackIndex()) {
+      SubstTemplateTypeParmTypeLoc TypeLoc =
+          TLB.push<SubstTemplateTypeParmTypeLoc>(TL.getType());
+      TypeLoc.setNameLoc(TL.getNameLoc());
+      return TypeLoc.getType();
+    }
+    return inherited::TransformSubstTemplateTypeParmType(TLB, TL);
+  }
+
+  bool SubstTemplateArguments(ArrayRef<TemplateArgumentLoc> Args,
+                              TemplateArgumentListInfo &Out) {
+    return inherited::TransformTemplateArguments(Args.begin(), Args.end(), Out);
+  }
+};
+
+} // namespace
+
 bool Sema::SubstTypeConstraint(
     TemplateTypeParmDecl *Inst, const TypeConstraint *TC,
     const MultiLevelTemplateArgumentList &TemplateArgs,
@@ -3064,9 +3131,62 @@ bool Sema::SubstTypeConstraint(
       TC->getTemplateArgsAsWritten();
 
   if (!EvaluateConstraints) {
-      Inst->setTypeConstraint(TC->getConceptReference(),
-                              TC->getImmediatelyDeclaredConstraint());
-      return false;
+    bool ShouldExpandExplicitTemplateArgs =
+        TemplArgInfo && ArgumentPackSubstitutionIndex != -1 &&
+        llvm::any_of(TemplArgInfo->arguments(), [](auto &Arg) {
+          return Arg.getArgument().containsUnexpandedParameterPack();
+        });
+
+    // We want to transform the packs into Subst* nodes for type constraints
+    // inside a pack expansion. For example,
+    //
+    //  template <class... Ts> void foo() {
+    //    bar([](C<Ts> auto value) {}...);
+    //  }
+    //
+    // As we expand Ts in the process of instantiating foo(), and retain
+    // the original template depths of Ts until the constraint evaluation, we
+    // would otherwise have no chance to expand Ts by the time of evaluating
+    // C<auto, Ts>.
+    //
+    // So we form a Subst* node for Ts along with a proper substitution index
+    // here, and substitute the node with a complete MLTAL later in evaluation.
+    if (ShouldExpandExplicitTemplateArgs) {
+      TemplateArgumentListInfo InstArgs;
+      InstArgs.setLAngleLoc(TemplArgInfo->LAngleLoc);
+      InstArgs.setRAngleLoc(TemplArgInfo->RAngleLoc);
+      if (ExpandPackedTypeConstraints(*this).SubstTemplateArguments(
+              TemplArgInfo->arguments(), InstArgs))
+        return true;
+
+      // The type of the original parameter.
+      auto *ConstraintExpr = TC->getImmediatelyDeclaredConstraint();
+      QualType ConstrainedType;
+
+      if (auto *FE = dyn_cast<CXXFoldExpr>(ConstraintExpr)) {
+        assert(FE->getLHS());
+        ConstraintExpr = FE->getLHS();
+      }
+      auto *CSE = cast<ConceptSpecializationExpr>(ConstraintExpr);
+      assert(!CSE->getTemplateArguments().empty() &&
+             "Empty template arguments?");
+      ConstrainedType = CSE->getTemplateArguments()[0].getAsType();
+      assert(!ConstrainedType.isNull() &&
+             "Failed to extract the original ConstrainedType?");
+
+      return AttachTypeConstraint(
+          TC->getNestedNameSpecifierLoc(), TC->getConceptNameInfo(),
+          TC->getNamedConcept(),
+          /*FoundDecl=*/TC->getConceptReference()->getFoundDecl(), &InstArgs,
+          Inst, ConstrainedType,
+          Inst->isParameterPack()
+              ? cast<CXXFoldExpr>(TC->getImmediatelyDeclaredConstraint())
+                    ->getEllipsisLoc()
+              : SourceLocation());
+    }
+    Inst->setTypeConstraint(TC->getConceptReference(),
+                            TC->getImmediatelyDeclaredConstraint());
+    return false;
   }
 
   TemplateArgumentListInfo InstArgs;
@@ -3082,6 +3202,7 @@ bool Sema::SubstTypeConstraint(
       TC->getNestedNameSpecifierLoc(), TC->getConceptNameInfo(),
       TC->getNamedConcept(),
       /*FoundDecl=*/TC->getConceptReference()->getFoundDecl(), &InstArgs, Inst,
+      Context.getTypeDeclType(Inst),
       Inst->isParameterPack()
           ? cast<CXXFoldExpr>(TC->getImmediatelyDeclaredConstraint())
                 ->getEllipsisLoc()
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 950bd6db0359d1..62a7357efcf9cc 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -3035,7 +3035,9 @@ InventTemplateParameter(TypeProcessingState &state, QualType T,
             AutoLoc.getNestedNameSpecifierLoc(), AutoLoc.getConceptNameInfo(),
             AutoLoc.getNamedConcept(), /*FoundDecl=*/AutoLoc.getFoundDecl(),
             AutoLoc.hasExplicitTemplateArgs() ? &TAL : nullptr,
-            InventedTemplateParam, D.getEllipsisLoc());
+            InventedTemplateParam,
+            S.Context.getTypeDeclType(InventedTemplateParam),
+            D.getEllipsisLoc());
       }
     } else {
       // The 'auto' appears in the decl-specifiers; we've not finished forming
@@ -3072,7 +3074,9 @@ InventTemplateParameter(TypeProcessingState &state, QualType T,
             /*FoundDecl=*/
             USD ? cast<NamedDecl>(USD) : CD,
             TemplateId->LAngleLoc.isValid() ? &TemplateArgsInfo : nullptr,
-            InventedTemplateParam, D.getEllipsisLoc());
+            InventedTemplateParam,
+            S.Context.getTypeDeclType(InventedTemplateParam),
+            D.getEllipsisLoc());
       }
     }
   }
diff --git a/clang/test/SemaCXX/fold_lambda_with_variadics.cpp b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
index 14e242f009dc51..2257a4c2d975a8 100644
--- a/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
+++ b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
@@ -179,3 +179,57 @@ void foo() {
 }
 
 } // namespace GH99877
+
+namespace GH101754 {
+
+template <typename... Ts> struct Overloaded : Ts... {
+  using Ts::operator()...;
+};
+
+template <typename... Ts> Overloaded(Ts...) -> Overloaded<Ts...>;
+
+template <class T, class U>
+concept same_as = __is_same(T, U);  // #same_as
+
+template <typename... Ts> constexpr auto foo() {
+  return Overloaded{[](same_as<Ts> auto value) { return value; }...}; // #lambda
+}
+
+static_assert(foo<int, double>()(123) == 123);
+static_assert(foo<int, double>()(2.718) == 2.718);
+
+static_assert(foo<int, double>()('c'));
+// expected-error@-1 {{no matching function}}
+
+// expected-note@#lambda {{constraints not satisfied}}
+// expected-note@#lambda {{'same_as<char, int>' evaluated to false}}
+// expected-note@#same_as {{evaluated to false}}
+
+// expected-note@#lambda {{constraints not satisfied}}
+// expected-note@#lambda {{'same_as<char, double>' evaluated to false}}
+// expected-note@#same_as {{evaluated to false}}
+
+template <class T, class U, class V>
+concept C = same_as<T, U> && same_as<U, V>; // #C
+
+template <typename... Ts> constexpr auto bar() {
+  return ([]<class Up>() {
+    return Overloaded{[](C<Up, Ts> auto value) { // #bar
+      return value;
+    }...};
+  }.template operator()<Ts>(), ...);
+}
+static_assert(bar<int, float>()(3.14f)); // OK, bar() returns the last overload i.e. <float>.
+
+static_assert(bar<int, float>()(123));
+// expected-error@-1 {{no matching function}}
+// expected-note@#bar {{constraints not satisfied}}
+// expected-note@#bar {{'C<int, float, int>' evaluated to false}}
+// expected-note@#C {{evaluated to false}}
+
+// expected-note@#bar {{constraints not satisfied}}
+// expected-note@#bar {{'C<int, float, float>' evaluated to false}}
+// expected-note@#C {{evaluated to false}}
+// expected-note@#same_as 2{{evaluated to false}}
+
+} // namespace GH101754

@zyn0217
Copy link
Contributor Author

zyn0217 commented Sep 30, 2024

Slightly ping

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the solution here, I think this is a good way of making our constraint instantiation more stable. Thanks!

@zyn0217 zyn0217 merged commit 50e5411 into llvm:main Oct 1, 2024
7 of 8 checks passed
@zyn0217 zyn0217 deleted the issue-101754 branch October 1, 2024 00:19
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
…ints (llvm#109518)

(This continues the effort of llvm#86265, fixing another piece of issue in
constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint
substitution. To that end, we avoided substituting type constraints by
copying the constraint expression when instantiating a template. This,
however, has left an issue in that for lambda's parameters, they can
reference outer template packs that would be expanded in the process of
an instantiation, where these parameters would make their way into the
constraint evaluation, wherein we have no other way to expand them later
in evaluation. For example,

    template <class... Ts> void foo() {
      bar([](C<Ts> auto value) {}...);
    }

The lambda references a pack `Ts` that should be expanded when
instantiating `foo()`. The `Ts` along with the constraint expression
would not be transformed until constraint evaluation, and at that point,
we would have no chance to expand `Ts` anyhow.

This patch takes an approach that transforms `Ts` from an unexpanded
TemplateTypeParmType into a SubstTemplateTypeParmType with the current
pack substitution index, such that we could use that to expand the type
during evaluation.

Fixes llvm#101754
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
…ints (llvm#109518)

(This continues the effort of llvm#86265, fixing another piece of issue in
constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint
substitution. To that end, we avoided substituting type constraints by
copying the constraint expression when instantiating a template. This,
however, has left an issue in that for lambda's parameters, they can
reference outer template packs that would be expanded in the process of
an instantiation, where these parameters would make their way into the
constraint evaluation, wherein we have no other way to expand them later
in evaluation. For example,

    template <class... Ts> void foo() {
      bar([](C<Ts> auto value) {}...);
    }

The lambda references a pack `Ts` that should be expanded when
instantiating `foo()`. The `Ts` along with the constraint expression
would not be transformed until constraint evaluation, and at that point,
we would have no chance to expand `Ts` anyhow.

This patch takes an approach that transforms `Ts` from an unexpanded
TemplateTypeParmType into a SubstTemplateTypeParmType with the current
pack substitution index, such that we could use that to expand the type
during evaluation.

Fixes llvm#101754
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
…ints (llvm#109518)

(This continues the effort of llvm#86265, fixing another piece of issue in
constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint
substitution. To that end, we avoided substituting type constraints by
copying the constraint expression when instantiating a template. This,
however, has left an issue in that for lambda's parameters, they can
reference outer template packs that would be expanded in the process of
an instantiation, where these parameters would make their way into the
constraint evaluation, wherein we have no other way to expand them later
in evaluation. For example,

    template <class... Ts> void foo() {
      bar([](C<Ts> auto value) {}...);
    }

The lambda references a pack `Ts` that should be expanded when
instantiating `foo()`. The `Ts` along with the constraint expression
would not be transformed until constraint evaluation, and at that point,
we would have no chance to expand `Ts` anyhow.

This patch takes an approach that transforms `Ts` from an unexpanded
TemplateTypeParmType into a SubstTemplateTypeParmType with the current
pack substitution index, such that we could use that to expand the type
during evaluation.

Fixes llvm#101754
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ints (llvm#109518)

(This continues the effort of llvm#86265, fixing another piece of issue in
constraint evaluation on variadic lambdas.)

We need the depth of the primary template parameters for constraint
substitution. To that end, we avoided substituting type constraints by
copying the constraint expression when instantiating a template. This,
however, has left an issue in that for lambda's parameters, they can
reference outer template packs that would be expanded in the process of
an instantiation, where these parameters would make their way into the
constraint evaluation, wherein we have no other way to expand them later
in evaluation. For example,

    template <class... Ts> void foo() {
      bar([](C<Ts> auto value) {}...);
    }

The lambda references a pack `Ts` that should be expanded when
instantiating `foo()`. The `Ts` along with the constraint expression
would not be transformed until constraint evaluation, and at that point,
we would have no chance to expand `Ts` anyhow.

This patch takes an approach that transforms `Ts` from an unexpanded
TemplateTypeParmType into a SubstTemplateTypeParmType with the current
pack substitution index, such that we could use that to expand the type
during evaluation.

Fixes llvm#101754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang] [frontend] crash when instantiating a parameter pack in a lambda constraint
3 participants