[Clang] Profile singly-resolved UnresolvedLookupExpr with the declaration #140029
Conversation
…tion For a dependent variable template specialization, we don't build a dependent Decl node or a DeclRefExpr to represent it. Instead, we preserve the UnresolvedLookupExpr until instantiation. However, this approach isn't ideal for constraint normalization. We consider the qualifier during profiling, but since that's based on the written code, it can introduce confusing differences, even when the expressions resolve to the same declaration. This change ensures that, if possible, we profile the resolved declaration instead of its qualifier. For expressions that resolve to more than one declarations, we still profile its qualifier, as otherwise it would make us depend on the order of lookup results.
zyn0217
requested review from
cor3ntin and
erichkeane
and removed request for
cor3ntin
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules Author: Younan Zhang (zyn0217) ChangesFor a dependent variable template specialization, we don't build a dependent Decl node or a DeclRefExpr to represent it. Instead, we preserve the UnresolvedLookupExpr until instantiation. However, this approach isn't ideal for constraint normalization. We consider the qualifier during profiling, but since that's based on the written code, it can introduce confusing differences, even when the expressions resolve to the same declaration. This change ensures that, if possible, we profile the resolved declaration instead of its qualifier. For expressions that resolve to more than one declarations, we still profile its qualifier, as otherwise it would make us depend on the order of lookup results. Fixes #139476 Full diff: https://github.com/llvm/llvm-project/pull/140029.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 44c38396c764f..2772821755a21 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -701,6 +701,7 @@ Bug Fixes to C++ Support
- Fixed the handling of pack indexing types in the constraints of a member function redeclaration. (#GH138255)
- Clang now correctly parses arbitrary order of ``[[]]``, ``__attribute__`` and ``alignas`` attributes for declarations (#GH133107)
- Fixed a crash when forming an invalid function type in a dependent context. (#GH138657) (#GH115725) (#GH68852)
+- Fixed a function declaration mismatch that caused inconsistencies between concepts and variable template declarations. (#GH139476)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index f7d1655f67ed1..19db338f760ba 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2189,7 +2189,10 @@ StmtProfiler::VisitCXXPseudoDestructorExpr(const CXXPseudoDestructorExpr *S) {
void StmtProfiler::VisitOverloadExpr(const OverloadExpr *S) {
VisitExpr(S);
- VisitNestedNameSpecifier(S->getQualifier());
+ if (S->getNumDecls() == 1)
+ VisitDecl(*S->decls_begin());
+ else
+ VisitNestedNameSpecifier(S->getQualifier());
VisitName(S->getName(), /*TreatAsDecl*/ true);
ID.AddBoolean(S->hasExplicitTemplateArgs());
if (S->hasExplicitTemplateArgs())
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index e5d00491d3fb8..bf505dec0ca14 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -853,3 +853,18 @@ template <int... Ts>
requires C<Ts...[0]>
auto TplClass<int>::buggy() -> void {}
}
+
+namespace GH139476 {
+
+namespace moo {
+ template <typename T>
+ constexpr bool baa = true;
+
+ template <typename T> requires baa<T>
+ void caw();
+}
+
+template <typename T> requires moo::baa<T>
+void moo::caw() {}
+
+}
|
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesFor a dependent variable template specialization, we don't build a dependent Decl node or a DeclRefExpr to represent it. Instead, we preserve the UnresolvedLookupExpr until instantiation. However, this approach isn't ideal for constraint normalization. We consider the qualifier during profiling, but since that's based on the written code, it can introduce confusing differences, even when the expressions resolve to the same declaration. This change ensures that, if possible, we profile the resolved declaration instead of its qualifier. For expressions that resolve to more than one declarations, we still profile its qualifier, as otherwise it would make us depend on the order of lookup results. Fixes #139476 Full diff: https://github.com/llvm/llvm-project/pull/140029.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 44c38396c764f..2772821755a21 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -701,6 +701,7 @@ Bug Fixes to C++ Support
- Fixed the handling of pack indexing types in the constraints of a member function redeclaration. (#GH138255)
- Clang now correctly parses arbitrary order of ``[[]]``, ``__attribute__`` and ``alignas`` attributes for declarations (#GH133107)
- Fixed a crash when forming an invalid function type in a dependent context. (#GH138657) (#GH115725) (#GH68852)
+- Fixed a function declaration mismatch that caused inconsistencies between concepts and variable template declarations. (#GH139476)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index f7d1655f67ed1..19db338f760ba 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2189,7 +2189,10 @@ StmtProfiler::VisitCXXPseudoDestructorExpr(const CXXPseudoDestructorExpr *S) {
void StmtProfiler::VisitOverloadExpr(const OverloadExpr *S) {
VisitExpr(S);
- VisitNestedNameSpecifier(S->getQualifier());
+ if (S->getNumDecls() == 1)
+ VisitDecl(*S->decls_begin());
+ else
+ VisitNestedNameSpecifier(S->getQualifier());
VisitName(S->getName(), /*TreatAsDecl*/ true);
ID.AddBoolean(S->hasExplicitTemplateArgs());
if (S->hasExplicitTemplateArgs())
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index e5d00491d3fb8..bf505dec0ca14 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -853,3 +853,18 @@ template <int... Ts>
requires C<Ts...[0]>
auto TplClass<int>::buggy() -> void {}
}
+
+namespace GH139476 {
+
+namespace moo {
+ template <typename T>
+ constexpr bool baa = true;
+
+ template <typename T> requires baa<T>
+ void caw();
+}
+
+template <typename T> requires moo::baa<T>
+void moo::caw() {}
+
+}
|
|
This PR seems to regress this test case: template <typename T>
void f(T&) noexcept;
template <typename T, int N>
void f(T (&arr)[N]) noexcept(noexcept(f(*arr)));
template <typename T>
inline void f(T&) noexcept {}
template <typename T, int N>
inline void f(T (&arr)[N]) noexcept(noexcept(f(*arr))) {}
void g() {
int x[1];
f(x);
}gcc at trunk still accepts this, but clang now produces this: <source>:11:13: error: exception specification in declaration does not match previous declaration
11 | inline void f(T (&arr)[N]) noexcept(noexcept(f(*arr))) {}
| ^
<source>:5:6: note: previous declaration is here
5 | void f(T (&arr)[N]) noexcept(noexcept(f(*arr)));
| ^Live link: https://godbolt.org/z/bdPePGoeh I don't know if clang is right to reject this, but at face value, the diagnostic seems incorrect: the |
|
@rupprecht That doesn't seem right... Will look into it (Feel free to revert if it blocks any of your build) |
… declaration" (#140655) This introduced a bug where noexcept specifiers are involved, as reported in #140029 (comment) Addressing that doesn't seem trivial at the moment, so I'll need some time to think it over; in the meantime let's revert the offending patch. Reverts #140029
|
I reverted it :( |
…pr with the declaration" (#140655) This introduced a bug where noexcept specifiers are involved, as reported in llvm/llvm-project#140029 (comment) Addressing that doesn't seem trivial at the moment, so I'll need some time to think it over; in the meantime let's revert the offending patch. Reverts llvm/llvm-project#140029
For a dependent variable template specialization, we don't build a dependent Decl node or a DeclRefExpr to represent it. Instead, we preserve the UnresolvedLookupExpr until instantiation.
However, this approach isn't ideal for constraint normalization. We consider the qualifier during profiling, but since that's based on the written code, it can introduce confusing differences, even when the expressions resolve to the same declaration.
This change ensures that, if possible, we profile the resolved declaration instead of its qualifier. For expressions that resolve to more than one declarations, we still profile its qualifier, as otherwise it would make us depend on the order of lookup results.
Fixes #139476