[RFC/PATCH] c++: Unwrap type traits defined in terms of builtins within concept diagnostics [PR117294]

Does this approach seem reasonable?  I'm pretty sure that the way I've
handled the templating here is unideal but I'm not sure what a neat way
to do what I'm trying to do here would be; any comments are welcome.

-- >8 --

Currently, concept failures of standard type traits just report
'expression X<T> evaluates to false'.  However, many type traits are
actually defined in terms of compiler builtins; we can do better here.
For instance, 'is_constructible_v' could go on to explain why the type
is not constructible, or 'is_invocable_v' could list potential
candidates.

As a first step to supporting that we need to be able to map the
standard type traits to the builtins that they use.  Rather than adding
another list that would need to be kept up-to-date whenever a builtin is
added, this patch instead tries to detect any variable template defined
directly in terms of a TRAIT_EXPR.

To avoid false positives, we ignore any variable templates that have any
specialisations (partial or explicit), even if we wouldn't have chosen
that specialisation anyway.  This shouldn't affect any of the standard
library type traits that I could see.

The new diagnostics from this patch are not immediately much better;
however, it would be relatively straight-forward to update the messages
in 'diagnose_trait_expr' to provide these new details.

This logic could also perhaps be used by 'diagnose_failing_condition' so
that cases like 'static_assert(std::is_constructible_v<T>)' get the same
treatment; this patch doesn't attempt to update this yet.

	PR c++/117294
	PR c++/113854

gcc/cp/ChangeLog:

	* constraint.cc (diagnose_trait_expr): Take location to diagnose
	at explicitly.
	(maybe_unwrap_standard_trait): New function.
	(diagnose_atomic_constraint): Use it; pass in the location of
	the atomic constraint to diagnose_trait_expr.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-traits4.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/constraint.cc                          | 52 +++++++++++++++++--
 gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C | 31 +++++++++++
 2 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C

On Fri, 8 Nov 2024, Nathaniel Shead wrote:

> Does this approach seem reasonable?  I'm pretty sure that the way I've
> handled the templating here is unideal but I'm not sure what a neat way
> to do what I'm trying to do here would be; any comments are welcome.

Clever approach, I like it!

> 
> -- >8 --
> 
> Currently, concept failures of standard type traits just report
> 'expression X<T> evaluates to false'.  However, many type traits are
> actually defined in terms of compiler builtins; we can do better here.
> For instance, 'is_constructible_v' could go on to explain why the type
> is not constructible, or 'is_invocable_v' could list potential
> candidates.

That'd be great improvement.

> 
> As a first step to supporting that we need to be able to map the
> standard type traits to the builtins that they use.  Rather than adding
> another list that would need to be kept up-to-date whenever a builtin is
> added, this patch instead tries to detect any variable template defined
> directly in terms of a TRAIT_EXPR.
> 
> To avoid false positives, we ignore any variable templates that have any
> specialisations (partial or explicit), even if we wouldn't have chosen
> that specialisation anyway.  This shouldn't affect any of the standard
> library type traits that I could see.

You should be able to tsubst the TEMPLATE_ID_EXPR directly and look at
its TI_PARTIAL_INFO in order to determine which (if any) partial
specialization was selected.  And if an explicit specialization was
selected the resulting VAR_DECL will have DECL_TEMPLATE_SPECIALIZATION
set.

