Re: [PATCH] c++: requires-expression outside of a template is misevaluated [PR94252]

[Jason Merrill via Gcc-patches]

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<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.

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) <case INTEGER_CST>: Remove this case so
        that we diagnose INTEGER_CST expressions of non-bool type via the
        default case.
        * cp-gimplify.c (cp_genericize_r) <case REQUIRES_EXPR>: New 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): Explain an assertion failure
        when the condition is a REQUIRES_EXPR like we do when it is a concept
        check.

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/cp-gimplify.c                          |  7 +++++
  gcc/cp/parser.c                               | 14 +++++++++-
  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 +-
  8 files changed, 68 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/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index d003f5b7825..3999695ae93 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -1747,6 +1747,13 @@ cp_genericize_r (tree *stmt_p, int *walk_subtrees, void 
*data)
        TARGET_EXPR_NO_ELIDE (stmt) = 1;
        break;

+ case REQUIRES_EXPR:

+      /* Emit the value of the requires-expression.  */
+      *stmt_p = constant_boolean_node (constraints_satisfied_p (stmt),
+                                      boolean_type_node);
+      *walk_subtrees = 0;
+      break;
+
      case TEMPLATE_ID_EXPR:
        gcc_assert (concept_check_p (stmt));
        /* Emit the value of the concept check.  */
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d9289578477..f7fe033eae1 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -27759,7 +27759,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;
    }
@@ -27768,7 +27770,17 @@ 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.  */
+      int saved_errorcount = errorcount;
+      tsubst_requires_expr (expr, NULL_TREE, tf_warning_or_error, NULL_TREE);
+      if (errorcount > saved_errorcount)
+       return error_mark_node;
+    }
+  return expr;
  }

/* Parse a parameterized requirement.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 1bc4f08649f..a9403591749 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -20320,8 +20320,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..56ce5f88bad
--- /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." }
+
+int
+foo()
+{
+  requires { requires c<int>; };
+  requires { { 5 } -> same_as<bool>; };
+  requires { requires !requires { { 5 } -> same_as<bool>; }; };
+  return requires { requires 5; }; // { dg-error "has type .int." }
+}
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" }