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

[Fix #7680] Add new Lint/StructNewOverride cop #7699

Merged
merged 6 commits into from Mar 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -12,6 +12,7 @@
* [#7786](https://github.com/rubocop-hq/rubocop/pull/7786): Support Ruby 2.7's pattern match for `Layout/ElseAlignment` cop. ([@koic][])
* [#7784](https://github.com/rubocop-hq/rubocop/pull/7784): Support Ruby 2.7's numbered parameter for `Lint/SafeNavigationChain`. ([@koic][])
* [#7331](https://github.com/rubocop-hq/rubocop/issues/7331): Add `forbidden` option to `Style/ModuleFunction` cop. ([@weh][])
* [#7699](https://github.com/rubocop-hq/rubocop/pull/7699): Add new `Lint/StructNewOverride` cop. ([@ybiquitous][])

### Bug fixes

Expand Down
5 changes: 5 additions & 0 deletions config/default.yml
Expand Up @@ -1714,6 +1714,11 @@ Lint/ShadowingOuterLocalVariable:
Enabled: true
VersionAdded: '0.9'

Lint/StructNewOverride:
Description: 'Disallow overriding the `Struct` built-in methods via `Struct.new`.'
Enabled: pending
VersionAdded: '0.81'

Lint/SuppressedException:
Description: "Don't suppress exceptions."
StyleGuide: '#dont-hide-exceptions'
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -348,6 +348,7 @@
require_relative 'rubocop/cop/lint/shadowed_argument'
require_relative 'rubocop/cop/lint/shadowed_exception'
require_relative 'rubocop/cop/lint/shadowing_outer_local_variable'
require_relative 'rubocop/cop/lint/struct_new_override'
require_relative 'rubocop/cop/lint/suppressed_exception'
require_relative 'rubocop/cop/lint/syntax'
require_relative 'rubocop/cop/lint/to_json'
Expand Down
58 changes: 58 additions & 0 deletions lib/rubocop/cop/lint/struct_new_override.rb
@@ -0,0 +1,58 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Lint
# This cop checks unexpected overrides of the `Struct` built-in methods
# via `Struct.new`.
#
# @example
# # bad
# Bad = Struct.new(:members, :clone, :count)
# b = Bad.new([], true, 1)
# b.members #=> [] (overriding `Struct#members`)
# b.clone #=> true (overriding `Object#clone`)
# b.count #=> 1 (overriding `Enumerable#count`)
#
# # good
# Good = Struct.new(:id, :name)
# g = Good.new(1, "foo")
# g.members #=> [:id, :name]
# g.clone #=> #<struct Good id=1, name="foo">
# g.count #=> 2
#
class StructNewOverride < Cop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then we can name this StructBuiltInMethodOverride which is more descriptive IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I also think that StructBuiltInMethodOverride is easier to understand than StructNewOverride. 👍
I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this cop detects some overrides by arguments of Struct.new, not normal overrides as follows:

Foo = Struct.new(:name) do
  def to_s
    name.to_s
  end
end

I'm a bit concerned that StructBuiltInMethodOverride may be considered as any overrides of the built-in methods. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, StructBuiltInMethodOverrideByConstructor may be more descriptive but too long. 😓
I may think too much... Please give me any help! 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard! :D Let's keep the shorter name and down the road we can maybe extend this cop to cover dangerous overrides by explicit definitions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. Thanks! 👍

MSG = '`%<member_name>s` member overrides `Struct#%<method_name>s`' \
' and it may be unexpected.'

STRUCT_METHOD_NAMES = Struct.instance_methods
STRUCT_MEMBER_NAME_TYPES = %i[sym str].freeze

def_node_matcher :struct_new, <<~PATTERN
(send
(const ${nil? cbase} :Struct) :new ...)
PATTERN

def on_send(node)
return unless struct_new(node) do
node.arguments.each_with_index do |arg, index|
# Ignore if the first argument is a class name
next if index.zero? && arg.str_type?

# Ignore if the argument is not a member name
next unless STRUCT_MEMBER_NAME_TYPES.include?(arg.type)

member_name = arg.value

next unless STRUCT_METHOD_NAMES.include?(member_name.to_sym)

message = format(MSG, member_name: member_name.inspect,
method_name: member_name.to_s)
add_offense(arg, message: message)
end
end
end
end
end
end
end
1 change: 1 addition & 0 deletions manual/cops.md
Expand Up @@ -243,6 +243,7 @@ In the following section you find all available cops:
* [Lint/ShadowedArgument](cops_lint.md#lintshadowedargument)
* [Lint/ShadowedException](cops_lint.md#lintshadowedexception)
* [Lint/ShadowingOuterLocalVariable](cops_lint.md#lintshadowingouterlocalvariable)
* [Lint/StructNewOverride](cops_lint.md#lintstructnewoverride)
* [Lint/SuppressedException](cops_lint.md#lintsuppressedexception)
* [Lint/Syntax](cops_lint.md#lintsyntax)
* [Lint/ToJSON](cops_lint.md#linttojson)
Expand Down
27 changes: 27 additions & 0 deletions manual/cops_lint.md
Expand Up @@ -2298,6 +2298,33 @@ def some_method
end
```

## Lint/StructNewOverride

Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
--- | --- | --- | --- | ---
Pending | Yes | No | 0.81 | -

This cop checks unexpected overrides of the `Struct` built-in methods
via `Struct.new`.

### Examples

```ruby
# bad
Bad = Struct.new(:members, :clone, :count)
b = Bad.new([], true, 1)
b.members #=> [] (overriding `Struct#members`)
b.clone #=> true (overriding `Object#clone`)
b.count #=> 1 (overriding `Enumerable#count`)

# good
Good = Struct.new(:id, :name)
g = Good.new(1, "foo")
g.members #=> [:id, :name]
g.clone #=> #<struct Good id=1, name="foo">
g.count #=> 2
```

## Lint/SuppressedException

Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
Expand Down
85 changes: 85 additions & 0 deletions spec/rubocop/cop/lint/struct_new_override_spec.rb
@@ -0,0 +1,85 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Lint::StructNewOverride do
subject(:cop) { described_class.new(config) }

let(:config) { RuboCop::Config.new }

it 'registers an offense using `Struct.new(symbol)`' do
expect_offense(<<~RUBY)
Bad = Struct.new(:members)
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `::Struct.new(symbol)`' do
expect_offense(<<~RUBY)
Bad = ::Struct.new(:members)
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `Struct.new(string, ...symbols)`' do
expect_offense(<<~RUBY)
Struct.new('Bad', :members, :name)
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `Struct.new(...symbols)`' do
expect_offense(<<~RUBY)
Bad = Struct.new(:name, :members, :address)
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `Struct.new(symbol, string)`' do
expect_offense(<<~RUBY)
Bad = Struct.new(:name, "members")
^^^^^^^^^ `"members"` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `Struct.new(...)` with a block' do
expect_offense(<<~RUBY)
Struct.new(:members) do
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
def members?
!members.empty?
end
end
RUBY
end

it 'registers an offense using `Struct.new(...)` with multiple overrides' do
expect_offense(<<~RUBY)
Struct.new(:members, :clone, :zip)
^^^^ `:zip` member overrides `Struct#zip` and it may be unexpected.
^^^^^^ `:clone` member overrides `Struct#clone` and it may be unexpected.
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'registers an offense using `Struct.new(...)` with an option argument' do
expect_offense(<<~RUBY)
Struct.new(:members, keyword_init: true)
^^^^^^^^ `:members` member overrides `Struct#members` and it may be unexpected.
RUBY
end

it 'does not register an offense with no overrides' do
expect_no_offenses(<<~RUBY)
Good = Struct.new(:id, :name)
RUBY
end

it 'does not register an offense with an override within a given block' do
expect_no_offenses(<<~RUBY)
Good = Struct.new(:id, :name) do
def members
super.tap { |ret| pp "members: " + ret.to_s }
end
end
RUBY
end
end