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] Defer instantiation of exception specification until after partial ordering when determining primary template #82417

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

sdkrystian
Copy link
Member

Consider the following:

struct A {
  static constexpr bool x = true;
};

template<typename T, typename U>
void f(T, U) noexcept(T::y); // #1, error: no member named 'y' in 'A'

template<typename T, typename U>
void f(T, U*) noexcept(T::x); // #2

template<>
void f(A, int*) noexcept; // explicit specialization of #2

We currently instantiate the exception specification of all candidate function template specializations when deducting template arguments for an explicit specialization, which results in a error despite #1 not being selected by partial ordering as the most specialized template. According to [except.spec] p13:

An exception specification is considered to be needed when:

  • [...]
  • the exception specification is compared to that of another declaration (e.g., an explicit specialization or an overriding virtual function);

Assuming that "comparing declarations" means "determining whether the declarations correspond and declare the same entity" (per [basic.scope.scope] p4 and [basic.link] p11.1, respectively), the exception specification does not need to be instantiated until after partial ordering, at which point we determine whether the implicitly instantiated specialization and the explicit specialization declare the same entity (the determination of whether two functions/function templates correspond does not consider the exception specifications).

This patch defers the instantiation of the exception specification until a single function template specialization is selected via partial ordering, matching the behavior of GCC, EDG, and MSVC.

