Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Clang][Sema] Do not mark template parameters in the exception specification as used during partial ordering #91534

Merged
merged 2 commits into from
May 15, 2024

Conversation

sdkrystian
Copy link
Member

We do not deduce template arguments from the exception specification when determining the primary template of a function template specialization or when taking the address of a function template. Therefore, this patch changes isAtLeastAsSpecializedAs such that we do not mark template parameters in the exception specification as 'used' during partial ordering (per [temp.deduct.partial] p12) to prevent the following from being ambiguous:

template<typename T, typename U>
void f(U) noexcept(noexcept(T())); // #1

template<typename T>
void f(T*) noexcept; // #2

template<>
void f<int>(int*) noexcept; // currently ambiguous, selects #2 with this patch applied 

Although there is no corresponding wording in the standard, this seems to be the intended behavior given the definition of deduction substitution loci in [temp.deduct.general] p7 (and EDG does the same thing).

@sdkrystian sdkrystian requested a review from erichkeane May 8, 2024 20:56
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 8, 2024
@llvmbot
Copy link
Member

llvmbot commented May 8, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

We do not deduce template arguments from the exception specification when determining the primary template of a function template specialization or when taking the address of a function template. Therefore, this patch changes isAtLeastAsSpecializedAs such that we do not mark template parameters in the exception specification as 'used' during partial ordering (per [[temp.deduct.partial] p12](http://eel.is/c++draft/temp.deduct.partial#12)) to prevent the following from being ambiguous:

template&lt;typename T, typename U&gt;
void f(U) noexcept(noexcept(T())); // #<!-- -->1

template&lt;typename T&gt;
void f(T*) noexcept; // #<!-- -->2

template&lt;&gt;
void f&lt;int&gt;(int*) noexcept; // currently ambiguous, selects #<!-- -->2 with this patch applied 

Although there is no corresponding wording in the standard, this seems to be the intended behavior given the definition of deduction substitution loci in [[temp.deduct.general] p7](http://eel.is/c++draft/temp.deduct.general#7) (and EDG does the same thing).


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+28-8)
  • (added) clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p3.cpp (+72)
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index fe7e35d841510..c17c5838803a8 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5453,7 +5453,7 @@ static bool isAtLeastAsSpecializedAs(Sema &S, SourceLocation Loc,
     //     is used.
     if (DeduceTemplateArgumentsByTypeMatch(
             S, TemplateParams, FD2->getType(), FD1->getType(), Info, Deduced,
-            TDF_None,
+            TDF_AllowCompatibleFunctionType,
             /*PartialOrdering=*/true) != TemplateDeductionResult::Success)
       return false;
     break;
@@ -5485,20 +5485,40 @@ static bool isAtLeastAsSpecializedAs(Sema &S, SourceLocation Loc,
   switch (TPOC) {
   case TPOC_Call:
     for (unsigned I = 0, N = Args2.size(); I != N; ++I)
-      ::MarkUsedTemplateParameters(S.Context, Args2[I], false,
-                                   TemplateParams->getDepth(),
-                                   UsedParameters);
+      ::MarkUsedTemplateParameters(S.Context, Args2[I], /*OnlyDeduced=*/false,
+                                   TemplateParams->getDepth(), UsedParameters);
     break;
 
   case TPOC_Conversion:
-    ::MarkUsedTemplateParameters(S.Context, Proto2->getReturnType(), false,
+    ::MarkUsedTemplateParameters(S.Context, Proto2->getReturnType(),
+                                 /*OnlyDeduced=*/false,
                                  TemplateParams->getDepth(), UsedParameters);
     break;
 
   case TPOC_Other:
-    ::MarkUsedTemplateParameters(S.Context, FD2->getType(), false,
-                                 TemplateParams->getDepth(),
-                                 UsedParameters);
+    // We do not deduce template arguments from the exception specification
+    // when determining the primary template of a function template
+    // specialization or when taking the address of a function template.
+    // Therefore, we do not mark template parameters in the exception
+    // specification as used during partial ordering to prevent the following
+    // from being ambiguous:
+    //
+    //   template<typename T, typename U>
+    //   void f(U) noexcept(noexcept(T())); // #1
+    //
+    //   template<typename T>
+    //   void f(T*) noexcept; // #2
+    //
+    //   template<>
+    //   void f<int>(int*) noexcept; // explicit specialization of #2
+    //
+    // Although there is no corresponding wording in the standard, this seems
+    // to be the intended behavior given the definition of
+    // 'deduction substitution loci' in [temp.deduct].
+    ::MarkUsedTemplateParameters(
+        S.Context,
+        S.Context.getFunctionTypeWithExceptionSpec(FD2->getType(), EST_None),
+        /*OnlyDeduced=*/false, TemplateParams->getDepth(), UsedParameters);
     break;
   }
 
diff --git a/clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p3.cpp b/clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p3.cpp
new file mode 100644
index 0000000000000..cc1d4ecda2ecc
--- /dev/null
+++ b/clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/p3.cpp
@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template<bool B>
+struct A { };
+
+constexpr A<false> a;
+constexpr A<false> b;
+
+constexpr int* x = nullptr;
+constexpr short* y = nullptr;
+
+namespace ExplicitArgs {
+  template<typename T, typename U>
+  constexpr int f(U) noexcept(noexcept(T())) {
+    return 0;
+  }
+
+  template<typename T>
+  constexpr int f(T*) noexcept {
+    return 1;
+  }
+
+  template<>
+  constexpr int f<int>(int*) noexcept {
+    return 2;
+  }
+
+  static_assert(f<int>(1) == 0);
+  static_assert(f<short>(y) == 1);
+  static_assert(f<int>(x) == 2);
+
+  template<typename T, typename U>
+  constexpr int g(U*) noexcept(noexcept(T())) {
+    return 3;
+  }
+
+  template<typename T>
+  constexpr int g(T) noexcept {
+    return 4;
+  }
+
+  template<>
+  constexpr int g<int>(int*) noexcept {
+    return 5;
+  }
+
+  static_assert(g<int>(y) == 3);
+  static_assert(g<short>(1) == 4);
+  static_assert(g<int>(x) == 5);
+} // namespace ExplicitArgs
+
+namespace DeducedArgs {
+  template<typename T, bool B>
+  constexpr int f(T, A<B>) noexcept(B) {
+    return 0;
+  }
+
+  template<typename T, bool B>
+  constexpr int f(T*, A<B>) noexcept(B && B) {
+    return 1;
+  }
+
+  template<>
+  constexpr int f(int*, A<false>) {
+    return 2;
+  }
+
+  static_assert(f<int*>(x, a) == 0);
+  static_assert(f<short>(y, a) == 1);
+  static_assert(f<int>(x, a) == 2);
+} // namespace DeducedArgs

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.

This probably also needs a release note.

// template<>
// void f<int>(int*) noexcept; // explicit specialization of #2
//
// Although there is no corresponding wording in the standard, this seems
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please file a core issue here! It would be great to get this fixed in CWG. @Endilll or @shafik can help with that if you need it. Also, put the CWG issue # in this comment once we have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open it tomorrow

Copy link
Member Author

Choose a reason for hiding this comment

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

@erichkeane Filed an issue here. It might be a little while before an actual core issue is opened, so would it be alright to merge this before that happens?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Jens is usually really quick at assigning a core issue, right? That said, you can merge this before getting one as long as you come back afterwards and fill it in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen it take anywhere from a day to a month. I'll be sure to update this once the issue is opened!

@sdkrystian sdkrystian force-pushed the partial-order-noexcept branch from 0ef86c6 to 7d62b17 Compare May 9, 2024 01:17
@sdkrystian
Copy link
Member Author

@erichkeane Release note added

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.

LGTM other than including the CWG issue to the comment. Feel free to merge once that is in place.

@sdkrystian sdkrystian force-pushed the partial-order-noexcept branch from 7d62b17 to d53c7fa Compare May 15, 2024 15:40
@sdkrystian sdkrystian merged commit 667d12f into llvm:main May 15, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants