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<struct ._anon_0>(f);
> > > > args = {}
> > > >
> > > > which does the full processing of the call (since we're not inside a
> > > > template)
> > > > and returns
> > > >
> > > > <lambda()>::operator() (&f);
> > > >
> > > > 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 = { &f }
> > > >
> > > > (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 errors
> > from type_deducible_p and expression_convertible_p, as they're not
> > substitution errors.
> > (diagnose_atomic_constraint) <case INTEGER_CST>: 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) <case REQUIRES_EXPR>: Remove dead code.
>
> Why is this dead code?
Because tsubst_requires_expr never returns error_mark_node, AFAICT..
>
> > * 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.
> > * g++.dg/cpp2a/concepts-requires18.C: Adjust to expect another
> > diagnostic.
> > ---
> > gcc/cp/constraint.cc | 10 +++----
> > gcc/cp/parser.c | 9 ++++++-
> > gcc/cp/pt.c | 2 --
> > gcc/cp/semantics.c | 6 +++--
> > gcc/testsuite/g++.dg/concepts/diagnostic7.C | 12 +++++++++
> > gcc/testsuite/g++.dg/concepts/pr94252.C | 27 +++++++++++++++++++
> > .../g++.dg/cpp2a/concepts-requires18.C | 2 +-
> > 7 files changed, 56 insertions(+), 12 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/constraint.cc b/gcc/cp/constraint.cc
> > index a86bcdf603a..a2f450520fd 100644
> > --- a/gcc/cp/constraint.cc
> > +++ b/gcc/cp/constraint.cc
> > @@ -1980,15 +1980,17 @@ tsubst_compound_requirement (tree t, tree args,
> > subst_info info)
> > if (type == error_mark_node)
> > return error_mark_node;
> > + subst_info quiet (tf_none, info.in_decl);
> > +
> > /* Check expression against the result type. */
> > if (type)
> > {
> > if (tree placeholder = type_uses_auto (type))
> > {
> > - if (!type_deducible_p (expr, type, placeholder, args, info))
> > + if (!type_deducible_p (expr, type, placeholder, args, quiet))
> > return error_mark_node;
> > }
> > - else if (!expression_convertible_p (expr, type, info))
> > + else if (!expression_convertible_p (expr, type, quiet))
> > return error_mark_node;
> > }
> > @@ -3362,10 +3364,6 @@ diagnose_atomic_constraint (tree t, tree map, tree
> > result, subst_info info)
> > case REQUIRES_EXPR:
> > diagnose_requires_expr (expr, map, info.in_decl);
> > break;
> > - case INTEGER_CST:
> > - /* This must be either 0 or false. */
> > - inform (loc, "%qE is never satisfied", expr);
> > - break;
> > default:
> > tree a = copy_node (t);
> > ATOMIC_CONSTR_MAP (a) = map;
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 7b03bdf5218..91d306da6af 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -27712,7 +27712,9 @@ cp_parser_requires_expression (cp_parser *parser)
> > parms = NULL_TREE;
> > /* Parse the requirement body. */
> > + ++processing_template_decl;
> > reqs = cp_parser_requirement_body (parser);
> > + --processing_template_decl;
> > if (reqs == error_mark_node)
> > return error_mark_node;
> > }
> > @@ -27721,7 +27723,12 @@ cp_parser_requires_expression (cp_parser *parser)
> > the parm chain. */
> > grokparms (parms, &parms);
> > loc = make_location (loc, loc, parser->lexer);
> > - return finish_requires_expr (loc, parms, reqs);
> > + tree expr = finish_requires_expr (loc, parms, reqs);
> > + if (!processing_template_decl)
> > + /* Perform semantic processing now to diagnose any invalid types and
> > + expressions. */
> > + tsubst_requires_expr (expr, NULL_TREE, tf_warning_or_error, NULL_TREE);
> > + return expr;
> > }
> > /* Parse a parameterized requirement.
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 496bf7c33ba..95cd35f82c8 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -20321,8 +20321,6 @@ tsubst_copy_and_build (tree t,
> > case REQUIRES_EXPR:
> > {
> > tree r = tsubst_requires_expr (t, args, tf_none, in_decl);
> > - if (r == error_mark_node && (complain & tf_error))
> > - tsubst_requires_expr (t, args, complain, in_decl);
> > RETURN (r);
> > }
> > 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 00000000000..f78e9bb8240
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/concepts/diagnostic7.C
> > @@ -0,0 +1,12 @@
> > +// { dg-do compile { target c++2a } }
> > +
> > +template<typename A, typename B>
> > + 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<int>; });
> > +// { 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 00000000000..ee05044abef
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/concepts/pr94252.C
> > @@ -0,0 +1,27 @@
> > +// PR c++/94252
> > +// { dg-do compile { target c++2a } }
> > +
> > +auto f = []{ return 0; };
> > +static_assert(requires { f(); });
> > +static_assert(requires { requires requires { f(); }; });
> > +
> > +template<typename A, typename B>
> > + concept same_as = __is_same(A, B);
> > +
> > +struct S { int f(int) noexcept; };
> > +static_assert(requires(S o, int i) {
> > + o.f(i);
> > + { o.f(i) } noexcept -> same_as<int>;
> > +});
> > +
> > +template<typename T>
> > + concept c = requires (T t) { requires (T)5; }; // { dg-error "has type
> > .int." }
> > +
> > +void
> > +foo()
> > +{
> > + requires { requires c<int>; };
> > + requires { requires 5; }; // { dg-error "has type .int." }
> > + requires { { 5 } -> same_as<bool>; };
> > + requires { requires !requires { { 5 } -> same_as<bool>; }; };
> > +}
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C
> > b/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C
> > index c76b12c6414..c97704565a1 100644
> > --- a/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C
> > @@ -4,7 +4,7 @@ template<typename T>
> > concept integer = __is_same_as(T, int);
> > template<typename T>
> > -concept subst = requires (T x) { requires true; };
> > +concept subst = requires (T x) { requires true; }; // { dg-error "parameter
> > type .void." }
> > template<typename T>
> > concept c1 = requires { requires integer<T> || subst<T&>; }; // {
> > dg-message "in requirements" }
> >
>
>