(Note: I'm working on a patch that eliminates the implicitly instantiated specialization of each candidate function template when determining the primary template of an explicit specialization, but I figured that this change to when exception specifications are instantiated would be better off as its own patch).

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 20, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Consider the following:

struct A {
  static constexpr bool x = true;
};

template&lt;typename T, typename U&gt;
void f(T, U) noexcept(T::y); // #<!-- -->1, error: no member named 'y' in 'A'

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

template&lt;&gt;
void f(A, int*) noexcept; // explicit specialization of #<!-- -->2

We currently instantiate the exception specification of all candidate function template specializations when deducting template arguments for an explicit specialization, which results in a error despite #<!-- -->1 not being selected by partial ordering as the most specialized template. According to [[except.spec] p13](http://eel.is/c++draft/except.spec#13):
> An exception specification is considered to be needed when:
> - [...]
> - the exception specification is compared to that of another declaration (e.g., an explicit specialization or an overriding virtual function);

Assuming that "comparing declarations" means "determining whether the declarations correspond and declare the same entity" (per [[basic.scope.scope] p4](http://eel.is/c++draft/basic.scope.scope#4) and [[basic.link] p11.1](http://eel.is/c++draft/basic.link#11.1), respectively), the exception specification does not need to be instantiated until after partial ordering, at which point we determine whether the implicitly instantiated specialization and the explicit specialization declare the same entity (the determination of whether two functions/function templates correspond does not consider the exception specifications).

This patch defers the instantiation of the exception specification until a single function template specialization is selected via partial ordering, matching the behavior of GCC, EDG, and MSVC.

(Note: I'm working on a patch that eliminates the implicitly instantiated specialization of each candidate function template when determining the primary template of an explicit specialization, but I figured that this change to when exception specifications are instantiated would be better off as its own patch).


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

4 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplate.cpp (+9-1)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+7-8)
  • (added) clang/test/CXX/except/except.spec/p13.cpp (+38)
  • (modified) clang/test/SemaTemplate/class-template-noexcept.cpp (+1-13)
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 1a975a8d0a0df5..9c75438011010a 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -9706,9 +9706,17 @@ bool Sema::CheckFunctionTemplateSpecialization(
   if (Result == Candidates.end())
     return true;
 
-  // Ignore access information;  it doesn't figure into redeclaration checking.
   FunctionDecl *Specialization = cast<FunctionDecl>(*Result);
+  auto *SpecializationFPT =
+      Specialization->getType()->castAs<FunctionProtoType>();
+  // If the function has a dependent exception specification, resolve it after
+  // we have selected the primary template so we can check whether it matches.
+  if (getLangOpts().CPlusPlus17 &&
+      isUnresolvedExceptionSpec(SpecializationFPT->getExceptionSpecType()) &&
+      !ResolveExceptionSpec(FD->getLocation(), SpecializationFPT))
+    return true;
 
+  // Ignore access information;  it doesn't figure into redeclaration checking.
   FunctionTemplateSpecializationInfo *SpecInfo
     = Specialization->getTemplateSpecializationInfo();
   assert(SpecInfo && "Function template specialization info missing?");
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 47cc22310c4eec..3c04dd030d5ebb 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -4632,11 +4632,10 @@ TemplateDeductionResult Sema::DeduceTemplateArguments(
                                                Info.getLocation()))
     return TemplateDeductionResult::MiscellaneousDeductionFailure;
 
-  // If the function has a dependent exception specification, resolve it now,
-  // so we can check that the exception specification matches.
+
   auto *SpecializationFPT =
       Specialization->getType()->castAs<FunctionProtoType>();
-  if (getLangOpts().CPlusPlus17 &&
+  if (IsAddressOfFunction && getLangOpts().CPlusPlus17 &&
       isUnresolvedExceptionSpec(SpecializationFPT->getExceptionSpecType()) &&
       !ResolveExceptionSpec(Info.getLocation(), SpecializationFPT))
     return TemplateDeductionResult::MiscellaneousDeductionFailure;
@@ -4662,11 +4661,11 @@ TemplateDeductionResult Sema::DeduceTemplateArguments(
   // specialization with respect to arguments of compatible pointer to function
   // types, template argument deduction fails.
   if (!ArgFunctionType.isNull()) {
-    if (IsAddressOfFunction
-            ? !isSameOrCompatibleFunctionType(
-                  Context.getCanonicalType(SpecializationType),
-                  Context.getCanonicalType(ArgFunctionType))
-            : !Context.hasSameType(SpecializationType, ArgFunctionType)) {
+    if (IsAddressOfFunction ? !isSameOrCompatibleFunctionType(
+                                  Context.getCanonicalType(SpecializationType),
+                                  Context.getCanonicalType(ArgFunctionType))
+                            : !Context.hasSameFunctionTypeIgnoringExceptionSpec(
+                                  SpecializationType, ArgFunctionType)) {
       Info.FirstArg = TemplateArgument(SpecializationType);
       Info.SecondArg = TemplateArgument(ArgFunctionType);
       return TemplateDeductionResult::NonDeducedMismatch;
diff --git a/clang/test/CXX/except/except.spec/p13.cpp b/clang/test/CXX/except/except.spec/p13.cpp
new file mode 100644
index 00000000000000..6a4f699ed340d2
--- /dev/null
+++ b/clang/test/CXX/except/except.spec/p13.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
+
+struct A {
+  static constexpr bool x = true;
+};
+
+template<typename T, typename U>
+void f(T, U) noexcept(T::x);
+
+template<typename T, typename U>
+void f(T, U*) noexcept(T::x);
+
+template<typename T, typename U>
+void f(T, U**) noexcept(T::y); // expected-error {{no member named 'y' in 'A'}}
+
+template<typename T, typename U>
+void f(T, U***) noexcept(T::x);
+
+template<>
+void f(A, int*) noexcept; // expected-note {{previous declaration is here}}
+
+template<>
+void f(A, int*); // expected-error {{'f<A, int>' is missing exception specification 'noexcept'}}
+
+template<>
+void f(A, int**) noexcept; // expected-error {{exception specification in declaration does not match previous declaration}}
+                           // expected-note@-1 {{in instantiation of exception specification for 'f<A, int>' requested here}}
+                           // expected-note@-2 {{previous declaration is here}}
+
+// FIXME: Exception specification is currently set to EST_None if instantiation fails.
+template<>
+void f(A, int**);
+
+template<>
+void f(A, int***) noexcept; // expected-note {{previous declaration is here}}
+
+template<>
+void f(A, int***); // expected-error {{'f<A, int>' is missing exception specification 'noexcept'}}
diff --git a/clang/test/SemaTemplate/class-template-noexcept.cpp b/clang/test/SemaTemplate/class-template-noexcept.cpp
index 5c4ac090f3166d..14d2e36bc0bfae 100644
--- a/clang/test/SemaTemplate/class-template-noexcept.cpp
+++ b/clang/test/SemaTemplate/class-template-noexcept.cpp
@@ -2,9 +2,7 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s
 // RUN: %clang_cc1 -std=c++17 -verify %s
 // RUN: %clang_cc1 -std=c++1z -verify %s
-#if __cplusplus >= 201703
-// expected-no-diagnostics
-#endif
+
 class A {
 public:
   static const char X;
@@ -14,19 +12,9 @@ const char A::X = 0;
 template<typename U> void func() noexcept(U::X);
 
 template<class... B, char x>
-#if __cplusplus >= 201703
-void foo(void(B...) noexcept(x)) {} 
-#else
 void foo(void(B...) noexcept(x)) {} // expected-note{{candidate template ignored}}
-#endif
 
 void bar()
 {
-#if __cplusplus >= 201703
-  foo(func<A>);
-#else
   foo(func<A>);	// expected-error{{no matching function for call}}
-#endif	
 }
-
-

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.

1 nit, else LGTM. We should probably have a release note as well.

clang/lib/Sema/SemaTemplate.cpp Show resolved Hide resolved
@sdkrystian sdkrystian force-pushed the delayed-exception-spec branch 2 times, most recently from 3966e98 to e669903 Compare February 26, 2024 13:41
@sdkrystian
Copy link
Member Author

@erichkeane Added release note, relevant standardese quote, and some exposition.

…ter partial ordering when determining primary template
@sdkrystian sdkrystian force-pushed the delayed-exception-spec branch from e669903 to 06fe8f5 Compare February 26, 2024 14:00
@sdkrystian
Copy link
Member Author

Also added a couple more tests since the diagnostics currently depend on whether instantiation occurs.

@sdkrystian sdkrystian merged commit 9cfb138 into llvm:main Feb 26, 2024
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