On Tue, 27 May 2025, Nathaniel Shead wrote:
> On Wed, Nov 27, 2024 at 11:45:40AM -0500, Patrick Palka wrote:
> > 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.
> >
> > > ...[snip]...
> >
> > If we substituted the TEMPLATE_ID_EXPR as a whole we could use the
> > DECL_TI_ARGS of that IIUC?
> >
>
> Thanks for your comments, they were very helpful. Here's a totally new
> approach which I'm much happier with. I've also removed the "disable in
> case any specialisation exists" logic, as on further reflection I don't
> imagine this to be the kind of issue I thought it might have been.
>
> With this patch,
>
> template <typename T>
> constexpr bool is_default_constructible_v = __is_constructible(T);
>
> template <typename T>
> concept default_constructible = is_default_constructible_v<T>;
>
> static_assert(default_constructible<void>);
>
> now emits the following error:
>
> test.cpp:6:15: error: static assertion failed
> 6 | static_assert(default_constructible<void>);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> test.cpp:6:15: note: constraints not satisfied
> test.cpp:4:9: required by the constraints of ‘template<class T> concept
> default_constructible’
> test.cpp:4:33: note: ‘void’ is not default constructible
> 4 | concept default_constructible = is_default_constructible_v<T>;
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> There's still a lot of improvements to be made in this area, I think:
>
> - I haven't yet looked into updating the specific diagnostics emitted by
> the traits; I'd like to try to avoid too much code duplication with
> the implementation in cp/semantics.cc. (I also don't think the manual
> indentation at the start of the message is particularly helpful?)
For is_xible / is_convertible etc, perhaps they could use a 'complain'
parameter that they propagate through instead of always passing tf_none,
similar to build_invoke? Then we can call those predicates directly
from diagnose_trait_expr with complain=tf_error so that they elaborate
why they failed.
Agreed about the extra indentation
>
> - The message doesn't print the mapping '[with T = void]'; I tried a
> couple of things but this doesn't currently look especially
> straight-forward, as we don't currently associate the args with the
> normalised atomic constraint of the declaration.
Maybe we can still print the
note: the expression ‘normal<T> [with T = void]’ evaluated to ‘false’
note alongside the extended diagnostics? Which would mean moving the
maybe_diagnose_standard_trait call a bit lower in
diagnose_atomic_constraint.
This would arguably make the diagnostic even noiser, but IMHO the
parameter mapping is an important piece of information to omit.
>
> - Just generally I think there's still a lot of noise in the diagnostic,
> and I find the back-to-front ordering of 'required by...' confusing.
Agreed in general, but in the case of these type trait diagnostics I
think noisiness is kind of inevitable especially if we start explaining
in detail why the trait is unsatisified (as e.g. PR117294 requests).
>
> Depending on how much time I find myself with I might take a look at
> some of these further issues later, but in the meantime, does this look
> like an improvement over the status quo?
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >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.
>
> 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, which can be done
> in a future patch.
>
> Apart from concept diagnostics, this is also useful when using such
> traits in a 'static_assert' directly, so this patch also adjusts the
> diagnostics in that context.
>
> PR c++/117294
> PR c++/113854
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (maybe_diagnose_standard_trait): Declare.
> * constexpr.cc (diagnose_failing_condition): Call it.
> * constraint.cc (diagnose_trait_expr): Add location parameter.
> (maybe_diagnose_standard_trait): New function.
> (diagnose_atomic_constraint): Call it.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp2a/concepts-traits4.C: New test.
> * g++.dg/diagnostic/static_assert5.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> Reviewed-by: Patrick Palka <ppa...@redhat.com>
> ---
> gcc/cp/constexpr.cc | 2 +
> gcc/cp/constraint.cc | 49 +++++++++++++++++--
> gcc/cp/cp-tree.h | 1 +
> gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C | 40 +++++++++++++++
> .../g++.dg/diagnostic/static_assert5.C | 26 ++++++++++
> 5 files changed, 114 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C
> create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert5.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index fa754b9a176..0c8a9199a67 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -2134,6 +2134,8 @@ diagnose_failing_condition (tree bad, location_t cloc,
> bool show_expr_p,
> tree cond = build2 (TREE_CODE (bad), boolean_type_node, op0, op1);
> inform (cloc, "the comparison reduces to %qE", cond);
> }
> + else if (maybe_diagnose_standard_trait (cloc, bad, NULL_TREE))
> + ;
> else if (show_expr_p)
> inform (cloc, "%qE evaluates to false", bad);
> }
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 44fb086c630..f21d61be94b 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -3016,10 +3016,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;
> @@ -3231,6 +3229,46 @@ 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
> + more helpful diagnostics. */
> +
> +bool
> +maybe_diagnose_standard_trait (location_t loc, tree expr, tree args)
> +{
> + if (TREE_CODE (expr) == TEMPLATE_ID_EXPR)
> + {
> + processing_template_decl_sentinel ptds;
> + expr = tsubst_expr (expr, args, tf_none, NULL_TREE);
> + expr = tree_strip_nop_conversions (expr);
> + if (TREE_CODE (expr) == CLEANUP_POINT_EXPR)
> + expr = TREE_OPERAND (expr, 0);
What's this CLEANUP_POINT_EXPR handling for? I'd be surprised to see a
CLEANUP_POINT_EXPR after instantiating a suitable variable template-id.
> + args = NULL_TREE;
> + }
> +
> + /* Ensure we haven't picked a specialization. This will mean we won't
> + have the nice error if the trait was only used in the specialization,
> + but that is not how the standard library will use these traits. */
> + if (VAR_P (expr)
> + && DECL_LANG_SPECIFIC (expr)
> + && DECL_USE_TEMPLATE (expr)
> + && !DECL_TEMPLATE_SPECIALIZATION (expr)
> + && PRIMARY_TEMPLATE_P (DECL_TI_TEMPLATE (expr))
> + && !TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (expr)))
Rather than giving up when a partial specialization is selected, I think
we can leverage the information in TI_PARTIAL_INFO to give a better
diagnostic in that case too (if the partial specialization is suitably
defined):
template <typename T> constexpr bool partial_spec = false;
template <typename T> constexpr bool partial_spec<T*> = __is_constructible(T);
static_assert(partial_spec<int*>);
We just need to use TI_TEMPLATE/ARGS (TI_PARTIAL_INFO (DECL_TEMPLATE_INFO
(expr))
instead of DECL_TI_TEMPLATE/ARGS in that case.
(For an explicit specialization, I think we have no choice but to give
up because we don't know what the original initializer looked like.)
> + {
> + gcc_assert (!args);
> + tree result = DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (expr));
> + tree initial = DECL_INITIAL (result);
> + if (TREE_CODE (initial) == TRAIT_EXPR)
> + {
> + diagnose_trait_expr (loc, initial, DECL_TI_ARGS (expr));
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> /* Diagnose a substitution failure in the atomic constraint T using ARGS. */
>
> static void
> @@ -3255,10 +3293,13 @@ 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);
> + if (maybe_diagnose_standard_trait (loc, expr, args))
> + return;
> +
> 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/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 175ab287490..e989a6302d7 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -8793,6 +8793,7 @@ extern bool atomic_constraints_identical_p (tree,
> tree);
> extern hashval_t iterative_hash_constraint (tree, hashval_t);
> extern hashval_t hash_atomic_constraint (tree);
> extern void diagnose_constraints (location_t, tree, tree);
> +extern bool maybe_diagnose_standard_trait (location_t, tree, tree);
>
> extern void note_failed_type_completion_for_satisfaction (tree);
>
> 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..4d5f1774f46
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-traits4.C
> @@ -0,0 +1,40 @@
> +// PR c++/117294
> +// { dg-do compile { target c++20 } }
> +// { dg-additional-options "-fconcepts-diagnostics-depth=2" }
> +
> +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-line normal }
> +
> +template <typename T>
> +concept test_partial_spec = partial_spec<T>; // { dg-line partial }
> +
> +template <typename T>
> +concept test_explicit_spec = explicit_spec<T>; // { dg-line explicit }
> +
> +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" }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> normal }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> partial }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> explicit }
> +
> +static_assert(test_partial_spec<int*>); // { dg-error "assert" }
> +static_assert(test_explicit_spec<int>); // { dg-error "assert" }
> +// { dg-message ".with T = int\\*.. evaluated to .false." "" { target *-*-*
> } partial }
> +// { dg-message ".with T = int.. evaluated to .false." "" { target *-*-* }
> explicit }
> +
> +struct S { S(int); };
> +static_assert(requires { requires test_normal<S>; }); // { dg-error
> "assert" }
> +static_assert(requires { requires test_partial_spec<S>; }); // { dg-error
> "assert" }
> +static_assert(requires { requires test_explicit_spec<S>; }); // { dg-error
> "assert" }
> +// { dg-message "'S' is not default constructible" "" { target *-*-* }
> normal }
> +// { dg-message "'S' is not default constructible" "" { target *-*-* }
> partial }
> +// { dg-message "'S' is not default constructible" "" { target *-*-* }
> explicit }
> diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert5.C
> b/gcc/testsuite/g++.dg/diagnostic/static_assert5.C
> new file mode 100644
> index 00000000000..e5a0684d01a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert5.C
> @@ -0,0 +1,26 @@
> +// PR c++/117294
> +// { dg-do compile { target c++14 } }
> +
> +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;
> +
> +static_assert(normal<void>); // { dg-error "assert" }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> .-1 }
> +static_assert(partial_spec<void>); // { dg-error "assert" }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> .-1 }
> +static_assert(explicit_spec<void>); // { dg-error "assert" }
> +// { dg-message "'void' is not default constructible" "" { target *-*-* }
> .-1 }
> +
> +static_assert(partial_spec<int*>); // { dg-error "assert" }
> +// { dg-bogus "default constructible" "" { target *-*-* } .-1 }
> +static_assert(explicit_spec<int>); // { dg-error "assert" }
> +// { dg-bogus "default constructible" "" { target *-*-* } .-1 }
> +
> +static_assert(__is_constructible(void)); // { dg-error "assert" }
> +// The above should not ICE, but we no longer know that the error was caused
> +// by the trait because it's been folded before we process the failure.
> --
> 2.47.0
>
>
