From 4d8eb11600f8d13fc74367fc704ba17459733744 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sun, 16 Aug 2020 16:20:34 +0300 Subject: [PATCH] Add new `Lint/UselessMethodDefinition` cop --- CHANGELOG.md | 1 + config/default.yml | 7 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 71 +++++++ lib/rubocop.rb | 1 + .../cop/lint/useless_method_definition.rb | 77 ++++++++ lib/rubocop/formatter/html_formatter.rb | 2 + .../lint/useless_method_definition_spec.rb | 181 ++++++++++++++++++ 8 files changed, 341 insertions(+) create mode 100644 lib/rubocop/cop/lint/useless_method_definition.rb create mode 100644 spec/rubocop/cop/lint/useless_method_definition_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index b0b8aff9efdd..cfea124ef326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#8451](https://github.com/rubocop-hq/rubocop/issues/8451): Add new `Style/RedundantSelfAssignment` cop. ([@fatkodima][]) * [#8474](https://github.com/rubocop-hq/rubocop/pull/8474): Add new `Lint/DuplicateRequire` cop. ([@fatkodima][]) +* [#8472](https://github.com/rubocop-hq/rubocop/issues/8472): Add new `Lint/UselessMethodDefinition` cop. ([@fatkodima][]) * [#8531](https://github.com/rubocop-hq/rubocop/issues/8531): Add new `Lint/EmptyFile` cop. ([@fatkodima][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index c93c6ce9a246..e153b511ff83 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1942,6 +1942,13 @@ Lint/UselessElseWithoutRescue: Enabled: true VersionAdded: '0.17' +Lint/UselessMethodDefinition: + Description: 'Checks for useless method definitions.' + Enabled: pending + VersionAdded: '0.90' + Safe: false + AllowComments: true + Lint/UselessSetterCall: Description: 'Checks for useless setter call to a local variable.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index bc650b12c0d0..97af6d9fab80 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -275,6 +275,7 @@ In the following section you find all available cops: * xref:cops_lint.adoc#lintuselessaccessmodifier[Lint/UselessAccessModifier] * xref:cops_lint.adoc#lintuselessassignment[Lint/UselessAssignment] * xref:cops_lint.adoc#lintuselesselsewithoutrescue[Lint/UselessElseWithoutRescue] +* xref:cops_lint.adoc#lintuselessmethoddefinition[Lint/UselessMethodDefinition] * xref:cops_lint.adoc#lintuselesssettercall[Lint/UselessSetterCall] * xref:cops_lint.adoc#lintvoid[Lint/Void] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 9add2135a0af..88fdc7779b10 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -4558,6 +4558,77 @@ else end ---- +== Lint/UselessMethodDefinition + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| No +| Yes (Unsafe) +| 0.90 +| - +|=== + +This cop checks for useless method definitions, specifically: empty constructors +and methods just delegating to `super`. + +This cop is marked as unsafe as it can trigger false positives for cases when +an empty constructor just overrides the parent constructor, which is bad anyway. + +=== Examples + +[source,ruby] +---- +# bad +def initialize +end + +def method + super +end + +# good +def initialize + initialize_internals +end + +def method + super + do_something_else +end +---- + +==== AllowComments: true (default) + +[source,ruby] +---- +# good +def initialize + # Comment. +end +---- + +==== AllowComments: false + +[source,ruby] +---- +# bad +def initialize + # Comment. +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| AllowComments +| `true` +| Boolean +|=== + == Lint/UselessSetterCall |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 990751f1bb2e..1966245e9e12 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -333,6 +333,7 @@ require_relative 'rubocop/cop/lint/useless_access_modifier' require_relative 'rubocop/cop/lint/useless_assignment' require_relative 'rubocop/cop/lint/useless_else_without_rescue' +require_relative 'rubocop/cop/lint/useless_method_definition' require_relative 'rubocop/cop/lint/useless_setter_call' require_relative 'rubocop/cop/lint/void' diff --git a/lib/rubocop/cop/lint/useless_method_definition.rb b/lib/rubocop/cop/lint/useless_method_definition.rb new file mode 100644 index 000000000000..e38b06e7532d --- /dev/null +++ b/lib/rubocop/cop/lint/useless_method_definition.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # This cop checks for useless method definitions, specifically: empty constructors + # and methods just delegating to `super`. + # + # This cop is marked as unsafe as it can trigger false positives for cases when + # an empty constructor just overrides the parent constructor, which is bad anyway. + # + # @example + # # bad + # def initialize + # end + # + # def method + # super + # end + # + # # good + # def initialize + # initialize_internals + # end + # + # def method + # super + # do_something_else + # end + # + # @example AllowComments: true (default) + # # good + # def initialize + # # Comment. + # end + # + # @example AllowComments: false + # # bad + # def initialize + # # Comment. + # end + # + class UselessMethodDefinition < Base + extend AutoCorrector + + MSG = 'Useless method definition detected.' + + def on_def(node) + return unless (constructor?(node) && empty_constructor?(node)) || + delegating?(node.body, node) + + add_offense(node) { |corrector| corrector.remove(node) } + end + alias on_defs on_def + + private + + def empty_constructor?(node) + return false if node.body + return false if cop_config['AllowComments'] && comment_lines?(node) + + true + end + + def constructor?(node) + node.def_type? && node.method?(:initialize) + end + + def delegating?(node, def_node) + return false unless node&.super_type? || node&.zsuper_type? + + !node.arguments? || node.arguments.map(&:source) == def_node.arguments.map(&:source) + end + end + end + end +end diff --git a/lib/rubocop/formatter/html_formatter.rb b/lib/rubocop/formatter/html_formatter.rb index b79f46218039..151051c18623 100644 --- a/lib/rubocop/formatter/html_formatter.rb +++ b/lib/rubocop/formatter/html_formatter.rb @@ -90,9 +90,11 @@ def initialize(files, summary) end # Make Kernel#binding public. + # rubocop:disable Lint/UselessMethodDefinition def binding super end + # rubocop:enable Lint/UselessMethodDefinition def decorated_message(offense) offense.message.gsub(/`(.+?)`/) do diff --git a/spec/rubocop/cop/lint/useless_method_definition_spec.rb b/spec/rubocop/cop/lint/useless_method_definition_spec.rb new file mode 100644 index 000000000000..77dfd4c90da6 --- /dev/null +++ b/spec/rubocop/cop/lint/useless_method_definition_spec.rb @@ -0,0 +1,181 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Lint::UselessMethodDefinition, :config do + subject(:cop) { described_class.new(config) } + + let(:cop_config) do + { 'AllowComments' => true } + end + + it 'registers an offense and corrects for empty constructor' do + expect_offense(<<~RUBY) + class Foo + def initialize(arg1, arg2) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo + + end + RUBY + end + + it 'does not register an offense for constructor with only comments' do + expect_no_offenses(<<~RUBY) + def initialize(arg) + # Comment. + end + RUBY + end + + it 'does not register an offense for constructor containing additional code to `super`' do + expect_no_offenses(<<~RUBY) + def initialize(arg) + super + do_something + end + RUBY + end + + it 'does not register an offense for empty class level `initialize` method' do + expect_no_offenses(<<~RUBY) + def self.initialize + end + RUBY + end + + it 'registers an offense and corrects for method containing only `super` call' do + expect_offense(<<~RUBY) + class Foo + def useful_instance_method + do_something + end + + def instance_method + ^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super + end + + def instance_method_with_args(arg) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super(arg) + end + + def self.useful_class_method + do_something + end + + def self.class_method + ^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super + end + + def self.class_method_with_args(arg) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super(arg) + end + + class << self + def self.other_useful_class_method + do_something + end + + def other_class_method + ^^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super + end + + def other_class_method_with_args(arg) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Useless method definition detected. + super(arg) + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo + def useful_instance_method + do_something + end + + + + + + def self.useful_class_method + do_something + end + + + + + + class << self + def self.other_useful_class_method + do_something + end + + + + + end + end + RUBY + end + + it 'does not register an offense for method containing additional code to `super`' do + expect_no_offenses(<<~RUBY) + def method + super + do_something + end + RUBY + end + + it 'does not register an offense when `super` arguments differ from method arguments' do + expect_no_offenses(<<~RUBY) + def method1(foo) + super(bar) + end + + def method2(foo, bar) + super(bar, foo) + end + RUBY + end + + it 'does not register an offense when non-constructor contains only comments' do + expect_no_offenses(<<~RUBY) + def non_constructor + # Comment. + end + RUBY + end + + context 'when AllowComments is false' do + let(:cop_config) do + { 'AllowComments' => false } + end + + it 'registers an offense when constructor contains only comments' do + expect_offense(<<~RUBY) + class Foo + def initialize + ^^^^^^^^^^^^^^ Useless method definition detected. + # Comment. + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo + + end + RUBY + end + end +end