[Clang] Normalize constraints before checking for satisfaction (llvm#161671)

In the standard, constraint satisfaction checking is done on the
normalized form of a constraint.

Clang instead substitutes on the non-normalized form, which causes us to
report substitution failures in template arguments or concept ids, which
is non-conforming but unavoidable without a parameter mapping

This patch normalizes before satisfaction checking. However, we preserve
concept-id nodes in the normalized form, solely for diagnostics
purposes.

This addresses llvm#61811 and
related concepts conformance bugs, ideally to make the remaining
implementation of concept template parameters easier

Fixes llvm#135190
Fixes llvm#61811

Co-authored-by: Younan Zhang
[[email protected]](mailto:[email protected])

---------

Co-authored-by: Younan Zhang <[email protected]>

Author: 2 people

clang/docs/InternalsManual.rst

@@ -2859,6 +2859,67 @@ This library is called by the :ref:`Parser library <Parser>` during parsing to
 do semantic analysis of the input.  For valid programs, Sema builds an AST for
 parsed constructs.
 
+
+Concept Satisfaction Checking and Subsumption
+---------------------------------------------
+
+As per the C++ standard, constraints are `normalized <https://eel.is/c++draft/temp.constr.normal>`_
+and the normal form is used both for subsumption, and constraint checking.
+Both depend on a parameter mapping that substitutes lazily. In particular,
+we should not substitute in unused arguments.
+
+Clang follows the order of operations prescribed by the standard.
+
+Normalization happens prior to satisfaction and subsumption
+and is handled by ``NormalizedConstraint``.
+
+Clang preserves in the normalized form intermediate concept-ids
+(``ConceptIdConstraint``) This is used for diagnostics only and no substitution
+happens in a ConceptIdConstraint if its expression is satisfied.
+
+The normal form of the associated constraints of a declaration is cached in
+Sema::NormalizationCache such that it is only computed once.
+
+A ``NormalizedConstraint`` is a recursive data structure, where each node
+contains a parameter mapping, represented by the indexes of all parameter
+being used.
+
+Checking satisfaction is done by ``ConstraintSatisfactionChecker``, recursively
+walking ``NormalizedConstraint``. At each level, we substitute the outermost
+level of the template arguments referenced in the parameter mapping of a
+normalized expression (``MultiLevelTemplateArgumentList``).
+
+For the following example,
+
+.. code-block:: c++
+
+  template <typename T>
+  concept A = __is_same(T, int);
+
+  template <typename U>
+  concept B = A<U> && __is_same(U, int);
+
+The normal form of B is
+
+.. code-block:: c++
+
+    __is_same(T, int) /*T->U, innermost level*/
+ && __is_same(U, int) {U->U} /*T->U, outermost level*/
+
+After substitution in the mapping, we substitute in the constraint expression
+using that copy of the ``MultiLevelTemplateArgumentList``, and then evaluate it.
+
+Because this is expensive, it is cached in
+``UnsubstitutedConstraintSatisfactionCache``.
+
+Any error during satisfaction is recorded in ``ConstraintSatisfaction``.
+for nested requirements, ``ConstraintSatisfaction`` is stored (including
+diagnostics) in the AST, which is something we might want to improve.
+
+When an atomic constraint is not satified, we try to substitute into any
+enclosing concept-id using the same mechanism described above, for
+diagnostics purpose, and inject that in the ``ConstraintSatisfaction``.
+
 .. _CodeGen:
 
 The CodeGen Library

clang/docs/ReleaseNotes.rst

@@ -160,6 +160,10 @@ C++23 Feature Support
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
+- Clang now normalizes constraints before checking whether they are satisfied, as mandated by the standard.
+  As a result, Clang no longer incorrectly diagnoses substitution failures in template arguments only
+  used in concept-ids, and produces better diagnostics for satisfaction failure. (#GH61811) (#GH135190)
+
 C++17 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 

clang/include/clang/AST/ASTConcept.h

@@ -28,10 +28,20 @@ namespace clang {
 
 class ConceptDecl;
 class TemplateDecl;
+class ConceptReference;
 class Expr;
 class NamedDecl;
 struct PrintingPolicy;
 
+/// Unsatisfied constraint expressions if the template arguments could be
+/// substituted into them, or a diagnostic if substitution resulted in
+/// an invalid expression.
+///
+using ConstraintSubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
+using UnsatisfiedConstraintRecord =
+    llvm::PointerUnion<const Expr *, const ConceptReference *,
+                       const ConstraintSubstitutionDiagnostic *>;
+
 /// The result of a constraint satisfaction check, containing the necessary
 /// information to diagnose an unsatisfied constraint.
 class ConstraintSatisfaction : public llvm::FoldingSetNode {
@@ -48,16 +58,13 @@ class ConstraintSatisfaction : public llvm::FoldingSetNode {
                          ArrayRef<TemplateArgument> TemplateArgs)
       : ConstraintOwner(ConstraintOwner), TemplateArgs(TemplateArgs) {}
 
-  using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
-  using Detail = llvm::PointerUnion<Expr *, SubstitutionDiagnostic *>;
-
   bool IsSatisfied = false;
   bool ContainsErrors = false;
 
   /// \brief The substituted constraint expr, if the template arguments could be
   /// substituted into them, or a diagnostic if substitution resulted in an
   /// invalid expression.
-  llvm::SmallVector<Detail, 4> Details;
+  llvm::SmallVector<UnsatisfiedConstraintRecord, 4> Details;
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &C) {
     Profile(ID, C, ConstraintOwner, TemplateArgs);
@@ -69,19 +76,12 @@ class ConstraintSatisfaction : public llvm::FoldingSetNode {
 
   bool HasSubstitutionFailure() {
     for (const auto &Detail : Details)
-      if (Detail.dyn_cast<SubstitutionDiagnostic *>())
+      if (Detail.dyn_cast<const ConstraintSubstitutionDiagnostic *>())
         return true;
     return false;
   }
 };
 
-/// Pairs of unsatisfied atomic constraint expressions along with the
-/// substituted constraint expr, if the template arguments could be
-/// substituted into them, or a diagnostic if substitution resulted in
-/// an invalid expression.
-using UnsatisfiedConstraintRecord =
-    llvm::PointerUnion<Expr *, std::pair<SourceLocation, StringRef> *>;
-
 /// \brief The result of a constraint satisfaction check, containing the
 /// necessary information to diagnose an unsatisfied constraint.
 ///
@@ -101,6 +101,10 @@ struct ASTConstraintSatisfaction final :
     return getTrailingObjects() + NumRecords;
   }
 
+  ArrayRef<UnsatisfiedConstraintRecord> records() const {
+    return {begin(), end()};
+  }
+
   ASTConstraintSatisfaction(const ASTContext &C,
                             const ConstraintSatisfaction &Satisfaction);
   ASTConstraintSatisfaction(const ASTContext &C,
@@ -282,6 +286,11 @@ class TypeConstraint {
   }
 };
 
+/// Insertion operator for diagnostics.  This allows sending ConceptReferences's
+/// into a diagnostic with <<.
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+                                      const ConceptReference *C);
+
 } // clang
 
 #endif // LLVM_CLANG_AST_ASTCONCEPT_H

clang/include/clang/AST/ASTContext.h

@@ -3877,7 +3877,6 @@ typename clang::LazyGenerationalUpdatePtr<Owner, T, Update>::ValueType
     return new (Ctx) LazyData(Source, Value);
   return Value;
 }
