[clang] visit constraint of NTTP #91842
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Qizhi Hu (jcsxky) Changesattempt to fix #77377 Full diff: https://github.com/llvm/llvm-project/pull/91842.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7c5dcc59c7016..30d359c582d3f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -707,6 +707,7 @@ Bug Fixes to C++ Support
initialized, rather than evaluating them as a part of the larger manifestly constant evaluated
expression.
- Fix a bug in access control checking due to dealyed checking of friend declaration. Fixes (#GH12361).
+- Fix a bug on template class partial specialization due to traverse of constraint of NTTP. Fixes (#GH77377).
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 8fb8940142eb0..0bb5a40d7350e 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2257,6 +2257,11 @@ void StmtProfiler::VisitSubstNonTypeTemplateParmExpr(
const SubstNonTypeTemplateParmExpr *E) {
// Profile exactly as the replacement expression.
Visit(E->getReplacement());
+ if (auto *NTTP =
+ dyn_cast_if_present<NonTypeTemplateParmDecl>(E->getParameter());
+ NTTP && NTTP->getPlaceholderTypeConstraint()) {
+ Visit(NTTP->getPlaceholderTypeConstraint());
+ }
}
void StmtProfiler::VisitFunctionParmPackExpr(const FunctionParmPackExpr *S) {
diff --git a/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp b/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
index 9f44878da6254..5f9719a2dc561 100644
--- a/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
@@ -79,14 +79,14 @@ template<C T, int I> struct Y2<T*, I, I+1+1> {}; // expected-note {{partial
template<C T, C auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
struct Y3 { Y3()=delete; };
template<C T, D auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
-struct Y3<T, I, W, S, U, Z...> { Y3()=delete; };
+struct Y3<T, I, W, S, U, Z...> { Y3()=delete; }; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}
template<C T, E auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
-struct Y3<T, I, W, S, U, Z...> {};
+struct Y3<T, I, W, S, U, Z...> {}; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}
void f() {
Y1<int, 2> a;
Y2<char*, 1, 3> b; // expected-error {{ambiguous partial specializations}}
- Y3<int, 1, 1, A{}, S, int> c;
+ Y3<int, 1, 1, A{}, S, int> c; // expected-error {{ambiguous partial specializations of 'Y3<int, 1, 1, A{}, S, int>'}}
}
// Per [temp.func.order]p6.2.2, specifically "if the function parameters that
diff --git a/clang/test/SemaCXX/PR77377.cpp b/clang/test/SemaCXX/PR77377.cpp
new file mode 100644
index 0000000000000..ae34809c3903d
--- /dev/null
+++ b/clang/test/SemaCXX/PR77377.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+// expected-no-diagnostics
+
+template<typename T, typename U>
+constexpr bool is_same_v = false;
+
+template<typename T>
+constexpr bool is_same_v<T, T> = true;
+
+template<typename T, typename U>
+concept same_as = is_same_v<T, U>;
+
+template <auto>
+struct A {};
+
+template <same_as<int> auto p>
+struct A<p> {};
+
+A<0> a;
|
|
@llvm/pr-subscribers-clang-modules Author: Qizhi Hu (jcsxky) Changesattempt to fix #77377 Full diff: https://github.com/llvm/llvm-project/pull/91842.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7c5dcc59c7016..30d359c582d3f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -707,6 +707,7 @@ Bug Fixes to C++ Support
initialized, rather than evaluating them as a part of the larger manifestly constant evaluated
expression.
- Fix a bug in access control checking due to dealyed checking of friend declaration. Fixes (#GH12361).
+- Fix a bug on template class partial specialization due to traverse of constraint of NTTP. Fixes (#GH77377).
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 8fb8940142eb0..0bb5a40d7350e 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2257,6 +2257,11 @@ void StmtProfiler::VisitSubstNonTypeTemplateParmExpr(
const SubstNonTypeTemplateParmExpr *E) {
// Profile exactly as the replacement expression.
Visit(E->getReplacement());
+ if (auto *NTTP =
+ dyn_cast_if_present<NonTypeTemplateParmDecl>(E->getParameter());
+ NTTP && NTTP->getPlaceholderTypeConstraint()) {
+ Visit(NTTP->getPlaceholderTypeConstraint());
+ }
}
void StmtProfiler::VisitFunctionParmPackExpr(const FunctionParmPackExpr *S) {
diff --git a/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp b/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
index 9f44878da6254..5f9719a2dc561 100644
--- a/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
@@ -79,14 +79,14 @@ template<C T, int I> struct Y2<T*, I, I+1+1> {}; // expected-note {{partial
template<C T, C auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
struct Y3 { Y3()=delete; };
template<C T, D auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
-struct Y3<T, I, W, S, U, Z...> { Y3()=delete; };
+struct Y3<T, I, W, S, U, Z...> { Y3()=delete; }; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}
template<C T, E auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
-struct Y3<T, I, W, S, U, Z...> {};
+struct Y3<T, I, W, S, U, Z...> {}; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}
void f() {
Y1<int, 2> a;
Y2<char*, 1, 3> b; // expected-error {{ambiguous partial specializations}}
- Y3<int, 1, 1, A{}, S, int> c;
+ Y3<int, 1, 1, A{}, S, int> c; // expected-error {{ambiguous partial specializations of 'Y3<int, 1, 1, A{}, S, int>'}}
}
// Per [temp.func.order]p6.2.2, specifically "if the function parameters that
diff --git a/clang/test/SemaCXX/PR77377.cpp b/clang/test/SemaCXX/PR77377.cpp
new file mode 100644
index 0000000000000..ae34809c3903d
--- /dev/null
+++ b/clang/test/SemaCXX/PR77377.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+// expected-no-diagnostics
+
+template<typename T, typename U>
+constexpr bool is_same_v = false;
+
+template<typename T>
+constexpr bool is_same_v<T, T> = true;
+
+template<typename T, typename U>
+concept same_as = is_same_v<T, U>;
+
+template <auto>
+struct A {};
+
+template <same_as<int> auto p>
+struct A<p> {};
+
+A<0> a;
|
|
|
||
| void f() { | ||
| Y1<int, 2> a; | ||
| Y2<char*, 1, 3> b; // expected-error {{ambiguous partial specializations}} | ||
| Y3<int, 1, 1, A{}, S, int> c; | ||
| Y3<int, 1, 1, A{}, S, int> c; // expected-error {{ambiguous partial specializations of 'Y3<int, 1, 1, A{}, S, int>'}} |
There was a problem hiding this comment.
This is suspicious... I don't see any FIXMEs around suggesting this was not in conformance with the standards.
@erichkeane @cor3ntin any thoughts?
There was a problem hiding this comment.
The only difference is D auto vs E auto
E subsumes D so the second partial specialization is more constrained and we should pick that.
So I believe the existing test is correct and we would be introducing a regression.
It is interesting to consider that GCC, EGG and MSVC all reject that code so it is possible
I am missing something
There was a problem hiding this comment.
There was a problem hiding this comment.
Two template-parameters are equivalent under the following conditions:
if they declare non-type template parameters, they have equivalent types ignoring the use of type-constraints for placeholder types
Oops, I forgot I posted the standard wording on that issue, and therefore this seems reasonable..?
There was a problem hiding this comment.
I think the quote can explain why we should report error here. Maybe more work is required to find out where we do the ignore and how this patch impacts this case.
There was a problem hiding this comment.
When instantiation, we are checking which one of the two partial specialization is more specialized. Obviously, the first one(auto D) is not more specialized than the second(auto E). When applied this patch, the second one is not more specialized than the first as well. This is because isSameTemplateArg return false and the result is not TemplateDeductionResult::Success.
Although we get correct result, it is not because of ignoring the type-constraint. Back to the quote, if we ignore the use of type-constraints for placeholder types, is the following example ill-formed due to their equivalent template arguments?
template <typename> constexpr bool True = true;
template <typename T> concept C = True<T>;
template <typename T> concept D = C<T> && sizeof(T) > 2;
template <typename T> concept E = D<T> && alignof(T) > 1;
template<C auto I>
struct Y3 { Y3()=delete; };
template<D auto I>
struct Y3<I> { Y3()=delete; };
template<E auto I>
struct Y3<I> {};But EDG, gcc and MSVC all accept this code. So I think the existing test is rejected may not be related to the quote. WDYT? @cor3ntin @zyn0217 @erichkeane
clang/lib/AST/StmtProfile.cpp
Outdated
| @@ -2257,6 +2257,11 @@ void StmtProfiler::VisitSubstNonTypeTemplateParmExpr( | |||
| const SubstNonTypeTemplateParmExpr *E) { | |||
| // Profile exactly as the replacement expression. | |||
| Visit(E->getReplacement()); | |||
| if (auto *NTTP = | |||
| dyn_cast_if_present<NonTypeTemplateParmDecl>(E->getParameter()); | |||
There was a problem hiding this comment.
In SubstNonTypeTemplateParmExpr::getParameter() we returned a NonTypeTemplateParmDecl after a cast; so an if_present is unnecessary, at least E->getParameter() was presumed to return always non-null.
jcsxky
force-pushed
the
fix-77377
branch
2 times, most recently
from
4133001 to
0ebdcec
Compare
|
I don't think this is the right approach. I stepped though the example and the reason we reject is because:
So, I think the correct fix is to propagate dependence in |
Yeah, that seems incredibly reasonable and a much lower touch here with fewer concerns about the side-effects that we got here. |
Should I open a PR then? |
Yep, I think you should. Sorry @jcsxky : We're going to close this one, it seems that @sdkrystian has a 'more correct' patch here, so we'll go with his. I hope/encourage you to help review it! |
That looks similar to the approach which I investigated some time ago. The difference is that I removed the constraint check in getUnconstrainedType but yours is to propagate the dependency — i think that looks more reasonable than mine that way. |
This is really a perfect approach and it has addressed the underlying issue. And thanks for your explanation! |
attempt to fix #77377
isSameTemplateArgreturns incorrect result since clang didn't visit constraint of NTTP. Furthermore, It makes class template partial specialization not more specialized than the class template itself.