From 340a54ec2122809710388d35f75e141841aec153 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Wed, 22 Jan 2020 13:22:33 -0500 Subject: [PATCH] [Fix #190] Fix `Rails/SaveBang` when return value is checked immediately Fixes #190. Rails/SaveBang had a false positive when the return value of a `create` call was checked directly with a call to `persisted?`. --- .rubocop_todo.yml | 12 ++++++------ CHANGELOG.md | 1 + lib/rubocop/cop/rails/save_bang.rb | 12 ++++++++++-- manual/cops_rails.md | 3 ++- spec/rubocop/cop/rails/save_bang_spec.rb | 6 ++++++ 5 files changed, 25 insertions(+), 9 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index a6b08f2b93..87f4e3912f 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2019-09-13 11:45:19 -0400 using RuboCop version 0.74.0. +# on 2020-01-22 14:25:37 -0500 using RuboCop version 0.79.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,27 +14,27 @@ InternalAffairs/NodeDestructuring: - 'lib/rubocop/cop/rails/relative_date_constant.rb' - 'lib/rubocop/cop/rails/time_zone.rb' -# Offense count: 11 +# Offense count: 10 Metrics/AbcSize: Max: 17 # Offense count: 4 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 169 + Max: 173 -# Offense count: 25 +# Offense count: 26 # Configuration parameters: CountComments, ExcludedMethods. Metrics/MethodLength: Max: 14 -# Offense count: 73 +# Offense count: 77 # Configuration parameters: Prefixes. # Prefixes: when, with, without RSpec/ContextWording: Enabled: false -# Offense count: 277 +# Offense count: 287 # Configuration parameters: Max. RSpec/ExampleLength: Enabled: false diff --git a/CHANGELOG.md b/CHANGELOG.md index ae7b45e1e1..5260697106 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#184](https://github.com/rubocop-hq/rubocop-rails/issues/184): Fix `Rake/Environment` to allow task with no block. ([@hanachin][]) * [#122](https://github.com/rubocop-hq/rubocop-rails/issues/122): Fix `Exclude` paths that were not inherited. ([@koic][]) * [#187](https://github.com/rubocop-hq/rubocop-rails/pull/187): Fix an issue that excluded files in rubocop-rails did not work. ([@sinsoku][]) +* [#190](https://github.com/rubocop-hq/rubocop-rails/issues/190): Fix `Rails/SaveBang` when return value is checked immediately. ([@jas14][]) ## 2.4.1 (2019-12-25) diff --git a/lib/rubocop/cop/rails/save_bang.rb b/lib/rubocop/cop/rails/save_bang.rb index 0a46ac99a5..6903866f5a 100644 --- a/lib/rubocop/cop/rails/save_bang.rb +++ b/lib/rubocop/cop/rails/save_bang.rb @@ -12,7 +12,8 @@ module Rails # - update or save calls, assigned to a variable, # or used as a condition in an if/unless/case statement. # - create calls, assigned to a variable that then has a - # call to `persisted?`. + # call to `persisted?`, or whose return value is checked by + # `persisted?` immediately # - calls if the result is explicitly returned from methods and blocks, # or provided as arguments. # - calls whose signature doesn't look like an ActiveRecord @@ -137,16 +138,19 @@ def check_assignment(assignment) add_offense_for_node(node, CREATE_MSG) end - def on_send(node) # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + def on_send(node) return unless persist_method?(node) return if return_value_assigned?(node) return if implicit_return?(node) return if check_used_in_condition_or_compound_boolean(node) return if argument?(node) return if explicit_return?(node) + return if checked_immediately?(node) add_offense_for_node(node) end + # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity alias on_csend on_send def autocorrect(node) @@ -238,6 +242,10 @@ def conditional?(parent) parent.if_type? || parent.case_type? end + def checked_immediately?(node) + node.parent && call_to_persisted?(node.parent) + end + def allowed_receiver?(node) return false unless node.receiver return false unless cop_config['AllowedReceivers'] diff --git a/manual/cops_rails.md b/manual/cops_rails.md index ec7388ffbf..4c30d2f926 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -2193,7 +2193,8 @@ This will allow: - update or save calls, assigned to a variable, or used as a condition in an if/unless/case statement. - create calls, assigned to a variable that then has a - call to `persisted?`. + call to `persisted?`, or whose return value is checked by + `persisted?` immediately - calls if the result is explicitly returned from methods and blocks, or provided as arguments. - calls whose signature doesn't look like an ActiveRecord diff --git a/spec/rubocop/cop/rails/save_bang_spec.rb b/spec/rubocop/cop/rails/save_bang_spec.rb index e27d952b9c..0f2c0ed075 100644 --- a/spec/rubocop/cop/rails/save_bang_spec.rb +++ b/spec/rubocop/cop/rails/save_bang_spec.rb @@ -571,6 +571,12 @@ def whatever RUBY end + it "when using persisted? directly on #{method} return value" do + expect_no_offenses(<<~RUBY) + return unless object.#{method}.persisted? + RUBY + end + it "when using #{method} with `||`" do expect_no_offenses(<<~RUBY) def find_or_create(**opts)