-
 template <> struct llvm::DenseMapInfo<llvm::FoldingSetNodeID> {
   static FoldingSetNodeID getEmptyKey() { return FoldingSetNodeID{}; }
 

clang/include/clang/Sema/Sema.h

@@ -65,6 +65,7 @@
 #include "clang/Sema/Redeclaration.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/SemaBase.h"
+#include "clang/Sema/SemaConcept.h"
 #include "clang/Sema/TypoCorrection.h"
 #include "clang/Sema/Weak.h"
 #include "llvm/ADT/APInt.h"
@@ -11694,8 +11695,9 @@ class Sema final : public SemaBase {
   ExprResult
   CheckConceptTemplateId(const CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
                          const DeclarationNameInfo &ConceptNameInfo,
-                         NamedDecl *FoundDecl, ConceptDecl *NamedConcept,
-                         const TemplateArgumentListInfo *TemplateArgs);
+                         NamedDecl *FoundDecl, TemplateDecl *NamedConcept,
+                         const TemplateArgumentListInfo *TemplateArgs,
+                         bool DoCheckConstraintSatisfaction = true);
 
   void diagnoseMissingTemplateArguments(TemplateName Name, SourceLocation Loc);
   void diagnoseMissingTemplateArguments(const CXXScopeSpec &SS,
@@ -12025,6 +12027,13 @@ class Sema final : public SemaBase {
                                  bool UpdateArgsWithConversions = true,
                                  bool *ConstraintsNotSatisfied = nullptr);
 
+  bool CheckTemplateArgumentList(
+      TemplateDecl *Template, TemplateParameterList *Params,
+      SourceLocation TemplateLoc, TemplateArgumentListInfo &TemplateArgs,
+      const DefaultArguments &DefaultArgs, bool PartialTemplateArgs,
+      CheckTemplateArgumentInfo &CTAI, bool UpdateArgsWithConversions = true,
+      bool *ConstraintsNotSatisfied = nullptr);
+
   bool CheckTemplateTypeArgument(
       TemplateTypeParmDecl *Param, TemplateArgumentLoc &Arg,
       SmallVectorImpl<TemplateArgument> &SugaredConverted,
@@ -12783,6 +12792,18 @@ class Sema final : public SemaBase {
   void MarkUsedTemplateParameters(const Expr *E, bool OnlyDeduced,
                                   unsigned Depth, llvm::SmallBitVector &Used);
 
+  /// Mark which template parameters are named in a given expression.
+  ///
+  /// Unlike MarkUsedTemplateParameters, this excludes parameter that
+  /// are used but not directly named by an expression - i.e. it excludes
+  /// any template parameter that denotes the type of a referenced NTTP.
+  ///
+  /// \param Used a bit vector whose elements will be set to \c true
+  /// to indicate when the corresponding template parameter will be
+  /// deduced.
+  void MarkUsedTemplateParametersForSubsumptionParameterMapping(
+      const Expr *E, unsigned Depth, llvm::SmallBitVector &Used);
+
   /// Mark which template parameters can be deduced from a given
   /// template argument list.
   ///
@@ -12799,6 +12820,9 @@ class Sema final : public SemaBase {
   void MarkUsedTemplateParameters(ArrayRef<TemplateArgument> TemplateArgs,
                                   unsigned Depth, llvm::SmallBitVector &Used);
 
+  void MarkUsedTemplateParameters(ArrayRef<TemplateArgumentLoc> TemplateArgs,
+                                  unsigned Depth, llvm::SmallBitVector &Used);
+
   void
   MarkDeducedTemplateParameters(const FunctionTemplateDecl *FunctionTemplate,
                                 llvm::SmallBitVector &Deduced) {
@@ -13096,6 +13120,9 @@ class Sema final : public SemaBase {
     /// Whether we're substituting into constraints.
     bool InConstraintSubstitution;
 
+    /// Whether we're substituting into the parameter mapping of a constraint.
+    bool InParameterMappingSubstitution;
+
     /// The point of instantiation or synthesis within the source code.
     SourceLocation PointOfInstantiation;
 
@@ -13146,8 +13173,10 @@ class Sema final : public SemaBase {
     CodeSynthesisContext()
         : Kind(TemplateInstantiation),
           SavedInNonInstantiationSFINAEContext(false),
-          InConstraintSubstitution(false), Entity(nullptr), Template(nullptr),
-          TemplateArgs(nullptr), NumTemplateArgs(0), DeductionInfo(nullptr) {}
+          InConstraintSubstitution(false),
+          InParameterMappingSubstitution(false), Entity(nullptr),
+          Template(nullptr), TemplateArgs(nullptr), NumTemplateArgs(0),
+          DeductionInfo(nullptr) {}
 
     /// Determines whether this template is an actual instantiation
     /// that should be counted toward the maximum instantiation depth.
@@ -13359,6 +13388,11 @@ class Sema final : public SemaBase {
                          const MultiLevelTemplateArgumentList &TemplateArgs,
                          TemplateArgumentListInfo &Outputs);
 
+  bool SubstTemplateArgumentsInParameterMapping(
+      ArrayRef<TemplateArgumentLoc> Args, SourceLocation BaseLoc,
+      const MultiLevelTemplateArgumentList &TemplateArgs,
+      TemplateArgumentListInfo &Out, bool BuildPackExpansionTypes);
+
   /// Retrieve the template argument list(s) that should be used to
   /// instantiate the definition of the given declaration.
   ///
@@ -13820,6 +13854,12 @@ class Sema final : public SemaBase {
            CodeSynthesisContexts.back().InConstraintSubstitution;
   }
 
+  bool inParameterMappingSubstitution() const {
+    return !CodeSynthesisContexts.empty() &&
+           CodeSynthesisContexts.back().InParameterMappingSubstitution &&
+           !inConstraintSubstitution();
+  }
+
   using EntityPrinter = llvm::function_ref<void(llvm::raw_ostream &)>;
 
   /// \brief create a Requirement::SubstitutionDiagnostic with only a
@@ -14704,6 +14744,10 @@ class Sema final : public SemaBase {
     SatisfactionStack.swap(NewSS);
   }
 
+  using ConstrainedDeclOrNestedRequirement =
+      llvm::PointerUnion<const NamedDecl *,
+                         const concepts::NestedRequirement *>;
+
   /// Check whether the given expression is a valid constraint expression.
   /// A diagnostic is emitted if it is not, false is returned, and
   /// PossibleNonPrimary will be set to true if the failure might be due to a
@@ -14728,44 +14772,12 @@ class Sema final : public SemaBase {
   /// \returns true if an error occurred and satisfaction could not be checked,
   /// false otherwise.
   bool CheckConstraintSatisfaction(
-      const NamedDecl *Template,
+      ConstrainedDeclOrNestedRequirement Entity,
       ArrayRef<AssociatedConstraint> AssociatedConstraints,
       const MultiLevelTemplateArgumentList &TemplateArgLists,
-      SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction) {
-    llvm::SmallVector<Expr *, 4> Converted;
-    return CheckConstraintSatisfaction(Template, AssociatedConstraints,
-                                       Converted, TemplateArgLists,
-                                       TemplateIDRange, Satisfaction);
-  }
-
-  /// \brief Check whether the given list of constraint expressions are
-  /// satisfied (as if in a 'conjunction') given template arguments.
-  /// Additionally, takes an empty list of Expressions which is populated with
-  /// the instantiated versions of the ConstraintExprs.
-  /// \param Template the template-like entity that triggered the constraints
-  /// check (either a concept or a constrained entity).
-  /// \param ConstraintExprs a list of constraint expressions, treated as if
-  /// they were 'AND'ed together.
-  /// \param ConvertedConstraints a out parameter that will get populated with
-  /// the instantiated version of the ConstraintExprs if we successfully checked
-  /// satisfaction.
-  /// \param TemplateArgList the multi-level list of template arguments to
-  /// substitute into the constraint expression. This should be relative to the
-  /// top-level (hence multi-level), since we need to instantiate fully at the
-  /// time of checking.
-  /// \param TemplateIDRange The source range of the template id that
-  /// caused the constraints check.
-  /// \param Satisfaction if true is returned, will contain details of the
-  /// satisfaction, with enough information to diagnose an unsatisfied
-  /// expression.
-  /// \returns true if an error occurred and satisfaction could not be checked,
-  /// false otherwise.
-  bool CheckConstraintSatisfaction(
-      const NamedDecl *Template,
-      ArrayRef<AssociatedConstraint> AssociatedConstraints,
-      llvm::SmallVectorImpl<Expr *> &ConvertedConstraints,
-      const MultiLevelTemplateArgumentList &TemplateArgList,
-      SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction);
+      SourceRange TemplateIDRange, ConstraintSatisfaction &Satisfaction,
+      const ConceptReference *TopLevelConceptId = nullptr,
+      Expr **ConvertedExpr = nullptr);
 
   /// \brief Check whether the given non-dependent constraint expression is
   /// satisfied. Returns false and updates Satisfaction with the satisfaction
@@ -14831,16 +14843,17 @@ class Sema final : public SemaBase {
   /// \param First whether this is the first time an unsatisfied constraint is
   /// diagnosed for this error.
   void DiagnoseUnsatisfiedConstraint(const ConstraintSatisfaction &Satisfaction,
+                                     SourceLocation Loc = {},
                                      bool First = true);
 
   /// \brief Emit diagnostics explaining why a constraint expression was deemed
   /// unsatisfied.
   void
-  DiagnoseUnsatisfiedConstraint(const ASTConstraintSatisfaction &Satisfaction,
+  DiagnoseUnsatisfiedConstraint(const ConceptSpecializationExpr *ConstraintExpr,
                                 bool First = true);
 
   const NormalizedConstraint *getNormalizedAssociatedConstraints(
-      const NamedDecl *ConstrainedDecl,
+      ConstrainedDeclOrNestedRequirement Entity,
       ArrayRef<AssociatedConstraint> AssociatedConstraints);
 
   /// \brief Check whether the given declaration's associated constraints are
@@ -14865,6 +14878,15 @@ class Sema final : public SemaBase {
       const NamedDecl *D1, ArrayRef<AssociatedConstraint> AC1,
       const NamedDecl *D2, ArrayRef<AssociatedConstraint> AC2);
 
+  /// Cache the satisfaction of an atomic constraint.
+  /// The key is based on the unsubstituted expression and the parameter
+  /// mapping. This lets us not substituting the mapping more than once,
+  /// which is (very!) expensive.
+  /// FIXME: this should be private.
+  llvm::DenseMap<llvm::FoldingSetNodeID,
+                 UnsubstitutedConstraintSatisfactionCacheResult>
+      UnsubstitutedConstraintSatisfactionCache;
+
 private:
   /// Caches pairs of template-like decls whose associated constraints were
   /// checked for subsumption and whether or not the first's constraints did in
@@ -14875,8 +14897,11 @@ class Sema final : public SemaBase {
   /// constrained declarations). If an error occurred while normalizing the
   /// associated constraints of the template or concept, nullptr will be cached
   /// here.
-  llvm::DenseMap<const NamedDecl *, NormalizedConstraint *> NormalizationCache;
+  llvm::DenseMap<ConstrainedDeclOrNestedRequirement, NormalizedConstraint *>
+      NormalizationCache;
 
+  /// Cache whether the associated constraint of a declaration
+  /// is satisfied.
   llvm::ContextualFoldingSet<ConstraintSatisfaction, const ASTContext &>
       SatisfactionCache;