c++: Improve static_assert diagnostic [PR97518]
authorMarek Polacek <polacek@redhat.com>
Fri, 6 Nov 2020 20:21:13 +0000 (15:21 -0500)
committerMarek Polacek <polacek@redhat.com>
Tue, 10 Nov 2020 20:08:06 +0000 (15:08 -0500)
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
gcc/cp/parser.c
gcc/cp/pt.c
gcc/cp/semantics.c
gcc/testsuite/g++.dg/diagnostic/pr87386.C
gcc/testsuite/g++.dg/diagnostic/static_assert1.C [new file with mode: 0644]
gcc/testsuite/g++.dg/diagnostic/static_assert2.C [new file with mode: 0644]
libcc1/libcp1plugin.cc

index b98d47a702f590e65586900bc361a2434f5bfcce..230a1525c637835fe1670f7bd08d0b25bc883828 100644 (file)
@@ -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);
index 9c08c0e46a2ef65162394e5893148dae68fd25e4..363228123108d0572a25fd8456b0fe97de6fa642 100644 (file)
@@ -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 ).  */
index a2655a0ff5215918a7c2ae382ce883bf8e08d7ec..6ba114c9da3350ef8cede7ee123ae95a6cee2611 100644 (file)
@@ -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;
 
index 33d715edaec19e0fd239a4f5be87183dc87fedd6..df698d51dfe3660ffdfa13f63ae3e2a991e66ef7 100644 (file)
@@ -9827,13 +9827,53 @@ init_cp_semantics (void)
 {
 }
 \f
+
+/* 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 artificial (bool) cast.  */
+                   (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;
     }
 }
 \f
index 85726af9f018d0569735e07bf41ee6e48de9951a..679a5177f64a554bbd27e064c2235b717fe36d3d 100644 (file)
@@ -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 (file)
index 0000000..fecf3cc
--- /dev/null
@@ -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 (file)
index 0000000..542697f
--- /dev/null
@@ -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 "" } */
+}
index bab2751a5ce7711e5a90e364b011ea5fb31e384f..67a235f90953ddd23bde789219e35467ee78cb62 100644 (file)
@@ -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;
 }