Re: [PATCH 1/2 v2] c++: Distinguish unsatisfaction vs errors during satisfaction [PR97093]

Jason Merrill via Gcc-patches Fri, 04 Dec 2020 14:41:55 -0800

On 12/4/20 4:33 PM, Patrick Palka wrote:

I've convinced myself to do away with the whole diagnose_requires_expr /
tsubst_requires_expr consolidation, since that part is just a pure
refactoring change and the added overloadedness of the flags is not
ideal.  This simplifies the patch considerably.

Does dropping that reduce the overloadedness? I'm always in favor ofreducing code duplication. But it certainly makes sense for that tohappen in a separate patch.

During satisfaction, the flag info.noisy() controls three things:
whether to diagnose ill-formed satisfaction (such as the satisfaction
value of an atom being non-bool or non-constant); whether to diagnose
unsatisfaction; and whether to bypass the satisfaction cache.

The flag turns out to be too coarse however, because in some cases we
want to diagnose ill-formed satisfaction (and bypass the satisfaction
cache) but not diagnose unsatisfaction, for instance when replaying an
erroneous satisfaction result from constraint_satisfaction_value,
evaluate_concept_check and tsubst_nested_requirement.

And when noisily evaluating a disjunction, we want to first evaluate its
branches noisily (bypassing the satisfaction cache) but suppress
unsatisfaction diagnostics.  We currently work around this by instead
first evaluating each branch quietly, but that means the recursive calls
to satisfy_atom will use the satisfaction cache.

To fix this, this patch adds the info.diagnose_unsatisfaction_p() flag,
which refines the info.noisy() flag as part of a new sat_info class that
derives from subst_info.  During satisfaction, info.noisy() now controls
whether to diagnose ill-formed satisfaction, and
info.diagnose_unsatisfaction_p() controls whether to additionally
diagnose unsatisfaction.  This enables us to address the above two
issues straightforwardly.

Incidentally, the change to satisfy_disjunction suppresses the ICE in
the PR97093 testcase because we no longer insert atoms into the
satisfaction cache that have been incorrectly re-normalized in
diagnose_nested_requirement (after losing the necessary template
context).  But the underlying re-normalization issue remains, and will
be fixed in a subsequent patch.

gcc/cp/ChangeLog:

        PR c++/97093
        * constraint.cc (struct sat_info): Define.
        (tsubst_nested_requirement): Pass a sat_info object to
        satisfy_constraint.
        (satisfy_constraint_r): Take a sat_info argument instead of
        subst_info.
        (satisfy_conjunction): Likewise.
        (satisfy_disjunction): Likewise.  Instead of first evaluating
        each branch quietly, evaluate each branch only with
        unsatisfaction diagnostics disabled.  Exit early if evaluation
        of a branch returns error_mark_node.
        (satisfy_atom): Take a sat_info argument instead of subst_info.
        Fix a comment.  Check diagnose_unsatisfaction_p() instead of
        noisy() before replaying a substitution failure.
        (satisfy_constraint): Take a sat_info argument instead of
        subst_info.
        (satisfy_associated_constraints): Likewise.
        (satisfy_constraint_expression): Likewise.
        (satisfy_declaration_constraints): Likewise.
        (constraint_satisfaction_value): Likewise and adjust
        accordingly.  Fix formatting.
        (constraints_satisfied_p): Pass a sat_info object to
        constraint_satisfaction_value.
        (evaluate_concept_check): Pass a sat_info object to
        satisfy_constraint_expression.
        (diagnose_nested_requirement): Likewise.
        (diagnose_constraints): Pass an appropriate sat_info object to
        constraint_satisfaction_value.

gcc/testsuite/ChangeLog:

        PR c++/97093
        * g++.dg/concepts/pr94252.C: Verify we no longer issue a
        spurious satisfaction failure note when diagnosing ill-formed
        satisfaction.
        * g++.dg/cpp2a/concepts-requires18.C: No longer expect a
        spurious satisfaction failure diagnostic when immediately
        evaluating the nested-requirement subst<void&> of a
        requires-expression that appears outside of a template.
        * g++.dg/cpp2a/concepts-requires21.C: Verify we no longer issue
        a spurious satisfaction failure note when immediately evaluating
        a nested-requirement of a requires-expression that appears
        outside of a template.
        * g++.dg/cpp2a/concepts-nonbool3.C: New test.
        * g++.dg/cpp2a/concepts-pr97093.C: New test.
