diff --git a/changelog/new_rails_rescue_from_exceptions_variable_name_cop.md b/changelog/new_rails_rescue_from_exceptions_variable_name_cop.md new file mode 100644 index 0000000000..f0bf179606 --- /dev/null +++ b/changelog/new_rails_rescue_from_exceptions_variable_name_cop.md @@ -0,0 +1 @@ +* [#49](https://github.com/rubocop/rubocop-rails/issues/49): Add new `Rails/RescueFromExceptionsVariableName` cop. ([@anthony-robin][], [@ydakuka][]) diff --git a/config/default.yml b/config/default.yml index 6b2a8b09e4..c5232ecae9 100644 --- a/config/default.yml +++ b/config/default.yml @@ -926,6 +926,12 @@ Rails/RequireDependency: Enabled: false VersionAdded: '2.10' +Rails/RescueFromExceptionsVariableName: + Description: 'Use consistent rescued exceptions variables naming.' + Enabled: 'pending' + VersionAdded: '<>' + PreferredName: e + Rails/ResponseParsedBody: Description: Prefer `response.parsed_body` to custom parsing logic for `response.body`. Enabled: pending diff --git a/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb b/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb new file mode 100644 index 0000000000..fa957dedd8 --- /dev/null +++ b/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb @@ -0,0 +1,193 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Ensures that rescued exception variables are named as expected. + # + # The `PreferredName` config option specifies the required name of the variable. + # Its default is `e`, as referenced from `Naming/RescuedExceptionsVariableName`. + # + # @example PreferredName: e (default) + # # bad + # rescue_from MyException do |exception| + # # do something + # end + # + # # bad + # rescue_from MyException do |_exception| + # # do something + # end + # + # # bad + # rescue_from MyException { |exception| do_something(exception) } + # + # # bad + # rescue_from MyException, with: ->(exception) do + # do_something(exception) + # end + # + # # bad + # rescue_from MyException, with: ->(exception) { do_something(exception) } + # + # + # # good + # rescue_from MyException do |e| + # # do something + # end + # + # # good + # rescue_from MyException do |_e| + # # do something + # end + # + # # good + # rescue_from MyException do |exception, context| + # # do something + # end + # + # # good + # rescue_from MyException { |e| do_something(e) } + # + # # good + # rescue_from MyException, with: ->(e) do + # do_something(e) + # end + # + # # good + # rescue_from MyException, with: ->(e) { do_something(e) } + # + # @example PreferredName: exception + # # bad + # rescue_from MyException do |e| + # # do something + # end + # + # # bad + # rescue_from MyException do |_e| + # # do something + # end + # + # # bad + # rescue_from MyException do |exception, context| + # # do something + # end + # + # # bad + # rescue_from MyException { |e| do_something(e) } + # + # # bad + # rescue_from MyException, with: ->(e) do + # do_something(e) + # end + # + # # bad + # rescue_from MyException, with: ->(e) { do_something(e) } + # + # + # # good + # rescue_from MyException do |exception| + # # do something + # end + # + # # good + # rescue_from MyException do |_exception| + # # do something + # end + # + # # good + # rescue_from MyException { |exception| do_something(exception) } + # + # # good + # rescue_from MyException, with: ->(exception) do + # do_something(exception) + # end + # + # # good + # rescue_from MyException, with: ->(exception) { do_something(exception) } + # + class RescueFromExceptionsVariableName < Base + include ConfigurableEnforcedStyle + extend AutoCorrector + + MSG = 'Use `%s` instead of `%s`.' + RESTRICT_ON_SEND = %i[rescue_from].freeze + + def_node_matcher :rescue_from_block_argument_variable?, <<~PATTERN + (block (send nil? :rescue_from ...) (args (arg $_)) _) + PATTERN + + def_node_matcher :rescue_from_with_lambda_variable?, <<~PATTERN + (send nil? :rescue_from ... (hash <(pair (sym :with) (block _ (args (arg $_)) _))>)) + PATTERN + + def_node_matcher :rescue_from_with_block_variable?, <<~PATTERN + (send nil? :rescue_from ... {(block _ (args (arg $_)) _) (splat (block _ (args (arg $_)) _))}) + PATTERN + + def on_block(node) + rescue_from_block_argument_variable?(node) do |arg_name| + check_offense(node.first_argument, arg_name) + end + end + alias on_numblock on_block + + def on_send(node) + check_rescue_from_variable(node, :rescue_from_with_lambda_variable?) + check_rescue_from_variable(node, :rescue_from_with_block_variable?) + end + + private + + def check_rescue_from_variable(node, matcher) + send(matcher, node) do |arg_name| + arg_node = node.each_descendant(:args).first.children.first + check_offense(arg_node, arg_name) + end + end + + def check_offense(arg_node, arg_name) + preferred_name = preferred_name(arg_name) + return if arg_name.to_s == preferred_name + + range = adjusted_range(arg_node) + preferred, current = format_names(arg_node, arg_name, preferred_name) + message = format(MSG, preferred: preferred, current: current) + + add_offense(range, message: message) do |corrector| + autocorrect(corrector, range, preferred, arg_node, preferred_name) + end + end + + def preferred_name(name) + config_name = cop_config.fetch('PreferredName', 'e') + name.start_with?('_') ? "_#{config_name}" : config_name + end + + def adjusted_range(arg_node) + arg_node.source_range.with( + begin_pos: arg_node.source_range.begin_pos - 1, + end_pos: arg_node.source_range.end_pos + 1 + ) + end + + def format_names(arg_node, arg_name, preferred_name) + if arg_node.parent.parent.lambda? + ["(#{preferred_name})", "(#{arg_name})"] + else + ["|#{preferred_name}|", "|#{arg_name}|"] + end + end + + def autocorrect(corrector, range, preferred, arg_node, preferred_name) + corrector.replace(range, preferred) + parent_block = arg_node.ancestors.find(&:block_type?) + + parent_block.each_descendant(:lvar).each do |lvar_node| + corrector.replace(lvar_node, preferred_name) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index d3b24ec9e9..48fed71b0c 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -107,6 +107,7 @@ require_relative 'rails/render_plain_text' require_relative 'rails/request_referer' require_relative 'rails/require_dependency' +require_relative 'rails/rescue_from_exceptions_variable_name' require_relative 'rails/response_parsed_body' require_relative 'rails/reversible_migration' require_relative 'rails/reversible_migration_method_definition' diff --git a/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb b/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb new file mode 100644 index 0000000000..9bb5da305e --- /dev/null +++ b/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb @@ -0,0 +1,565 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RescueFromExceptionsVariableName, :config do + it 'does not register an offense without variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do + # do something + end + RUBY + end + + context 'with default config' do + context 'when using default variable' do + context 'when using a multiline block' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |e| + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException do |e| + do_something(e) + end + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |_e| + # do something + end + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled do |e| + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |e, context| + puts [e.message, context.message] + end + RUBY + end + end + + context 'when using a single-line block' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |e| do_something(e) } + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException { |e| do_something(e) } + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |_e| do_something } + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled { |_e| do_something } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |e, context| do_something(e, context) } + RUBY + end + end + + context 'when using a multiline lambda' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(_e) do + # do something + end + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(e, context) do + puts [e.message, context.message] + end + RUBY + end + end + + context 'when using a single-line lambda' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(e) { do_something(e) } + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(e) { do_something(e) } + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(_e) { do_something } + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled, with: ->(e) { do_something(e) } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(e, context) { puts [e.message, context.message] } + RUBY + end + end + end + + context 'when using another variable' do + context 'when using a multiline block' do + it 'registers an offense with a single rescued exception' do + expect_offense(<<~RUBY) + rescue_from MyException do |exception| + ^^^^^^^^^^^ Use `|e|` instead of `|exception|`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |e| + do_something(e) + end + RUBY + end + + it 'registers an offense with multiple rescued exceptions' do + expect_offense(<<~RUBY) + rescue_from MyException, MyOtherException do |exception| + ^^^^^^^^^^^ Use `|e|` instead of `|exception|`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, MyOtherException do |e| + do_something(e) + end + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException do |_exception| + ^^^^^^^^^^^^ Use `|_e|` instead of `|_exception|`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |_e| + # do something + end + RUBY + end + + it 'registers an offense using splat operator' do + expect_offense(<<~RUBY) + rescue_from *handled do |exception| + ^^^^^^^^^^^ Use `|e|` instead of `|exception|`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from *handled do |e| + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |exception, context| + puts [exception.message, context.message] + end + RUBY + end + end + + context 'when using a single-line block' do + it 'registers an offense with a single rescued exception' do + expect_offense(<<~RUBY) + rescue_from MyException { |exception| do_something(exception) } + ^^^^^^^^^^^ Use `|e|` instead of `|exception|`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException { |e| do_something(e) } + RUBY + end + + it 'registers an offense with multiple rescued exceptions' do + expect_offense(<<~RUBY) + rescue_from MyException, MyOtherException { |exception| do_something(exception) } + ^^^^^^^^^^^ Use `|e|` instead of `|exception|`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, MyOtherException { |e| do_something(e) } + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException { |_exception| do_something(exception) } + ^^^^^^^^^^^^ Use `|_e|` instead of `|_exception|`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException { |_e| do_something(exception) } + RUBY + end + + it 'registers an offense using splat operator' do + expect_offense(<<~RUBY) + rescue_from *handled { |_exception| do_something(exception) } + ^^^^^^^^^^^^ Use `|_e|` instead of `|_exception|`. + RUBY + + expect_correction(<<~RUBY) + rescue_from *handled { |_e| do_something(exception) } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |exception, context| do_something(exception, context) } + RUBY + end + end + + context 'when using a multiline lambda' do + it 'registers an offense with a single rescued exception' do + expect_offense(<<~RUBY) + rescue_from MyException, with: ->(exception) do + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'registers an offense with multiple rescued exceptions' do + expect_offense(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(exception) do + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException, with: ->(_exception) do + ^^^^^^^^^^^^ Use `(_e)` instead of `(_exception)`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, with: ->(_e) do + # do something + end + RUBY + end + + it 'registers an offense using splat operator' do + expect_offense(<<~RUBY) + rescue_from *handled, with: ->(exception) do + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + do_something(exception) + end + RUBY + + expect_correction(<<~RUBY) + rescue_from *handled, with: ->(e) do + do_something(e) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception, context) do + puts [exception.message, context.message] + end + RUBY + end + end + + context 'when using a single-line lambda' do + it 'registers an offense with a single rescued exception' do + expect_offense(<<~RUBY) + rescue_from MyException, with: ->(exception) { do_something(exception) } + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, with: ->(e) { do_something(e) } + RUBY + end + + it 'registers an offense with multiple rescued exceptions' do + expect_offense(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(exception) { do_something(exception) } + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(e) { do_something(e) } + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException, with: ->(_exception) { do_something } + ^^^^^^^^^^^^ Use `(_e)` instead of `(_exception)`. + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, with: ->(_e) { do_something } + RUBY + end + + it 'registers an offense using splat operator' do + expect_offense(<<~RUBY) + rescue_from *handled, with: ->(exception) { do_something(exception) } + ^^^^^^^^^^^ Use `(e)` instead of `(exception)`. + RUBY + + expect_correction(<<~RUBY) + rescue_from *handled, with: ->(e) { do_something(e) } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception, context) { puts [exception.message, context.message] } + RUBY + end + end + end + end + + context 'with the `PreferredName` setup' do + let(:cop_config) do + { + 'PreferredName' => 'exception' + } + end + + context 'when using a multiline block' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |exception| + do_something(exception) + end + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException do |exception| + do_something(exception) + end + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |_exception| + # do something + end + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled do |exception| + do_something(exception) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |exception, context| + puts [exception.message, context.message] + end + RUBY + end + end + + context 'when using a single-line block' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |exception| do_something(exception) } + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException { |exception| do_something(exception) } + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |_exception| do_something } + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled { |_exception| do_something } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException { |exception, context| do_something(exception, context) } + RUBY + end + end + + context 'when using a multiline lambda' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception) do + do_something(exception) + end + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(exception) do + do_something(exception) + end + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(_exception) do + # do something + end + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled, with: ->(exception) do + do_something(exception) + end + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception, context) do + puts [exception.message, context.message] + end + RUBY + end + end + + context 'when using a single-line lambda' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception) { do_something(exception) } + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException, with: ->(exception) { do_something(exception) } + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(_exception) { do_something } + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled, with: ->(exception) { do_something(exception) } + RUBY + end + + it 'does not register an offense with multiple arguments' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, with: ->(exception, context) { puts [exception.message, context.message] } + RUBY + end + end + end +end