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] Allow pack expansions when partial ordering against template template parameters #91833

Merged

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented May 11, 2024

When partial ordering alias templates against template template parameters, allow pack expansions when the alias has a fixed-size parameter list.

These expansions were generally disallowed by proposed resolution for CWG1430.

By previously diagnosing these when checking template template parameters, we would be too strict in trying to prevent any potential invalid use.

This flows against the more general idea that template template parameters are weakly typed, that we would rather allow an argument that might be possibly misused, and only diagnose the actual misuses during instantiation.

Since this interaction between P0522R0 and CWG1430 is also a backwards-compat breaking change, we implement provisional wording to allow these.

Fixes #62529
Fixes #91787

@mizvekov mizvekov self-assigned this May 11, 2024
@mizvekov mizvekov requested a review from Endilll as a code owner May 11, 2024 03:29
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 11, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 11, 2024

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

When partial ordering alias templates against template template parameters, allow pack expansions when the alias has a fixed-size parameter list.

These expansions were generally disallowed by proposed resolution for CWG1430.

By previously diagnosing these when checking template template parameters, we would be too strict in trying to prevent any potential invalid use.

This flows against the more general idea that template template parameters are weakly typed, that we would rather allow an argument that might be possibly misused, and only diagnose the actual misuses during instantiation.

Since this interaction between P0522R0 and CWG1430 is also a backwards-compat breaking change, we implement provisional wording to allow these.

