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

Style/RedundantInitialize dislikes an empty initialize that changes the arity #10524

Closed
lugray opened this issue Apr 11, 2022 · 4 comments · Fixed by #10527
Closed

Style/RedundantInitialize dislikes an empty initialize that changes the arity #10524

lugray opened this issue Apr 11, 2022 · 4 comments · Fixed by #10527
Labels

Comments

@lugray
Copy link

lugray commented Apr 11, 2022

class A
  def initialize(a)
    @a = a
  end
end

class B < A
  def initialize
  end
end

B.new

Expected behavior

All initialize methods accomplish something, so there should be no violation of Style/RedundantInitialize.

Actual behavior

It considers B#initialize a violation of Style/RedundantInitialize, even though its not redundant. Removing it breaks the final line.

Steps to reproduce the problem

run rubocop with the Style/RedundantInitialize cop enabled on the code sample above.

RuboCop version

1.27.0 (using Parser 3.1.1.0, rubocop-ast 1.17.0, running on ruby 2.6.8 arm64-darwin21)
  - rubocop-performance 1.13.3
@koic koic added the bug label Apr 11, 2022
koic added a commit to koic/rubocop that referenced this issue Apr 11, 2022
Fixes rubocop#10524.

This PR marks `Style/RedundantInitialize` as unsafe because if subclass overrides
`initialize` method with a different arity than superclass.

```ruby
class Base
  def initialize
  end
end

class Derived < Base
  def initialize(arg)
  end
end

Derived.new(arg) # Requires one argument.
```

The same can be said for Module mix-in, which cannot be determined by static analysis.
koic added a commit to koic/rubocop that referenced this issue Apr 11, 2022
Fixes rubocop#10524.

This PR marks `Style/RedundantInitialize` as unsafe because if subclass overrides
`initialize` method with a different arity than superclass.

```ruby
class Base
  def initialize
  end
end

class Derived < Base
  def initialize(arg)
  end
end

Derived.new(arg) # Requires one argument.
```

The same can be said for Module mix-in, which cannot be determined by static analysis.
koic added a commit to koic/rubocop that referenced this issue Apr 11, 2022
Fixes rubocop#10524.

This PR marks `Style/RedundantInitialize` as unsafe because if subclass overrides
`initialize` method with a different arity than superclass.

```ruby
class Base
  def initialize
  end
end

class Derived < Base
  def initialize(arg)
  end
end

Derived.new(arg) # Requires one argument.
```

The same can be said for Module mix-in, which cannot be determined by static analysis.
koic added a commit to koic/rubocop that referenced this issue Apr 11, 2022
Fixes rubocop#10524.

This PR marks `Style/RedundantInitialize` as unsafe because if subclass overrides
`initialize` method with a different arity than superclass.

```ruby
class Base
  def initialize
  end
end

class Derived < Base
  def initialize(arg)
  end
end

Derived.new(arg) # Requires one argument.
```

Therefore, the following is a false positive.

```console
% bundle exec rubocop example.rb --only Style/RedundantInitialize
Inspecting 1 file
C

Offenses:

example.rb:8:3: C: Style/RedundantInitialize: Remove unnecessary empty initialize method.
  def initialize(a) ...
  ^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected
```

The same can be said for Module mix-in, which cannot be determined by static analysis.
bbatsov pushed a commit that referenced this issue Apr 13, 2022
Fixes #10524.

This PR marks `Style/RedundantInitialize` as unsafe because if subclass overrides
`initialize` method with a different arity than superclass.

```ruby
class Base
  def initialize
  end
end

class Derived < Base
  def initialize(arg)
  end
end

Derived.new(arg) # Requires one argument.
```

Therefore, the following is a false positive.

```console
% bundle exec rubocop example.rb --only Style/RedundantInitialize
Inspecting 1 file
C

Offenses:

example.rb:8:3: C: Style/RedundantInitialize: Remove unnecessary empty initialize method.
  def initialize(a) ...
  ^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected
```

The same can be said for Module mix-in, which cannot be determined by static analysis.
@koppen
Copy link

koppen commented Apr 19, 2022

I'd like to add another case here.

We've used null-style object for some operations in our codebase. Granted our use case is probably an edge case and warrants a # rubocop:disable, but conceptually it looks something like this:

class This
  def initialize(stuff)
    @stuff = stuff
  end
end

class This::NullObject
 def initialize(_stuff); end
end

This::NullObject needs to follow the same API as This, removing the initializer is not an option even though it does not change the arity.

koic added a commit to koic/rubocop that referenced this issue Apr 20, 2022
Follow up rubocop#10524 (comment).

This PR add `AllowComments` option to `Style/RedundantInitialize` is true by default.

```ruby
# AllowComments: true (default)

# good
def initialize
  # Overriding to negate superclass `initialize` method.
end

# AllowComments: false

# bad
def initialize
  # Overriding to negate superclass `initialize` method.
end
```

This cop has been reported as false positive and has already been marked as unsafe.
In addition, cases such as the follow up address have been reported. This PR allows
intentional overrides with explicit comments to be allowed by default.
@koic
Copy link
Member

koic commented Apr 20, 2022

This cop has been reported as false positive and has already been marked as unsafe. Additionally, I've opened #10551 that allows intentional overrides with explicit comment to be allowed by default. So you could suppress the warning by replacing # rubocop:disable with an explicit intentional comment.

koic added a commit to koic/rubocop that referenced this issue Apr 20, 2022
Follow up rubocop#10524 (comment).

This PR add `AllowComments` option to `Style/RedundantInitialize` is true by default.

```ruby
# AllowComments: true (default)

# good
def initialize
  # Overriding to negate superclass `initialize` method.
end

# AllowComments: false

# bad
def initialize
  # Overriding to negate superclass `initialize` method.
end
```

This cop has been reported as false positive and has already been marked as unsafe.
In addition, cases such as the follow up address have been reported. This PR allows
intentional overrides with explicit comments to be allowed by default.
bbatsov pushed a commit that referenced this issue Apr 20, 2022
Follow up #10524 (comment).

This PR add `AllowComments` option to `Style/RedundantInitialize` is true by default.

```ruby
# AllowComments: true (default)

# good
def initialize
  # Overriding to negate superclass `initialize` method.
end

# AllowComments: false

# bad
def initialize
  # Overriding to negate superclass `initialize` method.
end
```

This cop has been reported as false positive and has already been marked as unsafe.
In addition, cases such as the follow up address have been reported. This PR allows
intentional overrides with explicit comments to be allowed by default.
@koppen
Copy link

koppen commented Apr 20, 2022

@koic, that's a fine solution. Thank you for your efforts 👍

@koic
Copy link
Member

koic commented Apr 20, 2022

@koppen You are welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants