Skip to content

[clang] Concepts: support pack expansions for type constraints #132626

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

Merged
merged 3 commits into from
Apr 2, 2025

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Mar 23, 2025

This reverts an earlier attempt (adb0d8d and 50e5411) to support these expansions,
which was limited to type arguments and which subverted the purpose
of SubstTemplateTypeParmType.

This propagates the ArgumentPackSubstitutionIndex along with the
AssociatedConstraint, so that the pack expansion works, without
needing any new transforms or otherwise any changes to the template
instantiation process.

This keeps the tests from the reverted commits, and adds a few more
showing the new solution also works for NTTPs.

Fixes #131798

@mizvekov mizvekov self-assigned this Mar 23, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang-tidy clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Mar 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 23, 2025

@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang-tools-extra

Author: Matheus Izvekov (mizvekov)

Changes

This reverts an earlier attempt (adb0d8d and 50e5411) to support these expansions,
which was limited to type arguments and which subverted the purpose
of SubstTemplateTypeParmType.

This propagates the ArgumentPackSubstitutionIndex along with the
AssociatedConstraint, so that the pack expansion works, without
needing any new transforms or otherwise any changes to the template
instanntiation process.

This keeps the tests from the reverted commits, and adds a few more
showing the new solution also works for NTTPs.

This patch is incomplete:

  • It is likely missing plumbing the ArgumentPackSubstitutionIndex
    into more places.
  • The Normalization cache is not adapted so it indexes on the
    ArgumentPackSubstitutionIndex as well.

One new test is added, which in any case shows an ambiguous call,
but if the normalization cache were corrected, the reason for
ambighuity would change from subsumption both ways, to neither
subsumes the other.

I am not sure if the current language rules would allow
for a test case where the pack index would break an ambiguity,
this is left for future consideration.


Patch is 67.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/132626.diff

29 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp (+2-2)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/AST/ASTConcept.h (+9-2)
  • (modified) clang/include/clang/AST/ASTContext.h (+1-3)
  • (modified) clang/include/clang/AST/Decl.h (+13-2)
  • (modified) clang/include/clang/AST/DeclTemplate.h (+17-9)
  • (modified) clang/include/clang/AST/PropertiesBase.td (-1)
  • (modified) clang/include/clang/AST/Type.h (+3-26)
  • (modified) clang/include/clang/AST/TypeProperties.td (+1-4)
  • (modified) clang/include/clang/Sema/Sema.h (+15-11)
  • (modified) clang/include/clang/Sema/SemaConcept.h (+18-9)
  • (modified) clang/lib/AST/ASTContext.cpp (+3-4)
  • (modified) clang/lib/AST/ASTImporter.cpp (+4-3)
  • (modified) clang/lib/AST/DeclTemplate.cpp (+14-12)
  • (modified) clang/lib/AST/Type.cpp (+1-5)
  • (modified) clang/lib/Sema/SemaCodeComplete.cpp (+5-4)
  • (modified) clang/lib/Sema/SemaConcept.cpp (+103-74)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+5-2)
  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+5-2)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+10-8)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+9-7)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+9-137)
  • (modified) clang/lib/Sema/SemaType.cpp (+2-6)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+3-1)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+1)
  • (modified) clang/test/SemaCXX/cxx20-ctad-type-alias.cpp (+1-1)
  • (modified) clang/test/SemaCXX/fold_lambda_with_variadics.cpp (+29)
  • (modified) clang/unittests/AST/SourceLocationTest.cpp (+3-3)
