From 6866080ba6beea0769a3490f7ab8fe3f47446665 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 23 Apr 2022 15:12:57 +0900 Subject: [PATCH] Fix a false negative for `Rails/TransactionExitStatement` Follow up https://github.com/rubocop/rubocop-rails/pull/674#issuecomment-1107330877 Revert "[Fix #669] Fix a false positive for `Rails/TransactionExitStatement`" This reverts commit d9ec02de75d95337e7dc2a8f876ba8a4874276c1 and add an example, tweak a spec. So this PR fixes a false negative for `Rails/TransactionExitStatement` when `return` is used in `rescue`. --- ...false_negative_for_rails_transaction_exit_statement.md | 1 + lib/rubocop/cop/rails/transaction_exit_statement.rb | 8 +------- spec/rubocop/cop/rails/transaction_exit_statement_spec.rb | 5 +++-- 3 files changed, 5 insertions(+), 9 deletions(-) create mode 100644 changelog/fix_a_false_negative_for_rails_transaction_exit_statement.md diff --git a/changelog/fix_a_false_negative_for_rails_transaction_exit_statement.md b/changelog/fix_a_false_negative_for_rails_transaction_exit_statement.md new file mode 100644 index 0000000000..b9e1bec01b --- /dev/null +++ b/changelog/fix_a_false_negative_for_rails_transaction_exit_statement.md @@ -0,0 +1 @@ +* [#696](https://github.com/rubocop/rubocop-rails/pull/696): Fix a false negative for `Rails/TransactionExitStatement` when `return` is used in `rescue`. ([@koic][]) diff --git a/lib/rubocop/cop/rails/transaction_exit_statement.rb b/lib/rubocop/cop/rails/transaction_exit_statement.rb index 7972f9b18b..20c4c8f406 100644 --- a/lib/rubocop/cop/rails/transaction_exit_statement.rb +++ b/lib/rubocop/cop/rails/transaction_exit_statement.rb @@ -66,7 +66,7 @@ def on_send(node) return unless parent.block_type? && parent.body exit_statements(parent.body).each do |statement_node| - next if in_rescue?(statement_node) || nested_block?(statement_node) + next if statement_node.break_type? && nested_block?(statement_node) statement = statement(statement_node) message = format(MSG, statement: statement) @@ -87,13 +87,7 @@ def statement(statement_node) end end - def in_rescue?(statement_node) - statement_node.ancestors.any? { |n| rescue_body_return_node?(n) } - end - def nested_block?(statement_node) - return false unless statement_node.break_type? - !statement_node.ancestors.find(&:block_type?).method?(:transaction) end end diff --git a/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb b/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb index f4bb0e6416..49bd5e74cd 100644 --- a/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb +++ b/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb @@ -76,11 +76,12 @@ RUBY end - it 'does not register an offense when `return` is used in `rescue`' do - expect_no_offenses(<<~RUBY) + it 'registers an offense when `return` is used in `rescue`' do + expect_offense(<<~RUBY) ApplicationRecord.transaction do rescue return do_something + ^^^^^^^^^^^^^^^^^^^ Exit statement `return` is not allowed. Use `raise` (rollback) or `next` (commit). end RUBY end