[PATCH] c++: Improve static_assert diagnostic [PR97518]
Jason Merrill
jason@redhat.com
Tue Nov 10 19:15:56 GMT 2020
On 11/10/20 1:59 PM, Marek Polacek wrote:
> On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote:
>> On 11/9/20 7:21 PM, Marek Polacek wrote:
>>> Currently, when a static_assert fails, we only say "static assertion failed".
>>> It would be more useful if we could also print the expression that
>>> evaluated to false; this is especially useful when the condition uses
>>> template parameters. Consider the motivating example, in which we have
>>> this line:
>>>
>>> static_assert(is_same<X, Y>::value);
>>>
>>> if this fails, the user has to play dirty games to get the compiler to
>>> print the template arguments. With this patch, we say:
>>>
>>> static assertion failed due to requirement 'is_same<int*, int>::value'
>>
>> I'd rather avoid the word "requirement" here, to avoid confusion with
>> Concepts.
>>
>> Maybe have the usual failed error, and if we're showing the expression, have
>> a second inform to say e.g. "%qE evaluates to false"?
>
> Works for me.
>
>> Also, if we've narrowed the problem down to a particular subexpression,
>> let's only print that one.
>
> Done.
>
>>> which I think is much better. However, always printing the condition that
>>> evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is
>>> always parsed to true/false, so we would say "static assertion failed due
>>> to requirement 'false'" which doesn't help. So I wound up only printing
>>> the condition when it was instantiation-dependent, that is, we called
>>> finish_static_assert from tsubst_expr.
>>>
>>> Moreover, this patch also improves the diagnostic when the condition
>>> consists of a logical AND. Say you have something like this:
>>>
>>> static_assert(fn1() && fn2() && fn3() && fn4() && fn5());
>>>
>>> where fn4() evaluates to false and the other ones to true. Highlighting
>>> the whole thing is not that helpful because it won't say which clause
>>> evaluated to false. With the find_failing_clause tweak in this patch
>>> we emit:
>>>
>>> error: static assertion failed
>>> 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5());
>>> | ~~~^~
>>>
>>> so you know right away what's going on. Unfortunately, when you combine
>>> both things, that is, have an instantiation-dependent expr and && in
>>> a static_assert, we can't yet quite point to the clause that failed. It
>>> is because when we tsubstitute something like is_same<X, Y>::value, we
>>> generate a VAR_DECL that doesn't have any location. It would be awesome
>>> if we could wrap it with a location wrapper, but I didn't see anything
>>> obvious.
>>
>> Hmm, I vaguely remember that at first we were using location wrappers less
>> in templates, but I thought that was fixed. I don't see anything setting
>> suppress_location_wrappers, and it looks like tsubst_copy_and_build should
>> preserve a location wrapper.
>
> The problem here is that tsubst_qualified_id produces a VAR_DECL and for those
> CAN_HAVE_LOCATION_P is false.
Ah, then perhaps tsubst_qualified_id should call
maybe_wrap_with_location to preserve the location of the SCOPE_REF.
> If the user writes (bool) is_same<X, Y>::value,
> this works well. Maybe we could create an artificial (bool) for clauses of
> the logical AND operator; decay_conversion in cp_build_binary_op will likely
> do it anyway, it just doesn't have any location: cp_build_binary_op takes
> an op_location_t, but that doesn't track locations of the operands. Maybe
> some hacks in tsubst_copy_and_build/TRUTH_AND_EXPR would help, I'm not sure.
>
>>> Further tweak could be to print the failing clause of the condition if
>>> possible, whether or not it was instantiation-dependent.
>>
>> If not instantiation-dependent, underlining the clause seems sufficient.
>
> Ack.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> -- >8 --
> Currently, when a static_assert fails, we only say "static assertion failed".
> It would be more useful if we could also print the expression that
> evaluated to false; this is especially useful when the condition uses
> template parameters. Consider the motivating example, in which we have
> this line:
>
> static_assert(is_same<X, Y>::value);
>
> if this fails, the user has to play dirty games to get the compiler to
> print the template arguments. With this patch, we say:
>
> error: static assertion failed
> note: 'is_same<int*, int>::value' evaluates to false
>
> which I think is much better. However, always printing the condition that
> evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is
> always parsed to true/false, so we would say "'false' evaluates to false"
> which doesn't help. So I wound up only printing the condition when it was
> instantiation-dependent, that is, we called finish_static_assert from
> tsubst_expr.
>
> Moreover, this patch also improves the diagnostic when the condition
> consists of a logical AND. Say you have something like this:
>
> static_assert(fn1() && fn2() && fn3() && fn4() && fn5());
>
> where fn4() evaluates to false and the other ones to true. Highlighting
> the whole thing is not that helpful because it won't say which clause
> evaluated to false. With the find_failing_clause tweak in this patch
> we emit:
>
> error: static assertion failed
> 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5());
> | ~~~^~
>
> so you know right away what's going on. Unfortunately, when you combine
> both things, that is, have an instantiation-dependent expr and && in
> a static_assert, we can't yet quite point to the clause that failed. It
> is because when we tsubstitute something like is_same<X, Y>::value, we
> generate a VAR_DECL that doesn't have any location. It would be awesome
> if we could wrap it with a location wrapper, but I didn't see anything
> obvious.
>
> In passing, I've cleaned up some things:
> * use iloc_sentinel when appropriate,
> * it's nicer to call contextual_conv_bool instead of the rather verbose
> perform_implicit_conversion_flags,
> * no need to check for INTEGER_CST before calling integer_zerop.
>
> gcc/cp/ChangeLog:
>
> PR c++/97518
> * cp-tree.h (finish_static_assert): Adjust declaration.
> * parser.c (cp_parser_static_assert): Pass false to
> finish_static_assert.
> * pt.c (tsubst_expr): Pass true to finish_static_assert.
> * semantics.c (find_failing_clause_r): New function.
> (find_failing_clause): New function.
> (finish_static_assert): Add a bool parameter. Use
> iloc_sentinel. Call contextual_conv_bool instead of
> perform_implicit_conversion_flags. Don't check for INTEGER_CST before
> calling integer_zerop. Call find_failing_clause and maybe use its
> location. Print the original condition or the failing clause if
> SHOW_EXPR_P.
>
> gcc/testsuite/ChangeLog:
>
> PR c++/97518
> * g++.dg/diagnostic/pr87386.C: Adjust expected output.
> * g++.dg/diagnostic/static_assert1.C: New test.
> * g++.dg/diagnostic/static_assert2.C: New test.
>
> libcc1/ChangeLog:
>
> PR c++/97518
> * libcp1plugin.cc (plugin_add_static_assert): Pass false to
> finish_static_assert.
> ---
> gcc/cp/cp-tree.h | 2 +-
> gcc/cp/parser.c | 3 +-
> gcc/cp/pt.c | 6 +-
> gcc/cp/semantics.c | 73 ++++++++++++++++---
> gcc/testsuite/g++.dg/diagnostic/pr87386.C | 2 +-
> .../g++.dg/diagnostic/static_assert1.C | 30 ++++++++
> .../g++.dg/diagnostic/static_assert2.C | 68 +++++++++++++++++
> libcc1/libcp1plugin.cc | 2 +-
> 8 files changed, 167 insertions(+), 19 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert1.C
> create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert2.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index b98d47a702f..230a1525c63 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7234,7 +7234,7 @@ extern bool cxx_omp_create_clause_info (tree, tree, bool, bool,
> bool, bool);
> extern tree baselink_for_fns (tree);
> extern void finish_static_assert (tree, tree, location_t,
> - bool);
> + bool, bool);
> extern tree finish_decltype_type (tree, bool, tsubst_flags_t);
> extern tree finish_trait_expr (location_t, enum cp_trait_kind, tree, tree);
> extern tree build_lambda_expr (void);
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 9c08c0e46a2..36322812310 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -14886,7 +14886,8 @@ cp_parser_static_assert(cp_parser *parser, bool member_p)
>
> /* Complete the static assertion, which may mean either processing
> the static assert now or saving it for template instantiation. */
> - finish_static_assert (condition, message, assert_loc, member_p);
> + finish_static_assert (condition, message, assert_loc, member_p,
> + /*show_expr_p=*/false);
> }
>
> /* Parse the expression in decltype ( expression ). */
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index a2655a0ff52..6ba114c9da3 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -18492,8 +18492,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
> tree condition;
>
> ++c_inhibit_evaluation_warnings;
> - condition =
> - tsubst_expr (STATIC_ASSERT_CONDITION (t),
> + condition =
> + tsubst_expr (STATIC_ASSERT_CONDITION (t),
> args,
> complain, in_decl,
> /*integral_constant_expression_p=*/true);
> @@ -18502,7 +18502,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
> finish_static_assert (condition,
> STATIC_ASSERT_MESSAGE (t),
> STATIC_ASSERT_SOURCE_LOCATION (t),
> - /*member_p=*/false);
> + /*member_p=*/false, /*show_expr_p=*/true);
> }
> break;
>
> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> index 33d715edaec..089c83e511f 100644
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -9827,13 +9827,53 @@ init_cp_semantics (void)
> {
> }
>
> +
> +/* If we have a condition in conjunctive normal form (CNF), find the first
> + failing clause. In other words, given an expression like
> +
> + true && true && false && true && false
> +
> + return the first 'false'. EXPR is the expression. */
> +
> +static tree
> +find_failing_clause_r (tree expr)
> +{
> + if (TREE_CODE (expr) == TRUTH_ANDIF_EXPR)
> + {
> + /* First check the left side... */
> + tree e = find_failing_clause_r (TREE_OPERAND (expr, 0));
> + if (e == NULL_TREE)
> + /* ...if we didn't find a false clause, check the right side. */
> + e = find_failing_clause_r (TREE_OPERAND (expr, 1));
> + return e;
> + }
> + tree e = contextual_conv_bool (expr, tf_none);
> + e = fold_non_dependent_expr (e, tf_none, /*manifestly_const_eval=*/true);
> + if (integer_zerop (e))
> + /* This is the failing clause. */
> + return expr;
> + return NULL_TREE;
> +}
> +
> +/* Wrapper for find_failing_clause_r. */
> +
> +static tree
> +find_failing_clause (tree expr)
> +{
> + if (TREE_CODE (expr) != TRUTH_ANDIF_EXPR)
> + return NULL_TREE;
> + return find_failing_clause_r (expr);
> +}
> +
> /* Build a STATIC_ASSERT for a static assertion with the condition
> CONDITION and the message text MESSAGE. LOCATION is the location
> of the static assertion in the source code. When MEMBER_P, this
> - static assertion is a member of a class. */
> + static assertion is a member of a class. If SHOW_EXPR_P is true,
> + print the condition (because it was instantiation-dependent). */
> +
> void
> finish_static_assert (tree condition, tree message, location_t location,
> - bool member_p)
> + bool member_p, bool show_expr_p)
> {
> tsubst_flags_t complain = tf_warning_or_error;
>
> @@ -9871,8 +9911,7 @@ finish_static_assert (tree condition, tree message, location_t location,
> tree orig_condition = condition;
>
> /* Fold the expression and convert it to a boolean value. */
> - condition = perform_implicit_conversion_flags (boolean_type_node, condition,
> - complain, LOOKUP_NORMAL);
> + condition = contextual_conv_bool (condition, complain);
> condition = fold_non_dependent_expr (condition, complain,
> /*manifestly_const_eval=*/true);
>
> @@ -9881,21 +9920,32 @@ finish_static_assert (tree condition, tree message, location_t location,
> ;
> else
> {
> - location_t saved_loc = input_location;
> + iloc_sentinel ils (location);
>
> - input_location = location;
> - if (TREE_CODE (condition) == INTEGER_CST
> - && integer_zerop (condition))
> + if (integer_zerop (condition))
> {
> int sz = TREE_INT_CST_LOW (TYPE_SIZE_UNIT
> (TREE_TYPE (TREE_TYPE (message))));
> int len = TREE_STRING_LENGTH (message) / sz - 1;
> +
> + /* See if we can find which clause was failing (for logical AND). */
> + tree bad = find_failing_clause (orig_condition);
> + /* If not, or its location is unusable, fall back to the previous
> + location. */
> + location_t cloc = location;
> + if (cp_expr_location (bad) != UNKNOWN_LOCATION)
> + cloc = cp_expr_location (bad);
> +
> /* Report the error. */
> if (len == 0)
> - error ("static assertion failed");
> + error_at (cloc, "static assertion failed");
> else
> - error ("static assertion failed: %s",
> - TREE_STRING_POINTER (message));
> + error_at (cloc, "static assertion failed: %s",
> + TREE_STRING_POINTER (message));
> + if (show_expr_p)
> + inform (cloc, "%qE evaluates to false",
> + /* Nobody wants to see the artifical (bool) cast. */
"artificial"
OK with that typo fixed.
> + (bad ? tree_strip_nop_conversions (bad) : orig_condition));
>
> /* Actually explain the failure if this is a concept check or a
> requires-expression. */
> @@ -9909,7 +9959,6 @@ finish_static_assert (tree condition, tree message, location_t location,
> if (require_rvalue_constant_expression (condition))
> cxx_constant_value (condition);
> }
> - input_location = saved_loc;
> }
> }
>
> diff --git a/gcc/testsuite/g++.dg/diagnostic/pr87386.C b/gcc/testsuite/g++.dg/diagnostic/pr87386.C
> index 85726af9f01..679a5177f64 100644
> --- a/gcc/testsuite/g++.dg/diagnostic/pr87386.C
> +++ b/gcc/testsuite/g++.dg/diagnostic/pr87386.C
> @@ -14,5 +14,5 @@ static_assert (foo::test<int>::value, "foo"); // { dg-error "static assertion f
> static_assert (foo::test<int>::value && true, "bar"); // { dg-error "static assertion failed: bar" }
> /* { dg-begin-multiline-output "" }
> static_assert (foo::test<int>::value && true, "bar");
> - ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> + ~~~~~~~~~~~~~~~~^~~~~
> { dg-end-multiline-output "" } */
> diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert1.C b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C
> new file mode 100644
> index 00000000000..fecf3cc51a0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C
> @@ -0,0 +1,30 @@
> +// PR c++/97518
> +// { dg-do compile { target c++17 } }
> +
> +template <typename T, typename U> struct is_same { static constexpr bool value = false; };
> +template <typename T> struct is_same<T, T> { static constexpr bool value = true; };
> +
> +template <typename T> using some_metafunction_t = T;
> +
> +template <typename T>
> +void foo(T ) {
> + using X = T*;
> + using Y = some_metafunction_t<T>;
> +
> + static_assert(is_same<X, Y>::value); // { dg-error "static assertion failed" }
> + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 }
> + static_assert(is_same<X, Y>::value, "foo"); // { dg-error "static assertion failed: foo" }
> + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 }
> + static_assert(is_same<X, X>::value && is_same<X, Y>::value); // { dg-error "static assertion failed" }
> + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 }
> + static_assert(is_same<X, Y>::value && is_same<X, X>::value); // { dg-error "static assertion failed" }
> + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 }
> + static_assert(is_same<X, X>::value
> + && is_same<Y, Y>::value
> + && is_same<X, Y>::value); // { dg-error "static assertion failed" }
> + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 }
> +}
> +
> +void bar() {
> + foo(0);
> +}
> diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert2.C b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C
> new file mode 100644
> index 00000000000..542697f99de
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C
> @@ -0,0 +1,68 @@
> +// PR c++/97518
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-fdiagnostics-show-caret" }
> +
> +constexpr bool yes () { return true; }
> +constexpr bool no () { return false; }
> +constexpr bool yay = true;
> +constexpr bool nay = false;
> +
> +void
> +bar ()
> +{
> + static_assert (true && true && no(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (true && true && no(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert (yay && nay, ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (yay && nay, "");
> + ^~~
> + { dg-end-multiline-output "" } */
> + static_assert (yes() && no(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (yes() && no(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert (no() && yes(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (no() && yes(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert (no() && no() && yes(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (no() && no() && yes(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert (yes() && yes() && yes () && no() && yes(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (yes() && yes() && yes () && no() && yes(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert (yes() && yes() && yes () && (no() && yes()), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert (yes() && yes() && yes () && (no() && yes()), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert ((yes() && no()) && no(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert ((yes() && no()) && no(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> + static_assert ((no() || no()) && yes(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert ((no() || no()) && yes(), "");
> + ~~~~~~^~~~~~~~
> + { dg-end-multiline-output "" } */
> + static_assert ((yes() || no()) && no(), ""); // { dg-error "static assertion failed" }
> +/* { dg-begin-multiline-output "" }
> + static_assert ((yes() || no()) && no(), "");
> + ~~^~
> + { dg-end-multiline-output "" } */
> +}
> diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc
> index bab2751a5ce..67a235f9095 100644
> --- a/libcc1/libcp1plugin.cc
> +++ b/libcc1/libcp1plugin.cc
> @@ -3642,7 +3642,7 @@ plugin_add_static_assert (cc1_plugin::connection *self,
>
> bool member_p = at_class_scope_p ();
>
> - finish_static_assert (condition, message, loc, member_p);
> + finish_static_assert (condition, message, loc, member_p, false);
>
> return 1;
> }
>
> base-commit: 831f24a778a016c6ce1ae739235e3f7e1f28ed8c
>
More information about the Gcc-patches
mailing list