Re: [PATCH] c++: fix -Wparentheses with boolean-like class types

[Patrick Palka <ppalka@redhat.com>]

On Wed, 20 Dec 2023, Jason Merrill wrote:

> On 12/20/23 17:07, Patrick Palka wrote:
> > Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this
> > look OK for trunk if successful?
> > 
> > -- >8 --
> > 
> > Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably
> > warns on the idiom
> > 
> > Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used
> > as truth value [-Wparentheses]
> >      9 |   b = v[i] = true;
> >        |              ^~~~
> > 
> > where v has type std::vector<bool>.  That commit intended to only extend
> > the existing diagnostics so that they happen in a template context as
> > well, but the refactoring of is_assignment_op_expr_p caused us for this
> > particular -Wparentheses warning (from convert_for_assignment) to now
> > consider user-defined operator= instead of just built-in operator=.  And
> > since std::vector<bool> is really a bitset, whose operator[] returns a
> > class type with such a user-defined operator= (taking bool), we now warn.
> > 
> > But arguably "boolish" class types should be treated like ordinary bool
> > as far as the warning is concerned.  To that end this patch relaxes the
> > warning for such types, specifically when the (class) type can be
> > (implicitly) converted to a and assigned from a bool.  This should cover
> > at least implementations of std::vector<bool>::reference.
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (maybe_warn_unparenthesized_assignment): Add
> > 	'nested_p' bool parameter.
> > 	* semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool
> > 	parameter and set it accordingly.
> > 	(class_type_is_boolish_cache): Define.
> > 	(class_type_is_boolish): Define.
> > 	(maybe_warn_unparenthesized_assignment): Add 'nested_p'
> > 	bool parameter.  Relax the warning for nested assignments
> > 	to boolean-like class types.
> > 	(maybe_convert_cond): Pass nested_p=false to
> > 	maybe_warn_unparenthesized_assignment.
> > 	* typeck.cc (convert_for_assignment): Pass nested_p=true to
> > 	maybe_warn_unparenthesized_assignment.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/warn/Wparentheses-34.C: New test.
> > ---
> >   gcc/cp/cp-tree.h                            |   2 +-
> >   gcc/cp/semantics.cc                         | 106 ++++++++++++++++++--
> >   gcc/cp/typeck.cc                            |   2 +-
> >   gcc/testsuite/g++.dg/warn/Wparentheses-34.C |  31 ++++++
> >   4 files changed, 129 insertions(+), 12 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 1979572c365..97065cccf3d 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -7928,7 +7928,7 @@ extern tree lambda_regenerating_args
> > (tree);
> >   extern tree most_general_lambda			(tree);
> >   extern tree finish_omp_target			(location_t, tree,
> > tree, bool);
> >   extern void finish_omp_target_clauses		(location_t, tree,
> > tree *);
> > -extern void maybe_warn_unparenthesized_assignment (tree, tsubst_flags_t);
> > +extern void maybe_warn_unparenthesized_assignment (tree, bool,
> > tsubst_flags_t);
> >   extern tree cp_check_pragma_unroll		(location_t, tree);
> >     /* in tree.cc */
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 64839b1ac87..92acd560fa4 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination)
> >     return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
> >   }
> >   -/* Returns true if T corresponds to an assignment operator expression.
> > */
> > +/* Returns true if T corresponds to an assignment operator expression,
> > +   and sets *LHS to its left-hand-side operand if so.  */
> >     static bool
> > -is_assignment_op_expr_p (tree t)
> > +is_assignment_op_expr_p (tree t, tree *lhs)
> >   {
> >     if (t == NULL_TREE)
> >       return false;
> > @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t)
> >     if (TREE_CODE (t) == MODIFY_EXPR
> >         || (TREE_CODE (t) == MODOP_EXPR
> >   	  && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR))
> > -    return true;
> > +    {
> > +      *lhs = TREE_OPERAND (t, 0);
> > +      return true;
> > +    }
> >       tree call = extract_call_expr (t);
> >     if (call == NULL_TREE
> > @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t)
> >       return false;
> >       tree fndecl = cp_get_callee_fndecl_nofold (call);
> > -  return fndecl != NULL_TREE
> > -    && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
> > -    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
> > +  if (fndecl != NULL_TREE
> > +      && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
> > +      && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR))
> > +    {
> > +      *lhs = get_nth_callarg (call, 0);
> > +      return true;
> > +    }
> > +
> > +  return false;
> >   }
> >   +static GTY((deletable)) hash_map<tree, bool>
> > *class_type_is_boolish_cache;
> > +
> > +/* Return true if the class type TYPE can be converted to and assigned
> > +   from a boolean.  */
> > +
> > +static bool
> > +class_type_is_boolish (tree type)
> > +{
> > +  type = TYPE_MAIN_VARIANT (type);
> > +  if (!COMPLETE_TYPE_P (type))
> > +    return false;
> > +
> > +  if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type))
> > +    return *r;
> > +
> > +  bool has_bool_conversion = false;
> > +  bool has_bool_assignment = false;
> > +
> > +  tree ops = lookup_conversions (type);
> > +  for (; ops; ops = TREE_CHAIN (ops))
> > +    {
> > +      tree op = TREE_VALUE (ops);
> > +      if (!DECL_NONCONVERTING_P (op)
> > +	  && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE)
> > +	{
> > +	  has_bool_conversion = true;
> > +	  break;
> > +	}
> > +    }
> > +
> > +  if (!has_bool_conversion)
> > +    {
> > +      hash_map_safe_put<true> (class_type_is_boolish_cache, type, false);
> > +      return false;
> > +    }
> > +
> > +  ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0,
> > tf_none);
> > +  for (tree op : ovl_range (BASELINK_FUNCTIONS (ops)))
> > +    {
> > +      op = STRIP_TEMPLATE (op);
> > +      if (TREE_CODE (op) != FUNCTION_DECL)
> > +	continue;
> > +      tree parm = DECL_CHAIN (DECL_ARGUMENTS (op));
> > +      tree parm_type = non_reference (TREE_TYPE (parm));
> > +      if (TREE_CODE (parm_type) == BOOLEAN_TYPE)
> > +	{
> > +	  has_bool_assignment = true;
> > +	  break;
> > +	}
> > +    }
> > +
> > +  bool boolish = has_bool_conversion && has_bool_assignment;
> > +  hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish);
> > +  return boolish;
> > +}
> > +
> > +
> >   /* Maybe warn about an unparenthesized 'a = b' (appearing in a
> > -   boolean context where 'a == b' might have been intended).  */
> > +   boolean context where 'a == b' might have been intended).
> > +   NESTED_P is true if T appears as the RHS of another assignment.  */
> >     void
> > -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain)
> > +maybe_warn_unparenthesized_assignment (tree t, bool nested_p,
> > +				       tsubst_flags_t complain)
> >   {
> >     t = STRIP_REFERENCE_REF (t);
> >   +  tree lhs;
> >     if ((complain & tf_warning)
> >         && warn_parentheses
> > -      && is_assignment_op_expr_p (t)
> > +      && is_assignment_op_expr_p (t, &lhs)
> >         /* A parenthesized expression would've had this warning
> >   	 suppressed by finish_parenthesized_expr.  */
> >         && !warning_suppressed_p (t, OPT_Wparentheses))
> >       {
> > +      /* In a = b = c, don't warn if b is a boolean-like class type.  */
> > +      if (nested_p)
> > +	{
> > +	  /* The caller already checked this.  */
> > +	  gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE);
> 
> It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and bool-ish
> type here.  Can we check both in one place or the other?

Sounds good, it's easy enough to check both in
maybe_warn_unparenthesized_assignment.  Like so?  I've opted to preserve the
original slightly stronger form of the check which really checks that
(b = c) has bool type rather than b having bool type, to avoid
introducing new warnings.

- >8 --

Subject: [PATCH] c++: fix -Wparentheses with bool-like class types

Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably
warns on the idiom

Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
    9 |   b = v[i] = true;
      |              ^~~~

where v has type std::vector<bool>.  That commit intended to only extend
the existing diagnostics so that they happen in a template context as
well, but the refactoring of is_assignment_op_expr_p caused us for this
particular -Wparentheses warning (from convert_for_assignment) to now
consider user-defined operator= instead of just built-in operator=.  And
since std::vector<bool> is really a bitset, whose operator[] returns a
class type with such a user-defined operator= (taking bool), we now warn.

But arguably "boolish" class types should be treated like ordinary bool
as far as the warning is concerned.  To that end this patch relaxes the
warning for such types, specifically when the (class) type can be
(implicitly) converted to and assigned from bool.  This criterion at
least covers implementations of std::vector<bool>::reference.

gcc/cp/ChangeLog:

	* cp-tree.h (maybe_warn_unparenthesized_assignment): Add
	'nested_p' bool parameter.
	* semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool
	parameter and set it accordingly.
	(class_type_is_boolish_cache): Define.
	(class_type_is_boolish): Define.
	(maybe_warn_unparenthesized_assignment): Add 'nested_p'
	bool parameter.  Suppress the warning for nested assignments
	to bool and bool-like class types.
	(maybe_convert_cond): Pass nested_p=false to
	maybe_warn_unparenthesized_assignment.
	* typeck.cc (convert_for_assignment): Pass nested_p=true to
	maybe_warn_unparenthesized_assignment.  Remove now redundant
	check for 'rhs' having bool type.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-34.C: New test.
---
 gcc/cp/cp-tree.h                            |   2 +-
 gcc/cp/semantics.cc                         | 107 ++++++++++++++++++--
 gcc/cp/typeck.cc                            |   7 +-
 gcc/testsuite/g++.dg/warn/Wparentheses-34.C |  31 ++++++
 4 files changed, 131 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 1979572c365..97065cccf3d 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7928,7 +7928,7 @@ extern tree lambda_regenerating_args		(tree);
 extern tree most_general_lambda			(tree);
 extern tree finish_omp_target			(location_t, tree, tree, bool);
 extern void finish_omp_target_clauses		(location_t, tree, tree *);
-extern void maybe_warn_unparenthesized_assignment (tree, tsubst_flags_t);
+extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t);
 extern tree cp_check_pragma_unroll		(location_t, tree);
 
 /* in tree.cc */
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 64839b1ac87..6aa1dc46665 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -839,10 +839,11 @@ finish_goto_stmt (tree destination)
   return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
 }
 
