[Clang][Sema] Fix exception specification comparison for functions with different template depths

[sdkrystian]

Currently, we do not account for differences in template depth when comparing exception specifications for equivalence. This results in explicit specializations of member function templates specialized for an implicitly instantiated specialization of their enclosing class template & friend declarations being rejected when their exception specifications involve a template parameter, e.g.

template<typename T>
struct A {
  template<bool B>
  void f() noexcept(B);
};

template<>
template<bool B>
void A<int>::f() noexcept(B); // error: exception specification in declaration does not match previous declaration

In order to compare the noexcept-specifiers correctly, we need to normalize both expressions prior to comparing them (just like we do when comparing constraint-expressions). This patch implements this normalization.

Note: In its current state, this patch just copies the code from AreConstraintExpressionsEqual to implement expression normalization for noexcept-specifiers. I plan to factor out the common code from AreConstraintExpressionsEqual and AreExceptionSpecsEqual prior to merging this patch.

Fixes #101330, closes #102267

[llvmbot]

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Currently, we do not account for differences in template depth when comparing exception specifications for equivalence. This results in explicit specializations of member function templates specialized for an implicitly instantiated specialization of their enclosing class template & friend declarations being rejected when their exception specifications involve a template parameter, e.g.

template<typename T>
struct A {
  template<bool B>
  void f() noexcept(B);
};

template<>
template<bool B>
void A<int>::f() noexcept(B); // error: exception specification in declaration does not match previous declaration

In order to compare the noexcept-specifiers correctly, we need to normalize both expressions prior to comparing them (just like we do when comparing constraint-expressions). This patch implements this normalization.

Note: In its current state, this patch just copies the code from AreConstraintExpressionsEqual to implement expression normalization for noexcept-specifiers. I plan to factor out the common code from AreConstraintExpressionsEqual and AreExceptionSpecsEqual prior to merging this patch.

Fixes #101330, closes #102267

---

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

3 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+5)
• (modified) clang/lib/Sema/SemaExceptionSpec.cpp (+104-1)
• (added) clang/test/CXX/basic/basic.link/p11.cpp (+37)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7ff9c2754a6fe0..eea950582929d9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5026,6 +5026,11 @@ class Sema final : public SemaBase {
   /// special member function.
   void EvaluateImplicitExceptionSpec(SourceLocation Loc, FunctionDecl *FD);
 
+  bool AreExceptionSpecsEqual(const NamedDecl *Old,
+                              const Expr *OldExceptionSpec,
+                              const NamedDecl *New,
+                              const Expr *NewExceptionSpec);
+
   /// Check the given exception-specification and update the
   /// exception specification information with the results.
   void checkExceptionSpecification(bool IsTopLevel,
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index dbddd6c370aa07..c74686073df228 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/Expr.h"
@@ -19,6 +18,9 @@
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Sema/EnterExpressionEvaluationContext.h"
+#include "clang/Sema/SemaInternal.h"
+#include "clang/Sema/Template.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include <optional>
@@ -314,6 +316,22 @@ bool Sema::CheckEquivalentExceptionSpec(FunctionDecl *Old, FunctionDecl *New) {
     return false;
   }
 
+  if (Old->getExceptionSpecType() == EST_DependentNoexcept &&
+      New->getExceptionSpecType() == EST_DependentNoexcept) {
+    const auto *OldType = Old->getType()->getAs<FunctionProtoType>();
+    const auto *NewType = New->getType()->getAs<FunctionProtoType>();
+    OldType = ResolveExceptionSpec(New->getLocation(), OldType);
+    if (!OldType)
+      return false;
+    NewType = ResolveExceptionSpec(New->getLocation(), NewType);
+    if (!NewType)
+      return false;
+
+    if (AreExceptionSpecsEqual(Old, OldType->getNoexceptExpr(), New,
+                               NewType->getNoexceptExpr()))
+      return false;
+  }
+
   // Check the types as written: they must match before any exception
   // specification adjustment is applied.
   if (!CheckEquivalentExceptionSpecImpl(
@@ -501,6 +519,89 @@ bool Sema::CheckEquivalentExceptionSpec(
   return Result;
 }
 
+static const Expr *SubstituteExceptionSpecWithoutEvaluation(
+    Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo,
+    const Expr *ExceptionSpec) {
+  MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
+      DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(),
+      /*Final=*/false, /*Innermost=*/std::nullopt,
+      /*RelativeToPrimary=*/true, /*ForConstraintInstantiation=*/true);
+
+  if (!MLTAL.getNumSubstitutedLevels())
+    return ExceptionSpec;
+
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+
+  Sema::InstantiatingTemplate Inst(
+      S, DeclInfo.getLocation(),
+      const_cast<FunctionDecl *>(DeclInfo.getDecl()->getAsFunction()),
+      Sema::InstantiatingTemplate::ExceptionSpecification());
+  if (Inst.isInvalid())
+    return nullptr;
+
+  // Set up a dummy 'instantiation' scope in the case of reference to function
+  // parameters that the surrounding function hasn't been instantiated yet. Note
+  // this may happen while we're comparing two templates' constraint
+  // equivalence.
+  LocalInstantiationScope ScopeForParameters(S);
+  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
+    for (auto *PVD : FD->parameters())
+      ScopeForParameters.InstantiatedLocal(PVD, PVD);
+
+  std::optional<Sema::CXXThisScopeRAII> ThisScope;
+
+  // See TreeTransform::RebuildTemplateSpecializationType. A context scope is
+  // essential for having an injected class as the canonical type for a template
+  // specialization type at the rebuilding stage. This guarantees that, for
+  // out-of-line definitions, injected class name types and their equivalent
+  // template specializations can be profiled to the same value, which makes it
+  // possible that e.g. constraints involving C<Class<T>> and C<Class> are
+  // perceived identical.
+  std::optional<Sema::ContextRAII> ContextScope;
+  if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext())) {
+    ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers());
+    ContextScope.emplace(S, const_cast<DeclContext *>(cast<DeclContext>(RD)),
+                         /*NewThisContext=*/false);
+  }
+
+  EnterExpressionEvaluationContext ConstantEvaluated(
+      S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
+  ExprResult SubstExceptionSpec =
+      S.SubstExpr(const_cast<clang::Expr *>(ExceptionSpec), MLTAL);
+  if (SFINAE.hasErrorOccurred() || !SubstExceptionSpec.isUsable())
+    return nullptr;
+  return SubstExceptionSpec.get();
+}
+
+bool Sema::AreExceptionSpecsEqual(const NamedDecl *Old,
+                                  const Expr *OldExceptionSpec,
+                                  const NamedDecl *New,
+                                  const Expr *NewExceptionSpec) {
+  if (OldExceptionSpec == NewExceptionSpec)
+    return true;
+  if (Old && New &&
+      Old->getLexicalDeclContext() != New->getLexicalDeclContext()) {
+    if (const Expr *SubstExceptionSpec =
+            SubstituteExceptionSpecWithoutEvaluation(*this, Old,
+                                                     OldExceptionSpec))
+      OldExceptionSpec = SubstExceptionSpec;
+    else
+      return false;
+    if (const Expr *SubstExceptionSpec =
+            SubstituteExceptionSpecWithoutEvaluation(*this, New,
+                                                     NewExceptionSpec))
+      NewExceptionSpec = SubstExceptionSpec;
+    else
+      return false;
+  }
+
+  llvm::FoldingSetNodeID ID1, ID2;
+  OldExceptionSpec->Profile(ID1, Context, /*Canonical=*/true);
+  NewExceptionSpec->Profile(ID2, Context, /*Canonical=*/true);
+  return ID1 == ID2;
+}
+
 /// CheckEquivalentExceptionSpec - Check if the two types have compatible
 /// exception specifications. See C++ [except.spec]p3.
 ///
@@ -574,6 +675,7 @@ static bool CheckEquivalentExceptionSpecImpl(
     }
   }
 
