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?
-- >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.
* 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" }
--
2.26.0.rc1.11.g30e9940356