-/* Returns true if T corresponds to an assignment operator expression.  */
+/* Returns true if T corresponds to an assignment operator expression,
+   and sets *LHS to its left-hand-side operand if so.  */
 
 static bool
-is_assignment_op_expr_p (tree t)
+is_assignment_op_expr_p (tree t, tree *lhs)
 {
   if (t == NULL_TREE)
     return false;
@@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t)
   if (TREE_CODE (t) == MODIFY_EXPR
       || (TREE_CODE (t) == MODOP_EXPR
 	  && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR))
-    return true;
+    {
+      *lhs = TREE_OPERAND (t, 0);
+      return true;
+    }
 
   tree call = extract_call_expr (t);
   if (call == NULL_TREE
@@ -859,26 +863,108 @@ is_assignment_op_expr_p (tree t)
     return false;
 
   tree fndecl = cp_get_callee_fndecl_nofold (call);
-  return fndecl != NULL_TREE
-    && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
-    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
+  if (fndecl != NULL_TREE
+      && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
+      && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR))
+    {
+      *lhs = get_nth_callarg (call, 0);
+      return true;
+    }
+
+  return false;
 }
 
+static GTY((deletable)) hash_map<tree, bool> *class_type_is_boolish_cache;
+
+/* Return true if the class type TYPE can be converted to and assigned
+   from bool.  */
+
+static bool
+class_type_is_boolish (tree type)
+{
+  type = TYPE_MAIN_VARIANT (type);
+  if (!COMPLETE_TYPE_P (type))
+    return false;
+
+  if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type))
+    return *r;
+
+  bool has_bool_assignment = false;
+  bool has_bool_conversion = false;
+
+  tree ops = lookup_fnfields (type, assign_op_identifier,
+			      /*protect=*/0, tf_none);
+  for (tree op : ovl_range (BASELINK_FUNCTIONS (ops)))
+    {
+      op = STRIP_TEMPLATE (op);
+      if (TREE_CODE (op) != FUNCTION_DECL)
+	continue;
+      tree parm = DECL_CHAIN (DECL_ARGUMENTS (op));
+      tree parm_type = non_reference (TREE_TYPE (parm));
+      if (TREE_CODE (parm_type) == BOOLEAN_TYPE)
+	{
+	  has_bool_assignment = true;
+	  break;
+	}
+    }
+
+  if (!has_bool_assignment)
+    {
+      hash_map_safe_put<true> (class_type_is_boolish_cache, type, false);
+      return false;
+    }
+
+  ops = lookup_conversions (type);
+  for (; ops; ops = TREE_CHAIN (ops))
+    {
+      tree op = TREE_VALUE (ops);
+      if (!DECL_NONCONVERTING_P (op)
+	  && TREE_CODE (DECL_CONV_FN_TYPE (op)) == BOOLEAN_TYPE)
+	{
+	  has_bool_conversion = true;
+	  break;
+	}
+    }
+
+  bool boolish = has_bool_assignment && has_bool_conversion;
+  hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish);
+  return boolish;
+}
+
+
 /* Maybe warn about an unparenthesized 'a = b' (appearing in a
-   boolean context where 'a == b' might have been intended).  */
+   boolean context where 'a == b' might have been intended).
+   NESTED_P is true if T appears as the RHS of another assignment.  */
 
 void
-maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain)
+maybe_warn_unparenthesized_assignment (tree t, bool nested_p,
+				       tsubst_flags_t complain)
 {
   t = STRIP_REFERENCE_REF (t);
 
+  tree lhs;
   if ((complain & tf_warning)
       && warn_parentheses
-      && is_assignment_op_expr_p (t)
+      && is_assignment_op_expr_p (t, &lhs)
       /* A parenthesized expression would've had this warning
 	 suppressed by finish_parenthesized_expr.  */
       && !warning_suppressed_p (t, OPT_Wparentheses))
     {
+      if (nested_p)
+	{
+	  /* In ... = a = b, don't warn if a is a bool or bool-like class.  */
+	  if (TREE_CODE (TREE_TYPE (t)) == BOOLEAN_TYPE)
+	    return;
+
+	  if (TREE_CODE (lhs) == ADDR_EXPR)
+	    lhs = TREE_OPERAND (lhs, 0);
+
+	  tree lhs_type = non_reference (TREE_TYPE (lhs));
+	  if (CLASS_TYPE_P (lhs_type)
+	      && class_type_is_boolish (lhs_type))
+	    return;
+	}
+
       warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses,
 		  "suggest parentheses around assignment used as truth value");
       suppress_warning (t, OPT_Wparentheses);
@@ -903,7 +989,8 @@ maybe_convert_cond (tree cond)
   if (warn_sequence_point && !processing_template_decl)
     verify_sequence_points (cond);
 
-  maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error);
+  maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false,
+					 tf_warning_or_error);
 
   /* Do the conversion.  */
   cond = convert_from_reference (cond);
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index a6e2f4ee7da..2f94dcb7938 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -10377,11 +10377,8 @@ convert_for_assignment (tree type, tree rhs,
 	  }
     }
 
-  /* If -Wparentheses, warn about a = b = c when a has type bool and b
-     does not.  */
-  if (TREE_CODE (type) == BOOLEAN_TYPE
-      && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
-    maybe_warn_unparenthesized_assignment (rhs, complain);
+  if (TREE_CODE (type) == BOOLEAN_TYPE)
+    maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain);
 
   if (complain & tf_warning)
     warn_for_address_of_packed_member (type, rhs);
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C
new file mode 100644
index 00000000000..2100c8a193d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C
@@ -0,0 +1,31 @@
+// Verify our -Wparentheses warning handles "boolish" class types
+// such as std::vector<bool>'s reference type the same as ordinary
+// bool.
+// { dg-additional-options "-Wparentheses" }
+
+#include <vector>
+
+void f(std::vector<bool> v, int i) {
+  bool b;
+  b = v[i] = true;
+  b = v[i] = v[i+1];
+
+  if (v[i] = 42) { }     // { dg-message "parentheses" }
+  if (v[i] = v[i+1]) { } // { dg-message "parentheses" }
+
+  if ((v[i] = 42)) { }
+  if ((v[i] = v[i+1])) { }
+}
+
+template<class>
+void ft(std::vector<bool> v, int i) {
+  bool b;
+  b = v[i] = true;
+  b = v[i] = v[i+1];
+
+  if (v[i] = 42) { }     // { dg-message "parentheses" }
+  if (v[i] = v[i+1]) { } // { dg-message "parentheses" }
+
+  if ((v[i] = 42)) { }
+  if ((v[i] = v[i+1])) { }
+}
-- 
2.43.0.174.g055bb6e996