+#if 0
   // C++14 [except.spec]p3:
   //   Two exception-specifications are compatible if [...] both have the form
   //   noexcept(constant-expression) and the constant-expressions are equivalent
@@ -584,6 +686,7 @@ static bool CheckEquivalentExceptionSpecImpl(
     if (OldFSN == NewFSN)
       return false;
   }
+#endif
 
   // Dynamic exception specifications with the same set of adjusted types
   // are compatible.
diff --git a/clang/test/CXX/basic/basic.link/p11.cpp b/clang/test/CXX/basic/basic.link/p11.cpp
new file mode 100644
index 00000000000000..e244336417fd67
--- /dev/null
+++ b/clang/test/CXX/basic/basic.link/p11.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+namespace MemberSpecialization {
+  template<typename T>
+  struct A {
+    template<bool B>
+    void f() noexcept(B);
+
+    template<bool B>
+    void g() noexcept(B); // expected-note {{previous declaration is here}}
+  };
+
+  template<>
+  template<bool B>
+  void A<int>::f() noexcept(B);
+
+  template<>
+  template<bool B>
+  void A<int>::g() noexcept(!B); // expected-error {{exception specification in declaration does not match previous declaration}}
+}
+
+namespace Friend {
+  template<bool B>
+  void f() noexcept(B);
+
+  template<bool B>
+  void g() noexcept(B); // expected-note {{previous declaration is here}}
+
+  template<typename T>
+  struct A {
+    template<bool B>
+    friend void f() noexcept(B);
+
+    template<bool B>
+    friend void g() noexcept(!B); // expected-error {{exception specification in declaration does not match previous declaration}}
+  };
+}

[erichkeane]

This seems reasonable, other than 1 #if 0

[zyn0217]

This seems different from the latest version of AreConstraintExpressionEquivalent(), which expands parameter packs to a size-of-1 pack argument. Can you explain why there is a difference?

[zyn0217]

Ditto, a friend declaration was dealt with slightly differently in that function

[mizvekov]

So ContextRAII saves, changes and restores the CurContext. This is the current semantic context which will be used as the parent of any top level declarations created within the substitution below.

The DelcContext here could also be something else, like a namespace, and we would for example want a lambda appearing within the exception spec to be parented to that, instead of whatever just happens to be the CurContext.

Also, the semantic context of the exception spec should be the function, not the parent of the function.

Suggested change

	std::optional<Sema::ContextRAII> ContextScope;
	Sema::ContextRAII ContextScope(S, FD);