Re: [PATCH] c++: return-type-req in constraint using only outer tparms [PR104527]

[Patrick Palka <ppalka@redhat.com>]

On Wed, Feb 16, 2022 at 2:56 PM Patrick Palka <ppalka@redhat.com> wrote:
>
> On Tue, 15 Feb 2022, Jason Merrill wrote:
>
> > On 2/14/22 11:32, Patrick Palka wrote:
> > > Here the template context for the atomic constraint has two levels of
> > > template arguments, but since it depends only on the innermost argument
> > > T we use a single-level argument vector during substitution into the
> > > constraint (built by get_mapped_args).  We eventually pass this vector
> > > to do_auto_deduction as part of checking the return-type-requirement
> > > inside the atom, but do_auto_deduction expects outer_targs to be a full
> > > set of arguments for sake of satisfaction.
> >
> > Could we note the current number of levels in the map and use that in
> > get_mapped_args instead of the highest level parameter we happened to use?
>
> Ah yeah, that seems to work nicely.  IIUC it should suffice to remember
> whether the atomic constraint expression came from a concept definition.
> If it did, then the depth of the argument vector returned by
> get_mapped_args must be one, otherwise (as in the testcase) it must be
> the same as the template depth of the constrained entity, which is the
> depth of ARGS.
>
> How does the following look?  Bootstrapped and regtested on
> x86_64-pc-linux-gnu and also on cmcstl2 and range-v3.

Ping.

