[2/2] c++: Normalize nested-requirements twice at parse time [PR97093]

The re-normalization performed from diagnose_nested_requirement doesn't
always work because we may have already lost the necessary template
context that determines the set of in-scope template parameters used by
the nested-requirement.  This leads to normalization producing atoms
that have incomplete/bogus parameter mappings, which breaks satisfaction.

To fix this, we could just use the previously normalized form that we
computed at parse time, but this normal form lacks the diagnostic
information that leads to good error messages.

Instead, this patch makes diagnose_nested_requirement normalize twice at
parse time -- once without diagnostic information and once with -- so
that routines can use the "regular" normal form when performing
satisfaction quietly and the "diagnostic" normal form when performing
satisfaction noisily.  Moreover, this patch makes tsubst_nested_requirement
always first perform satisfaction quietly so that the satisfaction cache
can get consistently utilized.

Finally, this patch also adds more stringent checking to
build_parameter_mapping that would have caught the underlying bug
sooner (and deterministically).

gcc/cp/ChangeLog:

	PR c++/97093
	* constraint.cc (parameter_mapping_equivalent_p): Add more
	stringent checking.  Clarify comment.
	(tsubst_nested_requirement): Always perform satisfaction
	quietly first.  If that yields an erroneous result, emit a
	context message and replay satisfaction noisily with the
	diagnostic normal form.
	(finish_nested_requirement): Normalize the constraint-expression
	twice, once with diagnostic information and once without.  Store
	them in a TREE_LIST within the TREE_TYPE.
	(diagnose_nested_requirement): When replaying satisfaction, use
	the diagnostic normal form instead of renormalizing on the spot.

gcc/testsuite/ChangeLog:

	PR c++/97093
	* g++.dg/cpp2a/concepts-requires22.C: New test.
---
 gcc/cp/constraint.cc                          | 41 ++++++++++++-------
 .../g++.dg/cpp2a/concepts-requires22.C        | 18 ++++++++
 2 files changed, 44 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires22.C

On 12/4/20 4:33 PM, Patrick Palka wrote:
> The re-normalization performed from diagnose_nested_requirement doesn't
> always work because we may have already lost the necessary template
> context that determines the set of in-scope template parameters used by
> the nested-requirement.  This leads to normalization producing atoms
> that have incomplete/bogus parameter mappings, which breaks satisfaction.
> 
> To fix this, we could just use the previously normalized form that we
> computed at parse time, but this normal form lacks the diagnostic
> information that leads to good error messages.
> 
> Instead, this patch makes diagnose_nested_requirement normalize twice at
> parse time -- once without diagnostic information and once with -- so
> that routines can use the "regular" normal form when performing
> satisfaction quietly and the "diagnostic" normal form when performing
> satisfaction noisily.  Moreover, this patch makes tsubst_nested_requirement
> always first perform satisfaction quietly so that the satisfaction cache
> can get consistently utilized.

I wonder about building the "regular" form from the "diagnostic" form to 
avoid doing full normalization twice, but that would be a significantly 
bigger patch.  OK.

> Finally, this patch also adds more stringent checking to
> build_parameter_mapping that would have caught the underlying bug
> sooner (and deterministically).
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/97093
> 	* constraint.cc (parameter_mapping_equivalent_p): Add more
> 	stringent checking.  Clarify comment.
> 	(tsubst_nested_requirement): Always perform satisfaction
> 	quietly first.  If that yields an erroneous result, emit a
> 	context message and replay satisfaction noisily with the
> 	diagnostic normal form.
> 	(finish_nested_requirement): Normalize the constraint-expression
> 	twice, once with diagnostic information and once without.  Store
> 	them in a TREE_LIST within the TREE_TYPE.
> 	(diagnose_nested_requirement): When replaying satisfaction, use
> 	the diagnostic normal form instead of renormalizing on the spot.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/97093
> 	* g++.dg/cpp2a/concepts-requires22.C: New test.
> ---
>   gcc/cp/constraint.cc                          | 41 ++++++++++++-------
>   .../g++.dg/cpp2a/concepts-requires22.C        | 18 ++++++++
>   2 files changed, 44 insertions(+), 15 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-requires22.C
> 
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 2be1a841535..c6d4d8e7e64 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -612,7 +612,8 @@ build_parameter_mapping (tree expr, tree args, tree decl)
>     return map;
>   }
>   
> -/* True if the parameter mappings of two atomic constraints are equivalent.  */
> +/* True if the parameter mappings of two atomic constraints formed
> +   from the same expression are equivalent.  */
>   
>   static bool
>   parameter_mapping_equivalent_p (tree t1, tree t2)
> @@ -621,6 +622,7 @@ parameter_mapping_equivalent_p (tree t1, tree t2)
>     tree map2 = ATOMIC_CONSTR_MAP (t2);
>     while (map1 && map2)
>       {
> +      gcc_checking_assert (TREE_VALUE (map1) == TREE_VALUE (map2));
>         tree arg1 = TREE_PURPOSE (map1);
>         tree arg2 = TREE_PURPOSE (map2);
>         if (!template_args_equal (arg1, arg2))
> @@ -628,6 +630,7 @@ parameter_mapping_equivalent_p (tree t1, tree t2)
>         map1 = TREE_CHAIN (map1);
>         map2 = TREE_CHAIN (map2);
>       }
> +  gcc_checking_assert (!map1 && !map2);
>     return true;
>   }
>   
> @@ -2085,14 +2088,16 @@ tsubst_compound_requirement (tree t, tree args, subst_info info)
>   static tree
>   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,
> -				    sat_info (info.complain, info.in_decl));
> -  if (result == error_mark_node && info.quiet ())
> +  /* Perform satisfaction quietly with the regular normal form.  */
> +  sat_info quiet (tf_none, info.in_decl);
> +  tree norm = TREE_VALUE (TREE_TYPE (t));
> +  tree diag_norm = TREE_PURPOSE (TREE_TYPE (t));
> +  tree result = satisfy_constraint (norm, args, quiet);
> +  if (result == error_mark_node)
>       {
> +      /* Replay the error using the diagnostic normal form.  */
>         sat_info noisy (tf_warning_or_error, info.in_decl);
> -      satisfy_constraint (norm, args, noisy);
> +      satisfy_constraint (diag_norm, args, noisy);
>       }
>     if (result != boolean_true_node)
>       return error_mark_node;
> @@ -3139,10 +3144,15 @@ finish_compound_requirement (location_t loc, tree expr, tree type, bool noexcept
>   tree
>   finish_nested_requirement (location_t loc, tree expr)
>   {
> -  tree norm = normalize_constraint_expression (expr, false);
> +  /* We need to normalize the constraints now, at parse time, while
> +     we have the necessary template context.  We normalize twice,
> +     once without diagnostic information and once with, which we'll
> +     later use during quiet and noisy satisfaction respectively.  */
> +  tree norm = normalize_constraint_expression (expr, /*diag=*/false);
> +  tree diag_norm = normalize_constraint_expression (expr, /*diag=*/true);
>   
> -  /* Build the constraint, saving its normalization as its type.  */
> -  tree r = build1 (NESTED_REQ, norm, expr);
> +  /* Build the constraint, saving its two normalizations as its type.  */
> +  tree r = build1 (NESTED_REQ, build_tree_list (diag_norm, norm), expr);
>     SET_EXPR_LOCATION (r, loc);
>     return r;
>   }
> @@ -3543,9 +3553,10 @@ diagnose_type_requirement (tree req, tree args, tree in_decl)
>   static void
>   diagnose_nested_requirement (tree req, tree args)
>   {
> -  /* Quietly check for satisfaction first. We can elaborate details
> -     later if needed.  */
> -  tree norm = TREE_TYPE (req);
> +  /* Quietly check for satisfaction first using the regular normal form.
> +     We can elaborate details later if needed.  */
> +  tree norm = TREE_VALUE (TREE_TYPE (req));
> +  tree diag_norm = TREE_PURPOSE (TREE_TYPE (req));
>     sat_info info (tf_none, NULL_TREE);
>     tree result = satisfy_constraint (norm, args, info);
>     if (result == boolean_true_node)
> @@ -3555,11 +3566,11 @@ diagnose_nested_requirement (tree req, tree args)
>     location_t loc = cp_expr_location (expr);
>     if (diagnosing_failed_constraint::replay_errors_p ())
>       {
> -      /* Replay the substitution error.  */
> +      /* Replay the substitution error using the diagnostic normal form.  */
>         inform (loc, "nested requirement %qE is not satisfied, because", expr);
>         sat_info noisy (tf_warning_or_error, NULL_TREE);
>         noisy.diagnose_unsatisfaction = true;
> -      satisfy_constraint_expression (expr, args, noisy);
> +      satisfy_constraint (diag_norm, args, noisy);
>       }
>     else
>       inform (loc, "nested requirement %qE is not satisfied", expr);
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-requires22.C b/gcc/testsuite/g++.dg/cpp2a/concepts-requires22.C
> new file mode 100644
> index 00000000000..5afcbbecbd5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-requires22.C
> @@ -0,0 +1,18 @@
> +// PR c++/97093
> +// { dg-do compile { target c++20 } }
> +// { dg-additional-options "-fconcepts-diagnostics-depth=3" }
> +
> +template<class X, X x>
> +concept C = requires {
> +    requires (X)x;                     // { dg-message "false" }
> +  };
> +
> +template<class X, X x>
> +concept D = requires {
> +    requires false || (X)x;                    // { dg-message "false" }
> +  };
> +
> +int main() {
> +  static_assert(C<bool, 0>); // { dg-error "failed" }
> +  static_assert(D<bool, 0>); // { dg-error "failed" }
> +}
>