> 
> The new diagnostics from this patch are not immediately much better;
> however, it would be relatively straight-forward to update the messages
> in 'diagnose_trait_expr' to provide these new details.
> 
> This logic could also perhaps be used by 'diagnose_failing_condition' so
> that cases like 'static_assert(std::is_constructible_v<T>)' get the same
> treatment; this patch doesn't attempt to update this yet.
> 
> 	PR c++/117294
> 	PR c++/113854
> 
> gcc/cp/ChangeLog:
> 
> 	* constraint.cc (diagnose_trait_expr): Take location to diagnose
> 	at explicitly.
> 	(maybe_unwrap_standard_trait): New function.
> 	(diagnose_atomic_constraint): Use it; pass in the location of
> 	the atomic constraint to diagnose_trait_expr.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-traits4.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/constraint.cc                          | 52 +++++++++++++++++--
>  gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C | 31 +++++++++++
>  2 files changed, 79 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C
> 
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 8a36b9c88c4..c683e6a44dd 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -3108,10 +3108,8 @@ get_constraint_error_location (tree t)
>  /* Emit a diagnostic for a failed trait.  */
>  
>  static void
> -diagnose_trait_expr (tree expr, tree args)
> +diagnose_trait_expr (location_t loc, tree expr, tree args)
>  {
> -  location_t loc = cp_expr_location (expr);
> -
>    /* Build a "fake" version of the instantiated trait, so we can
>       get the instantiated types from result.  */
>    ++processing_template_decl;
> @@ -3323,6 +3321,51 @@ diagnose_trait_expr (tree expr, tree args)
>      }
>  }
>  
> +/* Attempt to detect if this is a standard type trait, defined in terms
> +   of a compiler builtin (above).  If so, this will allow us to provide
> +   slightly more helpful diagnostics.
> +
> +   However, don't unwrap if the type has been specialized (even if we
> +   wouldn't have used said specialization)..  */
> +
> +static void
> +maybe_unwrap_standard_trait (tree *expr, tree *args)
> +{
> +  if (TREE_CODE (*expr) != TEMPLATE_ID_EXPR)
> +    return;
> +
> +  tree templ = TREE_OPERAND (*expr, 0);
> +  if (TREE_CODE (templ) != TEMPLATE_DECL
> +      || !variable_template_p (templ))
> +    return;
> +
> +  tree gen_tmpl = most_general_template (templ);
> +  if (DECL_TEMPLATE_SPECIALIZATIONS (gen_tmpl))
> +    return;
> +
> +  for (tree inst = DECL_TEMPLATE_INSTANTIATIONS (gen_tmpl);
> +       inst; inst = TREE_CHAIN (inst))
> +    if (DECL_TEMPLATE_SPECIALIZATION (TREE_VALUE (inst)))
> +      return;
> +
> +  tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
> +  tree initial = DECL_INITIAL (pattern);
> +  if (TREE_CODE (initial) != TRAIT_EXPR)
> +    return;
> +
> +  /* At this point we're definitely providing a TRAIT_EXPR, update
> +     *expr to point at it and provide remapped *args for it.  */
> +  tree parms = DECL_INNERMOST_TEMPLATE_PARMS (gen_tmpl);
> +  tree targs = TREE_OPERAND (*expr, 1);
> +  if (targs)
> +    targs = tsubst_template_args (targs, *args, tf_none, NULL_TREE);
> +  targs = add_outermost_template_args (templ, targs);
> +  targs = coerce_template_parms (parms, targs, templ, tf_none);

If we substituted the TEMPLATE_ID_EXPR as a whole we could use the
DECL_TI_ARGS of that IIUC?

> +
> +  *expr = initial;
> +  *args = targs;
> +}
> +
>  /* Diagnose a substitution failure in the atomic constraint T using ARGS.  */
>  
>  static void
> @@ -3347,10 +3390,11 @@ diagnose_atomic_constraint (tree t, tree args, tree result, sat_info info)
>    /* Generate better diagnostics for certain kinds of expressions.  */
>    tree expr = ATOMIC_CONSTR_EXPR (t);
>    STRIP_ANY_LOCATION_WRAPPER (expr);
> +  maybe_unwrap_standard_trait (&expr, &args);
>    switch (TREE_CODE (expr))
>      {
>      case TRAIT_EXPR:
> -      diagnose_trait_expr (expr, args);
> +      diagnose_trait_expr (loc, expr, args);
>        break;
>      case REQUIRES_EXPR:
>        gcc_checking_assert (info.diagnose_unsatisfaction_p ());
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C
> new file mode 100644
> index 00000000000..a2f9609c4e1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C
> @@ -0,0 +1,31 @@
> +// PR c++/117294
> +// { dg-do compile { target c++20 } }
> +
> +template <typename T> constexpr bool normal = __is_constructible(T);
> +
> +template <typename T> constexpr bool partial_spec = __is_constructible(T);
> +template <typename T> constexpr bool partial_spec<T*> = false;
> +
> +template <typename T> constexpr bool explicit_spec = __is_constructible(T);
> +template <> constexpr bool explicit_spec<int> = false;
> +
> +template <typename T>
> +concept test_normal = normal<T>;
> +// { dg-message "'void' is not default constructible" "" { target *-*-* } .-1 }
> +
> +template <typename T>
> +concept test_partial_spec = partial_spec<T>;
> +// { dg-message "evaluated to .false." "" { target *-*-* } .-1 }
> +// { dg-bogus "not default constructible" "" { target *-*-* } .-2 }
> +
> +template <typename T>
> +concept test_explicit_spec = explicit_spec<T>;
> +// { dg-message "evaluated to .false." "" { target *-*-* } .-1 }
> +// { dg-bogus "not default constructible" "" { target *-*-* } .-2 }
> +
> +static_assert(test_normal<void>);  // { dg-error "assert" }
> +static_assert(test_partial_spec<void>);  // { dg-error "assert" }
> +static_assert(test_explicit_spec<void>);  // { dg-error "assert" }
> +
> +static_assert(test_partial_spec<int*>);  // { dg-error "assert" }
> +static_assert(test_explicit_spec<int>);  // { dg-error "assert" }
> -- 
> 2.47.0
> 
>