>
> -- >8 --
>
> Subject: [PATCH] c++: return-type-req in constraint using only outer tparms
>  [PR104527]
>
> Here the template context for the atomic constraint has two levels of
> template parameters, but since it depends only on the innermost parameter
> T we use a single-level argument vector (built by get_mapped_args) during
> substitution into the atom.  We eventually pass this vector to
> do_auto_deduction as part of checking the return-type-requirement within
> the atom, but do_auto_deduction expects outer_targs to be a full set of
> arguments for sake of satisfaction.
>
> This patch fixes this by making get_mapped_args always return an
> argument vector whose depth corresponds to the template depth of the
> context in which the atomic constraint expression was written, instead
> of the highest parameter level that the expression happens to use.
>
>         PR c++/104527
>
> gcc/cp/ChangeLog:
>
>         * constraint.cc (normalize_atom): Set
>         ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P appropriately.
>         (get_mapped_args):  Make static, adjust parameters.  Always
>         return a vector whose depth corresponds to the template depth of
>         the context of the atomic constraint expression.  Micro-optimize
>         by passing false as exact to safe_grow_cleared and by collapsing
>         a multi-level depth-one argument vector.
>         (satisfy_atom): Adjust call to get_mapped_args and
>         diagnose_atomic_constraint.
>         (diagnose_atomic_constraint): Replace map parameter with an args
>         parameter.
>         * cp-tree.h (ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P): Define.
>         (get_mapped_args): Remove declaration.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/cpp2a/concepts-return-req4.C: New test.
> ---
>  gcc/cp/constraint.cc                          | 64 +++++++++++--------
>  gcc/cp/cp-tree.h                              |  7 +-
>  .../g++.dg/cpp2a/concepts-return-req4.C       | 24 +++++++
>  3 files changed, 69 insertions(+), 26 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-return-req4.C
>
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 12db7e5cf14..306e28955c6 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -764,6 +764,8 @@ normalize_atom (tree t, tree args, norm_info info)
>    tree ci = build_tree_list (t, info.context);
>
>    tree atom = build1 (ATOMIC_CONSTR, ci, map);
> +  if (info.in_decl && concept_definition_p (info.in_decl))
> +    ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P (atom) = true;
>    if (!info.generate_diagnostics ())
>      {
>        /* Cache the ATOMIC_CONSTRs that we return, so that sat_hasher::equal
> @@ -2826,33 +2828,37 @@ satisfaction_value (tree t)
>      return boolean_true_node;
>  }
>
> -/* Build a new template argument list with template arguments corresponding
> -   to the parameters used in an atomic constraint.  */
> +/* Build a new template argument vector according to the parameter
> +   mapping of the atomic constraint T, using arguments from ARGS.  */
>
> -tree
> -get_mapped_args (tree map)
> +static tree
> +get_mapped_args (tree t, tree args)
>  {
> +  tree map = ATOMIC_CONSTR_MAP (t);
> +
>    /* No map, no arguments.  */
>    if (!map)
>      return NULL_TREE;
>
> -  /* Find the mapped parameter with the highest level.  */
> -  int count = 0;
> -  for (tree p = map; p; p = TREE_CHAIN (p))
> -    {
> -      int level;
> -      int index;
> -      template_parm_level_and_index (TREE_VALUE (p), &level, &index);
> -      if (level > count)
> -        count = level;
> -    }
> +  /* Determine the depth of the resulting argument vector.  */
> +  int depth;
> +  if (ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P (t))
> +    /* The expression of this atomic constraint comes from a concept definition,
> +       whose template depth is always one, so the resulting argument vector
> +       will also have depth one.  */
> +    depth = 1;
> +  else
> +    /* Otherwise, the expression of this atomic constraint was written in
> +       the context of the constrained entity, whose template depth is that
> +       of ARGS.  */
> +    depth = TMPL_ARGS_DEPTH (args);
>
>    /* Place each argument at its corresponding position in the argument
>       list. Note that the list will be sparse (not all arguments supplied),
>       but instantiation is guaranteed to only use the parameters in the
>       mapping, so null arguments would never be used.  */
> -  auto_vec< vec<tree> > lists (count);
> -  lists.quick_grow_cleared (count);
> +  auto_vec< vec<tree> > lists (depth);
> +  lists.quick_grow_cleared (depth);
>    for (tree p = map; p; p = TREE_CHAIN (p))
>      {
>        int level;
> @@ -2862,12 +2868,12 @@ get_mapped_args (tree map)
>        /* Insert the argument into its corresponding position.  */
>        vec<tree> &list = lists[level - 1];
>        if (index >= (int)list.length ())
> -       list.safe_grow_cleared (index + 1, true);
> +       list.safe_grow_cleared (index + 1, /*exact=*/false);
>        list[index] = TREE_PURPOSE (p);
>      }
>
>    /* Build the new argument list.  */
> -  tree args = make_tree_vec (lists.length ());
> +  args = make_tree_vec (lists.length ());
>    for (unsigned i = 0; i != lists.length (); ++i)
>      {
>        vec<tree> &list = lists[i];
> @@ -2879,6 +2885,16 @@ get_mapped_args (tree map)
>      }
>    SET_NON_DEFAULT_TEMPLATE_ARGS_COUNT (args, 0);
>
> +  if (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (args)
> +      && TMPL_ARGS_DEPTH (args) == 1)
> +    {
> +      /* Micro-optimization: represent a depth-one argument vector
> +        using a single level.  */
> +      tree level = TMPL_ARGS_LEVEL (args, 1);
> +      ggc_free (args);
> +      args = level;
> +    }
> +
>    return args;
>  }
>
> @@ -2933,7 +2949,7 @@ satisfy_atom (tree t, tree args, sat_info info)
>      }
>
>    /* Rebuild the argument vector from the parameter mapping.  */
> -  args = get_mapped_args (map);
> +  args = get_mapped_args (t, args);
>
>    /* Apply the parameter mapping (i.e., just substitute).  */
>    tree expr = ATOMIC_CONSTR_EXPR (t);
> @@ -2955,7 +2971,7 @@ satisfy_atom (tree t, tree args, sat_info info)
>    if (!same_type_p (TREE_TYPE (result), boolean_type_node))
>      {
>        if (info.noisy ())
> -       diagnose_atomic_constraint (t, map, result, info);
> +       diagnose_atomic_constraint (t, args, result, info);
>        return cache.save (inst_cache.save (error_mark_node));
>      }
>
> @@ -2974,7 +2990,7 @@ satisfy_atom (tree t, tree args, sat_info info)
>      }
>    result = satisfaction_value (result);
>    if (result == boolean_false_node && info.diagnose_unsatisfaction_p ())
> -    diagnose_atomic_constraint (t, map, result, info);
> +    diagnose_atomic_constraint (t, args, result, info);
>
>    return cache.save (inst_cache.save (result));
>  }
> @@ -3642,11 +3658,10 @@ diagnose_trait_expr (tree expr, tree args)
>      }
>  }
>
> -/* Diagnose a substitution failure in the atomic constraint T when applied
> -   with the instantiated parameter mapping MAP.  */
> +/* Diagnose a substitution failure in the atomic constraint T using ARGS.  */
>
>  static void
> -diagnose_atomic_constraint (tree t, tree map, tree result, sat_info info)
> +diagnose_atomic_constraint (tree t, tree args, tree result, sat_info info)
>  {
>    /* If the constraint is already ill-formed, we've previously diagnosed
>       the reason. We should still say why the constraints aren't satisfied.  */
> @@ -3667,7 +3682,6 @@ diagnose_atomic_constraint (tree t, tree map, tree result, sat_info info)
>    /* Generate better diagnostics for certain kinds of expressions.  */
>    tree expr = ATOMIC_CONSTR_EXPR (t);
>    STRIP_ANY_LOCATION_WRAPPER (expr);
> -  tree args = get_mapped_args (map);
>    switch (TREE_CODE (expr))
>      {
>      case TRAIT_EXPR:
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index f253b32c3f2..dc2429a8406 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -466,6 +466,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
>        IMPLICIT_CONV_EXPR_NONTYPE_ARG (in IMPLICIT_CONV_EXPR)
>        BASELINK_FUNCTIONS_MAYBE_INCOMPLETE_P (in BASELINK)
>        BIND_EXPR_VEC_DTOR (in BIND_EXPR)
> +      ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P (in ATOMIC_CONSTR)
>     2: IDENTIFIER_KIND_BIT_2 (in IDENTIFIER_NODE)
>        ICS_THIS_FLAG (in _CONV)
>        DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (in VAR_DECL)
> @@ -1679,6 +1680,11 @@ check_constraint_info (tree t)
>  #define ATOMIC_CONSTR_MAP_INSTANTIATED_P(NODE) \
>    TREE_LANG_FLAG_0 (ATOMIC_CONSTR_CHECK (NODE))
>
> +/* Whether the expression for this atomic constraint belongs to a
> +   concept definition.  */
> +#define ATOMIC_CONSTR_EXPR_FROM_CONCEPT_P(NODE) \
> +  TREE_LANG_FLAG_1 (ATOMIC_CONSTR_CHECK (NODE))
> +
>  /* The expression of an atomic constraint. */
>  #define ATOMIC_CONSTR_EXPR(NODE) \
>    CONSTR_EXPR (ATOMIC_CONSTR_CHECK (NODE))
> @@ -8306,7 +8312,6 @@ extern tree evaluate_requires_expr                (tree);
>  extern tree tsubst_constraint                   (tree, tree, tsubst_flags_t, tree);
>  extern tree tsubst_constraint_info              (tree, tree, tsubst_flags_t, tree);
>  extern tree tsubst_parameter_mapping           (tree, tree, tsubst_flags_t, tree);
> -extern tree get_mapped_args                    (tree);
>
>  struct processing_constraint_expression_sentinel
>  {
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-return-req4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-return-req4.C
> new file mode 100644
> index 00000000000..471946bc8eb
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-return-req4.C
> @@ -0,0 +1,24 @@
> +// PR c++/104527
> +// { dg-do compile { target c++20 } }
> +
> +template<class T, class U>
> +concept is_same = __is_same(T, U);
> +
> +template<class T>
> +struct A {
> +  template<class...>
> +    requires requires { { 0 } -> is_same<T>; }
> +  struct B {};
> +
> +  template<class...>
> +    requires requires { { 1 } -> is_same<T>; }
> +  static void f();
> +};
> +
> +A<int>::B<> a1;
> +A<bool>::B<> a2; // { dg-error "constraint" }
> +
> +int main() {
> +  A<int>::f();
> +  A<bool>::f(); // { dg-error "no match" }
> +}
> --
> 2.35.1.129.gb80121027d
>