Fixes #62529


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

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/include/clang/Sema/Sema.h (+7-1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+9-3)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+3-1)
  • (renamed) clang/test/SemaTemplate/temp_arg_template_p0522.cpp (+10-2)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7c5dcc59c7016..ddc5d5ff16a34 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -707,6 +707,7 @@ Bug Fixes to C++ Support
   initialized, rather than evaluating them as a part of the larger manifestly constant evaluated
   expression.
 - Fix a bug in access control checking due to dealyed checking of friend declaration. Fixes (#GH12361).
+- When partial ordering alias templates against template template parameters, allow pack expansions when the alias has a fixed-size parameter list. Fixes (#GH62529).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 4efd3878e861b..3fc59488243e5 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -9216,6 +9216,12 @@ class Sema final : public SemaBase {
   /// receive true if the cause for the error is the associated constraints of
   /// the template not being satisfied by the template arguments.
   ///
+  /// \param PartialOrderTTP If true, assume these template arguments are
+  /// the injected template arguments for a template template parameter.
+  /// This will relax the requirement that all it's possible uses are valid.
+  /// TTP checking is loose, and assumes that invalid uses will be diagnosed
+  /// during instantiation.
+  ///
   /// \returns true if an error occurred, false otherwise.
   bool CheckTemplateArgumentList(
       TemplateDecl *Template, SourceLocation TemplateLoc,
@@ -9223,7 +9229,7 @@ class Sema final : public SemaBase {
       SmallVectorImpl<TemplateArgument> &SugaredConverted,
       SmallVectorImpl<TemplateArgument> &CanonicalConverted,
       bool UpdateArgsWithConversions = true,
-      bool *ConstraintsNotSatisfied = nullptr);
+      bool *ConstraintsNotSatisfied = nullptr, bool PartialOrderTTP = false);
 
   bool CheckTemplateTypeArgument(
       TemplateTypeParmDecl *Param, TemplateArgumentLoc &Arg,
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 480c0103ae335..88d0af191e6c8 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -6436,7 +6436,8 @@ bool Sema::CheckTemplateArgumentList(
     TemplateArgumentListInfo &TemplateArgs, bool PartialTemplateArgs,
     SmallVectorImpl<TemplateArgument> &SugaredConverted,
     SmallVectorImpl<TemplateArgument> &CanonicalConverted,
-    bool UpdateArgsWithConversions, bool *ConstraintsNotSatisfied) {
+    bool UpdateArgsWithConversions, bool *ConstraintsNotSatisfied,
+    bool PartialOrderTTP) {
 
   if (ConstraintsNotSatisfied)
     *ConstraintsNotSatisfied = false;
@@ -6507,8 +6508,13 @@ bool Sema::CheckTemplateArgumentList(
       bool PackExpansionIntoNonPack =
           NewArgs[ArgIdx].getArgument().isPackExpansion() &&
           (!(*Param)->isTemplateParameterPack() || getExpandedPackSize(*Param));
-      if (PackExpansionIntoNonPack && (isa<TypeAliasTemplateDecl>(Template) ||
-                                       isa<ConceptDecl>(Template))) {
+      // Core issue 1430: Don't diagnose this pack expansion when partial
+      // ordering template template parameters. Some uses of the template could
+      // be valid, and invalid uses will be diagnosed later during
+      // instantiation.
+      if (PackExpansionIntoNonPack && !PartialOrderTTP &&
+          (isa<TypeAliasTemplateDecl>(Template) ||
+           isa<ConceptDecl>(Template))) {
         // Core issue 1430: we have a pack expansion as an argument to an
         // alias template, and it's not part of a parameter pack. This
         // can't be canonicalized, so reject it now.
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index fe7e35d841510..853c0e1b50619 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -6243,7 +6243,9 @@ bool Sema::isTemplateTemplateParameterAtLeastAsSpecializedAs(
     //   specialized as A.
     SmallVector<TemplateArgument, 4> SugaredPArgs;
     if (CheckTemplateArgumentList(AArg, Loc, PArgList, false, SugaredPArgs,
-                                  PArgs) ||
+                                  PArgs, /*UpdateArgsWithConversions=*/true,
+                                  /*ConstraintsNotSatisfied=*/nullptr,
+                                  /*PartialOrderTTP=*/true) ||
         Trap.hasErrorOccurred())
       return false;
   }
diff --git a/clang/test/SemaTemplate/temp_arg_template_cxx1z.cpp b/clang/test/SemaTemplate/temp_arg_template_p0522.cpp
similarity index 91%
rename from clang/test/SemaTemplate/temp_arg_template_cxx1z.cpp
rename to clang/test/SemaTemplate/temp_arg_template_p0522.cpp
index 372a00efc601e..251b6fc7d2be1 100644
--- a/clang/test/SemaTemplate/temp_arg_template_cxx1z.cpp
+++ b/clang/test/SemaTemplate/temp_arg_template_p0522.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++1z %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
 
-// expected-note@temp_arg_template_cxx1z.cpp:* 1+{{}}
+// expected-note@temp_arg_template_p0522.cpp:* 1+{{}}
 
 template<template<int> typename> struct Ti;
 template<template<int...> typename> struct TPi;
@@ -118,3 +118,11 @@ namespace Auto {
   TInt<SubstFailure> isf; // FIXME: this should be ill-formed
   TIntPtr<SubstFailure> ipsf;
 }
+
+namespace GH62529 {
+  // Note: the constraint here is just for bypassing a fast-path.
+  template<class T1> requires(true) using A = int;
+  template<template<class ...T2s> class TT1, class T3> struct B {};
+  template<class T4> B<A, T4> f();
+  auto t = f<int>();
+} // namespace GH62529

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.

Looks sensible to me but let's wait a bit (I'll try to run tests on stdexec with that change)

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved
clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved
clang/lib/Sema/SemaTemplate.cpp Outdated Show resolved Hide resolved
@cor3ntin
Copy link
Contributor

I have confirmed that with this change, stdexec compiles successfully, addressing both #89807 and #91787

@mizvekov mizvekov force-pushed the users/mizvekov/GH62529-tweak-CWG1430-TTP-partial-order branch from 98fdfb9 to f882dca Compare May 13, 2024 16:31
…template parameters

When partial ordering alias templates against template template parameters,
allow pack expansions when the alias has a fixed-size parameter list.

These expansions were generally disallowed by proposed resolution for CWG1430.

By previously diagnosing these when checking template template parameters, we
would be too strict in trying to prevent any potential invalid use.

This flows against the more general idea that template template parameters are
weakly typed, that we would rather allow an argument that might be possibly
misused, and only diagnose the actual misuses during instantiation.

Since this interaction between P0522R0 and CWG1430 is also a backwards-compat
breaking change, we implement provisional wording to allow these.

Fixes #62529
@mizvekov mizvekov force-pushed the users/mizvekov/GH62529-tweak-CWG1430-TTP-partial-order branch from f882dca to 06b9c19 Compare May 13, 2024 16:37
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 mizvekov merged commit b8f802f into main May 13, 2024
4 of 5 checks passed
@mizvekov mizvekov deleted the users/mizvekov/GH62529-tweak-CWG1430-TTP-partial-order branch May 13, 2024 17:28
nhasabni pushed a commit to nhasabni/llvm-project that referenced this pull request May 14, 2024
…template parameters (llvm#91833)

When partial ordering alias templates against template template
parameters, allow pack expansions when the alias has a fixed-size
parameter list.

These expansions were generally disallowed by proposed resolution for
CWG1430.

By previously diagnosing these when checking template template
parameters, we would be too strict in trying to prevent any potential
invalid use.

This flows against the more general idea that template template
parameters are weakly typed, that we would rather allow an argument that
might be possibly misused, and only diagnose the actual misuses during
instantiation.

Since this interaction between P0522R0 and CWG1430 is also a
backwards-compat breaking change, we implement provisional wording to
allow these.

Fixes llvm#62529
mub-at-arm pushed a commit to mub-at-arm/llvm-project that referenced this pull request May 16, 2024
…template parameters (llvm#91833)

When partial ordering alias templates against template template
parameters, allow pack expansions when the alias has a fixed-size
parameter list.

These expansions were generally disallowed by proposed resolution for
CWG1430.

By previously diagnosing these when checking template template
parameters, we would be too strict in trying to prevent any potential
invalid use.

This flows against the more general idea that template template
parameters are weakly typed, that we would rather allow an argument that
might be possibly misused, and only diagnose the actual misuses during
instantiation.

Since this interaction between P0522R0 and CWG1430 is also a
backwards-compat breaking change, we implement provisional wording to
allow these.

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