Rubocop: RedundantSelf regression

[printercu]

Hi! I've got this code:

  let(:klass) do
    described_class = self.described_class
    Class.new do
      extend described_class
      # ...
    end
  end

It's valid in 0.42.0, but 0.43.0 and 0.44.1 give an offence.

Expected behavior

Rubocop should accept such code.

Actual behavior

spec/rails_stuff/types_tracker_spec.rb:5:23: C: Style/RedundantSelf: Redundant self detected.
    described_class = self.described_class
                      ^^^^^^^^^^^^^^^^^^^^

Steps to reproduce the problem

Copy example into test.rb, run rubocop test.rb

RuboCop version

$ rubocop -V
0.44.1 (using Parser 2.3.1.4, running on ruby 2.3.0 x86_64-darwin13)

[tejasbubane]

Style/RedundantSelf has a list of ruby methods to skip here which are ruby keywords. described_class is defined by rspec hence not there.

You can use described_class directly as given in the docs. This test passes and obviously does not give the error since it does not use self.

class Foo; end

describe Foo do
  let(:klass) { described_class }

  it 'passes' do
    expect(klass).to eq(Foo)
  end
end

Hope that solves your issue.

[printercu]

@tejasbubane I have several places where I use local variable with same name as instance method. And all of them are triggers warning now. Some places use instance_exec/instance_eval:

# this is very simplified code, just for example
attr_reader :object

def smth
  object = self.object
  subject.instance_exec { do_smth(object) }
end

My example from 1st comment differs from your one. Class.new executes block in the context of new class, and there is no access to (example.)described_class, so I need to copy it to local variable and this variable is visible inside block.

However, this code is valid, and works fine. If I remove self. as rubocop suggests, I'll actually get described_class = nil. So I can assume, that this is false warning. And having this code valid in 0.42.0 I assume that this is regression. Am I not right?

[printercu]

Here is a real example: https://github.com/printercu/rails_stuff/blob/master/lib/rails_stuff/statusable/builder.rb#L46-L49
Down the code there some more places with define_method.

[jonas054]

I located the regression by running git bisect.

4cffe0526a0a062d597d9c8382fc57f39eab81ce is the first bad commit
commit 4cffe0526a0a062d597d9c8382fc57f39eab81ce
Author: Alexandre Ignjatovic <...>
Date:   Wed Aug 31 18:56:36 2016 +0200

    [Fix #3383] Handle properly nested methods in RedundantSelf (#3459)

    That commit adds failing test for issue #3383, and
    fixes the problem by adding scoped local vars
    management.

@bankair Can you take a look?

[bankair]

@jonas054 will do

[lamont-granquist]

it mangled some code that we have which looks like this:

         before { resource_class_a } # pull on it so it gets defined before the recipe runs

         it "two_classes_one_dsl resolves to A (alphabetically earliest)" do
-          two_classes_one_dsl = self.two_classes_one_dsl
+          two_classes_one_dsl = two_classes_one_dsl
           recipe = converge do
             instance_eval("#{two_classes_one_dsl} 'blah'")
           end

It seems like it should at least avoid warning on A = self.A since that is usually intended to setup a local variable scope (and the autofixing to A = A is something nobody ever wants). the fact that you have local variable overwriting an instance method name is normally terrible, but in this case it produces fairly idiomatic code and the identical name is a tipoff that its just a scope issue that is being worked around.

[bankair]

Okay, got it.

Previously, I used to register local variables on assignment scope:

        def on_lvasgn(node)
          lhs, _rhs = *node
          @local_variables_scopes[node] << lhs
        end

But actually, what we need is to bind it to right hand scope:

        def on_lvasgn(node)
          lhs, rhs = *node
          @local_variables_scopes[rhs] << lhs
        end

Added a failing regression test, tried the fix and ran the whole test suite. Seems like it fix the issue without other side effect.

PR to come pretty soon. Thanks for your patience, folks !

[bankair]

PR open: https://github.com/bbatsov/rubocop/pull/4037