-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Clang][Sema] Fix exception specification comparison for functions with different template depths #111561
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang Author: Krystian Stasiowski (sdkrystian) ChangesCurrently, 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 Fixes #101330, closes #102267 Full diff: https://github.com/llvm/llvm-project/pull/111561.diff 3 Files Affected:
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}}
+ };
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable, other than 1 #if 0
clang/lib/Sema/SemaExceptionSpec.cpp
Outdated
LocalInstantiationScope ScopeForParameters(S); | ||
if (auto *FD = DeclInfo.getDecl()->getAsFunction()) | ||
for (auto *PVD : FD->parameters()) | ||
ScopeForParameters.InstantiatedLocal(PVD, PVD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AreConstraintExpressionEquivalent
probably changed after I wrote this. Since the functions are almost identical, I'm going to combine them into a single function.
clang/lib/Sema/SemaExceptionSpec.cpp
Outdated
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, a friend declaration was dealt with slightly differently in that function
7b28289
to
45a2d49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
489c06b
to
6105c29
Compare
6105c29
to
7df3262
Compare
This needs a changelog entry |
MD ? MD->getMethodQualifiers() : Qualifiers{}, MD != nullptr); | ||
|
||
EnterExpressionEvaluationContext ConstantEvaluated( | ||
S, Sema::ExpressionEvaluationContext::ConstantEvaluated); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that not be be Unevaluated?
https://eel.is/c++draft/expr.unary.noexcept#1
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.
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 fromAreConstraintExpressionsEqual
andAreExceptionSpecsEqual
prior to merging this patch.Fixes #101330, closes #102267