From b82c20f7a6dd762a27b319175464c38cd16eab43 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 10 Feb 2021 14:31:45 -0500 Subject: [PATCH] [Fix #9500] Update `Lint/Debugger` so that only specific receivers for debug methods lead to offenses. Prior to #8929, `Lint/Debugger` looked for a specific receiver/method pair, but that PR changed it so that any of the `DebuggerReceivers` would apply, *or also* no receiver. This caused regressions (#7636 and #9500, for instance) because method names specified as debugger by this cop would now register offenses when called with no receiver. In order to fix it, I changed the configuration for `Lint/Debugger` a bit, but in a way that should not cause any major incompatibility. The allow list is now specified including its receiver so that the receiver can be checked. Existing configurations that just pass in a method name will be treated as expecting no receiver (there's a slight incompatibility there because previously `foo` would be matched by `Kernel.foo` but I think this is ok (and is resolvable by adding `Kernel.foo` to the configuration). I've also grouped the debugger methods by source (it's often confusing to me where each method comes from), which also allows an end-user to disable a pre-defined group of methods by specifying `Byebug: ~` for example. Finally I've deprecated the `DebuggerReceivers` configuration as it is no longer useful. --- .../fix_update_lintdebugger_so_that_only.md | 1 + config/default.yml | 40 ++- config/obsoletion.yml | 4 + lib/rubocop/cop/lint/debugger.rb | 72 +++- spec/rubocop/cop/lint/debugger_spec.rb | 339 +++++++++++------- 5 files changed, 302 insertions(+), 154 deletions(-) create mode 100644 changelog/fix_update_lintdebugger_so_that_only.md diff --git a/changelog/fix_update_lintdebugger_so_that_only.md b/changelog/fix_update_lintdebugger_so_that_only.md new file mode 100644 index 00000000000..4d97126f291 --- /dev/null +++ b/changelog/fix_update_lintdebugger_so_that_only.md @@ -0,0 +1 @@ +* [#9500](https://github.com/rubocop-hq/rubocop/issues/9500): Update `Lint/Debugger` so that only specific receivers for debug methods lead to offenses. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index 5549a982da0..91c920028e9 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1453,23 +1453,31 @@ Lint/Debugger: Description: 'Check for debugger calls.' Enabled: true VersionAdded: '0.14' - VersionChanged: '0.49' - DebuggerReceivers: - - binding - - Kernel - - Pry + VersionChanged: <> + DebuggerReceivers: [] # deprecated DebuggerMethods: - - debugger - - byebug - - remote_byebug - - pry - - remote_pry - - pry_remote - - console - - rescue - - save_and_open_page - - save_and_open_screenshot - - irb + # Groups are available so that a specific group can be disabled in + # a user's configuration, but are otherwise not significant. + Kernel: + - binding.irb + Byebug: + - byebug + - remote_byebug + - Kernel.byebug + - Kernel.remote_byebug + Capybara: + - save_and_open_page + - save_and_open_screenshot + Pry: + - binding.pry + - binding.remote_pry + - binding.pry_remote + - Pry.rescue + Rails: + - debugger + - Kernel.debugger + WebConsole: + - binding.console Lint/DeprecatedClassMethods: Description: 'Check for deprecated class method calls.' diff --git a/config/obsoletion.yml b/config/obsoletion.yml index 937bc47f3ea..9ecec17e0be 100644 --- a/config/obsoletion.yml +++ b/config/obsoletion.yml @@ -187,6 +187,10 @@ changed_parameters: parameters: ExcludedMethods alternative: IgnoredMethods severity: warning + - cops: Lint/Debugger + parameters: DebuggerReceivers + reason: "`DebuggerReceivers` is no longer necessary, method receivers should be specified in `DebuggerMethods` instead." + severity: warning # Enforced styles that have been removed or replaced changed_enforced_styles: diff --git a/lib/rubocop/cop/lint/debugger.rb b/lib/rubocop/cop/lint/debugger.rb index 4d90a87c4a1..427e6ca7be1 100644 --- a/lib/rubocop/cop/lint/debugger.rb +++ b/lib/rubocop/cop/lint/debugger.rb @@ -3,8 +3,21 @@ module RuboCop module Cop module Lint - # This cop checks for calls to debugger or pry. - # The cop can be configured to define which methods and receivers must be fixed. + # This cop checks for debug calls (such as `debugger` or `binding.pry`) that should + # not be kept for production code. + # + # The cop can be configured using `DebuggerMethods`. By default, a number of gems + # debug entrypoints are configured (`Kernel`, `Byebug`, `Capybara`, `Pry`, `Rails`, + # and `WebConsole`). Additional methods can be added. + # + # Specific default groups can be disabled if necessary: + # + # [source,yaml] + # ---- + # Lint/Debugger: + # WebConsole: ~ + # --- + # # # @example # @@ -33,14 +46,32 @@ module Lint # def some_method # do_something # end + # + # @example DebuggerMethods: [my_debugger] + # + # # bad (ok during development) + # + # def some_method + # my_debugger + # end class Debugger < Base MSG = 'Remove debugger entry point `%s`.' RESTRICT_ON_SEND = [].freeze + def_node_matcher :kernel?, <<~PATTERN + (const {nil? cbase} :Kernel) + PATTERN + + def_node_matcher :valid_receiver?, <<~PATTERN + { + (const {nil? cbase} %1) + (send {nil? #kernel?} %1) + } + PATTERN + def on_send(node) - return unless debugger_method?(node.method_name) - return if !node.receiver.nil? && !debugger_receiver?(node) + return unless debugger_method?(node) add_offense(node) end @@ -51,19 +82,32 @@ def message(node) format(MSG, source: node.source) end - def debugger_method?(name) - cop_config.fetch('DebuggerMethods', []).include?(name.to_s) + def debugger_methods + @debugger_methods ||= begin + config = cop_config.fetch('DebuggerMethods', []) + values = config.is_a?(Array) ? config : config.values.flatten + values.map do |v| + next unless v + + *receiver, method_name = v.split('.') + { + receiver: receiver.empty? ? nil : receiver.join.to_sym, + method_name: method_name.to_sym + } + end.compact + end end - def debugger_receiver?(node) - receiver = case node.receiver - when RuboCop::AST::SendNode - node.receiver.method_name - when RuboCop::AST::ConstNode - node.receiver.const_name - end + def debugger_method?(send_node) + debugger_methods.any? do |method| + next unless method[:method_name] == send_node.method_name - cop_config.fetch('DebuggerReceivers', []).include?(receiver.to_s) + if method[:receiver].nil? + send_node.receiver.nil? + else + valid_receiver?(send_node.receiver, method[:receiver]) + end + end end end end diff --git a/spec/rubocop/cop/lint/debugger_spec.rb b/spec/rubocop/cop/lint/debugger_spec.rb index a5f12d374d5..1fb9cd20ef3 100644 --- a/spec/rubocop/cop/lint/debugger_spec.rb +++ b/spec/rubocop/cop/lint/debugger_spec.rb @@ -1,69 +1,104 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Lint::Debugger, :config do - context 'with configured debugger methods' do + context 'with the DebuggerMethods configuration' do let(:cop_config) do { 'DebuggerMethods' => %w[custom_debugger] } end - it 'does not report an offense for a byebug call' do + it 'does not register an offense for a byebug call' do expect_no_offenses(<<~RUBY) byebug RUBY end - it 'reports an offense for a debugger method call' do + it 'registers an offense for a `custom_debugger` call' do expect_offense(<<~RUBY) custom_debugger ^^^^^^^^^^^^^^^ Remove debugger entry point `custom_debugger`. RUBY end - end - it 'reports an offense for a debugger call' do - expect_offense(<<~RUBY) - debugger - ^^^^^^^^ Remove debugger entry point `debugger`. - RUBY - end + context 'nested custom configurations' do + let(:cop_config) do + { + 'DebuggerMethods' => { + 'Custom' => %w[custom_debugger] + } + } + end - it 'reports an offense for a byebug call' do - expect_offense(<<~RUBY) - byebug - ^^^^^^ Remove debugger entry point `byebug`. - RUBY + it 'registers an offense for a `custom_debugger call' do + expect_offense(<<~RUBY) + custom_debugger + ^^^^^^^^^^^^^^^ Remove debugger entry point `custom_debugger`. + RUBY + end + end end - it 'reports an offense for a pry binding call' do - expect_offense(<<~RUBY) - binding.pry - ^^^^^^^^^^^ Remove debugger entry point `binding.pry`. - RUBY - end + context 'built-in methods' do + it 'registers an offense for a irb binding call' do + expect_offense(<<~RUBY) + binding.irb + ^^^^^^^^^^^ Remove debugger entry point `binding.irb`. + RUBY + end - it 'reports an offense for a remote_pry binding call' do - expect_offense(<<~RUBY) - binding.remote_pry - ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.remote_pry`. - RUBY + it 'registers an offense for a binding.irb with Kernel call' do + expect_offense(<<~RUBY) + Kernel.binding.irb + ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.binding.irb`. + RUBY + end end - it 'reports an offense for a pry_remote binding call' do - expect_offense(<<~RUBY) - binding.pry_remote - ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry_remote`. - RUBY + context 'byebug' do + it 'registers an offense for a byebug call' do + expect_offense(<<~RUBY) + byebug + ^^^^^^ Remove debugger entry point `byebug`. + RUBY + end + + it 'registers an offense for a byebug with an argument call' do + expect_offense(<<~RUBY) + byebug foo + ^^^^^^^^^^ Remove debugger entry point `byebug foo`. + RUBY + end + + it 'registers an offense for a Kernel.byebug call' do + expect_offense(<<~RUBY) + Kernel.byebug + ^^^^^^^^^^^^^ Remove debugger entry point `Kernel.byebug`. + RUBY + end + + it 'registers an offense for a remote_byebug call' do + expect_offense(<<~RUBY) + remote_byebug + ^^^^^^^^^^^^^ Remove debugger entry point `remote_byebug`. + RUBY + end + + it 'registers an offense for a Kernel.remote_byebug call' do + expect_offense(<<~RUBY) + Kernel.remote_byebug + ^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.remote_byebug`. + RUBY + end end - context 'with capybara debug method call' do - it 'reports an offense for save_and_open_page' do + context 'capybara' do + it 'registers an offense for save_and_open_page' do expect_offense(<<~RUBY) save_and_open_page ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `save_and_open_page`. RUBY end - it 'reports an offense for save_and_open_screenshot' do + it 'registers an offense for save_and_open_screenshot' do expect_offense(<<~RUBY) save_and_open_screenshot ^^^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `save_and_open_screenshot`. @@ -71,14 +106,14 @@ end context 'with an argument' do - it 'reports an offense for save_and_open_page' do + it 'registers an offense for save_and_open_page' do expect_offense(<<~RUBY) save_and_open_page foo ^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `save_and_open_page foo`. RUBY end - it 'reports an offense for save_and_open_screenshot' do + it 'registers an offense for save_and_open_screenshot' do expect_offense(<<~RUBY) save_and_open_screenshot foo ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `save_and_open_screenshot foo`. @@ -87,120 +122,176 @@ end end - it 'reports an offense for a debugger with an argument call' do - expect_offense(<<~RUBY) - debugger foo - ^^^^^^^^^^^^ Remove debugger entry point `debugger foo`. - RUBY - end + context 'pry' do + it 'registers an offense for a pry binding call' do + expect_offense(<<~RUBY) + binding.pry + ^^^^^^^^^^^ Remove debugger entry point `binding.pry`. + RUBY + end - it 'reports an offense for a byebug with an argument call' do - expect_offense(<<~RUBY) - byebug foo - ^^^^^^^^^^ Remove debugger entry point `byebug foo`. - RUBY - end + it 'registers an offense for a remote_pry binding call' do + expect_offense(<<~RUBY) + binding.remote_pry + ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.remote_pry`. + RUBY + end - it 'reports an offense for a pry binding with an argument call' do - expect_offense(<<~RUBY) - binding.pry foo - ^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry foo`. - RUBY - end + it 'registers an offense for a pry_remote binding call' do + expect_offense(<<~RUBY) + binding.pry_remote + ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry_remote`. + RUBY + end - it 'reports an offense for a remote_pry binding with an argument call' do - expect_offense(<<~RUBY) - binding.remote_pry foo - ^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.remote_pry foo`. - RUBY - end + it 'registers an offense for a pry binding with an argument call' do + expect_offense(<<~RUBY) + binding.pry foo + ^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry foo`. + RUBY + end - it 'reports an offense for a pry_remote binding with an argument call' do - expect_offense(<<~RUBY) - binding.pry_remote foo - ^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry_remote foo`. - RUBY - end + it 'registers an offense for a remote_pry binding with an argument call' do + expect_offense(<<~RUBY) + binding.remote_pry foo + ^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.remote_pry foo`. + RUBY + end - it 'reports an offense for a remote_byebug call' do - expect_offense(<<~RUBY) - remote_byebug - ^^^^^^^^^^^^^ Remove debugger entry point `remote_byebug`. - RUBY - end + it 'registers an offense for a pry_remote binding with an argument call' do + expect_offense(<<~RUBY) + binding.pry_remote foo + ^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `binding.pry_remote foo`. + RUBY + end - it 'reports an offense for a web console binding call' do - expect_offense(<<~RUBY) - binding.console - ^^^^^^^^^^^^^^^ Remove debugger entry point `binding.console`. - RUBY - end + it 'registers an offense for a binding.pry with Kernel call' do + expect_offense(<<~RUBY) + Kernel.binding.pry + ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.binding.pry`. + RUBY + end - it 'does not report an offense for a non-pry binding' do - expect_no_offenses('binding.pirate') - end + it 'registers an offense for a Pry.rescue call' do + expect_offense(<<~RUBY) + def method + Pry.rescue { puts 1 } + ^^^^^^^^^^ Remove debugger entry point `Pry.rescue`. + ::Pry.rescue { puts 1 } + ^^^^^^^^^^^^ Remove debugger entry point `::Pry.rescue`. + end + RUBY + end - it 'reports an offense for a debugger with Kernel call' do - expect_offense(<<~RUBY) - Kernel.debugger - ^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.debugger`. - RUBY + it 'does not register an offense for a `pry` call without binding' do + expect_no_offenses('pry') + end + + it 'does not register an offense for a `rescue` call without Pry' do + expect_no_offenses(<<~RUBY) + begin + rescue StandardError + end + RUBY + end end - it 'reports an offense for a debugger with ::Kernel call' do - expect_offense(<<~RUBY) - ::Kernel.debugger - ^^^^^^^^^^^^^^^^^ Remove debugger entry point `::Kernel.debugger`. - RUBY + context 'rails' do + it 'registers an offense for a debugger call' do + expect_offense(<<~RUBY) + debugger + ^^^^^^^^ Remove debugger entry point `debugger`. + RUBY + end + + it 'registers an offense for a debugger with an argument call' do + expect_offense(<<~RUBY) + debugger foo + ^^^^^^^^^^^^ Remove debugger entry point `debugger foo`. + RUBY + end + + it 'registers an offense for a debugger with Kernel call' do + expect_offense(<<~RUBY) + Kernel.debugger + ^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.debugger`. + RUBY + end + + it 'registers an offense for a debugger with ::Kernel call' do + expect_offense(<<~RUBY) + ::Kernel.debugger + ^^^^^^^^^^^^^^^^^ Remove debugger entry point `::Kernel.debugger`. + RUBY + end end - it 'reports an offense for a binding.pry with Kernel call' do - expect_offense(<<~RUBY) - Kernel.binding.pry - ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.binding.pry`. - RUBY + context 'web console' do + it 'registers an offense for a `binding.console` call' do + expect_offense(<<~RUBY) + binding.console + ^^^^^^^^^^^^^^^ Remove debugger entry point `binding.console`. + RUBY + end + + it 'does not register an offense for `console` without a receiver' do + expect_no_offenses('console') + end end - it 'reports an offense for save_and_open_page with Kernel' do - expect_offense(<<~RUBY) - Kernel.save_and_open_page - ^^^^^^^^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.save_and_open_page`. - RUBY + it 'does not register an offense for a binding method that is not disallowed' do + expect_no_offenses('binding.pirate') end %w[debugger byebug console pry remote_pry pry_remote irb save_and_open_page save_and_open_screenshot remote_byebug].each do |src| - it "does not report an offense for a #{src} in comments" do - expect_no_offenses("# #{src}") + it "does not register an offense for a #{src} in comments" do + expect_no_offenses(<<~RUBY) + # #{src} + # Kernel.#{src} + RUBY end - it "does not report an offense for a #{src} method" do + it "does not register an offense for a #{src} method" do expect_no_offenses("code.#{src}") end end - it 'reports an offense for a Pry.rescue call' do - expect_offense(<<~RUBY) - def method - Pry.rescue { puts 1 } - ^^^^^^^^^^ Remove debugger entry point `Pry.rescue`. - ::Pry.rescue { puts 1 } - ^^^^^^^^^^^^ Remove debugger entry point `::Pry.rescue`. - end - RUBY - end + context 'when a method group is disabled with nil' do + let!(:old_pry_config) { cur_cop_config['DebuggerMethods']['Pry'] } + + before { cur_cop_config['DebuggerMethods']['Pry'] = nil } - it 'reports an offense for a irb binding call' do - expect_offense(<<~RUBY) - binding.irb - ^^^^^^^^^^^ Remove debugger entry point `binding.irb`. - RUBY + after { cur_cop_config['DebuggerMethods']['Pry'] = old_pry_config } + + it 'does not register an offense for a Pry debugger call' do + expect_no_offenses('binding.pry') + end + + it 'does register an offense for another group' do + expect_offense(<<~RUBY) + binding.irb + ^^^^^^^^^^^ Remove debugger entry point `binding.irb`. + RUBY + end end - it 'reports an offense for a binding.irb with Kernel call' do - expect_offense(<<~RUBY) - Kernel.binding.irb - ^^^^^^^^^^^^^^^^^^ Remove debugger entry point `Kernel.binding.irb`. - RUBY + context 'when a method group is disabled with false' do + let!(:old_pry_config) { cur_cop_config['DebuggerMethods']['Pry'] } + + before { cur_cop_config['DebuggerMethods']['Pry'] = false } + + after { cur_cop_config['DebuggerMethods']['Pry'] = old_pry_config } + + it 'does not register an offense for a Pry debugger call' do + expect_no_offenses('binding.pry') + end + + it 'does register an offense for another group' do + expect_offense(<<~RUBY) + binding.irb + ^^^^^^^^^^^ Remove debugger entry point `binding.irb`. + RUBY + end end end