Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint/Debugger complains about using "console" when it is not appropriate #9500

Closed
drbrain opened this issue Feb 10, 2021 · 2 comments · Fixed by #9504
Closed

Lint/Debugger complains about using "console" when it is not appropriate #9500

drbrain opened this issue Feb 10, 2021 · 2 comments · Fixed by #9504
Assignees

Comments

@drbrain
Copy link

drbrain commented Feb 10, 2021

If I define and use a console method, attr_reader :console or a minitest/spec let(:console) Lint/Debugger says I should "Remove debugger entry point console".

I have not required a debugger, nor have I added one to a Gemfile, so I cannot be entering a debugger, I'm working with a method or accessor that is appropriately named for my usage and is in-scope. No method_missing magic is involved.

Expected behavior

Lint/Debugger does not complain by default about methods I define that happen collide with a debugging tool I have not even loaded a gem for.

I expect zero lint warnings for the code included below.

Actual behavior

$ cat t.rb
require "stringio"
require "minitest/spec"

describe "bug" do
  let(:console) { StringIO.new }
  let(:thing) { [console] }

  it "does not complain about console" do
    assert thing
  end
end

class T1
  def console
  end

  def test
    console.puts "this is not a debugger"
  end
end

class T2
  attr_reader :console

  def test
    console.puts "this is not a debugger"
  end
end

module T3
  def console
    $stdeout
  end
end

class T4
  include T3

  def test
    console.puts "this is not a debugger"
  end
end

$ rubocop -l t.rb
Inspecting 1 file
W

Offenses:

t.rb:6:18: W: Lint/Debugger: Remove debugger entry point console.
  let(:thing) { [console] }
                 ^^^^^^^
t.rb:18:5: W: Lint/Debugger: Remove debugger entry point console.
    console.puts "this is not a debugger"
    ^^^^^^^
t.rb:26:5: W: Lint/Debugger: Remove debugger entry point console.
    console.puts "this is not a debugger"
    ^^^^^^^
t.rb:40:5: W: Lint/Debugger: Remove debugger entry point console.
    console.puts "this is not a debugger"
    ^^^^^^^

1 file inspected, 4 offenses detected
$ ls -a
.	..	t.rb

Steps to reproduce the problem

Run rubocop -l on test embedded above

RuboCop version

$ rubocop -V
1.9.1 (using Parser 3.0.0.0, rubocop-ast 1.3.0, running on ruby 2.6.6 x86_64-darwin19)
@dvandersluis
Copy link
Member

RuboCop is not currently aware of where a method being called is defined, so your various ways of defining console are clearly false positives, but there isn't an easy way of resolving that.

That being said, when #8929 changed how Lint/Debugger is configured, it went from registering binding.console (for example) to console with any receiver, which is clearly wrong here, so I think we can fix it to use an explicit receiver again. @bbatsov thoughts?

@dvandersluis
Copy link
Member

Actually looks like #7636 fixed this a while ago (for console specifically) and then it too was broken by #8929.

@dvandersluis dvandersluis self-assigned this Feb 10, 2021
dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Feb 10, 2021
…vers for debug methods lead to offenses.

Prior to rubocop#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 (rubocop#7636 and rubocop#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.
bbatsov pushed a commit that referenced this issue Feb 12, 2021
…r 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants