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] Finish implementation of P0522 #96023

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

mizvekov
Copy link
Contributor

This finishes the clang implementation of P0522, getting rid of the fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters, we would perform, in order:

  • If the old rules would match, we would accept it. Otherwise, don't generate diagnostics yet.
  • If the new rules would match, just accept it. Otherwise, don't generate any diagnostics yet again.
  • Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:

  • Accept some things we shouldn't.
  • Reject some things we shouldn't.
  • Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite, which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed, with related test suite churn.

The problem here is that the old rules were very simple and non-recursive, making it easy to provide customized diagnostics, and to keep them consistent with each other.

The new rules are a lot more complex and rely on template argument deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error produced there unmodified, and just attach notes to it explaining that it occurred in the context of partial ordering this template argument against that template parameter.

This diverges from the old diagnostics, which would lead with an error pointing to the template argument, explain the problem in subsequent notes, and produce a final note pointing to the parameter.

@mizvekov mizvekov self-assigned this Jun 19, 2024
@mizvekov mizvekov requested a review from Endilll as a code owner June 19, 2024 05:11
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Jun 19, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 19, 2024

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

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

This finishes the clang implementation of P0522, getting rid of the fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters, we would perform, in order:

  • If the old rules would match, we would accept it. Otherwise, don't generate diagnostics yet.
  • If the new rules would match, just accept it. Otherwise, don't generate any diagnostics yet again.
  • Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:

  • Accept some things we shouldn't.
  • Reject some things we shouldn't.
  • Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite, which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed, with related test suite churn.

The problem here is that the old rules were very simple and non-recursive, making it easy to provide customized diagnostics, and to keep them consistent with each other.

The new rules are a lot more complex and rely on template argument deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error produced there unmodified, and just attach notes to it explaining that it occurred in the context of partial ordering this template argument against that template parameter.

This diverges from the old diagnostics, which would lead with an error pointing to the template argument, explain the problem in subsequent notes, and produce a final note pointing to the parameter.


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

17 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+8-1)
  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+7)
  • (modified) clang/include/clang/Sema/Sema.h (+12-2)
  • (modified) clang/lib/Frontend/FrontendActions.cpp (+2)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+40-55)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+242-99)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+15)
  • (modified) clang/test/CXX/temp/temp.arg/temp.arg.template/p3-0x.cpp (+18-13)
  • (modified) clang/test/CXX/temp/temp.param/p12.cpp (+11-10)
  • (modified) clang/test/Modules/cxx-templates.cpp (+3-12)
  • (modified) clang/test/SemaCXX/make_integer_seq.cpp (+2-3)
  • (modified) clang/test/SemaTemplate/cwg2398.cpp (+23-7)
  • (modified) clang/test/SemaTemplate/temp_arg_nontype.cpp (+20-25)
  • (modified) clang/test/SemaTemplate/temp_arg_template.cpp (+23-15)
  • (modified) clang/test/SemaTemplate/temp_arg_template_p0522.cpp (+44-26)
  • (modified) clang/test/Templight/templight-empty-entries-fix.cpp (+12)
  • (modified) clang/test/Templight/templight-prior-template-arg.cpp (+23-10)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7112d1f889fef..abe535f55fb2a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -176,6 +176,8 @@ C++17 Feature Support
   the values produced by GCC, so these macros should not be used from header
   files because they may not be stable across multiple TUs (the values may vary
   based on compiler version as well as CPU tuning). #GH60174
+- The implementation of the relaxed template template argument matching rules is
+  more complete and reliable, and should provide more accurate diagnostics.
 
 C++14 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
@@ -589,6 +591,10 @@ Improvements to Clang's diagnostics
 - Clang no longer emits a "declared here" note for a builtin function that has no declaration in source.
   Fixes #GH93369.
 
+- Clang now properly explains the reason a template template argument failed to
+  match a template template parameter, in terms of the C++17 relaxed matching rules
+  instead of the old ones.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
@@ -887,7 +893,8 @@ Bug Fixes to C++ Support
   between the addresses of two labels (a GNU extension) to a pointer within a constant expression. (#GH95366).
 - Fix immediate escalation bugs in the presence of dependent call arguments. (#GH94935)
 - Clang now diagnoses explicit specializations with storage class specifiers in all contexts.
-
+- Fixes to several issues in partial ordering of template template parameters, which
+  were documented in the test suite.
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 14736784cff5f..397d10d4036f0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5207,6 +5207,13 @@ def note_template_arg_refers_here_func : Note<
 def err_template_arg_template_params_mismatch : Error<
   "template template argument has different template parameters than its "
   "corresponding template template parameter">;
+def note_template_arg_template_params_mismatch : Note<
+  "template template argument has different template parameters than its "
+  "corresponding template template parameter">;
+def err_non_deduced_mismatch : Error<
+  "could not match %diff{$ against $|types}0,1">;
+def err_inconsistent_deduction : Error<
+  "conflicting deduction %diff{$ against $|types}0,1 for parameter">;
 def err_template_arg_not_integral_or_enumeral : Error<
   "non-type template argument of type %0 must have an integral or enumeration"
   " type">;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a847fb8fe5500..7022da6dcee1d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -9728,8 +9728,9 @@ class Sema final : public SemaBase {
                                     sema::TemplateDeductionInfo &Info);
 
   bool isTemplateTemplateParameterAtLeastAsSpecializedAs(
-      TemplateParameterList *PParam, TemplateDecl *AArg,
-      const DefaultArguments &DefaultArgs, SourceLocation Loc, bool IsDeduced);
+      TemplateParameterList *PParam, TemplateDecl *PArg, TemplateDecl *AArg,
+      const DefaultArguments &DefaultArgs, SourceLocation ArgLoc,
+      bool IsDeduced);
 
   void MarkUsedTemplateParameters(const Expr *E, bool OnlyDeduced,
                                   unsigned Depth, llvm::SmallBitVector &Used);
@@ -9934,6 +9935,9 @@ class Sema final : public SemaBase {
 
       /// We are instantiating a type alias template declaration.
       TypeAliasTemplateInstantiation,
+
+      /// We are performing partial ordering for template template parameters.
+      PartialOrderTTP,
     } Kind;
 
     /// Was the enclosing context a non-instantiation SFINAE context?
@@ -10155,6 +10159,12 @@ class Sema final : public SemaBase {
                           TemplateDecl *Entity, BuildingDeductionGuidesTag,
                           SourceRange InstantiationRange = SourceRange());
 
+    struct PartialOrderTTP {};
+    /// \brief Note that we are partial ordering template template parameters.
+    InstantiatingTemplate(Sema &SemaRef, SourceLocation ArgLoc, PartialOrderTTP,
+                          TemplateDecl *PArg,
+                          SourceRange InstantiationRange = SourceRange());
+
     /// Note that we have finished instantiating this template.
     void Clear();
 
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 4f064321997a2..d31b704f186a6 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -456,6 +456,8 @@ class DefaultTemplateInstCallback : public TemplateInstantiationCallback {
       return "BuildingDeductionGuides";
     case CodeSynthesisContext::TypeAliasTemplateInstantiation:
       return "TypeAliasTemplateInstantiation";
+    case CodeSynthesisContext::PartialOrderTTP:
+      return "PartialOrderTTP";
     }
     return "";
   }
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 2296750cebc72..c9806afc1a32d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -6652,8 +6652,7 @@ bool Sema::CheckTemplateArgumentList(
         DefaultArgs && ParamIdx >= DefaultArgs.StartPos) {
       // All written arguments should have been consumed by this point.
       assert(ArgIdx == NumArgs && "bad default argument deduction");
-      // FIXME: Don't ignore parameter packs.
-      if (ParamIdx == DefaultArgs.StartPos && !(*Param)->isParameterPack()) {
+      if (ParamIdx == DefaultArgs.StartPos) {
         assert(Param + DefaultArgs.Args.size() <= ParamEnd);
         // Default arguments from a DeducedTemplateName are already converted.
         for (const TemplateArgument &DefArg : DefaultArgs.Args) {
@@ -6897,8 +6896,9 @@ bool Sema::CheckTemplateArgumentList(
   // pack expansions; they might be empty. This can happen even if
   // PartialTemplateArgs is false (the list of arguments is complete but
   // still dependent).
-  if (ArgIdx < NumArgs && CurrentInstantiationScope &&
-      CurrentInstantiationScope->getPartiallySubstitutedPack()) {
+  if (PartialOrderingTTP ||
+      (CurrentInstantiationScope &&
+       CurrentInstantiationScope->getPartiallySubstitutedPack())) {
     while (ArgIdx < NumArgs &&
            NewArgs[ArgIdx].getArgument().isPackExpansion()) {
       const TemplateArgument &Arg = NewArgs[ArgIdx++].getArgument();
@@ -8513,64 +8513,49 @@ bool Sema::CheckTemplateTemplateArgument(TemplateTemplateParmDecl *Param,
       << Template;
   }
 
+  if (!getLangOpts().RelaxedTemplateTemplateArgs)
+    return !TemplateParameterListsAreEqual(
+        Template->getTemplateParameters(), Params, /*Complain=*/true,
+        TPL_TemplateTemplateArgumentMatch, Arg.getLocation());
+
   // C++1z [temp.arg.template]p3: (DR 150)
   //   A template-argument matches a template template-parameter P when P
   //   is at least as specialized as the template-argument A.
-  if (getLangOpts().RelaxedTemplateTemplateArgs) {
-    // Quick check for the common case:
-    //   If P contains a parameter pack, then A [...] matches P if each of A's
-    //   template parameters matches the corresponding template parameter in
-    //   the template-parameter-list of P.
-    if (TemplateParameterListsAreEqual(
-            Template->getTemplateParameters(), Params, false,
-            TPL_TemplateTemplateArgumentMatch, Arg.getLocation()) &&
-        // If the argument has no associated constraints, then the parameter is
-        // definitely at least as specialized as the argument.
-        // Otherwise - we need a more thorough check.
-        !Template->hasAssociatedConstraints())
-      return false;
 
-    if (isTemplateTemplateParameterAtLeastAsSpecializedAs(
-            Params, Template, DefaultArgs, Arg.getLocation(), IsDeduced)) {
-      // P2113
-      // C++20[temp.func.order]p2
-      //   [...] If both deductions succeed, the partial ordering selects the
-      // more constrained template (if one exists) as determined below.
-      SmallVector<const Expr *, 3> ParamsAC, TemplateAC;
-      Params->getAssociatedConstraints(ParamsAC);
-      // C++2a[temp.arg.template]p3
-      //   [...] In this comparison, if P is unconstrained, the constraints on A
-      //   are not considered.
-      if (ParamsAC.empty())
-        return false;
+  // FIXME: Create context for recursive diagnostics, in the PartialOrderingTTP
+  // case.
+  if (!isTemplateTemplateParameterAtLeastAsSpecializedAs(
+          Params, Param, Template, DefaultArgs, Arg.getLocation(), IsDeduced))
+    return true;
+  // P2113
+  // C++20[temp.func.order]p2
+  //   [...] If both deductions succeed, the partial ordering selects the
+  // more constrained template (if one exists) as determined below.
+  SmallVector<const Expr *, 3> ParamsAC, TemplateAC;
+  Params->getAssociatedConstraints(ParamsAC);
+  // C++2a[temp.arg.template]p3
+  //   [...] In this comparison, if P is unconstrained, the constraints on A
+  //   are not considered.
+  if (ParamsAC.empty())
+    return false;
 
-      Template->getAssociatedConstraints(TemplateAC);
+  Template->getAssociatedConstraints(TemplateAC);
 
-      bool IsParamAtLeastAsConstrained;
-      if (IsAtLeastAsConstrained(Param, ParamsAC, Template, TemplateAC,
-                                 IsParamAtLeastAsConstrained))
-        return true;
-      if (!IsParamAtLeastAsConstrained) {
-        Diag(Arg.getLocation(),
-             diag::err_template_template_parameter_not_at_least_as_constrained)
-            << Template << Param << Arg.getSourceRange();
-        Diag(Param->getLocation(), diag::note_entity_declared_at) << Param;
-        Diag(Template->getLocation(), diag::note_entity_declared_at)
-            << Template;
-        MaybeEmitAmbiguousAtomicConstraintsDiagnostic(Param, ParamsAC, Template,
-                                                      TemplateAC);
-        return true;
-      }
-      return false;
-    }
-    // FIXME: Produce better diagnostics for deduction failures.
+  bool IsParamAtLeastAsConstrained;
+  if (IsAtLeastAsConstrained(Param, ParamsAC, Template, TemplateAC,
+                             IsParamAtLeastAsConstrained))
+    return true;
+  if (!IsParamAtLeastAsConstrained) {
+    Diag(Arg.getLocation(),
+         diag::err_template_template_parameter_not_at_least_as_constrained)
+        << Template << Param << Arg.getSourceRange();
+    Diag(Param->getLocation(), diag::note_entity_declared_at) << Param;
+    Diag(Template->getLocation(), diag::note_entity_declared_at) << Template;
+    MaybeEmitAmbiguousAtomicConstraintsDiagnostic(Param, ParamsAC, Template,
+                                                  TemplateAC);
+    return true;
   }
-
-  return !TemplateParameterListsAreEqual(Template->getTemplateParameters(),
-                                         Params,
-                                         true,
-                                         TPL_TemplateTemplateArgumentMatch,
-                                         Arg.getLocation());
+  return false;
 }
 
 static Sema::SemaDiagnosticBuilder noteLocation(Sema &S, const NamedDecl &Decl,
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 3efc3030fc261..cc9d54d35fda8 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -139,7 +139,7 @@ static TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
     SmallVectorImpl<DeducedTemplateArgument> &Deduced, unsigned TDF,
     bool PartialOrdering = false, bool DeducedFromArrayBound = false);
 
-enum class PackFold { ParameterToArgument, ArgumentToParameter };
+enum class PackFold { ParameterToArgument, ArgumentToParameter, Both };
 static TemplateDeductionResult
 DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
                         ArrayRef<TemplateArgument> Ps,
@@ -1640,7 +1640,18 @@ static TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
     DeducedTemplateArgument Result =
         checkDeducedTemplateArguments(S.Context, Deduced[Index], NewDeduced);
     if (Result.isNull()) {
-      Info.Param = cast<TemplateTypeParmDecl>(TemplateParams->getParam(Index));
+      // We can also get inconsistencies when matching NTTP type.
+      switch (NamedDecl *Param = TemplateParams->getParam(Index);
+              Param->getKind()) {
+      case Decl::TemplateTypeParm:
+        Info.Param = cast<TemplateTypeParmDecl>(Param);
+        break;
+      case Decl::NonTypeTemplateParm:
+        Info.Param = cast<NonTypeTemplateParmDecl>(Param);
+        break;
+      default:
+        llvm_unreachable("unexpected kind");
+      }
       Info.FirstArg = Deduced[Index];
       Info.SecondArg = NewDeduced;
       return TemplateDeductionResult::Inconsistent;
@@ -2426,8 +2437,29 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
     if (const NonTypeTemplateParmDecl *NTTP =
             getDeducedParameterFromExpr(Info, P.getAsExpr())) {
       switch (A.getKind()) {
+      case TemplateArgument::Expression: {
+        const Expr *E = A.getAsExpr();
+        // When checking NTTP, if either the parameter or the argument is
+        // dependent, as there would be otherwise nothing to deduce, we force
+        // the argument to the parameter type using this dependent implicit
+        // cast, in order to maintain invariants. Now we can deduce the
+        // resulting type from the original type, and deduce the original type
+        // against the parameter we are checking.
+        if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E);
+            ICE && ICE->getCastKind() == clang::CK_Dependent) {
+          E = ICE->getSubExpr();
+          if (auto Result = DeduceTemplateArgumentsByTypeMatch(
+                  S, TemplateParams, ICE->getType(), E->getType(), Info,
+                  Deduced, TDF_SkipNonDependent, /*PartialOrdering=*/false,
+                  /*DeducedFromArrayBound=*/false);
+              Result != TemplateDeductionResult::Success)
+            return Result;
+        }
+        return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP,
+                                             DeducedTemplateArgument(A),
+                                             E->getType(), Info, Deduced);
+      }
       case TemplateArgument::Integral:
-      case TemplateArgument::Expression:
       case TemplateArgument::StructuralValue:
         return DeduceNonTypeTemplateArgument(
             S, TemplateParams, NTTP, DeducedTemplateArgument(A),
@@ -2513,49 +2545,76 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
                         TemplateDeductionInfo &Info,
                         SmallVectorImpl<DeducedTemplateArgument> &Deduced,
                         bool NumberOfArgumentsMustMatch, PackFold PackFold) {
-  if (PackFold == PackFold::ArgumentToParameter)
-    std::swap(Ps, As);
+  bool FoldPackParameter = PackFold == PackFold::ParameterToArgument ||
+                           PackFold == PackFold::Both,
+       FoldPackArgument = PackFold == PackFold::ArgumentToParameter ||
+                          PackFold == PackFold::Both;
+
   // C++0x [temp.deduct.type]p9:
   //   If the template argument list of P contains a pack expansion that is not
   //   the last template argument, the entire template argument list is a
   //   non-deduced context.
-  if (hasPackExpansionBeforeEnd(Ps))
+  if (FoldPackParameter && hasPackExpansionBeforeEnd(Ps))
+    return TemplateDeductionResult::Success;
+
+  if (FoldPackArgument && hasPackExpansionBeforeEnd(As))
     return TemplateDeductionResult::Success;
 
   // C++0x [temp.deduct.type]p9:
   //   If P has a form that contains <T> or <i>, then each argument Pi of the
   //   respective template argument list P is compared with the corresponding
   //   argument Ai of the corresponding template argument list of A.
-  unsigned ArgIdx = 0, ParamIdx = 0;
-  for (; hasTemplateArgumentForDeduction(Ps, ParamIdx); ++ParamIdx) {
-    const TemplateArgument &P = Ps[ParamIdx];
-    if (!P.isPackExpansion()) {
+  for (unsigned ArgIdx = 0, ParamIdx = 0; /**/; /**/) {
+    if (!hasTemplateArgumentForDeduction(Ps, ParamIdx))
+      return !FoldPackParameter && NumberOfArgumentsMustMatch &&
+                     hasTemplateArgumentForDeduction(As, ArgIdx) &&
+                     !As[ArgIdx].isPackExpansion()
+                 ? TemplateDeductionResult::MiscellaneousDeductionFailure
+                 : TemplateDeductionResult::Success;
+
+    if (!Ps[ParamIdx].isPackExpansion()) {
       // The simple case: deduce template arguments by matching Pi and Ai.
 
       // Check whether we have enough arguments.
       if (!hasTemplateArgumentForDeduction(As, ArgIdx))
-        return NumberOfArgumentsMustMatch
+        return !FoldPackArgument && NumberOfArgumentsMustMatch
                    ? TemplateDeductionResult::MiscellaneousDeductionFailure
                    : TemplateDeductionResult::Success;
 
-      // C++1z [temp.deduct.type]p9:
-      //   During partial ordering, if Ai was originally a pack expansion [and]
-      //   Pi is not a pack expansion, template argument deduction fails.
-      if (As[ArgIdx].isPackExpansion())
-        return TemplateDeductionResult::MiscellaneousDeductionFailure;
+      if (As[ArgIdx].isPackExpansion()) {
+        // C++1z [temp.deduct.type]p9:
+        //   During partial ordering, if Ai was originally a pack expansion
+        //   [and] Pi is not a pack expansion, template argument deduction
+        //   fails.
+        if (!FoldPackArgument)
+          return TemplateDeductionResult::MiscellaneousDeductionFailure;
+
+        for (TemplateArgument Pattern = As[ArgIdx].getPackExpansionPattern();
+             /**/;
+             /**/) {
+          // Deduce template parameters from the pattern.
+          if (auto Result = DeduceTemplateArguments(
+                  S, TemplateParams, Ps[ParamIdx], Pattern, Info, Deduced);
+              Result != TemplateDeductionResult::Success)
+            return Result;
 
-      // Perform deduction for this Pi/Ai pair.
-      TemplateArgument Pi = P, Ai = As[ArgIdx];
-      if (PackFold == PackFold::ArgumentToParameter)
-        std::swap(Pi, Ai);
-      if (auto Result =
-              DeduceTemplateArguments(S, TemplateParams, Pi, Ai, Info, Deduced);
-          Result != TemplateDeductionResult::Success)
-        return Result;
+          ++ParamIdx;
+          if (!hasTemplateArgumentForDeduction(Ps, ParamIdx))
+            return TemplateDeductionResult::Success;
+          if (Ps[ParamIdx].isPackExpansion())
+            break;
+        }
+      } else {
+        // Perform deduction for this Pi/Ai pair.
+        if (auto Result = DeduceTemplateArguments(
+                S, TemplateParams, Ps[ParamIdx], As[ArgIdx], Info, Deduced);
+            Result != TemplateDeductionResult::Success)
+          return Result;
 
-      // Move to the next argument.
-      ++ArgIdx;
-      continue;
+        ++ArgIdx;
+        ++ParamIdx;
+        continue;
+      }
     }
 
     // The parameter is a pack expansion.
@@ -2565,7 +2624,7 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
     //   each remaining argument in the template argument list of A. Each
     //   comparison deduces template arguments for subsequent positions in the
     //   template parameter packs expanded by Pi.
-    TemplateArgument Pattern = P.getPackExpansionPattern();
+    TemplateArgument Pattern = Ps[ParamIdx].getPackExpansionPattern();
 
     // Prepare to deduce the packs within the pattern.
     PackDeductionScope PackScope(S, TemplateParams, Deduced, Info, Pattern);
@@ -2576,12 +2635,11 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
     for (; hasTemplateArgumentForDeduction(As, ArgIdx) &&
            PackScope.hasNextElement();
          ++ArgIdx) {
-      TemplateArgument Pi = Pattern, Ai = As[ArgIdx];
-      if (PackFold == PackFold::ArgumentToParameter)
-        std::swap(Pi, Ai);
+      if (!FoldPackParameter && !As[ArgIdx].isPackExpansion())
+        return TemplateDeductionResult::MiscellaneousDeductionFailure;
       // Deduce template arguments from the pattern.
-      if (auto Result...
[truncated]

@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch from 453df30 to 03d5720 Compare June 19, 2024 05:16
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.

Thanks Matheus.
I left a few nitpicky comments.
The design looks good to me I think but it's not a trivial change so Ill want to review it more and have other people look at it too.

clang/lib/Sema/SemaTemplate.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaTemplateDeduction.cpp Show resolved Hide resolved
clang/lib/Sema/SemaTemplateDeduction.cpp Outdated Show resolved Hide resolved
// The simple case: deduce template arguments by matching Pi and Ai.

// Check whether we have enough arguments.
if (!hasTemplateArgumentForDeduction(As, ArgIdx))
return NumberOfArgumentsMustMatch
return !FoldPackArgument && NumberOfArgumentsMustMatch
? TemplateDeductionResult::MiscellaneousDeductionFailure
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be IncompletePack (rather than MiscellaneousDeductionFailure)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pre-existing error we have used for this is MiscellaneousDeductionFailure. IncompletePack means something else, it's used when we have an already expanded pack and we failed to deduce all elements of it.

This is a separate thing we have to fix, basically get rid of the MiscellaneousDeductionFailure, create new representations for all the separate things it was used for, and remove the generic bad deduction diagnostic.

But this would touch tests far outside of the scope of this patch, so I think it's best left for a separate PR.

clang/lib/Sema/SemaTemplateDeduction.cpp Show resolved Hide resolved
Comment on lines +6119 to +6227

if (Result != TemplateDeductionResult::Success)
return false;

if (Trap.hasErrorOccurred())
return false;

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines +6461 to +6555
case TemplateDeductionResult::MiscellaneousDeductionFailure:
Diag(AArg->getLocation(), diag::err_template_param_list_different_arity)
<< (A->size() > P->size()) << /*isTemplateTemplateParameter=*/true
<< SourceRange(A->getTemplateLoc(), P->getRAngleLoc());
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

So now i think that either

  • We should use IncompletePack, or
  • Introduce a new enumerator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The right thing would be to introduce a new enumerator, but I think this patch keeps the status quo while being large enough already.

Comment on lines +6478 to +6582
// None of these should happen for a plain deduction.
case TemplateDeductionResult::Invalid:
case TemplateDeductionResult::InstantiationDepth:
case TemplateDeductionResult::Incomplete:
case TemplateDeductionResult::IncompletePack:
case TemplateDeductionResult::Underqualified:
case TemplateDeductionResult::SubstitutionFailure:
case TemplateDeductionResult::DeducedMismatch:
case TemplateDeductionResult::DeducedMismatchNested:
case TemplateDeductionResult::TooManyArguments:
case TemplateDeductionResult::TooFewArguments:
case TemplateDeductionResult::InvalidExplicitArguments:
case TemplateDeductionResult::NonDependentConversionFailure:
case TemplateDeductionResult::ConstraintsNotSatisfied:
case TemplateDeductionResult::CUDATargetMismatch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well just use default here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding default would suppress the warning you get when one of the enumerators is missing a case. I think this is an immensely useful warning, it helps you not forget to look for and do something about each place the enumerator could be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the sheer amount of enumerators here is an indication of this growing without careful design.

We could probably refactor this and collapse a lot of these cases into fewer cases, and such.

I think it would also be helpful to not have one TemplateDeductionResult to rule them all, when some times you are dealing with the subset of the problem and a lot of these cases don't apply at all.

Comment on lines +6519 to +6623
case TemplateDeductionResult::Invalid:
case TemplateDeductionResult::Incomplete:
case TemplateDeductionResult::IncompletePack:
case TemplateDeductionResult::Inconsistent:
case TemplateDeductionResult::Underqualified:
case TemplateDeductionResult::DeducedMismatch:
case TemplateDeductionResult::DeducedMismatchNested:
case TemplateDeductionResult::TooManyArguments:
case TemplateDeductionResult::TooFewArguments:
case TemplateDeductionResult::InvalidExplicitArguments:
case TemplateDeductionResult::NonDependentConversionFailure:
case TemplateDeductionResult::ConstraintsNotSatisfied:
case TemplateDeductionResult::MiscellaneousDeductionFailure:
case TemplateDeductionResult::CUDATargetMismatch:
llvm_unreachable("Unexpected Result");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

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

Sema.h changes look good.

@Endilll Endilll requested a review from Sirraide June 19, 2024 09:49
@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch from 03d5720 to 7c84cd6 Compare June 19, 2024 18:21
@mizvekov mizvekov requested a review from a team as a code owner June 19, 2024 18:21
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 19, 2024
@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch from 7c84cd6 to dd761ef Compare June 19, 2024 18:54
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

libcxx/ nit LGTM.

@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch from dd761ef to e5df110 Compare June 21, 2024 20:43
@mizvekov
Copy link
Contributor Author

@ldionne I had to adjust the libcxx expectation change.

The simple regex won't work, as the error diagnostic is now produced in different source locations.

I have changed it so it uses a preprocessor conditional, but that new solution will still break if libcxx-CI is using an outdated trunk clang.

Let me know if you have a better idea.

@mizvekov mizvekov force-pushed the users/mizvekov/clang-cwg2398-ttp-matches-class-template branch 2 times, most recently from 63d51f7 to 9d06fd6 Compare July 30, 2024 03:54
@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch 3 times, most recently from 7e98320 to be948d5 Compare July 31, 2024 16:04
@mizvekov mizvekov force-pushed the users/mizvekov/clang-cwg2398-ttp-matches-class-template branch from 9d06fd6 to 3f95419 Compare August 20, 2024 05:25
Base automatically changed from users/mizvekov/clang-cwg2398-ttp-matches-class-template to main September 7, 2024 18:49
This finishes the clang implementation of P0522, getting rid
of the fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters,
we would perform, in order:
* If the old rules would match, we would accept it. Otherwise, don't
  generate diagnostics yet.
* If the new rules would match, just accept it. Otherwise, don't
  generate any diagnostics yet again.
* Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:
* Accept some things we shouldn't.
* Reject some things we shouldn't.
* Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order
to accept the historial rule for TTP matching pack parameter to non-pack
arguments.
This change also makes us accept some combinations of historical and P0522
allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite,
which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed,
with related test suite churn.

The problem here is that the old rules were very simple and
non-recursive, making it easy to provide customized diagnostics,
and to keep them consistent with each other.

The new rules are a lot more complex and rely on template argument
deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics,
and create a new instantiation context that keeps track of this context.

So for example when a substitution failure occurs, we use the error
produced there unmodified, and just attach notes to it explaining
that it occurred in the context of partial ordering this template
argument against that template parameter.

This diverges from the old diagnostics, which would lead with an
error pointing to the template argument, explain the problem
in subsequent notes, and produce a final note pointing to the parameter.
@mizvekov mizvekov force-pushed the users/mizvekov/clang-p0522-complete-implementation branch from 958703d to b29e3d3 Compare September 7, 2024 18:52
@cor3ntin
Copy link
Contributor

cor3ntin commented Sep 7, 2024

My concern here is that there is an ongoing lively discussion in CWG. And while I think most solutions considered will end up being isomorphic in outcome, I don't know that we want to go experimenting too much before the dust settles a bit (there is wild variation between implementations both pre and post P0522)
@zygoloid @erichkeane

@mizvekov
Copy link
Contributor Author

mizvekov commented Sep 7, 2024

I believe we have achieved consensus on getting rid of the fallback rules, and this patch does not do much beyond that, if anything.

@mizvekov
Copy link
Contributor Author

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'm OK with this as long as it is the direction CWG is going.

@mizvekov mizvekov merged commit 6afe567 into main Oct 1, 2024
9 checks passed
@mizvekov mizvekov deleted the users/mizvekov/clang-p0522-complete-implementation branch October 1, 2024 23:50
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
This finishes the clang implementation of P0522, getting rid of the
fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters,
we would perform, in order:
* If the old rules would match, we would accept it. Otherwise, don't
generate diagnostics yet.
* If the new rules would match, just accept it. Otherwise, don't
generate any diagnostics yet again.
* Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:
* Accept some things we shouldn't.
* Reject some things we shouldn't.
* Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the
historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and
P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite,
which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed,
with related test suite churn.

The problem here is that the old rules were very simple and
non-recursive, making it easy to provide customized diagnostics, and to
keep them consistent with each other.

The new rules are a lot more complex and rely on template argument
deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and
create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error
produced there unmodified, and just attach notes to it explaining that
it occurred in the context of partial ordering this template argument
against that template parameter.

This diverges from the old diagnostics, which would lead with an error
pointing to the template argument, explain the problem in subsequent
notes, and produce a final note pointing to the parameter.
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
This finishes the clang implementation of P0522, getting rid of the
fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters,
we would perform, in order:
* If the old rules would match, we would accept it. Otherwise, don't
generate diagnostics yet.
* If the new rules would match, just accept it. Otherwise, don't
generate any diagnostics yet again.
* Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:
* Accept some things we shouldn't.
* Reject some things we shouldn't.
* Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the
historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and
P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite,
which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed,
with related test suite churn.

The problem here is that the old rules were very simple and
non-recursive, making it easy to provide customized diagnostics, and to
keep them consistent with each other.

The new rules are a lot more complex and rely on template argument
deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and
create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error
produced there unmodified, and just attach notes to it explaining that
it occurred in the context of partial ordering this template argument
against that template parameter.

This diverges from the old diagnostics, which would lead with an error
pointing to the template argument, explain the problem in subsequent
notes, and produce a final note pointing to the parameter.
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
This finishes the clang implementation of P0522, getting rid of the
fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters,
we would perform, in order:
* If the old rules would match, we would accept it. Otherwise, don't
generate diagnostics yet.
* If the new rules would match, just accept it. Otherwise, don't
generate any diagnostics yet again.
* Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:
* Accept some things we shouldn't.
* Reject some things we shouldn't.
* Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the
historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and
P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite,
which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed,
with related test suite churn.

The problem here is that the old rules were very simple and
non-recursive, making it easy to provide customized diagnostics, and to
keep them consistent with each other.

The new rules are a lot more complex and rely on template argument
deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and
create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error
produced there unmodified, and just attach notes to it explaining that
it occurred in the context of partial ordering this template argument
against that template parameter.

This diverges from the old diagnostics, which would lead with an error
pointing to the template argument, explain the problem in subsequent
notes, and produce a final note pointing to the parameter.
mizvekov added a commit that referenced this pull request Oct 3, 2024
…ck argument

This fixes a regression introduced in #96023, reported in
#110231 (comment)
mizvekov added a commit that referenced this pull request Oct 3, 2024
…ck argument

This fixes a regression introduced in #96023, reported in
#110231 (comment)
mizvekov added a commit that referenced this pull request Oct 3, 2024
…ck argument (#110963)

This fixes a regression introduced in #96023, reported in
#110231 (comment)
@zmodem
Copy link
Collaborator

zmodem commented Oct 4, 2024

We're hitting a new error after this change. I'm not familiar with P0522, can you confirm whether foo(bad) is supposed to work or not?

template <template <typename T, typename... Ms> typename H, typename T, typename... Ms>
void foo(H<T, Ms...> h) {}

template <typename T, typename... Ms> class Good {};

template <typename T> class Bad {};

void bar() {
  // This works.
  Good<int> good;
  foo(good);

  // This used to work, but no longer.
  Bad<int> bad;
  foo(bad);
}

(For what it's worth, GCC and MSVC still accept both variants: https://godbolt.org/z/zEhhfa6GW)

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
This finishes the clang implementation of P0522, getting rid of the
fallback to the old, pre-P0522 rules.

Before this patch, when partial ordering template template parameters,
we would perform, in order:
* If the old rules would match, we would accept it. Otherwise, don't
generate diagnostics yet.
* If the new rules would match, just accept it. Otherwise, don't
generate any diagnostics yet again.
* Apply the old rules again, this time with diagnostics.

This situation was far from ideal, as we would sometimes:
* Accept some things we shouldn't.
* Reject some things we shouldn't.
* Only diagnose rejection in terms of the old rules.

With this patch, we apply the P0522 rules throughout.

This needed to extend template argument deduction in order to accept the
historial rule for TTP matching pack parameter to non-pack arguments.
This change also makes us accept some combinations of historical and
P0522 allowances we wouldn't before.

It also fixes a bunch of bugs that were documented in the test suite,
which I am not sure there are issues already created for them.

This causes a lot of changes to the way these failures are diagnosed,
with related test suite churn.

The problem here is that the old rules were very simple and
non-recursive, making it easy to provide customized diagnostics, and to
keep them consistent with each other.

The new rules are a lot more complex and rely on template argument
deduction, substitutions, and they are recursive.

The approach taken here is to mostly rely on existing diagnostics, and
create a new instantiation context that keeps track of things.

So for example when a substitution failure occurs, we use the error
produced there unmodified, and just attach notes to it explaining that
it occurred in the context of partial ordering this template argument
against that template parameter.

This diverges from the old diagnostics, which would lead with an error
pointing to the template argument, explain the problem in subsequent
notes, and produce a final note pointing to the parameter.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
@zmodem
Copy link
Collaborator

zmodem commented Oct 7, 2024

@mizvekov ping ^

@cor3ntin
Copy link
Contributor

cor3ntin commented Oct 7, 2024

Yes, it is supposed to work.
Can you look into it @mizvekov ?

@mizvekov
Copy link
Contributor Author

mizvekov commented Oct 7, 2024

Ops, somehow missed the original notification.
Will look into it, thanks for the report.

@zmodem
Copy link
Collaborator

zmodem commented Oct 7, 2024

Thanks for confirming! Please consider reverting in the meantime to keep trunk green.

@mizvekov
Copy link
Contributor Author

mizvekov commented Oct 8, 2024

This will be fixed by #111457

@zmodem
Copy link
Collaborator

zmodem commented Oct 9, 2024

I'll revert to unbreak trunk in the meantime.

zmodem added a commit that referenced this pull request Oct 9, 2024
This caused Clang to reject valid code, see discussion on the PR
#96023 (comment)
and #111363

This reverts commit 6afe567 and
follow-up commit 9abb97f.
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 libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants