Reject chained assignment, don't emit [[nodiscard]] on = and ?= (See #368 comments)

Author: hsutter

Diff for: regression-tests/pure2-bugfix-for-discard-precedence.cpp2

@@ -5,12 +5,12 @@ quantity: type = {
     _      = _;
   }
   operator+=: (inout this, that) -> forward quantity = {
-    _ = number += that.number;
+    number += that.number;
     return this;
   }
 }
 
 main: () = {
   x: quantity = (std::in_place, 1729);
-  _ = x += x;
+  x += x;
 }

Diff for: regression-tests/test-results/pure2-bugfix-for-discard-precedence.cpp

@@ -19,7 +19,7 @@ class quantity {
 
 
 #line 7 "pure2-bugfix-for-discard-precedence.cpp2"
-  public: [[nodiscard]] auto operator+=(quantity const& that) -> quantity&;
+  public: auto operator+=(quantity const& that) -> quantity&;
 
   public: quantity(quantity const&) = delete; /* No 'that' constructor, suppress copy */
   public: auto operator=(quantity const&) -> void = delete;
@@ -42,14 +42,14 @@ auto main() -> int;
 
     static_cast<void>(_);
   }
-  [[nodiscard]] auto quantity::operator+=(quantity const& that) -> quantity&{
-    static_cast<void>(number += that.number);
+  auto quantity::operator+=(quantity const& that) -> quantity&{
+    number += that.number;
     return (*this); 
   }
 
 #line 13 "pure2-bugfix-for-discard-precedence.cpp2"
 auto main() -> int{
   quantity x {std::in_place, 1729}; 
-  static_cast<void>(x += std::move(x));
+  x += std::move(x);
 }
 

Diff for: regression-tests/test-results/version

@@ -1,5 +1,5 @@
 
-cppfront compiler v0.2.1   Build 8810:1840
+cppfront compiler v0.2.1   Build 8811:0910
 Copyright(c) Herb Sutter   All rights reserved
 
 SPDX-License-Identifier: CC-BY-NC-ND-4.0

Diff for: source/build.info

@@ -1 +1 @@
-"8810:1840"
+"8811:0910"

Diff for: source/cppfront.cpp

@@ -5459,6 +5459,8 @@ class cppfront
                 //  -- for now there's no opt-out, wait and see whether we actually need one
                 if (
                     func->has_non_void_return_type()
+                    && !func->is_assignment()
+                    && !func->is_compound_assignment()
                     && (
                         printer.get_phase() == printer.phase1_type_defs_func_decls
                         || n.has_initializer()  // so we're printing it in phase 2

Diff for: source/parse.h

@@ -280,6 +280,12 @@ struct binary_expression_node
 
     //  API
     //
+    auto terms_size() const
+        -> int
+    {
+        return std::ssize(terms);
+    }
+
     auto is_identifier() const
         -> bool
     {
@@ -1971,6 +1977,9 @@ struct function_type_node
     auto is_constructor_with_move_that() const
         -> bool;
 
+    auto is_compound_assignment() const
+        -> bool;
+
     auto is_assignment() const
         -> bool;
 
@@ -2844,6 +2853,16 @@ struct declaration_node
         return false;
     }
 
+    auto is_compound_assignment() const
+        -> bool
+    {
+        if (auto func = std::get_if<a_function>(&type)) {
+            return (*func)->is_compound_assignment();
+        }
+        //  else
+        return false;
+    }
+
     auto is_assignment() const
         -> bool
     {
@@ -3298,6 +3317,33 @@ auto function_type_node::is_constructor_with_move_that() const
 }
 
 
+auto function_type_node::is_compound_assignment() const
+    -> bool
+{
+    if (
+        (
+            my_decl->has_name("operator+=")
+            || my_decl->has_name("operator-=")
+            || my_decl->has_name("operator*=")
+            || my_decl->has_name("operator/=")
+            || my_decl->has_name("operator%=")
+            || my_decl->has_name("operator&=")
+            || my_decl->has_name("operator|=")
+            || my_decl->has_name("operator^=")
+            || my_decl->has_name("operator<<=")
+            || my_decl->has_name("operator>>=")
+        )
+        && (*parameters).ssize() > 1
+        && (*parameters)[0]->has_name("this")
+        && (*parameters)[0]->direction() == passing_style::inout
+        )
+    {
+        return true;
+    }
+    return false;
+}
+
+
 auto function_type_node::is_assignment() const
     -> bool
 {
@@ -4555,7 +4601,7 @@ class parser
     {
         if (allow_angle_operators)
         {
-            return binary_expression<assignment_expression_node> (
+            auto ret = binary_expression<assignment_expression_node> (
                 [this](token const& t, token const& next) -> token const* {
                     if (is_assignment_operator(t.type())) {
                         return &t;
@@ -4573,13 +4619,27 @@ class parser
                 },
                 [=,this]{ return logical_or_expression(allow_angle_operators); }
             );
+
+            if (ret && ret->terms_size() > 1) {
+                error("assignment cannot be chained - instead of 'c = b = a;', write 'b = a; c = b;'");
+                return {};
+            }
+
+            return ret;
         }
         else
         {
-            return binary_expression<assignment_expression_node> (
+            auto ret = binary_expression<assignment_expression_node> (
                 [](token const&, token const&) -> token const* { return nullptr; },
                 [=,this]{ return logical_or_expression(allow_angle_operators); }
             );
+
+            if (ret && ret->terms_size() > 1) {
+                error("assignment cannot be chained - instead of 'c = b = a;', write 'b = a; c = b;'");
+                return {};
+            }
+
+            return ret;
         }
     }