Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On 3/27/20 11:28 AM, Patrick Palka wrote:
On Thu, 26 Mar 2020, Patrick Palka wrote:
On Thu, 26 Mar 2020, Jason Merrill wrote:
On 3/26/20 5:24 PM, Patrick Palka wrote:
On Wed, 25 Mar 2020, Jason Merrill wrote:
On 3/25/20 12:17 PM, Patrick Palka wrote:
This PR reports that the requires-expression in
auto f = [] { };
static_assert(requires { f(); });
erroneously evaluates to false. The way we end up evaluating to false
goes
as
follows. During the parsing of the requires-expression, we call
finish_call_expr from cp_parser_requires_expression with the arguments
fn = VIEW_CONVERT_EXPR(f);
args = {}
which does the full processing of the call (since we're not inside a
template)
and returns
::operator() ();
Later, when evaluating the requires-expression, we call finish_call_expr
again,
this time from tsubst_expr from satisfy_atom, with the arguments
fn = operator()
args = { }
(which, as expected, correspond to the CALL_EXPR returned by
finish_call_expr
the first time). But this time, finish_call_expr returns
error_mark_node
because
it doesn't expect to see an explicit 'this' argument in the args array,
treating
it instead as a user-written argument which causes the only candidate
function
to be discarded. This causes the requires-expression to evaluate to
false.
In short, it seems finish_call_expr is not idempotent on certain inputs
when
!processing_template_decl. Assuming this idempotency issue is not
specific
to
finish_call_expr, it seems that the safest thing to do is to avoid doing
full
semantic processing twice when parsing and evaluating a
requires-expression
that
lives outside of a template definition.
Absolutely. We shouldn't call tsubst_expr on non-template trees.
This patch achieves this by temporarily setting processing_template_decl
to
non-zero when parsing the body of a requires-expression. This way, full
semantic processing will always be done only during evaluation and not
during
parsing.
Hmm, interesting approach, but I think the standard requires us to treat
requires-expressions outside of template context like normal non-template
code: "[Note: If a requires-expression contains invalid
types or expressions in its requirements, and it does not appear within
the
declaration of a templated entity, then the program is ill-formed. --end
note]"
So I think better to avoid the tsubst_expr later, either by immediately
evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do
that
by immediately resolving the requires-expression in non-template context,
or
by marking it up somehow to prevent the substitution.
If we go the route of marking the REQUIRES_EXPR or its subtrees, then we
would need to change tsubst_requires_expr and its callees as well as
changing diagnose_requires_expr and its callees so that they
conditionally avoid doing tsubst_expr, which seems likes like an
undesirable amount of churn.
Another downside is that we apparently already always pretend we're
inside a template when parsing a nested-requirement, which means marking
a REQUIRES_EXPR based on processing_template_decl won't work correctly
for a REQUIRES_EXPR inside a nested-requirement like this one:
requires { requires requires { ... }; };
These two points nudged me to instead go the route of pretending we're
inside a template when parsing the body of a requires-expr, and then
immediately afterwards doing tsubst_requires_expr on the body to perform
the full semantic processing.
Does the following look OK?
I notice that we currently fail to handle requires-expressions in regular
expression context:
int main() { return requires { 42; }; } // ICE
This remains unchanged with the patch. We could return the result of
tsubst_requires_expr from cp_parser_requires_expression instead of
throwing it away, but that would break our support for explaining an
unsatisfied REQUIRES_EXPR inside a static_assert, that the testcase
g++.dg/concepts/diagnostic7.C verifies.
What would be the best way to handle these stray REQUIRES_EXPRs?
We probably want to lower them in cp_genericize_r. And I suppose if we're
doing that we may not need the tsubst_requires_expr at parse time.
Sounds good, I'll look into this.
If we don't do tsubst_requires_expr at parse time, then it seems we'd
have to do it in convert_to_void (to handle REQUIRES_EXPRs that are
operands of EXPR_STMT, which don't survive to see gimplification) as
well as in cp_genericize_r.
If we keep doing it at parse time, then we don't have to do any special
processing in convert_to_void, and in cp_genericize_r we just have to
check constraints_satisfied_p. So I opted to keep the
tsubst_requires_expr at parse time:
OK, thanks.
-- >8 --
gcc/cp/ChangeLog:
PR c++/94252
* constraint.cc (tsubst_compound_requirement): Always suppress errors
from type_deducible_p and expression_convertible_p, as they're not
substitution errors.
(diagnose_atomic_constraint) :
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On Thu, 26 Mar 2020, Patrick Palka wrote:
> On Thu, 26 Mar 2020, Jason Merrill wrote:
>
> > On 3/26/20 5:24 PM, Patrick Palka wrote:
> > > On Wed, 25 Mar 2020, Jason Merrill wrote:
> > >
> > > > On 3/25/20 12:17 PM, Patrick Palka wrote:
> > > > > This PR reports that the requires-expression in
> > > > >
> > > > > auto f = [] { };
> > > > > static_assert(requires { f(); });
> > > > >
> > > > > erroneously evaluates to false. The way we end up evaluating to false
> > > > > goes
> > > > > as
> > > > > follows. During the parsing of the requires-expression, we call
> > > > > finish_call_expr from cp_parser_requires_expression with the arguments
> > > > >
> > > > > fn = VIEW_CONVERT_EXPR(f);
> > > > > args = {}
> > > > >
> > > > > which does the full processing of the call (since we're not inside a
> > > > > template)
> > > > > and returns
> > > > >
> > > > > ::operator() ();
> > > > >
> > > > > Later, when evaluating the requires-expression, we call
> > > > > finish_call_expr
> > > > > again,
> > > > > this time from tsubst_expr from satisfy_atom, with the arguments
> > > > >
> > > > > fn = operator()
> > > > > args = { }
> > > > >
> > > > > (which, as expected, correspond to the CALL_EXPR returned by
> > > > > finish_call_expr
> > > > > the first time). But this time, finish_call_expr returns
> > > > > error_mark_node
> > > > > because
> > > > > it doesn't expect to see an explicit 'this' argument in the args
> > > > > array,
> > > > > treating
> > > > > it instead as a user-written argument which causes the only candidate
> > > > > function
> > > > > to be discarded. This causes the requires-expression to evaluate to
> > > > > false.
> > > > >
> > > > > In short, it seems finish_call_expr is not idempotent on certain
> > > > > inputs
> > > > > when
> > > > > !processing_template_decl. Assuming this idempotency issue is not
> > > > > specific
> > > > > to
> > > > > finish_call_expr, it seems that the safest thing to do is to avoid
> > > > > doing
> > > > > full
> > > > > semantic processing twice when parsing and evaluating a
> > > > > requires-expression
> > > > > that
> > > > > lives outside of a template definition.
> > > >
> > > > Absolutely. We shouldn't call tsubst_expr on non-template trees.
> > > >
> > > > > This patch achieves this by temporarily setting
> > > > > processing_template_decl
> > > > > to
> > > > > non-zero when parsing the body of a requires-expression. This way,
> > > > > full
> > > > > semantic processing will always be done only during evaluation and not
> > > > > during
> > > > > parsing.
> > > >
> > > > Hmm, interesting approach, but I think the standard requires us to treat
> > > > requires-expressions outside of template context like normal
> > > > non-template
> > > > code: "[Note: If a requires-expression contains invalid
> > > > types or expressions in its requirements, and it does not appear within
> > > > the
> > > > declaration of a templated entity, then the program is ill-formed. --end
> > > > note]"
> > > >
> > > > So I think better to avoid the tsubst_expr later, either by immediately
> > > > evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do
> > > > that
> > > > by immediately resolving the requires-expression in non-template
> > > > context,
> > > > or
> > > > by marking it up somehow to prevent the substitution.
> > >
> > > If we go the route of marking the REQUIRES_EXPR or its subtrees, then we
> > > would need to change tsubst_requires_expr and its callees as well as
> > > changing diagnose_requires_expr and its callees so that they
> > > conditionally avoid doing tsubst_expr, which seems likes like an
> > > undesirable amount of churn.
> > >
> > > Another downside is that we apparently already always pretend we're
> > > inside a template when parsing a nested-requirement, which means marking
> > > a REQUIRES_EXPR based on processing_template_decl won't work correctly
> > > for a REQUIRES_EXPR inside a nested-requirement like this one:
> > >
> > > requires { requires requires { ... }; };
> > >
> > > These two points nudged me to instead go the route of pretending we're
> > > inside a template when parsing the body of a requires-expr, and then
> > > immediately afterwards doing tsubst_requires_expr on the body to perform
> > > the full semantic processing.
> > >
> > > Does the following look OK?
> > >
> > > >
> > > > I notice that we currently fail to handle requires-expressions in
> > > > regular
> > > > expression context:
> > > >
> > > > int main() { return requires { 42; }; } // ICE
> > >
> > > This remains unchanged with the patch. We could return the result of
> > > tsubst_requires_expr from cp_parser_requires_expression instead of
> > > throwing it away, but that would break our support for explaining an
> > > unsatisfied REQUIRES_EXPR inside a static_assert, that the testcase
> > > g++.dg/concepts/diagnostic7.C verifies.
> > >
> > > What would be the best way to
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On Thu, 26 Mar 2020, Jason Merrill wrote:
> On 3/26/20 5:24 PM, Patrick Palka wrote:
> > On Wed, 25 Mar 2020, Jason Merrill wrote:
> >
> > > On 3/25/20 12:17 PM, Patrick Palka wrote:
> > > > This PR reports that the requires-expression in
> > > >
> > > > auto f = [] { };
> > > > static_assert(requires { f(); });
> > > >
> > > > erroneously evaluates to false. The way we end up evaluating to false
> > > > goes
> > > > as
> > > > follows. During the parsing of the requires-expression, we call
> > > > finish_call_expr from cp_parser_requires_expression with the arguments
> > > >
> > > > fn = VIEW_CONVERT_EXPR(f);
> > > > args = {}
> > > >
> > > > which does the full processing of the call (since we're not inside a
> > > > template)
> > > > and returns
> > > >
> > > > ::operator() ();
> > > >
> > > > Later, when evaluating the requires-expression, we call finish_call_expr
> > > > again,
> > > > this time from tsubst_expr from satisfy_atom, with the arguments
> > > >
> > > > fn = operator()
> > > > args = { }
> > > >
> > > > (which, as expected, correspond to the CALL_EXPR returned by
> > > > finish_call_expr
> > > > the first time). But this time, finish_call_expr returns
> > > > error_mark_node
> > > > because
> > > > it doesn't expect to see an explicit 'this' argument in the args array,
> > > > treating
> > > > it instead as a user-written argument which causes the only candidate
> > > > function
> > > > to be discarded. This causes the requires-expression to evaluate to
> > > > false.
> > > >
> > > > In short, it seems finish_call_expr is not idempotent on certain inputs
> > > > when
> > > > !processing_template_decl. Assuming this idempotency issue is not
> > > > specific
> > > > to
> > > > finish_call_expr, it seems that the safest thing to do is to avoid doing
> > > > full
> > > > semantic processing twice when parsing and evaluating a
> > > > requires-expression
> > > > that
> > > > lives outside of a template definition.
> > >
> > > Absolutely. We shouldn't call tsubst_expr on non-template trees.
> > >
> > > > This patch achieves this by temporarily setting processing_template_decl
> > > > to
> > > > non-zero when parsing the body of a requires-expression. This way, full
> > > > semantic processing will always be done only during evaluation and not
> > > > during
> > > > parsing.
> > >
> > > Hmm, interesting approach, but I think the standard requires us to treat
> > > requires-expressions outside of template context like normal non-template
> > > code: "[Note: If a requires-expression contains invalid
> > > types or expressions in its requirements, and it does not appear within
> > > the
> > > declaration of a templated entity, then the program is ill-formed. --end
> > > note]"
> > >
> > > So I think better to avoid the tsubst_expr later, either by immediately
> > > evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do
> > > that
> > > by immediately resolving the requires-expression in non-template context,
> > > or
> > > by marking it up somehow to prevent the substitution.
> >
> > If we go the route of marking the REQUIRES_EXPR or its subtrees, then we
> > would need to change tsubst_requires_expr and its callees as well as
> > changing diagnose_requires_expr and its callees so that they
> > conditionally avoid doing tsubst_expr, which seems likes like an
> > undesirable amount of churn.
> >
> > Another downside is that we apparently already always pretend we're
> > inside a template when parsing a nested-requirement, which means marking
> > a REQUIRES_EXPR based on processing_template_decl won't work correctly
> > for a REQUIRES_EXPR inside a nested-requirement like this one:
> >
> > requires { requires requires { ... }; };
> >
> > These two points nudged me to instead go the route of pretending we're
> > inside a template when parsing the body of a requires-expr, and then
> > immediately afterwards doing tsubst_requires_expr on the body to perform
> > the full semantic processing.
> >
> > Does the following look OK?
> >
> > >
> > > I notice that we currently fail to handle requires-expressions in regular
> > > expression context:
> > >
> > > int main() { return requires { 42; }; } // ICE
> >
> > This remains unchanged with the patch. We could return the result of
> > tsubst_requires_expr from cp_parser_requires_expression instead of
> > throwing it away, but that would break our support for explaining an
> > unsatisfied REQUIRES_EXPR inside a static_assert, that the testcase
> > g++.dg/concepts/diagnostic7.C verifies.
> >
> > What would be the best way to handle these stray REQUIRES_EXPRs?
>
> We probably want to lower them in cp_genericize_r. And I suppose if we're
> doing that we may not need the tsubst_requires_expr at parse time.
Sounds good, I'll look into this.
>
> > -- >8 --
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/94252
> > * constraint.cc (tsubst_compound_requirement): Always suppress
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On 3/26/20 5:24 PM, Patrick Palka wrote:
On Wed, 25 Mar 2020, Jason Merrill wrote:
On 3/25/20 12:17 PM, Patrick Palka wrote:
This PR reports that the requires-expression in
auto f = [] { };
static_assert(requires { f(); });
erroneously evaluates to false. The way we end up evaluating to false goes
as
follows. During the parsing of the requires-expression, we call
finish_call_expr from cp_parser_requires_expression with the arguments
fn = VIEW_CONVERT_EXPR(f);
args = {}
which does the full processing of the call (since we're not inside a
template)
and returns
::operator() ();
Later, when evaluating the requires-expression, we call finish_call_expr
again,
this time from tsubst_expr from satisfy_atom, with the arguments
fn = operator()
args = { }
(which, as expected, correspond to the CALL_EXPR returned by
finish_call_expr
the first time). But this time, finish_call_expr returns error_mark_node
because
it doesn't expect to see an explicit 'this' argument in the args array,
treating
it instead as a user-written argument which causes the only candidate
function
to be discarded. This causes the requires-expression to evaluate to false.
In short, it seems finish_call_expr is not idempotent on certain inputs when
!processing_template_decl. Assuming this idempotency issue is not specific
to
finish_call_expr, it seems that the safest thing to do is to avoid doing
full
semantic processing twice when parsing and evaluating a requires-expression
that
lives outside of a template definition.
Absolutely. We shouldn't call tsubst_expr on non-template trees.
This patch achieves this by temporarily setting processing_template_decl to
non-zero when parsing the body of a requires-expression. This way, full
semantic processing will always be done only during evaluation and not
during
parsing.
Hmm, interesting approach, but I think the standard requires us to treat
requires-expressions outside of template context like normal non-template
code: "[Note: If a requires-expression contains invalid
types or expressions in its requirements, and it does not appear within the
declaration of a templated entity, then the program is ill-formed. --end
note]"
So I think better to avoid the tsubst_expr later, either by immediately
evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do that
by immediately resolving the requires-expression in non-template context, or
by marking it up somehow to prevent the substitution.
If we go the route of marking the REQUIRES_EXPR or its subtrees, then we
would need to change tsubst_requires_expr and its callees as well as
changing diagnose_requires_expr and its callees so that they
conditionally avoid doing tsubst_expr, which seems likes like an
undesirable amount of churn.
Another downside is that we apparently already always pretend we're
inside a template when parsing a nested-requirement, which means marking
a REQUIRES_EXPR based on processing_template_decl won't work correctly
for a REQUIRES_EXPR inside a nested-requirement like this one:
requires { requires requires { ... }; };
These two points nudged me to instead go the route of pretending we're
inside a template when parsing the body of a requires-expr, and then
immediately afterwards doing tsubst_requires_expr on the body to perform
the full semantic processing.
Does the following look OK?
I notice that we currently fail to handle requires-expressions in regular
expression context:
int main() { return requires { 42; }; } // ICE
This remains unchanged with the patch. We could return the result of
tsubst_requires_expr from cp_parser_requires_expression instead of
throwing it away, but that would break our support for explaining an
unsatisfied REQUIRES_EXPR inside a static_assert, that the testcase
g++.dg/concepts/diagnostic7.C verifies.
What would be the best way to handle these stray REQUIRES_EXPRs?
We probably want to lower them in cp_genericize_r. And I suppose if
we're doing that we may not need the tsubst_requires_expr at parse time.
-- >8 --
gcc/cp/ChangeLog:
PR c++/94252
* constraint.cc (tsubst_compound_requirement): Always suppress errors
from type_deducible_p and expression_convertible_p, as they're not
substitution errors.
(diagnose_atomic_constraint) : Remove this case so
that we diagnose INTEGER_CST expressions of non-bool type via the
default case.
* parser.c (cp_parser_requires_expression): Always parse the requirement
body as if we're processing a template, by temporarily incrementing
processing_template_decl. Afterwards, if we're not actually in a
template context, perform semantic processing to diagnose any invalid
types and expressions.
* pt.c (tsubst_copy_and_build) : Remove dead code.
Why is this dead code?
* semantics.c (finish_static_assert): Also explain an assertion failure
when the condition is a
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On Wed, 25 Mar 2020, Jason Merrill wrote:
> On 3/25/20 12:17 PM, Patrick Palka wrote:
> > This PR reports that the requires-expression in
> >
> >auto f = [] { };
> >static_assert(requires { f(); });
> >
> > erroneously evaluates to false. The way we end up evaluating to false goes
> > as
> > follows. During the parsing of the requires-expression, we call
> > finish_call_expr from cp_parser_requires_expression with the arguments
> >
> >fn = VIEW_CONVERT_EXPR(f);
> >args = {}
> >
> > which does the full processing of the call (since we're not inside a
> > template)
> > and returns
> >
> >::operator() ();
> >
> > Later, when evaluating the requires-expression, we call finish_call_expr
> > again,
> > this time from tsubst_expr from satisfy_atom, with the arguments
> >
> >fn = operator()
> >args = { }
> >
> > (which, as expected, correspond to the CALL_EXPR returned by
> > finish_call_expr
> > the first time). But this time, finish_call_expr returns error_mark_node
> > because
> > it doesn't expect to see an explicit 'this' argument in the args array,
> > treating
> > it instead as a user-written argument which causes the only candidate
> > function
> > to be discarded. This causes the requires-expression to evaluate to false.
> >
> > In short, it seems finish_call_expr is not idempotent on certain inputs when
> > !processing_template_decl. Assuming this idempotency issue is not specific
> > to
> > finish_call_expr, it seems that the safest thing to do is to avoid doing
> > full
> > semantic processing twice when parsing and evaluating a requires-expression
> > that
> > lives outside of a template definition.
>
> Absolutely. We shouldn't call tsubst_expr on non-template trees.
>
> > This patch achieves this by temporarily setting processing_template_decl to
> > non-zero when parsing the body of a requires-expression. This way, full
> > semantic processing will always be done only during evaluation and not
> > during
> > parsing.
>
> Hmm, interesting approach, but I think the standard requires us to treat
> requires-expressions outside of template context like normal non-template
> code: "[Note: If a requires-expression contains invalid
> types or expressions in its requirements, and it does not appear within the
> declaration of a templated entity, then the program is ill-formed. --end
> note]"
>
> So I think better to avoid the tsubst_expr later, either by immediately
> evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do that
> by immediately resolving the requires-expression in non-template context, or
> by marking it up somehow to prevent the substitution.
If we go the route of marking the REQUIRES_EXPR or its subtrees, then we
would need to change tsubst_requires_expr and its callees as well as
changing diagnose_requires_expr and its callees so that they
conditionally avoid doing tsubst_expr, which seems likes like an
undesirable amount of churn.
Another downside is that we apparently already always pretend we're
inside a template when parsing a nested-requirement, which means marking
a REQUIRES_EXPR based on processing_template_decl won't work correctly
for a REQUIRES_EXPR inside a nested-requirement like this one:
requires { requires requires { ... }; };
These two points nudged me to instead go the route of pretending we're
inside a template when parsing the body of a requires-expr, and then
immediately afterwards doing tsubst_requires_expr on the body to perform
the full semantic processing.
Does the following look OK?
>
> I notice that we currently fail to handle requires-expressions in regular
> expression context:
>
> int main() { return requires { 42; }; } // ICE
This remains unchanged with the patch. We could return the result of
tsubst_requires_expr from cp_parser_requires_expression instead of
throwing it away, but that would break our support for explaining an
unsatisfied REQUIRES_EXPR inside a static_assert, that the testcase
g++.dg/concepts/diagnostic7.C verifies.
What would be the best way to handle these stray REQUIRES_EXPRs?
-- >8 --
gcc/cp/ChangeLog:
PR c++/94252
* constraint.cc (tsubst_compound_requirement): Always suppress errors
from type_deducible_p and expression_convertible_p, as they're not
substitution errors.
(diagnose_atomic_constraint) : Remove this case so
that we diagnose INTEGER_CST expressions of non-bool type via the
default case.
* parser.c (cp_parser_requires_expression): Always parse the requirement
body as if we're processing a template, by temporarily incrementing
processing_template_decl. Afterwards, if we're not actually in a
template context, perform semantic processing to diagnose any invalid
types and expressions.
* pt.c (tsubst_copy_and_build) : Remove dead code.
* semantics.c (finish_static_assert): Also explain an assertion failure
when the condition is a
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On 3/25/20 12:17 PM, Patrick Palka wrote:
This PR reports that the requires-expression in
auto f = [] { };
static_assert(requires { f(); });
erroneously evaluates to false. The way we end up evaluating to false goes as
follows. During the parsing of the requires-expression, we call
finish_call_expr from cp_parser_requires_expression with the arguments
fn = VIEW_CONVERT_EXPR(f);
args = {}
which does the full processing of the call (since we're not inside a template)
and returns
::operator() ();
Later, when evaluating the requires-expression, we call finish_call_expr again,
this time from tsubst_expr from satisfy_atom, with the arguments
fn = operator()
args = { }
(which, as expected, correspond to the CALL_EXPR returned by finish_call_expr
the first time). But this time, finish_call_expr returns error_mark_node
because
it doesn't expect to see an explicit 'this' argument in the args array, treating
it instead as a user-written argument which causes the only candidate function
to be discarded. This causes the requires-expression to evaluate to false.
In short, it seems finish_call_expr is not idempotent on certain inputs when
!processing_template_decl. Assuming this idempotency issue is not specific to
finish_call_expr, it seems that the safest thing to do is to avoid doing full
semantic processing twice when parsing and evaluating a requires-expression that
lives outside of a template definition.
Absolutely. We shouldn't call tsubst_expr on non-template trees.
This patch achieves this by temporarily setting processing_template_decl to
non-zero when parsing the body of a requires-expression. This way, full
semantic processing will always be done only during evaluation and not during
parsing.
Hmm, interesting approach, but I think the standard requires us to treat
requires-expressions outside of template context like normal
non-template code: "[Note: If a requires-expression contains invalid
types or expressions in its requirements, and it does not appear within
the declaration of a templated entity, then the program is ill-formed.
--end note]"
So I think better to avoid the tsubst_expr later, either by immediately
evaluating the REQUIRES_EXPR or marking the ATOMIC_CONSTR. We could do
that by immediately resolving the requires-expression in non-template
context, or by marking it up somehow to prevent the substitution.
I notice that we currently fail to handle requires-expressions in
regular expression context:
int main() { return requires { 42; }; } // ICE
Testing is in progress. Does this look like the right solution?
gcc/cp/ChangeLog:
PR c++/94252
* parser.c (cp_parser_requires_expression): Always parse the requirement
body as if we're processing a template, by temporarily setting
processing_template_decl to non-zero.
* semantics.c (finish_static_assert): Also explain an assertion failure
when the condition is a REQUIRES_EXPR.
gcc/testsuite/ChangeLog:
PR c++/94252
* g++.dg/concepts/diagnostic7.C: New test.
* g++.dg/concepts/pr94252.C: New test.
---
gcc/cp/parser.c | 7 ++-
gcc/cp/semantics.c | 6 --
gcc/testsuite/g++.dg/concepts/diagnostic7.C | 12
gcc/testsuite/g++.dg/concepts/pr94252.C | 14 ++
4 files changed, 36 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic7.C
create mode 100644 gcc/testsuite/g++.dg/concepts/pr94252.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 03ecd7838f6..14a22b85318 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -27689,7 +27689,12 @@ cp_parser_requires_expression (cp_parser *parser)
else
parms = NULL_TREE;
-/* Parse the requirement body. */
+/* Always parse the requirement body as if we're inside a template so that
+ we always do full semantic processing only during evaluation of this
+ requires-expression and not also now during parsing. */
+processing_template_decl_sentinel ptds;
+if (!processing_template_decl)
+ processing_template_decl = 1;
reqs = cp_parser_requirement_body (parser);
if (reqs == error_mark_node)
return error_mark_node;
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index bcb2e72fbb5..e998b373af4 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -9691,8 +9691,10 @@ finish_static_assert (tree condition, tree message,
location_t location,
error ("static assertion failed: %s",
TREE_STRING_POINTER (message));
- /* Actually explain the failure if this is a concept check. */
- if (concept_check_p (orig_condition))
+ /* Actually explain the failure if this is a concept check or a
+requires-expression. */
+ if (concept_check_p (orig_condition)
+ || TREE_CODE (orig_condition) == REQUIRES_EXPR)
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On Wed, 25 Mar 2020, Marek Polacek wrote:
> On Wed, Mar 25, 2020 at 12:17:41PM -0400, Patrick Palka via Gcc-patches wrote:
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 03ecd7838f6..14a22b85318 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -27689,7 +27689,12 @@ cp_parser_requires_expression (cp_parser *parser)
> > else
> >parms = NULL_TREE;
> >
> > -/* Parse the requirement body. */
> > +/* Always parse the requirement body as if we're inside a template so
> > that
> > + we always do full semantic processing only during evaluation of this
> > + requires-expression and not also now during parsing. */
> > +processing_template_decl_sentinel ptds;
>
> This looks weird; this sentinel will always set processing_template_decl to 0,
> right? So...
>
> > +if (!processing_template_decl)
> > + processing_template_decl = 1;
>
> ...this will always be true? Did you mean to use temp_override instead?
Oops, good point. The intention was to set processing_template_decl to
1 when it is 0, and to otherwise not change it. So it looks like I
should pass false to processing_template_decl_sentinel's ctor here, like
so.
>
> I don't dare to say if this approach is OK or not, but I've certainly had
> my share of fun with CALL_EXPRs in templates :).
Hehe :)
-- >8 --
gcc/cp/ChangeLog:
PR c++/94252
* parser.c (cp_parser_requires_expression): Always parse the requirement
body as if we're processing a template, by temporarily setting
processing_template_decl to non-zero.
* semantics.c (finish_static_assert): Also explain an assertion failure
when the condition is a REQUIRES_EXPR.
gcc/testsuite/ChangeLog:
PR c++/94252
* g++.dg/concepts/diagnostic7.C: New test.
* g++.dg/concepts/pr94252.C: New test.
---
gcc/cp/parser.c | 7 ++-
gcc/cp/semantics.c | 6 --
gcc/testsuite/g++.dg/concepts/diagnostic7.C | 12
gcc/testsuite/g++.dg/concepts/pr94252.C | 14 ++
4 files changed, 36 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic7.C
create mode 100644 gcc/testsuite/g++.dg/concepts/pr94252.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 03ecd7838f6..6150814ea9e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -27689,7 +27689,12 @@ cp_parser_requires_expression (cp_parser *parser)
else
parms = NULL_TREE;
-/* Parse the requirement body. */
+/* Always parse the requirement body as if we're inside a template so that
+ we always do full semantic processing only during evaluation of this
+ requires-expression and not also now during parsing. */
+processing_template_decl_sentinel ptds (false);
+if (!processing_template_decl)
+ processing_template_decl = 1;
reqs = cp_parser_requirement_body (parser);
if (reqs == error_mark_node)
return error_mark_node;
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index bcb2e72fbb5..e998b373af4 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -9691,8 +9691,10 @@ finish_static_assert (tree condition, tree message,
location_t location,
error ("static assertion failed: %s",
TREE_STRING_POINTER (message));
- /* Actually explain the failure if this is a concept check. */
- if (concept_check_p (orig_condition))
+ /* Actually explain the failure if this is a concept check or a
+requires-expression. */
+ if (concept_check_p (orig_condition)
+ || TREE_CODE (orig_condition) == REQUIRES_EXPR)
diagnose_constraints (location, orig_condition, NULL_TREE);
}
else if (condition && condition != error_mark_node)
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic7.C
b/gcc/testsuite/g++.dg/concepts/diagnostic7.C
new file mode 100644
index 000..f78e9bb8240
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic7.C
@@ -0,0 +1,12 @@
+// { dg-do compile { target c++2a } }
+
+template
+ concept same_as = __is_same(A, B); // { dg-message ".void. is not the same
as .int." }
+
+void f();
+
+static_assert(requires { { f() } noexcept -> same_as; });
+// { dg-error "static assertion failed" "" { target *-*-* } .-1 }
+// { dg-message "not .noexcept." "" { target *-*-* } .-2 }
+// { dg-message "return-type-requirement" "" { target *-*-* } .-3 }
+// { dg-error "does not satisfy placeholder constraints" "" { target *-*-* }
.-4 }
diff --git a/gcc/testsuite/g++.dg/concepts/pr94252.C
b/gcc/testsuite/g++.dg/concepts/pr94252.C
new file mode 100644
index 000..7b93997363a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/pr94252.C
@@ -0,0 +1,14 @@
+// PR c++/94252
+// { dg-do compile { target c++2a } }
+
+auto f = []{ return 0; };
+static_assert(requires { f(); });
+
+template
+ concept same_as = __is_same(A, B);
+
Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
On Wed, Mar 25, 2020 at 12:17:41PM -0400, Patrick Palka via Gcc-patches wrote: > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 03ecd7838f6..14a22b85318 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -27689,7 +27689,12 @@ cp_parser_requires_expression (cp_parser *parser) > else >parms = NULL_TREE; > > -/* Parse the requirement body. */ > +/* Always parse the requirement body as if we're inside a template so > that > + we always do full semantic processing only during evaluation of this > + requires-expression and not also now during parsing. */ > +processing_template_decl_sentinel ptds; This looks weird; this sentinel will always set processing_template_decl to 0, right? So... > +if (!processing_template_decl) > + processing_template_decl = 1; ...this will always be true? Did you mean to use temp_override instead? I don't dare to say if this approach is OK or not, but I've certainly had my share of fun with CALL_EXPRs in templates :). > reqs = cp_parser_requirement_body (parser); > if (reqs == error_mark_node) >return error_mark_node; Marek
[PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]
This PR reports that the requires-expression in
auto f = [] { };
static_assert(requires { f(); });
erroneously evaluates to false. The way we end up evaluating to false goes as
follows. During the parsing of the requires-expression, we call
finish_call_expr from cp_parser_requires_expression with the arguments
fn = VIEW_CONVERT_EXPR(f);
args = {}
which does the full processing of the call (since we're not inside a template)
and returns
::operator() ();
Later, when evaluating the requires-expression, we call finish_call_expr again,
this time from tsubst_expr from satisfy_atom, with the arguments
fn = operator()
args = { }
(which, as expected, correspond to the CALL_EXPR returned by finish_call_expr
the first time). But this time, finish_call_expr returns error_mark_node
because
it doesn't expect to see an explicit 'this' argument in the args array, treating
it instead as a user-written argument which causes the only candidate function
to be discarded. This causes the requires-expression to evaluate to false.
In short, it seems finish_call_expr is not idempotent on certain inputs when
!processing_template_decl. Assuming this idempotency issue is not specific to
finish_call_expr, it seems that the safest thing to do is to avoid doing full
semantic processing twice when parsing and evaluating a requires-expression that
lives outside of a template definition.
This patch achieves this by temporarily setting processing_template_decl to
non-zero when parsing the body of a requires-expression. This way, full
semantic processing will always be done only during evaluation and not during
parsing.
Testing is in progress. Does this look like the right solution?
gcc/cp/ChangeLog:
PR c++/94252
* parser.c (cp_parser_requires_expression): Always parse the requirement
body as if we're processing a template, by temporarily setting
processing_template_decl to non-zero.
* semantics.c (finish_static_assert): Also explain an assertion failure
when the condition is a REQUIRES_EXPR.
gcc/testsuite/ChangeLog:
PR c++/94252
* g++.dg/concepts/diagnostic7.C: New test.
* g++.dg/concepts/pr94252.C: New test.
---
gcc/cp/parser.c | 7 ++-
gcc/cp/semantics.c | 6 --
gcc/testsuite/g++.dg/concepts/diagnostic7.C | 12
gcc/testsuite/g++.dg/concepts/pr94252.C | 14 ++
4 files changed, 36 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic7.C
create mode 100644 gcc/testsuite/g++.dg/concepts/pr94252.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 03ecd7838f6..14a22b85318 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -27689,7 +27689,12 @@ cp_parser_requires_expression (cp_parser *parser)
else
parms = NULL_TREE;
-/* Parse the requirement body. */
+/* Always parse the requirement body as if we're inside a template so that
+ we always do full semantic processing only during evaluation of this
+ requires-expression and not also now during parsing. */
+processing_template_decl_sentinel ptds;
+if (!processing_template_decl)
+ processing_template_decl = 1;
reqs = cp_parser_requirement_body (parser);
if (reqs == error_mark_node)
return error_mark_node;
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index bcb2e72fbb5..e998b373af4 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -9691,8 +9691,10 @@ finish_static_assert (tree condition, tree message,
location_t location,
error ("static assertion failed: %s",
TREE_STRING_POINTER (message));
- /* Actually explain the failure if this is a concept check. */
- if (concept_check_p (orig_condition))
+ /* Actually explain the failure if this is a concept check or a
+requires-expression. */
+ if (concept_check_p (orig_condition)
+ || TREE_CODE (orig_condition) == REQUIRES_EXPR)
diagnose_constraints (location, orig_condition, NULL_TREE);
}
else if (condition && condition != error_mark_node)
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic7.C
b/gcc/testsuite/g++.dg/concepts/diagnostic7.C
new file mode 100644
index 000..f78e9bb8240
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic7.C
@@ -0,0 +1,12 @@
+// { dg-do compile { target c++2a } }
+
+template
+ concept same_as = __is_same(A, B); // { dg-message ".void. is not the same
as .int." }
+
+void f();
+
+static_assert(requires { { f() } noexcept -> same_as; });
+// { dg-error "static assertion failed" "" { target *-*-* } .-1 }
+// { dg-message "not .noexcept." "" { target *-*-* } .-2 }
+// { dg-message "return-type-requirement" "" { target *-*-* } .-3 }
+// { dg-error "does not satisfy placeholder constraints" "" { target *-*-* }
.-4 }
diff --git a/gcc/testsuite/g++.dg/concepts/pr94252.C
