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?)
- 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.
- 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.
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);
+ 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)))
+ {
+ 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