diff --git a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
index ea4d99586c711..fb82efb4dd211 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
@@ -356,7 +356,7 @@ static std::vector<FixItHint> handleReturnType(const FunctionDecl *Function,
   if (!TypeText)
     return {};
 
-  SmallVector<const Expr *, 3> ExistingConstraints;
+  SmallVector<AssociatedConstraint, 3> ExistingConstraints;
   Function->getAssociatedConstraints(ExistingConstraints);
   if (!ExistingConstraints.empty()) {
     // FIXME - Support adding new constraints to existing ones. Do we need to
@@ -404,7 +404,7 @@ handleTrailingTemplateType(const FunctionTemplateDecl *FunctionTemplate,
   if (!ConditionText)
     return {};
 
-  SmallVector<const Expr *, 3> ExistingConstraints;
+  SmallVector<AssociatedConstraint, 3> ExistingConstraints;
   Function->getAssociatedConstraints(ExistingConstraints);
   if (!ExistingConstraints.empty()) {
     // FIXME - Support adding new constraints to existing ones. Do we need to
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8182bccdd2da8..954e4a1ed5a97 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -346,6 +346,8 @@ Bug Fixes to C++ Support
 - Clang now uses the parameter location for abbreviated function templates in ``extern "C"``. (#GH46386)
 - Clang will emit an error instead of crash when use co_await or co_yield in
   C++26 braced-init-list template parameter initialization. (#GH78426)
+- Improved fix for an issue with pack expansions of type constraints, where this
+  now also works if the constraint has non-type or template template parameters.
 - Fixes matching of nested template template parameters. (#GH130362)
 - Correctly diagnoses template template paramters which have a pack parameter
   not in the last position.
diff --git a/clang/include/clang/AST/ASTConcept.h b/clang/include/clang/AST/ASTConcept.h
index 00500e214f4ce..f89899c3ea7b1 100644
--- a/clang/include/clang/AST/ASTConcept.h
+++ b/clang/include/clang/AST/ASTConcept.h
@@ -229,12 +229,15 @@ class TypeConstraint {
   /// type-constraint.
   Expr *ImmediatelyDeclaredConstraint = nullptr;
   ConceptReference *ConceptRef;
+  int ArgumentPackSubstitutionIndex;
 
 public:
   TypeConstraint(ConceptReference *ConceptRef,
-                 Expr *ImmediatelyDeclaredConstraint)
+                 Expr *ImmediatelyDeclaredConstraint,
+                 int ArgumentPackSubstitutionIndex)
       : ImmediatelyDeclaredConstraint(ImmediatelyDeclaredConstraint),
-        ConceptRef(ConceptRef) {}
+        ConceptRef(ConceptRef),
+        ArgumentPackSubstitutionIndex(ArgumentPackSubstitutionIndex) {}
 
   /// \brief Get the immediately-declared constraint expression introduced by
   /// this type-constraint, that is - the constraint expression that is added to
@@ -245,6 +248,10 @@ class TypeConstraint {
 
   ConceptReference *getConceptReference() const { return ConceptRef; }
 
+  int getArgumentPackSubstitutionIndex() const {
+    return ArgumentPackSubstitutionIndex;
+  }
+
   // FIXME: Instead of using these concept related functions the callers should
   // directly work with the corresponding ConceptReference.
   ConceptDecl *getNamedConcept() const { return ConceptRef->getNamedConcept(); }
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index af8c49e99a7ce..1f7c75559e1e9 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1798,9 +1798,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
   QualType
   getSubstTemplateTypeParmType(QualType Replacement, Decl *AssociatedDecl,
                                unsigned Index,
-                               std::optional<unsigned> PackIndex,
-                               SubstTemplateTypeParmTypeFlag Flag =
-                                   SubstTemplateTypeParmTypeFlag::None) const;
+                               std::optional<unsigned> PackIndex) const;
   QualType getSubstTemplateTypeParmPackType(Decl *AssociatedDecl,
                                             unsigned Index, bool Final,
                                             const TemplateArgument &ArgPack);
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index efac36e49351e..22efb4f3ec3f9 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -78,6 +78,16 @@ class UnresolvedSetImpl;
 class VarTemplateDecl;
 enum class ImplicitParamKind;
 
+struct AssociatedConstraint {
+  const Expr *ConstraintExpr;
+  int ArgumentPackSubstitutionIndex;
+
+  constexpr AssociatedConstraint(const Expr *ConstraintExpr,
+                                 int ArgumentPackSubstitutionIndex)
+      : ConstraintExpr(ConstraintExpr),
+        ArgumentPackSubstitutionIndex(ArgumentPackSubstitutionIndex) {}
+};
+
 /// The top declaration context.
 class TranslationUnitDecl : public Decl,
                             public DeclContext,
@@ -2631,9 +2641,10 @@ class FunctionDecl : public DeclaratorDecl,
   ///
   /// Use this instead of getTrailingRequiresClause for concepts APIs that
   /// accept an ArrayRef of constraint expressions.
-  void getAssociatedConstraints(SmallVectorImpl<const Expr *> &AC) const {
+  void
+  getAssociatedConstraints(SmallVectorImpl<AssociatedConstraint> &AC) const {
     if (auto *TRC = getTrailingRequiresClause())
-      AC.push_back(TRC);
+      AC.emplace_back(TRC, /*ArgumentPackSubstitutionIndex=*/-1);
   }
 
   /// Get the message that indicates why this function was deleted.
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index b27e698236c02..6123273ce5ec1 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -195,7 +195,8 @@ class TemplateParameterList final
   ///
   /// The constraints in the resulting list are to be treated as if in a
   /// conjunction ("and").
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const;
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const;
 
   bool hasAssociatedConstraints() const;
 
@@ -422,7 +423,8 @@ class TemplateDecl : public NamedDecl {
   /// including constraint-expressions derived from the requires-clause,
   /// trailing requires-clause (for functions and methods) and constrained
   /// template parameters.
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const;
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const;
 
   bool hasAssociatedConstraints() const;
 
@@ -1341,7 +1343,8 @@ class TemplateTypeParmDecl final : public TypeDecl,
   }
 
   void setTypeConstraint(ConceptReference *CR,
-                         Expr *ImmediatelyDeclaredConstraint);
+                         Expr *ImmediatelyDeclaredConstraint,
+                         int ArgumentPackSubstitutionIndex);
 
   /// Determine whether this template parameter has a type-constraint.
   bool hasTypeConstraint() const {
@@ -1353,9 +1356,11 @@ class TemplateTypeParmDecl final : public TypeDecl,
   ///
   /// Use this instead of getTypeConstraint for concepts APIs that
   /// accept an ArrayRef of constraint expressions.
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const {
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const {
     if (HasTypeConstraint)
-      AC.push_back(getTypeConstraint()->getImmediatelyDeclaredConstraint());
+      AC.emplace_back(getTypeConstraint()->getImmediatelyDeclaredConstraint(),
+                      getTypeConstraint()->getArgumentPackSubstitutionIndex());
   }
 
   SourceRange getSourceRange() const override LLVM_READONLY;
@@ -1574,9 +1579,10 @@ class NonTypeTemplateParmDecl final
   ///
   /// Use this instead of getPlaceholderImmediatelyDeclaredConstraint for
   /// concepts APIs that accept an ArrayRef of constraint expressions.
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const {
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const {
     if (Expr *E = getPlaceholderTypeConstraint())
-      AC.push_back(E);
+      AC.emplace_back(E, /*ArgumentPackSubstitutionIndex=*/-1);
   }
 
   // Implement isa/cast/dyncast/etc.
@@ -2169,7 +2175,8 @@ class ClassTemplatePartialSpecializationDecl
   ///
   /// The constraints in the resulting list are to be treated as if in a
   /// conjunction ("and").
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const {
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const {
     TemplateParams->getAssociatedConstraints(AC);
   }
 
@@ -2943,7 +2950,8 @@ class VarTemplatePartialSpecializationDecl
   ///
   /// The constraints in the resulting list are to be treated as if in a
   /// conjunction ("and").
-  void getAssociatedConstraints(llvm::SmallVectorImpl<const Expr *> &AC) const {
+  void getAssociatedConstraints(
+      llvm::SmallVectorImpl<AssociatedConstraint> &AC) const {
     TemplateParams->getAssociatedConstraints(AC);
   }
 
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 42883b6419261..5f3a885832e2e 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -137,7 +137,6 @@ def Selector : PropertyType;
 def SourceLocation : PropertyType;
 def StmtRef : RefPropertyType<"Stmt"> { let ConstWhenWriting = 1; }
   def ExprRef : SubclassPropertyType<"Expr", StmtRef>;
-def SubstTemplateTypeParmTypeFlag : EnumPropertyType;
 def TemplateArgument : PropertyType;
 def TemplateArgumentKind : EnumPropertyType<"TemplateArgument::ArgKind">;
 def TemplateName : DefaultValuePropertyType;
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 65756203f2073..3b80f962a3bc6 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1786,15 +1786,6 @@ enum class AutoTypeKeyword {
   GNUAutoType
 };
 
-enum class SubstTemplateTypeParmTypeFlag {
-  None,
-
-  /// Whether to expand the pack using the stored PackIndex in place. This is
-  /// useful for e.g. substituting into an atomic constraint expression, where
-  /// that expression is part of an unexpanded pack.
-  ExpandPacksInPlace,
-};
-
 enum class ArraySizeModifier;
 enum class ElaboratedTypeKeyword;
 enum class VectorKind;
@@ -2164,9 +2155,6 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasNonCanonicalUnderlyingType : 1;
 
-    LLVM_PREFERRED_TYPE(SubstTemplateTypeParmTypeFlag)
-    unsigned SubstitutionFlag : 1;
-
     // The index of the template parameter this substitution represents.
     unsigned Index : 15;
 
@@ -6396,8 +6384,7 @@ class SubstTemplateTypeParmType final
   Decl *AssociatedDecl;
 
   SubstTemplateTypeParmType(QualType Replacement, Decl *AssociatedDecl,
-                            unsigned Index, std::optional<unsigned> PackIndex,
-                            SubstTemplateTypeParmTypeFlag Flag);
+                            unsigned Index, std::optional<unsigned> PackIndex);
 
 public:
   /// Gets the type that was substituted for the template
@@ -6426,31 +6413,21 @@ class SubstTemplateTypeParmType final
     return SubstTemplateTypeParmTypeBits.PackIndex - 1;
   }
 
-  SubstTemplateTypeParmTypeFlag getSubstitutionFlag() const {
-    return static_cast<SubstTemplateTypeParmTypeFlag>(
-        SubstTemplateTypeParmTypeBits.SubstitutionFlag);
-  }
-
   bool isSugared() const { return true; }
   QualType desugar() const { return getReplacementType(); }
 
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, getReplacementType(), getAssociatedDecl(), getIndex(),
-            getPackIndex(), getSubstitutionFlag());
+            getPackIndex());
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID, QualType Replacement,
                       const Decl *AssociatedDecl, unsigned Index,
-                      std::optional<unsigned> PackIndex,
-                      SubstTemplateTypeParmTypeFlag Flag) {
+                      std::optional<unsigned> PackIndex) {
     Replacement.Profile(ID);
     ID.AddPointer(AssociatedDecl);
     ID.AddInteger(Index);
     ID.AddInteger(PackIndex ? *PackIndex - 1 : 0);
-    ID.AddInteger(llvm::to_underlying(Flag));
-    assert((Flag != SubstTemplateTypeParmTypeFlag::ExpandPacksInPlace ||
-            PackIndex) &&
-           "ExpandPacksInPlace needs a valid PackIndex");
   }
 
   static bool classof(const Type *T) {
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 27f71bf5cc62f..4d89d8a47396a 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -827,14 +827,11 @@ let Class = SubstTemplateTypeParmType in {
   def : Property<"PackIndex", Optional<UInt32>> {
     let Read = [{ node->getPackIndex() }];
   }
-  def : Property<"SubstitutionFlag", SubstTemplateTypeParmTypeFlag> {
-    let Read = [{ node->getSubstitutionFlag() }];
-  }
 
   // The call to getCanonicalType here existed in ASTReader.cpp, too.
   def : Creator<[{
     return ctx.getSubstTemplateTypeParmType(
-        replacementType, associatedDecl, Index, PackIndex, SubstitutionFlag);
+        replacementType, associatedDecl, Index, PackIndex);
   }]>;
 }
 
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 066bce61c74c1..c98eea1c05060 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11350,7 +11350,6 @@ class Sema final : public SemaBase {
                             ConceptDecl *NamedConcept, NamedDecl *FoundDecl,
                             const TemplateArgumentListInfo *TemplateArgs,
                             TemplateTypeParmDecl *ConstrainedParameter,
-                            QualType ConstrainedType,
                             SourceLocation EllipsisLoc);
 
   bool AttachTypeConstraint(AutoTypeLoc TL,
@@ -14551,13 +14550,14 @@ class Sema final : public SemaBase {
   /// \returns true if an error occurred and satisfaction could not be checked,
   /// false otherwise.
   bool CheckConstraintSatisfaction(
-      const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
+      const NamedDecl *Template,
+      ArrayRef<AssociatedConstraint> AssociatedConstraints,
       const MultiLevelTemplateArgumentList &TemplateArgLists,
       SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction) {
     llvm::SmallVector<Expr *, 4> Converted;
-    return CheckConstraintSatisfaction(Template, ConstraintExprs, Converted,
-                                       TemplateArgLists, TemplateIDRange,
-                                       Satisfaction);
+    return CheckConstraintSatisfaction(Template, AssociatedConstraints,
+                                       Converted, TemplateArgLists,
+                                       TemplateIDRange, Satisfaction);
   }
 
   /// \brief Check whether the given list of constraint expressions are
@@ -14583,7 +14583,8 @@ class Sema final : public SemaBase {
   /// \returns true if an error occurred and satisfaction could not be checked,
   /// false otherwise.
   bool CheckConstraintSatisfaction(
-      const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
+      const NamedDecl *Template,
+      ArrayRef<AssociatedConstraint> AssociatedConstraints,
       llvm::SmallVectorImpl<Expr *> &ConvertedConstraints,
       const MultiLevelTemplateArgumentList &TemplateArgList,
       SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction);
@@ -14660,7 +14661,8 @@ class Sema final : public SemaBase {
                                 bool First = true);
 
   const NormalizedConstraint *getNormalizedAssociatedConstraints(
-      NamedDecl *ConstrainedDecl, ArrayRef<const Expr *> AssociatedConstraints);
+      NamedDecl *ConstrainedDecl,
+      ArrayRef<AssociatedConstraint> AssociatedConstraints);
 
   /// \brief Check whether the given declaration's associated constraints are
   /// at least as constrained than another declaration's according to the
@@ -14670,8 +14672,10 @@ class Sema final : public SemaBase {
   /// at least constrained than D2, and false otherwise.
   ///
   /// \returns true if an error occurred, false otherwise.
-  bool IsAtLeastAsConstrained(NamedDecl *D1, MutableArrayRef<const Expr *> AC1,
-                              NamedDecl *D2, MutableArrayRef<const Expr *> AC2,
+  bool IsAtLeastAsConstrained(NamedDecl *D1,
+                              MutableArrayRef<AssociatedConstraint> AC1,
+                              NamedDecl *D2,
+                              MutableArrayRef<AssociatedConstraint> AC2,
                               bool &Result);
 
   /// If D1 was not at least as constrained as D2, but would've been if a pair
@@ -14679,8 +14683,8 @@ class Sema final : public SemaBase {
   /// repeated in two separate places in code.
   /// \returns true if such a diagnostic was emitted, false otherwise.
   bool MaybeEmitAmbiguousAtomicConstraintsDiagnostic(
-      NamedDecl *D1, ArrayRef<const Expr *> AC1, NamedDecl *D2,
-      ArrayRef<const Expr *> AC2);
+      NamedDecl *D1, ArrayRef<AssociatedConstraint> AC1, NamedDecl *D2,
+      ArrayRef<AssociatedConstraint> AC2);
 
 private:
   /// Caches pairs of template-like decls whose associated constraints were
diff --git a/clang/include/clang/Sema/SemaConcept.h b/clang/include/clang/Sema/SemaConcept.h
index 5c599a70532f6..fe27a03df4863 100644
--- a/clang/include/clang/Sema/SemaConcept.h
+++ b/clang/include/clang/Sema/SemaConcept.h
@@ -29,12 +29,12 @@ class Sema;
 enum { ConstraintAlignment = 8 };
 
 struct alignas(ConstraintAlignment) AtomicConstraint {
-  const Expr *ConstraintExpr;
+  AssociatedConstraint AC;
   NamedDecl *ConstraintDecl;
   std::optional<ArrayRef<TemplateArgumentLoc>> ParameterMapping;
 
-  AtomicConstraint(const Expr *ConstraintExpr, NamedDecl *ConstraintDecl)
-      : ConstraintExpr(ConstraintExpr), ConstraintDecl(ConstraintDecl) {};
+  AtomicConstraint(const AssociatedConstraint &AC, NamedDecl *ConstraintDecl)
+      : AC(AC), ConstraintDecl(ConstraintDecl) {};
 
   bool hasMatchingParameterMapping(ASTContext &C,
                                    const AtomicConstraint &Other) const {
@@ -70,7 +70,14 @@ struct alignas(ConstraintAlignment) AtomicConstraint {
     // We do not actually substitute the parameter mappings into the
     // constraint expressions, therefore the constraint expressions are
     // the originals, and comparing them will suffice.
-    if (ConstraintExpr != Other.ConstraintExpr)
+    if (AC.ConstraintExpr != Other.AC.ConstraintExpr)
+      return false;
+
+    // FIXME: As the normalization cache doesn't take
+    // ArgumentPackSubstitutionIndex into account,
+    // this won't have an effect.
+    if (AC.ArgumentPackSubstitutionIndex !=
+        Other.AC.ArgumentPackSubstitutionIndex)
       return false;
 
     // Check that the parameter lists are identical
@@ -152,9 +159,11 @@ struct NormalizedConstraint {
 
 private:
   static std::optional<NormalizedConstraint>
-  fromConstraintExprs(Sema &S, NamedDecl *D, ArrayRef<const Expr *> E);
+  fromConstraintAssociatedConstraints(Sema &S, NamedDecl *D,
+                                      ArrayRef<AssociatedConstraint> ACs);
   static std::optional<NormalizedConstraint>
-  fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E);
+  fromConstraintAssociatedConstraint(Sema &S, NamedDecl *D,
+                                     AssociatedConstraint AC);
 };
 
 struct alignas(ConstraintAlignment) NormalizedConstraintPair {
@@ -180,7 +189,7 @@ struct alignas(ConstraintAlignment) FoldExpandedConstraint {
 
 const NormalizedConstraint *getNormalizedAssociatedConstraints(
     Sema &S, NamedDecl *ConstrainedDecl,
-    ArrayRef<const Expr *> AssociatedConstraints);
+    ArrayRef<AssociatedConstraint> AssociatedConstraints);
 
 template <typename AtomicSubsumptionEvaluator>
 bool subsumes(const NormalForm &PDNF, const NormalForm &QCNF,
@@ -226,8 +235,8 @@ bool subsumes(const NormalForm &PDNF, const NormalForm &QCNF,
 }
 
 template...
[truncated]

Copy link
Contributor

@cor3ntin cor3ntin 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 simplification but the changes to subsumption do not look correct to me

I also want to make sure @zyn0217 sees this

Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

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

Thanks for working on it.

The patch looks good overall, but as you've suggested that this is incomplete, I think we want to go over it again after the cache part gets adapted.

I left a question, for which we probably want an assertion there.

Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

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

And, I think this also merits a compile-time regression test - we now hold more bytes when instantiating the constraint

@mizvekov
Copy link
Contributor Author

This is a compile-time improvement overall.

The reverted patch adds a 0.07 cost to stage2-clang, which we avoid with the new approach:
Commit stage1-O3 stage1-ReleaseThinLTO stage1-ReleaseLTO-g stage1-O0-g stage2-O3 stage2-O0-g stage2-clang
9a089c5fbb 60976M (+0.00%) 77222M (-0.00%) 89531M (-0.01%) 18999M (+0.01%) 53187M (+0.01%) 16612M (-0.02%) 34778529M (+0.01%)
C e7ecd60e5f 60974M (-0.00%) 77223M (+0.01%) 89537M (+0.01%) 18998M (-0.01%) 53181M (-0.00%) 16615M (+0.01%) 34773327M (-0.07%)
C 17bbac4d1f 60975M (-0.01%) 77211M (-0.01%) 89525M (-0.01%) 19000M (-0.01%) 53184M (-0.03%) 16614M (-0.04%) 34798333M (-0.01%)

@mizvekov
Copy link
Contributor Author

The patch looks good overall, but as you've suggested that this is incomplete, I think we want to go over it again after the cache part gets adapted.

My intention is to leave the main branch in a better state, not necessarily to finish everything here in one go.
I think as far as the scope of the changes, this looks good.

@mizvekov mizvekov force-pushed the users/mizvekov/GH101754 branch from 9a089c5 to 6b4f7cb Compare March 24, 2025 12:19
Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

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

LGTM after @cor3ntin 's concerns are resolved

@mizvekov mizvekov force-pushed the users/mizvekov/GH101754 branch from c4cbf3b to 1b3a8e3 Compare March 27, 2025 01:06
@mizvekov
Copy link
Contributor Author

mizvekov commented Mar 27, 2025

I have also pushed a follow up to this patch, which extends the same fix for trailing requires clauses.
Here: #133190

@mizvekov mizvekov force-pushed the users/mizvekov/GH101754 branch from 1b3a8e3 to a2a372d Compare March 30, 2025 17:25
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.

Ah, looks like thanks to a troublesome github review-nonsense, I already reviewed this. No real new comments here, so make corentin happy + the few suggestions I made there (re-how they get constructed/etc), and I'm happy here too.

@mizvekov
Copy link
Contributor Author

mizvekov commented Apr 1, 2025

FYI the part of the PR @cor3ntin had concerns about is not part of this PR anymore.

So after addressing Eric's concerns on this patch I will go ahead and merge.

@erichkeane
Copy link
Collaborator

FYI the part of the PR @cor3ntin had concerns about is not part of this PR anymore.

So after addressing Eric's concerns on this patch I will go ahead and merge.

s/Eric/Erich, else SGTM.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

mizvekov added 2 commits April 1, 2025 19:24
This reverts an earlier attempt to support these expansions,
which was limited to type arguments and which subverted the purpose
of SubstTemplateTypeParmType.

This propagates the ArgumentPackSubstitutionIndex along with the
AssociatedConstraint, so that the pack expansion works, without
needing any new transforms or otherwise any changes to the template
instanntiation process.

This keeps the tests from the reverted commits, and adds a few more
showing the new solution also works for NTTPs.

This patch is incomplete:
* It is likely missing plumbing the ArgumentPackSubstitutionIndex
  into more places.
* The Normalization cache is not adapted so it indexes on the
  ArgumentPackSubstitutionIndex as well.

One new test is added, which in any case shows an ambiguous call,
but if the normalization cache were corrected, the reason for
ambighuity would change from subsumption both ways, to neither
subsumes the other.

I am not sure if the current language rules would allow
for a test case where the pack index would break an ambiguity,
this is left for future consideration.

Fixes #131798
@mizvekov mizvekov force-pushed the users/mizvekov/GH101754 branch from a2a372d to 6ba9bbe Compare April 1, 2025 22:48
@mizvekov mizvekov merged commit ad1ca5f into main Apr 2, 2025
10 of 12 checks passed
@mizvekov mizvekov deleted the users/mizvekov/GH101754 branch April 2, 2025 00:12
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request Apr 2, 2025
…132626)

This reverts an earlier attempt
(adb0d8d and
50e5411) to support these expansions,
which was limited to type arguments and which subverted the purpose
of SubstTemplateTypeParmType.

This propagates the ArgumentPackSubstitutionIndex along with the
AssociatedConstraint, so that the pack expansion works, without
needing any new transforms or otherwise any changes to the template
instantiation process.

This keeps the tests from the reverted commits, and adds a few more
showing the new solution also works for NTTPs.

Fixes llvm#131798
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:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tidy clang-tools-extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang] Crash involving use of NTTP pack in type constraint for lambda parameter
5 participants