Message ID | 20200804192234.2282332-1-ppalka@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: dependent constraint on placeholder return type [PR96443] | expand |
On Tue, 4 Aug 2020, Patrick Palka wrote: > In the testcase below, we never substitute function-template arguments > into f15<int>'s placeholder-return-type constraint, which leads to us > incorrectly rejecting this instantiation in do_auto_deduction due to > satisfaction failure (of the constraint SameAs<int, decltype(x)>). > > The fact that we incorrectly reject this testcase is masked by the > other instantiation f15<char>, which we correctly reject and diagnose > (by accident). > > A good place to do this missing substitution seems to be during > TEMPLATE_TYPE_PARM level lowering. So this patch adds a call to > tsubst_constraint there, and also adds dg-bogus directives to this > testcase wherever we expect instantiation to succeed. (So without the > substitution fix, this last dg-bogus would FAIL). > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > range-v3 projects. Does this look OK to commit? > > gcc/cp/ChangeLog: > > PR c++/96443 > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > the constraints on a placeholder type when its level. > > gcc/testsuite/ChangeLog: > > PR c++/96443 > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect > instantiation to succeed. Looking back at this patch with fresh eyes, I realized that the commit message is not the best. I rewrote the commit message to hopefully be more coherent below: -- >8 -- Subject: [PATCH] c++: dependent constraint on placeholder return type [PR96443] In the testcase concepts-ts1.C, we're incorrectly rejecting the call to 'f15(0)' due to satisfaction failure of the function's placeholder-return-type constraint. The testcase doesn't spot this rejection because the error we emit for the constraint failure points to f15's return statement instead of the call site, and we already have a dg-error at the return statement to verify the (correct) rejection of the call f15('a'). So in order to verify that we indeed accept the call 'f15(0)', we need to add a dg-bogus directive at the call site to look for the "required from here" diagnostic line that generally accompanies an instantiation failure. As for why satisfaction failure occurs, it turns out that we never substitute the template arguments of a function template specialization in to its placeholder-return-type constraint. So in this case during do_auto_deduction, we end up checking satisfaction of the still-dependent constraint SameAs<int, decltype(x)> from do_auto_deduction, which fails because it's dependent. A good place to do this missing substitution seems to be during TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to tsubst_constraint there. Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and range-v3 projects. Does this look OK to commit? gcc/cp/ChangeLog: PR c++/96443 * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into the constraints on a placeholder type when reducing its level. gcc/testsuite/ChangeLog: PR c++/96443 * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15 that we expect to accept. --- gcc/cp/pt.c | 7 ++++--- gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index e7496002c1c..9f3426f8249 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl) if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) { - /* Propagate constraints on placeholders since they are - only instantiated during satisfaction. */ + /* Substitute constraints on placeholders when reducing + their level. */ if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; + PLACEHOLDER_TYPE_CONSTRAINTS (r) + = tsubst_constraint (constr, args, complain, in_decl); else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) { pl = tsubst_copy (pl, args, complain, in_decl); diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C index 1cefe3b243f..a116cac4ea4 100644 --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C @@ -40,7 +40,7 @@ void driver() f3('a'); // { dg-error "" } f4(0, 0); f4(0, 'a'); // { dg-error "" } - f15(0); + f15(0); // { dg-bogus "" } f15('a'); // { dg-message "" } }
On 8/4/20 8:00 PM, Patrick Palka wrote: > On Tue, 4 Aug 2020, Patrick Palka wrote: > >> In the testcase below, we never substitute function-template arguments >> into f15<int>'s placeholder-return-type constraint, which leads to us >> incorrectly rejecting this instantiation in do_auto_deduction due to >> satisfaction failure (of the constraint SameAs<int, decltype(x)>). >> >> The fact that we incorrectly reject this testcase is masked by the >> other instantiation f15<char>, which we correctly reject and diagnose >> (by accident). >> >> A good place to do this missing substitution seems to be during >> TEMPLATE_TYPE_PARM level lowering. So this patch adds a call to >> tsubst_constraint there, and also adds dg-bogus directives to this >> testcase wherever we expect instantiation to succeed. (So without the >> substitution fix, this last dg-bogus would FAIL). >> Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and >> range-v3 projects. Does this look OK to commit? >> >> gcc/cp/ChangeLog: >> >> PR c++/96443 >> * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into >> the constraints on a placeholder type when its level. >> >> gcc/testsuite/ChangeLog: >> >> PR c++/96443 >> * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect >> instantiation to succeed. > > Looking back at this patch with fresh eyes, I realized that the commit > message is not the best. I rewrote the commit message to hopefully be > more coherent below: > > -- >8 -- > > Subject: [PATCH] c++: dependent constraint on placeholder return type > [PR96443] > > In the testcase concepts-ts1.C, we're incorrectly rejecting the call to > 'f15(0)' due to satisfaction failure of the function's > placeholder-return-type constraint. > > The testcase doesn't spot this rejection because the error we emit for > the constraint failure points to f15's return statement instead of the > call site, and we already have a dg-error at the return statement to > verify the (correct) rejection of the call f15('a'). So in order to > verify that we indeed accept the call 'f15(0)', we need to add a > dg-bogus directive at the call site to look for the "required from here" > diagnostic line that generally accompanies an instantiation failure. > > As for why satisfaction failure occurs, it turns out that we never > substitute the template arguments of a function template specialization > in to its placeholder-return-type constraint. So in this case during > do_auto_deduction, we end up checking satisfaction of the still-dependent > constraint SameAs<int, decltype(x)> from do_auto_deduction, which fails > because it's dependent. > > A good place to do this missing substitution seems to be during > TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to > tsubst_constraint there. Doing substitution seems like the wrong approach here; requirements should never be substituted except as part of satisfaction calculation (or, rarely, for declaration matching). If we aren't communicating all the necessary template arguments to the later satisfaction, that's what we need to fix. > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > range-v3 projects. Does this look OK to commit? > > gcc/cp/ChangeLog: > > PR c++/96443 > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > the constraints on a placeholder type when reducing its level. > > gcc/testsuite/ChangeLog: > > PR c++/96443 > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15 > that we expect to accept. > --- > gcc/cp/pt.c | 7 ++++--- > gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index e7496002c1c..9f3426f8249 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl) > > if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) > { > - /* Propagate constraints on placeholders since they are > - only instantiated during satisfaction. */ > + /* Substitute constraints on placeholders when reducing > + their level. */ > if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) > - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; > + PLACEHOLDER_TYPE_CONSTRAINTS (r) > + = tsubst_constraint (constr, args, complain, in_decl); > else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) > { > pl = tsubst_copy (pl, args, complain, in_decl); > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > index 1cefe3b243f..a116cac4ea4 100644 > --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > @@ -40,7 +40,7 @@ void driver() > f3('a'); // { dg-error "" } > f4(0, 0); > f4(0, 'a'); // { dg-error "" } > - f15(0); > + f15(0); // { dg-bogus "" } > f15('a'); // { dg-message "" } > } > >
On Wed, 5 Aug 2020, Jason Merrill wrote: > On 8/4/20 8:00 PM, Patrick Palka wrote: > > On Tue, 4 Aug 2020, Patrick Palka wrote: > > > > > In the testcase below, we never substitute function-template arguments > > > into f15<int>'s placeholder-return-type constraint, which leads to us > > > incorrectly rejecting this instantiation in do_auto_deduction due to > > > satisfaction failure (of the constraint SameAs<int, decltype(x)>). > > > > > > The fact that we incorrectly reject this testcase is masked by the > > > other instantiation f15<char>, which we correctly reject and diagnose > > > (by accident). > > > > > > A good place to do this missing substitution seems to be during > > > TEMPLATE_TYPE_PARM level lowering. So this patch adds a call to > > > tsubst_constraint there, and also adds dg-bogus directives to this > > > testcase wherever we expect instantiation to succeed. (So without the > > > substitution fix, this last dg-bogus would FAIL). > > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > > range-v3 projects. Does this look OK to commit? > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/96443 > > > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > > > the constraints on a placeholder type when its level. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/96443 > > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect > > > instantiation to succeed. > > > > Looking back at this patch with fresh eyes, I realized that the commit > > message is not the best. I rewrote the commit message to hopefully be > > more coherent below: > > > > -- >8 -- > > > > Subject: [PATCH] c++: dependent constraint on placeholder return type > > [PR96443] > > > > In the testcase concepts-ts1.C, we're incorrectly rejecting the call to > > 'f15(0)' due to satisfaction failure of the function's > > placeholder-return-type constraint. > > > > The testcase doesn't spot this rejection because the error we emit for > > the constraint failure points to f15's return statement instead of the > > call site, and we already have a dg-error at the return statement to > > verify the (correct) rejection of the call f15('a'). So in order to > > verify that we indeed accept the call 'f15(0)', we need to add a > > dg-bogus directive at the call site to look for the "required from here" > > diagnostic line that generally accompanies an instantiation failure. > > > > As for why satisfaction failure occurs, it turns out that we never > > substitute the template arguments of a function template specialization > > in to its placeholder-return-type constraint. So in this case during > > do_auto_deduction, we end up checking satisfaction of the still-dependent > > constraint SameAs<int, decltype(x)> from do_auto_deduction, which fails > > because it's dependent. > > > > A good place to do this missing substitution seems to be during > > TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to > > tsubst_constraint there. > > Doing substitution seems like the wrong approach here; requirements should > never be substituted except as part of satisfaction calculation (or, rarely, > for declaration matching). If we aren't communicating all the necessary > template arguments to the later satisfaction, that's what we need to fix. Ah okay, that makes sense. It also looks like the question of perform a single full substitution (during auto deduction) vs two substitutions may also be a correctness issue in light of SFINAE. Consider the following testcase: template<typename T, typename U> concept C = true; auto f(auto x) -> C<decltype(x.fail())> auto { return 0; } auto f(auto x, ...) { return 1; } int a = f(0); If we do a single substitution, then the substitution failure is a hard error and we'll reject this testcase. If we do two substitutions, then it's a SFINAE error and we select the second overload. Would we be correct to issue a hard error here? > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > range-v3 projects. Does this look OK to commit? > > > > gcc/cp/ChangeLog: > > > > PR c++/96443 > > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > > the constraints on a placeholder type when reducing its level. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/96443 > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15 > > that we expect to accept. > > --- > > gcc/cp/pt.c | 7 ++++--- > > gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > > index e7496002c1c..9f3426f8249 100644 > > --- a/gcc/cp/pt.c > > +++ b/gcc/cp/pt.c > > @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t > > complain, tree in_decl) > > if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) > > { > > - /* Propagate constraints on placeholders since they are > > - only instantiated during satisfaction. */ > > + /* Substitute constraints on placeholders when reducing > > + their level. */ > > if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) > > - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; > > + PLACEHOLDER_TYPE_CONSTRAINTS (r) > > + = tsubst_constraint (constr, args, complain, in_decl); > > else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) > > { > > pl = tsubst_copy (pl, args, complain, in_decl); > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > index 1cefe3b243f..a116cac4ea4 100644 > > --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > @@ -40,7 +40,7 @@ void driver() > > f3('a'); // { dg-error "" } > > f4(0, 0); > > f4(0, 'a'); // { dg-error "" } > > - f15(0); > > + f15(0); // { dg-bogus "" } > > f15('a'); // { dg-message "" } > > } > > > >
On Thu, 6 Aug 2020, Patrick Palka wrote: > On Wed, 5 Aug 2020, Jason Merrill wrote: > > > On 8/4/20 8:00 PM, Patrick Palka wrote: > > > On Tue, 4 Aug 2020, Patrick Palka wrote: > > > > > > > In the testcase below, we never substitute function-template arguments > > > > into f15<int>'s placeholder-return-type constraint, which leads to us > > > > incorrectly rejecting this instantiation in do_auto_deduction due to > > > > satisfaction failure (of the constraint SameAs<int, decltype(x)>). > > > > > > > > The fact that we incorrectly reject this testcase is masked by the > > > > other instantiation f15<char>, which we correctly reject and diagnose > > > > (by accident). > > > > > > > > A good place to do this missing substitution seems to be during > > > > TEMPLATE_TYPE_PARM level lowering. So this patch adds a call to > > > > tsubst_constraint there, and also adds dg-bogus directives to this > > > > testcase wherever we expect instantiation to succeed. (So without the > > > > substitution fix, this last dg-bogus would FAIL). > > > > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > > > range-v3 projects. Does this look OK to commit? > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > PR c++/96443 > > > > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > > > > the constraints on a placeholder type when its level. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > PR c++/96443 > > > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect > > > > instantiation to succeed. > > > > > > Looking back at this patch with fresh eyes, I realized that the commit > > > message is not the best. I rewrote the commit message to hopefully be > > > more coherent below: > > > > > > -- >8 -- > > > > > > Subject: [PATCH] c++: dependent constraint on placeholder return type > > > [PR96443] > > > > > > In the testcase concepts-ts1.C, we're incorrectly rejecting the call to > > > 'f15(0)' due to satisfaction failure of the function's > > > placeholder-return-type constraint. > > > > > > The testcase doesn't spot this rejection because the error we emit for > > > the constraint failure points to f15's return statement instead of the > > > call site, and we already have a dg-error at the return statement to > > > verify the (correct) rejection of the call f15('a'). So in order to > > > verify that we indeed accept the call 'f15(0)', we need to add a > > > dg-bogus directive at the call site to look for the "required from here" > > > diagnostic line that generally accompanies an instantiation failure. > > > > > > As for why satisfaction failure occurs, it turns out that we never > > > substitute the template arguments of a function template specialization > > > in to its placeholder-return-type constraint. So in this case during > > > do_auto_deduction, we end up checking satisfaction of the still-dependent > > > constraint SameAs<int, decltype(x)> from do_auto_deduction, which fails > > > because it's dependent. > > > > > > A good place to do this missing substitution seems to be during > > > TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to > > > tsubst_constraint there. > > > > Doing substitution seems like the wrong approach here; requirements should > > never be substituted except as part of satisfaction calculation (or, rarely, > > for declaration matching). If we aren't communicating all the necessary > > template arguments to the later satisfaction, that's what we need to fix. > > Ah okay, that makes sense. > > It also looks like the question of perform a single full substitution > (during auto deduction) vs two substitutions may also be a correctness > issue in light of SFINAE. Consider the following testcase: > > template<typename T, typename U> > concept C = true; > > auto f(auto x) -> C<decltype(x.fail())> auto { return 0; } > auto f(auto x, ...) { return 1; } > > int a = f(0); > > If we do a single substitution, then the substitution failure is a hard > error and we'll reject this testcase. If we do two substitutions, then > it's a SFINAE error and we select the second overload. Would we be > correct to issue a hard error here? Please ignore the previous message :) I had missed that the substitution failure should result in the constraint evaluating to false as part of satisfaction. Sorry for the noise. > > > > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > > range-v3 projects. Does this look OK to commit? > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/96443 > > > * pt.c (tsubst) <case TEMPLATE_TYPE_PARM>: Substitute into > > > the constraints on a placeholder type when reducing its level. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/96443 > > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15 > > > that we expect to accept. > > > --- > > > gcc/cp/pt.c | 7 ++++--- > > > gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > > > index e7496002c1c..9f3426f8249 100644 > > > --- a/gcc/cp/pt.c > > > +++ b/gcc/cp/pt.c > > > @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t > > > complain, tree in_decl) > > > if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) > > > { > > > - /* Propagate constraints on placeholders since they are > > > - only instantiated during satisfaction. */ > > > + /* Substitute constraints on placeholders when reducing > > > + their level. */ > > > if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) > > > - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; > > > + PLACEHOLDER_TYPE_CONSTRAINTS (r) > > > + = tsubst_constraint (constr, args, complain, in_decl); > > > else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) > > > { > > > pl = tsubst_copy (pl, args, complain, in_decl); > > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > > b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > > index 1cefe3b243f..a116cac4ea4 100644 > > > --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > > @@ -40,7 +40,7 @@ void driver() > > > f3('a'); // { dg-error "" } > > > f4(0, 0); > > > f4(0, 'a'); // { dg-error "" } > > > - f15(0); > > > + f15(0); // { dg-bogus "" } > > > f15('a'); // { dg-message "" } > > > } > > > > > > > >
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index e7496002c1c..04bf6da0cdd 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl) if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) { - /* Propagate constraints on placeholders since they are - only instantiated during satisfaction. */ + /* Substitute constraints on placeholder when reducing + their level. */ if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; + PLACEHOLDER_TYPE_CONSTRAINTS (r) + = tsubst_constraint (constr, args, complain, in_decl); else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) { pl = tsubst_copy (pl, args, complain, in_decl); diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C index 1cefe3b243f..1a9b71c2296 100644 --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C @@ -34,13 +34,13 @@ auto f15(auto x) -> SameAs<decltype(x)> { return 0; } // { dg-error "deduced ret void driver() { - f1(0); + f1(0); // { dg-bogus "" } f2(0); // { dg-error "" } - f3(0); + f3(0); // { dg-bogus "" } f3('a'); // { dg-error "" } - f4(0, 0); + f4(0, 0); // { dg-bogus "" } f4(0, 'a'); // { dg-error "" } - f15(0); + f15(0); // { dg-bogus "" } f15('a'); // { dg-message "" } }