* [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] @ 2020-10-26 21:37 Patrick Palka 2020-10-26 21:37 ` [PATCH] c++: Check constraints only on candidate conversion functions Patrick Palka 2020-10-27 14:07 ` [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Jason Merrill 0 siblings, 2 replies; 5+ messages in thread From: Patrick Palka @ 2020-10-26 21:37 UTC (permalink / raw) To: gcc-patches This makes mark_used check constraints of a function _before_ calling maybe_instantiate_decl, so that we don't try instantiating a function (as part of return type deduction) with unsatisfied constraints. Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for trunk and perhaps the 10 branch? gcc/cp/ChangeLog: PR c++/95132 * decl2.c (mark_used): Move up the constraints_satisfied_p check so that it happens before calling maybe_instantiate_decl. gcc/testsuite/ChangeLog: PR c++/95132 * g++.dg/cpp2a/concepts-fn7.C: New test. --- gcc/cp/decl2.c | 30 +++++++++++------------ gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C | 11 +++++++++ 2 files changed, 26 insertions(+), 15 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c index 2f0d6370146..de2956aa5f0 100644 --- a/gcc/cp/decl2.c +++ b/gcc/cp/decl2.c @@ -5604,6 +5604,21 @@ mark_used (tree decl, tsubst_flags_t complain) if (DECL_ODR_USED (decl)) return true; + if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL + && !constraints_satisfied_p (decl)) + { + if (complain & tf_error) + { + auto_diagnostic_group d; + error ("use of function %qD with unsatisfied constraints", + decl); + location_t loc = DECL_SOURCE_LOCATION (decl); + inform (loc, "declared here"); + diagnose_constraints (loc, decl, NULL_TREE); + } + return false; + } + /* Normally, we can wait until instantiation-time to synthesize DECL. However, if DECL is a static data member initialized with a constant or a constexpr function, we need it right now because a reference to @@ -5614,21 +5629,6 @@ mark_used (tree decl, tsubst_flags_t complain) directly. */ maybe_instantiate_decl (decl); - if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL - && !constraints_satisfied_p (decl)) - { - if (complain & tf_error) - { - auto_diagnostic_group d; - error ("use of function %qD with unsatisfied constraints", - decl); - location_t loc = DECL_SOURCE_LOCATION (decl); - inform (loc, "declared here"); - diagnose_constraints (loc, decl, NULL_TREE); - } - return false; - } - if (processing_template_decl || in_template_function ()) return true; diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C new file mode 100644 index 00000000000..7fad6f374b7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C @@ -0,0 +1,11 @@ +// PR c++/95132 +// { dg-do compile { target c++20 } } + +template<typename T> struct A { + static auto f() requires false { return T::fail; } +}; + +template<typename T> +constexpr bool v = requires { A<T>::f(); }; + +static_assert(!v<int>); -- 2.29.0.rc0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] c++: Check constraints only on candidate conversion functions 2020-10-26 21:37 [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Patrick Palka @ 2020-10-26 21:37 ` Patrick Palka 2020-10-27 16:56 ` Jason Merrill 2020-10-27 14:07 ` [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Jason Merrill 1 sibling, 1 reply; 5+ messages in thread From: Patrick Palka @ 2020-10-26 21:37 UTC (permalink / raw) To: gcc-patches In the testcase below, we're overeagerly checking the constraints on the conversion function B<int>::operator bool() as part of finding an implicit conversion sequence from B<int> to const A&. This behavior seems to be nonconforming because according to [over.match.copy] and [over.match.conv], only those conversion functions which yield a type that can be converted to the target type via a standard conversion sequence are candidate functions, and according to [over.match.viable], only candidate functions get their constraints checked. This patch fixes this by swapping the order of the calls to add_candidates and implicit_conversion in build_user_type_conversion_1 when the conversion function has a non-dependent result type. (Conversion function templates with a dependent result type already get handled appropriately in add_template_candidate, it seems.) Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK fo trunk? gcc/cp/ChangeLog: * call.c (build_user_type_conversion_1): When the conversion function has a non-dependent result type, pre-check the conversion sequence that would follow the conversion function. gcc/testsuite/ChangeLog: * g++.dg/cpp2a/concepts-conv3.C: New test. --- gcc/cp/call.c | 33 +++++++++++++++++---- gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 14 +++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index bd662518958..6d955ea083f 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -4099,6 +4099,23 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, if (TYPE_REF_P (totype)) convflags |= LOOKUP_NO_TEMP_BIND; + conversion *pre_ics = NULL; + if (!uses_template_parms (TREE_TYPE (conv_fns))) + { + /* A conversion function is a candidate function only if it + yields a type that can be converted to the target type. + We check this for conversion functions with non-dependent + result type now to disregard the non-candidate functions + before calling add_candidates. */ + pre_ics = implicit_conversion (totype, + TREE_TYPE (conv_fns), + 0, + /*c_cast_p=*/false, convflags, + complain); + if (!pre_ics) + continue; + } + old_candidates = candidates; add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype, NULL_TREE, false, @@ -4112,12 +4129,16 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, continue; tree rettype = TREE_TYPE (TREE_TYPE (cand->fn)); - conversion *ics - = implicit_conversion (totype, - rettype, - 0, - /*c_cast_p=*/false, convflags, - complain); + conversion *ics; + if (pre_ics) + /* We've already computed the conversion sequence above. */ + ics = pre_ics; + else + ics = implicit_conversion (totype, + rettype, + 0, + /*c_cast_p=*/false, convflags, + complain); /* If LOOKUP_NO_TEMP_BIND isn't set, then this is copy-initialization. In that case, "The result of the diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C new file mode 100644 index 00000000000..bcbc08fe869 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C @@ -0,0 +1,14 @@ +// { dg-do compile { target c++20 } } + +template <class T> +struct Error { static constexpr auto value = T::value; }; + +struct A { A(const A&); }; + +template <class T> +struct B { operator bool() requires Error<T>::value; }; + +template <class T> +concept C = requires (B<T> b) { A(b); }; + +static_assert(!C<int>); -- 2.29.0.rc0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: Check constraints only on candidate conversion functions 2020-10-26 21:37 ` [PATCH] c++: Check constraints only on candidate conversion functions Patrick Palka @ 2020-10-27 16:56 ` Jason Merrill 2020-10-27 20:13 ` Patrick Palka 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2020-10-27 16:56 UTC (permalink / raw) To: Patrick Palka, gcc-patches On 10/26/20 5:37 PM, Patrick Palka wrote: > In the testcase below, we're overeagerly checking the constraints on > the conversion function B<int>::operator bool() as part of finding an > implicit conversion sequence from B<int> to const A&. > > This behavior seems to be nonconforming because according to > [over.match.copy] and [over.match.conv], only those conversion functions > which yield a type that can be converted to the target type via a > standard conversion sequence are candidate functions, and according to > [over.match.viable], only candidate functions get their constraints > checked. Hmm. I agree with your reading of the standard, but I'm not sure this is what we actually want. It's the reverse of the agreed direction for CWG2369, to check constraints before non-dependent conversions for function arguments, and goes against the more general principle that the fix for unwanted instantiation should be additional constraints. A different testcase would succeed before your patch, and fail after: template <class T> struct Error { static constexpr auto value = T::value; }; struct A { A(const A&); }; template <class T> struct B { operator Error<T>*() requires false; }; template <class T> concept C = requires (B<T> b) { (A*)b; }; static_assert(!C<int>); I've raised this on the reflector. > This patch fixes this by swapping the order of the calls to > add_candidates and implicit_conversion in build_user_type_conversion_1 > when the conversion function has a non-dependent result type. > (Conversion function templates with a dependent result type already get > handled appropriately in add_template_candidate, it seems.) Testcase? But yes, for dependent result types if there isn't a conversion, deduction will fail. > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK fo > trunk? > gcc/cp/ChangeLog: > > * call.c (build_user_type_conversion_1): When the conversion > function has a non-dependent result type, pre-check the > conversion sequence that would follow the conversion function. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp2a/concepts-conv3.C: New test. > --- > gcc/cp/call.c | 33 +++++++++++++++++---- > gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 14 +++++++++ > 2 files changed, 41 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C > > diff --git a/gcc/cp/call.c b/gcc/cp/call.c > index bd662518958..6d955ea083f 100644 > --- a/gcc/cp/call.c > +++ b/gcc/cp/call.c > @@ -4099,6 +4099,23 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, > if (TYPE_REF_P (totype)) > convflags |= LOOKUP_NO_TEMP_BIND; > > + conversion *pre_ics = NULL; > + if (!uses_template_parms (TREE_TYPE (conv_fns))) > + { > + /* A conversion function is a candidate function only if it > + yields a type that can be converted to the target type. > + We check this for conversion functions with non-dependent > + result type now to disregard the non-candidate functions > + before calling add_candidates. */ > + pre_ics = implicit_conversion (totype, > + TREE_TYPE (conv_fns), > + 0, > + /*c_cast_p=*/false, convflags, > + complain); > + if (!pre_ics) > + continue; > + } > + > old_candidates = candidates; > add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype, > NULL_TREE, false, > @@ -4112,12 +4129,16 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, > continue; > > tree rettype = TREE_TYPE (TREE_TYPE (cand->fn)); > - conversion *ics > - = implicit_conversion (totype, > - rettype, > - 0, > - /*c_cast_p=*/false, convflags, > - complain); > + conversion *ics; > + if (pre_ics) > + /* We've already computed the conversion sequence above. */ > + ics = pre_ics; > + else > + ics = implicit_conversion (totype, > + rettype, > + 0, > + /*c_cast_p=*/false, convflags, > + complain); > > /* If LOOKUP_NO_TEMP_BIND isn't set, then this is > copy-initialization. In that case, "The result of the > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C > new file mode 100644 > index 00000000000..bcbc08fe869 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C > @@ -0,0 +1,14 @@ > +// { dg-do compile { target c++20 } } > + > +template <class T> > +struct Error { static constexpr auto value = T::value; }; > + > +struct A { A(const A&); }; > + > +template <class T> > +struct B { operator bool() requires Error<T>::value; }; > + > +template <class T> > +concept C = requires (B<T> b) { A(b); }; > + > +static_assert(!C<int>); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: Check constraints only on candidate conversion functions 2020-10-27 16:56 ` Jason Merrill @ 2020-10-27 20:13 ` Patrick Palka 0 siblings, 0 replies; 5+ messages in thread From: Patrick Palka @ 2020-10-27 20:13 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Tue, 27 Oct 2020, Jason Merrill wrote: > On 10/26/20 5:37 PM, Patrick Palka wrote: > > In the testcase below, we're overeagerly checking the constraints on > > the conversion function B<int>::operator bool() as part of finding an > > implicit conversion sequence from B<int> to const A&. > > > > This behavior seems to be nonconforming because according to > > [over.match.copy] and [over.match.conv], only those conversion functions > > which yield a type that can be converted to the target type via a > > standard conversion sequence are candidate functions, and according to > > [over.match.viable], only candidate functions get their constraints > > checked. > > Hmm. I agree with your reading of the standard, but I'm not sure this is what > we actually want. It's the reverse of the agreed direction for CWG2369, to > check constraints before non-dependent conversions for function arguments, and > goes against the more general principle that the fix for unwanted > instantiation should be additional constraints. > > A different testcase would succeed before your patch, and fail after: > > template <class T> > struct Error { static constexpr auto value = T::value; }; > > struct A { A(const A&); }; > > template <class T> > struct B { operator Error<T>*() requires false; }; > > template <class T> > concept C = requires (B<T> b) { (A*)b; }; > > static_assert(!C<int>); > > I've raised this on the reflector. I see, thanks. I should note I originally hit this issue in some of the <ranges> testcases while working on https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557237.html to make us cache satisfaction results aggressively. Though after some experimentation, I managed to come up with a <ranges> testcase that misbehaves in the same way, even with GCC 10's conservative satisfaction caching. I filed PR libstdc++/96700 and posted a more detailed analysis of the problem. > > > This patch fixes this by swapping the order of the calls to > > add_candidates and implicit_conversion in build_user_type_conversion_1 > > when the conversion function has a non-dependent result type. > > (Conversion function templates with a dependent result type already get > > handled appropriately in add_template_candidate, it seems.) > > Testcase? > > But yes, for dependent result types if there isn't a conversion, deduction > will fail. Ah, indeed. Here's an updated patch that adds to the testcase a conversion function template for which deduction fails, to verify we don't check its constraints either: -- >8 -- gcc/cp/ChangeLog: * call.c (build_user_type_conversion_1): When the conversion function has a non-dependent result type, pre-check the conversion sequence that would follow the conversion function. gcc/testsuite/ChangeLog: * g++.dg/cpp2a/concepts-conv3.C: New test. --- gcc/cp/call.c | 31 +++++++++++++++++---- gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C | 19 +++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C diff --git a/gcc/cp/call.c b/gcc/cp/call.c index bd662518958..01adc5dc2ab 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -4099,6 +4099,22 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, if (TYPE_REF_P (totype)) convflags |= LOOKUP_NO_TEMP_BIND; + conversion *pre_ics = NULL; + if (!uses_template_parms (TREE_TYPE (conv_fns))) + { + /* A conversion function is a candidate function only if it + yields a type that can be converted to the target type. + For conversion functions with non-dependent result type, + we check this now to discard non-candidates before calling + add_candidates. */ + pre_ics = implicit_conversion (totype, TREE_TYPE (conv_fns), + /*expr=*/NULL_TREE, + /*c_cast_p=*/false, convflags, + complain); + if (!pre_ics) + continue; + } + old_candidates = candidates; add_candidates (TREE_VALUE (conv_fns), first_arg, NULL, totype, NULL_TREE, false, @@ -4112,12 +4128,15 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags, continue; tree rettype = TREE_TYPE (TREE_TYPE (cand->fn)); - conversion *ics - = implicit_conversion (totype, - rettype, - 0, - /*c_cast_p=*/false, convflags, - complain); + conversion *ics; + if (pre_ics) + /* No need to recompute it. */ + ics = pre_ics; + else + ics = implicit_conversion (totype, rettype, + /*expr=*/NULL_TREE, + /*c_cast_p=*/false, convflags, + complain); /* If LOOKUP_NO_TEMP_BIND isn't set, then this is copy-initialization. In that case, "The result of the diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C new file mode 100644 index 00000000000..e0e49c737a8 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-conv3.C @@ -0,0 +1,19 @@ +// { dg-do compile { target c++20 } } + +template <class T> +struct Error { static constexpr auto value = T::value; }; + +struct A { A(const A&); }; + +template <class T> +struct B { + operator bool() requires Error<T>::value; + + template <class U> + operator U*() requires Error<T>::value; +}; + +template <class T> +concept C = requires (B<T> b) { A(b); }; + +static_assert(!C<int>); -- 2.29.0.rc0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] 2020-10-26 21:37 [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Patrick Palka 2020-10-26 21:37 ` [PATCH] c++: Check constraints only on candidate conversion functions Patrick Palka @ 2020-10-27 14:07 ` Jason Merrill 1 sibling, 0 replies; 5+ messages in thread From: Jason Merrill @ 2020-10-27 14:07 UTC (permalink / raw) To: Patrick Palka, gcc-patches On 10/26/20 5:37 PM, Patrick Palka wrote: > This makes mark_used check constraints of a function _before_ calling > maybe_instantiate_decl, so that we don't try instantiating a function > (as part of return type deduction) with unsatisfied constraints. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk and perhaps the 10 branch? OK for both. > gcc/cp/ChangeLog: > > PR c++/95132 > * decl2.c (mark_used): Move up the constraints_satisfied_p check > so that it happens before calling maybe_instantiate_decl. > > gcc/testsuite/ChangeLog: > > PR c++/95132 > * g++.dg/cpp2a/concepts-fn7.C: New test. > --- > gcc/cp/decl2.c | 30 +++++++++++------------ > gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C | 11 +++++++++ > 2 files changed, 26 insertions(+), 15 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C > > diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c > index 2f0d6370146..de2956aa5f0 100644 > --- a/gcc/cp/decl2.c > +++ b/gcc/cp/decl2.c > @@ -5604,6 +5604,21 @@ mark_used (tree decl, tsubst_flags_t complain) > if (DECL_ODR_USED (decl)) > return true; > > + if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL > + && !constraints_satisfied_p (decl)) > + { > + if (complain & tf_error) > + { > + auto_diagnostic_group d; > + error ("use of function %qD with unsatisfied constraints", > + decl); > + location_t loc = DECL_SOURCE_LOCATION (decl); > + inform (loc, "declared here"); > + diagnose_constraints (loc, decl, NULL_TREE); > + } > + return false; > + } > + > /* Normally, we can wait until instantiation-time to synthesize DECL. > However, if DECL is a static data member initialized with a constant > or a constexpr function, we need it right now because a reference to > @@ -5614,21 +5629,6 @@ mark_used (tree decl, tsubst_flags_t complain) > directly. */ > maybe_instantiate_decl (decl); > > - if (flag_concepts && TREE_CODE (decl) == FUNCTION_DECL > - && !constraints_satisfied_p (decl)) > - { > - if (complain & tf_error) > - { > - auto_diagnostic_group d; > - error ("use of function %qD with unsatisfied constraints", > - decl); > - location_t loc = DECL_SOURCE_LOCATION (decl); > - inform (loc, "declared here"); > - diagnose_constraints (loc, decl, NULL_TREE); > - } > - return false; > - } > - > if (processing_template_decl || in_template_function ()) > return true; > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C > new file mode 100644 > index 00000000000..7fad6f374b7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn7.C > @@ -0,0 +1,11 @@ > +// PR c++/95132 > +// { dg-do compile { target c++20 } } > + > +template<typename T> struct A { > + static auto f() requires false { return T::fail; } > +}; > + > +template<typename T> > +constexpr bool v = requires { A<T>::f(); }; > + > +static_assert(!v<int>); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-27 20:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-26 21:37 [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Patrick Palka 2020-10-26 21:37 ` [PATCH] c++: Check constraints only on candidate conversion functions Patrick Palka 2020-10-27 16:56 ` Jason Merrill 2020-10-27 20:13 ` Patrick Palka 2020-10-27 14:07 ` [PATCH] c++: Check constraints before instantiation from mark_used [PR95132] Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).