---
  gcc/cp/constraint.cc                          | 149 +++++++++++-------
  gcc/testsuite/g++.dg/concepts/pr94252.C       |   1 +
  .../g++.dg/cpp2a/concepts-nonbool3.C          |   5 +
  .../g++.dg/cpp2a/concepts-requires18.C        |   2 +-
  .../g++.dg/cpp2a/concepts-requires21.C        |   1 +
  5 files changed, 104 insertions(+), 54 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-nonbool3.C

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 7f02aa0a215..2be1a841535 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -98,7 +98,35 @@ struct subst_info
    tree in_decl;
  };

-static tree satisfy_constraint (tree, tree, subst_info);

+/* Provides additional context for satisfaction.
+
+   The flag noisy() controls whether to diagnose ill-formed satisfaction,
+   such as the satisfaction value of an atom being non-bool or non-constant.
+
+   The flag diagnose_unsatisfaction_p(), which implies noisy(), controls
+   whether to explain why a constraint is not satisfied.  */


Please also mention the circumstances for these flags being on.

+struct sat_info : subst_info
+{
+  sat_info (tsubst_flags_t cmp, tree in, bool diagnose_unsat = false)
+    : subst_info (cmp, in), diagnose_unsatisfaction (diagnose_unsat)
+  {
+    if (diagnose_unsatisfaction_p ())
+      gcc_checking_assert (noisy ());
+  }
+
+  /* True if we should diagnose the cause of satisfaction failure.
+     Implies noisy().  */
+  bool
+  diagnose_unsatisfaction_p () const
+  {
+    return diagnose_unsatisfaction;
+  }
+
+  bool diagnose_unsatisfaction;
+};
+
+static tree satisfy_constraint (tree, tree, sat_info);

/* True if T is known to be some type other than bool. Note that this

     is false for dependent types and errors.  */
@@ -2059,10 +2087,11 @@ tsubst_nested_requirement (tree t, tree args, 
subst_info info)
  {
    /* Ensure that we're in an evaluation context prior to satisfaction.  */
    tree norm = TREE_TYPE (t);
-  tree result = satisfy_constraint (norm, args, info);
+  tree result = satisfy_constraint (norm, args,
+                                   sat_info (info.complain, info.in_decl));

Maybe add a constructor sat_info(const subst_info &)? OK either way(with the above comment change).

    if (result == error_mark_node && info.quiet ())
      {
-      subst_info noisy (tf_warning_or_error, info.in_decl);
+      sat_info noisy (tf_warning_or_error, info.in_decl);
        satisfy_constraint (norm, args, noisy);
      }
    if (result != boolean_true_node)
@@ -2508,12 +2537,12 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t 
complain, tree in_decl)
    return expr;
  }

-static tree satisfy_constraint_r (tree, tree, subst_info info);

+static tree satisfy_constraint_r (tree, tree, sat_info info);

/* Compute the satisfaction of a conjunction. */static tree

-satisfy_conjunction (tree t, tree args, subst_info info)
+satisfy_conjunction (tree t, tree args, sat_info info)
  {
    tree lhs = satisfy_constraint_r (TREE_OPERAND (t, 0), args, info);
    if (lhs == error_mark_node || lhs == boolean_false_node)
@@ -2567,20 +2596,25 @@ collect_operands_of_disjunction (tree t, 
auto_vec<tree_pair> *operands)
  /* Compute the satisfaction of a disjunction.  */

static tree

-satisfy_disjunction (tree t, tree args, subst_info info)
+satisfy_disjunction (tree t, tree args, sat_info info)
  {
-  /* Evaluate the operands quietly.  */
-  subst_info quiet (tf_none, NULL_TREE);
+  /* Evaluate each operand with unsatisfaction diagnostics disabled.  */
+  sat_info sub = info;
+  sub.diagnose_unsatisfaction = false;

- /* Register the constraint for diagnostics, if needed. */

-  diagnosing_failed_constraint failure (t, args, info.noisy ());
+  tree lhs = satisfy_constraint_r (TREE_OPERAND (t, 0), args, sub);
+  if (lhs == boolean_true_node || lhs == error_mark_node)
+    return lhs;

- tree lhs = satisfy_constraint_r (TREE_OPERAND (t, 0), args, quiet);

-  if (lhs == boolean_true_node)
-    return boolean_true_node;
-  tree rhs = satisfy_constraint_r (TREE_OPERAND (t, 1), args, quiet);
-  if (rhs != boolean_true_node && info.noisy ())
+  tree rhs = satisfy_constraint_r (TREE_OPERAND (t, 1), args, sub);
+  if (rhs == boolean_true_node || rhs == error_mark_node)
+    return rhs;
+
+  /* Both branches evaluated to false.  Explain the satisfaction failure in
+     each branch.  */
+  if (info.diagnose_unsatisfaction_p ())
      {
+      diagnosing_failed_constraint failure (t, args, info.noisy ());
        cp_expr disj_expr = CONSTR_EXPR (t);
        inform (disj_expr.get_location (),
              "no operand of the disjunction is satisfied");
@@ -2601,7 +2635,8 @@ satisfy_disjunction (tree t, tree args, subst_info info)
            }
        }
      }
