Rubocop: Lint/UselessMethodDefinition marks empty `initialize` with arguments as a violation

[andycandrea]

Hi Rubocop team,

I just upgraded to Rubocop 0.90.0 and enabled the Lint/UselessMethodDefinition cop. After running it on my application, I came across a case where I find this cop's current behavior a little misleading: it marks empty initialize methods that accept arguments as a violation even though it's different from the default initialize in that it does accept an argument. I'm guessing the cop only looks at the method's body, not whether it takes arguments.

---

Expected behavior

Lint/UselessMethodDefinition would not mark an empty initialize method that takes arguments as a violation (or would at least offer configuration to mark such cases as valid).

Actual behavior

Lint/UselessMethodDefinition marks empty initialize methods that take arguments as a violation even though they differ from the default initialize by accepting arguments.

Steps to reproduce the problem

echo "class Foo
  def initialize(_unused_arg)
  end
end" > my_rubocop_issue.rb
bundle exec rubocop my_rubocop_issue.rb

RuboCop version

$ bundle exec rubocop -V
0.90.0 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.6.2 x86_64-darwin18)

Thanks!

[jtannas]

I've run into a similar issue using the following code:

class HttpBaseError < ::StandardError
  DEFAULT_MESSAGE = 'A server error has occured'
  def initialize(msg = self.class::DEFAULT_MESSAGE)
    super
  end
end

class BadRequest < HttpBaseError
  DEFAULT_MESSAGE = '400 BadRequest: Your request was not valid - please make corrections before trying again'
end
# etc...

[jaredbeck]

Here's another example, similar to @jtannas above, except with keyword arguments:

class MyClient < Client
  def initialize(token: MY_TOKEN)
    super
  end
end

class Client
  def initialize(options={})
    # .. uses options[:token] ..
  end
end

Offenses:

my_client.rb: W: Lint/UselessMethodDefinition: Useless method definition detected.
    def initialize(token: MY_TOKEN) ...

[marcandre]

@jtannas & @jaredbeck: cases with default arguments (keyword or not) have been fixed by #8659.

I think the case of @andycandrea should not raise an issue (from this cop) as it doesn't call super. If it called super() it also should not raise an issue, only if it called super should it be really useless. @fatkodima what do you think?

[schm]

In our code base, we had two occurrences of this warning, which were incorrect. We're dealing with a lot of duck typing, where we have many classes, that provide the same API, but are used in different contexts. E.g. we have noop classes, that can be used like their regular counter parts but they don't do anything.

class Base
  # not defining `initialize`, therefore depending on Object's default initialize w/o arguments
end

class RegularSub < Base
  def initialize(item)
    super()
    @item = item
  end
end

class NoopSub < Base
  def initialize(_item)
    super()
  end
end

class NoopAgain
  def initialize(_item); end
end

For both NoopSub#initialize and for NoopAgain#initialize, I get a warning. But I need to define initialize, because I want to divert from the default no-args constructor of Object.

Disclaimer: I'm fine with adding a # rubocop:disable tag for both cases in my code base. I just wanted to leave the examples here to provide some more context, if you decide to change the cop's behaviour somehow.

[marcandre]

Right. As I stated, I think the case to be detected are single calls to super (with no parenthesis, no default values) or super(same, list, of, arguments, as, initialize). Others should not warn (like NoopSub#initialize and for NoopAgain#initialize)

Note: you will get an offense from another cop about not calling super at all in NoopAgain#initialize (which I feel is well deserved)

I imagine that @fatkodima will want to tweak this himself, but otherwise I'll do it :-)

[fatkodima]

@marcandre Agreed with your points.

but otherwise I'll do it :-)

Yes, please, do it 😄

[schm]

Thanks for taking the time to look at the example and for providing feedback. It's great if these get changed.

Note: you will get an offense from another cop about not calling super at all in NoopAgain#initialize (which I feel is well deserved)

I don't think so as NoopAgain is inheriting from Object directly, i.e. we're _just_ skipping Object's empty constructor. At least today, I'm not getting any such warning.

[marcandre]

I don't think so as NoopAgain is inheriting from Object directly,

Oh right, I missed that.