[Fix #120] Fix save-in-conditional checking in Rails/SaveBang

Fixes #120.

The Rails/SaveBang rule emitted incorrect errors about using the return
value of save-like calls in conditional nodes. It did not differentiate
between such calls in the body versus the condition, where only the
latter is necessarily a boolean expression.

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2019-07-20 17:51:45 +0900 using RuboCop version 0.72.0.
+# on 2019-09-13 11:45:19 -0400 using RuboCop version 0.74.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -14,16 +14,16 @@ InternalAffairs/NodeDestructuring:
     - 'lib/rubocop/cop/rails/relative_date_constant.rb'
     - 'lib/rubocop/cop/rails/time_zone.rb'
 
-# Offense count: 9
+# Offense count: 11
 Metrics/AbcSize:
   Max: 17
 
 # Offense count: 4
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 164
+  Max: 169
 
-# Offense count: 23
+# Offense count: 25
 # Configuration parameters: CountComments, ExcludedMethods.
 Metrics/MethodLength:
   Max: 14
@@ -34,7 +34,7 @@ Metrics/MethodLength:
 RSpec/ContextWording:
   Enabled: false
 
-# Offense count: 261
+# Offense count: 277
 # Configuration parameters: Max.
 RSpec/ExampleLength:
   Enabled: false

CHANGELOG.md

@@ -12,6 +12,7 @@
 
 * [#118](https://github.com/rubocop-hq/rubocop-rails/issues/118): Fix an incorrect autocorrect for `Rails/Validation` when attributes are specified with array literal. ([@koic][])
 * [#116](https://github.com/rubocop-hq/rubocop-rails/issues/116): Fix an incorrect autocorrect for `Rails/Presence` when `else` branch of ternary operator is not nil. ([@koic][])
+* [#120](https://github.com/rubocop-hq/rubocop-rails/issues/120): Fix message for `Rails/SaveBang` when the save is in the body of a conditional. ([@jas14][])
 
 ## 2.3.1 (2019-08-26)
 
@@ -85,3 +86,4 @@
 [@prathamesh-sonpatki]: https://github.com/prathamesh-sonpatki
 [@MaximeLaurenty]: https://github.com/MaximeLaurenty
 [@eugeneius]: https://github.com/eugeneius
+[@jas14]: https://github.com/jas14

lib/rubocop/cop/rails/save_bang.rb

@@ -141,7 +141,7 @@ def on_send(node) # rubocop:disable Metrics/CyclomaticComplexity
           return unless persist_method?(node)
           return if return_value_assigned?(node)
           return if implicit_return?(node)
-          return if check_used_in_conditional(node)
+          return if check_used_in_condition_or_compound_boolean(node)
           return if argument?(node)
           return if explicit_return?(node)
 
@@ -211,8 +211,8 @@ def array_parent(node)
           array
         end
 
-        def check_used_in_conditional(node)
-          return false unless conditional?(node)
+        def check_used_in_condition_or_compound_boolean(node)
+          return false unless in_condition_or_compound_boolean?(node)
 
           unless MODIFY_PERSIST_METHODS.include?(node.method_name)
             add_offense_for_node(node, CREATE_CONDITIONAL_MSG)
@@ -221,15 +221,21 @@ def check_used_in_conditional(node)
           true
         end
 
-        def conditional?(node) # rubocop:disable Metrics/CyclomaticComplexity
+        def in_condition_or_compound_boolean?(node)
           node = node.block_node || node
+          parent = node.parent
+          return false unless parent
 
-          condition = node.parent
-          return false unless condition
+          operator_or_single_negative?(parent) ||
+            (conditional?(parent) && node == parent.condition)
+        end
+
+        def operator_or_single_negative?(node)
+          node.or_type? || node.and_type? || single_negative?(node)
+        end
 
-          condition.if_type? || condition.case_type? ||
-            condition.or_type? || condition.and_type? ||
-            single_negative?(condition)
+        def conditional?(parent)
+          parent.if_type? || parent.case_type? || parent.while_type?
         end
 
         def allowed_receiver?(node)

spec/rubocop/cop/rails/save_bang_spec.rb

@@ -223,6 +223,28 @@
       end
     end
 
+    it "when using #{method} in the body of a oneline if" do
+      inspect_source("object.#{method} if false")
+
+      expect(cop.messages)
+        .to eq(["Use `#{method}!` instead of `#{method}` " \
+                'if the return value is not checked.'])
+    end
+
+    it "when using #{method} in the body of an else" do
+      inspect_source(<<~RUBY)
+        if condition
+          puts "true"
+        else
+          object.#{method}
+        end
+      RUBY
+
+      expect(cop.messages)
+        .to eq(["Use `#{method}!` instead of `#{method}` " \
+                'if the return value is not checked.'])
+    end
+
     it "when using #{method} with a bunch of hashes & arrays" do
       inspect_source(<<~RUBY)
         return [{ success: object.#{method} }, true]