-  return rhs;
+
+  return boolean_false_node;
  }

/* Ensures that T is a truth value and not (accidentally, as sometimes

@@ -2682,7 +2717,7 @@ static void diagnose_atomic_constraint (tree, tree, tree, 
subst_info);
  /* Compute the satisfaction of an atomic constraint.  */

static tree

-satisfy_atom (tree t, tree args, subst_info info)
+satisfy_atom (tree t, tree args, sat_info info)
  {
    satisfaction_cache cache (t, args, info.complain);
    if (tree r = cache.get ())
@@ -2700,9 +2735,9 @@ satisfy_atom (tree t, tree args, subst_info info)
    tree map = tsubst_parameter_mapping (ATOMIC_CONSTR_MAP (t), args, quiet);
    if (map == error_mark_node)
      {
-      /* If instantiation of the parameter mapping fails, the program
-         is ill-formed.  */
-      if (info.noisy())
+      /* If instantiation of the parameter mapping fails, the constraint is
+        not satisfied.  Replay the substitution.  */
+      if (info.diagnose_unsatisfaction_p ())
        tsubst_parameter_mapping (ATOMIC_CONSTR_MAP (t), args, info);
        return cache.save (boolean_false_node);
      }
@@ -2729,7 +2764,7 @@ satisfy_atom (tree t, tree args, subst_info info)
      {
        /* If substitution results in an invalid type or expression, the 
constraint
         is not satisfied. Replay the substitution.  */
-      if (info.noisy ())
+      if (info.diagnose_unsatisfaction_p ())
        tsubst_expr (expr, args, info.complain, info.in_decl, false);
        return cache.save (inst_cache.save (boolean_false_node));
      }
@@ -2757,7 +2792,7 @@ satisfy_atom (tree t, tree args, subst_info info)
        result = error_mark_node;
      }
    result = satisfaction_value (result);
-  if (result == boolean_false_node && info.noisy ())
+  if (result == boolean_false_node && info.diagnose_unsatisfaction_p ())
      diagnose_atomic_constraint (t, map, result, info);

return cache.save (inst_cache.save (result));

@@ -2775,7 +2810,7 @@ satisfy_atom (tree t, tree args, subst_info info)
     constraint only matters for subsumption.  */

static tree

