diff --git a/changelog/fix_false_negative_for_transaction_exit_with_rescue.md b/changelog/fix_false_negative_for_transaction_exit_with_rescue.md new file mode 100644 index 0000000000..c9ecc3368d --- /dev/null +++ b/changelog/fix_false_negative_for_transaction_exit_with_rescue.md @@ -0,0 +1 @@ +* [#695](https://github.com/rubocop/rubocop-rails/pull/695): Fixes a false negative where the `in_rescue?` check would bypass situations where the return was inside a transaction but outside of a rescue. ([@dorkrawk][]) diff --git a/lib/rubocop/cop/rails/transaction_exit_statement.rb b/lib/rubocop/cop/rails/transaction_exit_statement.rb index 373d9403d9..7972f9b18b 100644 --- a/lib/rubocop/cop/rails/transaction_exit_statement.rb +++ b/lib/rubocop/cop/rails/transaction_exit_statement.rb @@ -53,6 +53,14 @@ class TransactionExitStatement < Base ({return | break | send nil? :throw} ...) PATTERN + def_node_matcher :rescue_body_return_node?, <<~PATTERN + (:resbody ... + ... + ({return | break | send nil? :throw} ...) + ... + ) + PATTERN + def on_send(node) return unless (parent = node.parent) return unless parent.block_type? && parent.body @@ -80,7 +88,7 @@ def statement(statement_node) end def in_rescue?(statement_node) - statement_node.ancestors.find(&:rescue_type?) + statement_node.ancestors.any? { |n| rescue_body_return_node?(n) } end def nested_block?(statement_node) diff --git a/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb b/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb index 953e19a2d2..f4bb0e6416 100644 --- a/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb +++ b/spec/rubocop/cop/rails/transaction_exit_statement_spec.rb @@ -85,6 +85,17 @@ RUBY end + it 'registers an officense when `return` is used outside of a `rescue`' do + expect_offense(<<~RUBY) + ApplicationRecord.transaction do + return if user.active? + ^^^^^^ Exit statement `return` is not allowed. Use `raise` (rollback) or `next` (commit). + rescue + pass + end + RUBY + end + it 'does not register an offense when transaction block is empty' do expect_no_offenses(<<~RUBY) ApplicationRecord.transaction do