-satisfy_constraint_r (tree t, tree args, subst_info info)
+satisfy_constraint_r (tree t, tree args, sat_info info)
  {
    if (t == error_mark_node)
      return error_mark_node;
@@ -2796,7 +2831,7 @@ satisfy_constraint_r (tree t, tree args, subst_info info)
  /* Check that the normalized constraint T is satisfied for ARGS.  */

static tree

-satisfy_constraint (tree t, tree args, subst_info info)
+satisfy_constraint (tree t, tree args, sat_info info)
  {
    auto_timevar time (TV_CONSTRAINT_SAT);

@@ -2814,7 +2849,7 @@ satisfy_constraint (tree t, tree args, subst_info info)

     value (either true, false, or error).  */

static tree

-satisfy_associated_constraints (tree t, tree args, subst_info info)
+satisfy_associated_constraints (tree t, tree args, sat_info info)
  {
    /* If there are no constraints then this is trivially satisfied.  */
    if (!t)
@@ -2832,7 +2867,7 @@ satisfy_associated_constraints (tree t, tree args, 
subst_info info)
     satisfaction value. */

static tree

-satisfy_constraint_expression (tree t, tree args, subst_info info)
+satisfy_constraint_expression (tree t, tree args, sat_info info)
  {
    if (t == error_mark_node)
      return error_mark_node;
@@ -2861,12 +2896,12 @@ satisfy_constraint_expression (tree t, tree args, 
subst_info info)
  tree
  satisfy_constraint_expression (tree expr)
  {
-  subst_info info (tf_none, NULL_TREE);
+  sat_info info (tf_none, NULL_TREE);
    return satisfy_constraint_expression (expr, NULL_TREE, info);
  }

static tree

-satisfy_declaration_constraints (tree t, subst_info info)
+satisfy_declaration_constraints (tree t, sat_info info)
  {
    gcc_assert (DECL_P (t));
    const tree saved_t = t;
@@ -2926,7 +2961,7 @@ satisfy_declaration_constraints (tree t, subst_info info)
  }

static tree

-satisfy_declaration_constraints (tree t, tree args, subst_info info)
+satisfy_declaration_constraints (tree t, tree args, sat_info info)
  {
    /* Update the declaration for diagnostics.  */
    info.in_decl = t;
@@ -2951,9 +2986,8 @@ satisfy_declaration_constraints (tree t, tree args, 
subst_info info)
  }

static tree

-constraint_satisfaction_value (tree t, tsubst_flags_t complain)
+constraint_satisfaction_value (tree t, sat_info info)
  {
-  subst_info info (complain, NULL_TREE);
    tree r;
    if (DECL_P (t))
      r = satisfy_declaration_constraints (t, info);
@@ -2961,26 +2995,31 @@ constraint_satisfaction_value (tree t, tsubst_flags_t 
complain)
      r = satisfy_constraint_expression (t, NULL_TREE, info);
    if (r == error_mark_node && info.quiet ()
        && !(DECL_P (t) && TREE_NO_WARNING (t)))
-      {
-       constraint_satisfaction_value (t, tf_warning_or_error);
-       if (DECL_P (t))
-         /* Avoid giving these errors again.  */
-         TREE_NO_WARNING (t) = true;
-      }
+    {
+      /* Replay the error with re-normalized requirements.  */
+      sat_info noisy (tf_warning_or_error, info.in_decl);
+      constraint_satisfaction_value (t, noisy);
+      if (DECL_P (t))
+       /* Avoid giving these errors again.  */
+       TREE_NO_WARNING (t) = true;
+    }
    return r;
  }

static tree

-constraint_satisfaction_value (tree t, tree args, tsubst_flags_t complain)
+constraint_satisfaction_value (tree t, tree args, sat_info info)
  {
-  subst_info info (complain, NULL_TREE);
    tree r;
    if (DECL_P (t))
      r = satisfy_declaration_constraints (t, args, info);
    else
      r = satisfy_constraint_expression (t, args, info);
    if (r == error_mark_node && info.quiet ())
-    constraint_satisfaction_value (t, args, tf_warning_or_error);
+    {
+      /* Replay the error with re-normalized requirements.  */
+      sat_info noisy (tf_warning_or_error, info.in_decl);
+      constraint_satisfaction_value (t, args, noisy);
+    }
    return r;
  }

@@ -2993,7 +3032,8 @@ constraints_satisfied_p (tree t)

    if (!flag_concepts)
      return true;

- return constraint_satisfaction_value (t, tf_none) == boolean_true_node;

+  sat_info quiet (tf_none, NULL_TREE);
+  return constraint_satisfaction_value (t, quiet) == boolean_true_node;
  }

/* True iff the result of satisfying T with ARGS is BOOLEAN_TRUE_NODE

@@ -3005,7 +3045,8 @@ constraints_satisfied_p (tree t, tree args)
    if (!flag_concepts)
      return true;

- return constraint_satisfaction_value (t, args, tf_none) == boolean_true_node;

+  sat_info quiet (tf_none, NULL_TREE);
+  return constraint_satisfaction_value (t, args, quiet) == boolean_true_node;
  }

/* Evaluate a concept check of the form C<ARGS>. This is only used for the

@@ -3020,14 +3061,14 @@ evaluate_concept_check (tree check, tsubst_flags_t 
complain)
    gcc_assert (concept_check_p (check));

/* Check for satisfaction without diagnostics. */

-  subst_info quiet (tf_none, NULL_TREE);
+  sat_info quiet (tf_none, NULL_TREE);
    tree result = satisfy_constraint_expression (check, NULL_TREE, quiet);
    if (result == error_mark_node && (complain & tf_error))
-  {
-    /* Replay the error with re-normalized requirements.  */
-    subst_info noisy (tf_warning_or_error, NULL_TREE);
-    satisfy_constraint_expression (check, NULL_TREE, noisy);
-  }
+    {
+      /* Replay the error with re-normalized requirements.  */
+      sat_info noisy (tf_warning_or_error, NULL_TREE);
+      satisfy_constraint_expression (check, NULL_TREE, noisy);
+    }
    return result;
  }

@@ -3505,7 +3546,7 @@ diagnose_nested_requirement (tree req, tree args)

    /* Quietly check for satisfaction first. We can elaborate details
       later if needed.  */
    tree norm = TREE_TYPE (req);
-  subst_info info (tf_none, NULL_TREE);
+  sat_info info (tf_none, NULL_TREE);
    tree result = satisfy_constraint (norm, args, info);
    if (result == boolean_true_node)
      return;
@@ -3516,7 +3557,8 @@ diagnose_nested_requirement (tree req, tree args)
      {
        /* Replay the substitution error.  */
        inform (loc, "nested requirement %qE is not satisfied, because", expr);
-      subst_info noisy (tf_warning_or_error, NULL_TREE);
+      sat_info noisy (tf_warning_or_error, NULL_TREE);
+      noisy.diagnose_unsatisfaction = true;
        satisfy_constraint_expression (expr, args, noisy);
      }
    else
@@ -3660,11 +3702,12 @@ diagnose_constraints (location_t loc, tree t, tree args)
    if (concepts_diagnostics_max_depth == 0)
      return;

- /* Replay satisfaction, but diagnose errors. */

+  /* Replay satisfaction, but diagnose unsatisfaction.  */
+  sat_info noisy (tf_warning_or_error, NULL_TREE, /*diagnose_unsat=*/true);
    if (!args)
-    constraint_satisfaction_value (t, tf_warning_or_error);
+    constraint_satisfaction_value (t, noisy);
    else
-    constraint_satisfaction_value (t, args, tf_warning_or_error);
+    constraint_satisfaction_value (t, args, noisy);

static bool suggested_p;

    if (concepts_diagnostics_max_depth_exceeded_p
diff --git a/gcc/testsuite/g++.dg/concepts/pr94252.C 
b/gcc/testsuite/g++.dg/concepts/pr94252.C
index 56ce5f88bad..b0457037ede 100644
--- a/gcc/testsuite/g++.dg/concepts/pr94252.C
+++ b/gcc/testsuite/g++.dg/concepts/pr94252.C
@@ -16,6 +16,7 @@ static_assert(requires(S o, int i) {

template<typename T>

    concept c = requires (T t) { requires (T)5; }; // { dg-error "has type 
.int." }
+// { dg-bogus "not satisfied" "" { target *-*-* } .-1 }

int

  foo()
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-nonbool3.C 
b/gcc/testsuite/g++.dg/cpp2a/concepts-nonbool3.C
new file mode 100644
index 00000000000..2a2af54847b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-nonbool3.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target c++20 } }
+
+template <auto V> concept C = false || V || false; // { dg-error "has type 
'int'" }
+template <auto V> int f() requires C<V>;
+int a = f<0>(); // { dg-error "no match" }
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C 
b/gcc/testsuite/g++.dg/cpp2a/concepts-requires18.C
index a9b7720cc6c..9e45c586917 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; }; // { dg-error "parameter type 
.void." }
+concept subst = requires (T x) { requires true; };

template<typename T>

  concept c1 = requires { requires integer<T> || subst<T&>; }; // { dg-message "in 
requirements" }
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires21.C 
b/gcc/testsuite/g++.dg/cpp2a/concepts-requires21.C
index bc38b893c68..8aead2fe2c5 100644
--- a/gcc/testsuite/g++.dg/cpp2a/concepts-requires21.C
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires21.C
@@ -5,3 +5,4 @@ template<typename T, typename U>
  constexpr bool is_same_v = __is_same (T, U);

static_assert(is_same_v<bool, decltype(requires { requires false; })>);

+// { dg-bogus "evaluated to 'false" "" { target *-*-* } .-1 }

Re: [PATCH 1/2 v2] c++: Distinguish unsatisfaction vs errors during satisfaction [PR97093]

Reply via email to