From 914221494b7ee1fe735df79111a8aa2cfac1d26c Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Thu, 17 Sep 2020 21:12:01 -0400 Subject: [PATCH] [Fix #8372] Fix `Lint/RedundantCopEnableDirective` autocorrection not removing empty `# rubocop:enable` comments. Added `RuboCop::DirectiveComment` to encapsulate some logic about determining whether a rubocop directive comment should be removed (if the cops in it are all redundant). --- CHANGELOG.md | 1 + lib/rubocop.rb | 1 + lib/rubocop/comment_config.rb | 14 ++- .../lint/redundant_cop_enable_directive.rb | 18 +++- lib/rubocop/directive_comment.rb | 32 ++++++ spec/rubocop/cli/cli_autocorrect_spec.rb | 5 +- .../redundant_cop_enable_directive_spec.rb | 42 +++++++- spec/rubocop/directive_comment_spec.rb | 99 +++++++++++++++++++ 8 files changed, 196 insertions(+), 16 deletions(-) create mode 100644 lib/rubocop/directive_comment.rb create mode 100644 spec/rubocop/directive_comment_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b3d19712ea..a9362795d8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ * [#8750](https://github.com/rubocop-hq/rubocop/pull/8750): Fix an incorrect auto-correct for `Style/MultilineWhenThen` when line break for multiple condidate values of `when` statement. ([@koic][]) * [#8754](https://github.com/rubocop-hq/rubocop/pull/8754): Fix an error for `Style/RandomWithOffset` when using a range with non-integer bounds. ([@eugeneius][]) * [#8756](https://github.com/rubocop-hq/rubocop/issues/8756): Fix an infinite loop error for `Layout/EmptyLinesAroundAccessModifier` with `Layout/EmptyLinesAroundBlockBody` when using access modifier with block argument. ([@koic][]) +* [#8372](https://github.com/rubocop-hq/rubocop/issues/8372): Fix `Lint/RedundantCopEnableDirective` autocorrection to not leave orphaned empty `# rubocop:enable` comments. ([@dvandersluis][]) ### Changes diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 5760952ee2e..a2fb0120853 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -614,6 +614,7 @@ require_relative 'rubocop/config_store' require_relative 'rubocop/config_validator' require_relative 'rubocop/target_finder' +require_relative 'rubocop/directive_comment' require_relative 'rubocop/comment_config' require_relative 'rubocop/magic_comment' require_relative 'rubocop/result_cache' diff --git a/lib/rubocop/comment_config.rb b/lib/rubocop/comment_config.rb index e11ee85afa8..7c285f7a746 100644 --- a/lib/rubocop/comment_config.rb +++ b/lib/rubocop/comment_config.rb @@ -41,12 +41,15 @@ def cop_disabled_line_ranges end def extra_enabled_comments - extra_enabled_comments_with_names([], {}) + extra_enabled_comments_with_names( + extras: Hash.new { |h, k| h[k] = [] }, + names: Hash.new(0) + ) end private - def extra_enabled_comments_with_names(extras, names) + def extra_enabled_comments_with_names(extras:, names:) each_directive do |comment, cop_names, disabled| next unless comment_only_line?(comment.loc.expression.line) @@ -190,18 +193,19 @@ def handle_enable_all(names, extras, comment) enabled_cops += 1 end - extras << [comment, 'all'] if enabled_cops.zero? + extras[comment] << 'all' if enabled_cops.zero? end + # Collect cops that have been disabled or enabled by name in a directive comment + # so that `Lint/RedundantCopEnableDirective` can register offenses correctly. def handle_switch(cop_names, names, disabled, extras, comment) cop_names.each do |name| - names[name] ||= 0 if disabled names[name] += 1 elsif (names[name]).positive? names[name] -= 1 else - extras << [comment, name] + extras[comment] << name end end end diff --git a/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb b/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb index 41ba0a951fe..01f94ee2c37 100644 --- a/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb +++ b/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb @@ -45,18 +45,28 @@ def on_new_investigation return if processed_source.blank? offenses = processed_source.comment_config.extra_enabled_comments - offenses.each do |comment, name| + offenses.each { |comment, cop_names| register_offense(comment, cop_names) } + end + + private + + def register_offense(comment, cop_names) + directive = DirectiveComment.new(comment) + + cop_names.each do |name| add_offense( range_of_offense(comment, name), message: format(MSG, cop: all_or_name(name)) ) do |corrector| - corrector.remove(range_with_comma(comment, name)) + if directive.match?(cop_names) + corrector.remove(range_with_surrounding_space(range: directive.range, side: :right)) + else + corrector.remove(range_with_comma(comment, name)) + end end end end - private - def range_of_offense(comment, name) start_pos = comment_start(comment) + cop_name_indention(comment, name) range_between(start_pos, start_pos + name.size) diff --git a/lib/rubocop/directive_comment.rb b/lib/rubocop/directive_comment.rb new file mode 100644 index 00000000000..3ff72566de6 --- /dev/null +++ b/lib/rubocop/directive_comment.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module RuboCop + # This class wraps the `Parser::Source::Comment` object that represents a + # special `rubocop:disable` and `rubocop:enable` comment and exposes what + # cops it contains. + class DirectiveComment + attr_reader :comment + + def initialize(comment) + @comment = comment + end + + # Return all the cops specified in the directive + def cops + match = comment.text.match(CommentConfig::COMMENT_DIRECTIVE_REGEXP) + return unless match + + cops_string = match.captures[1] + cops_string.split(/,\s*/).uniq.sort + end + + # Checks if this directive contains all the given cop names + def match?(cop_names) + cops == cop_names.uniq.sort + end + + def range + comment.location.expression + end + end +end diff --git a/spec/rubocop/cli/cli_autocorrect_spec.rb b/spec/rubocop/cli/cli_autocorrect_spec.rb index 514839eb11e..569e172b03e 100644 --- a/spec/rubocop/cli/cli_autocorrect_spec.rb +++ b/spec/rubocop/cli/cli_autocorrect_spec.rb @@ -677,13 +677,12 @@ def func C: 2: 1: [Corrected] Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments. C: 3: 1: Style/Documentation: Missing top-level class documentation comment. W: 4: 3: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Metrics/MethodLength. - C: 5: 1: [Corrected] Layout/EmptyLinesAroundMethodBody: Extra empty line detected at method body beginning. - C: 5: 1: [Corrected] Layout/TrailingWhitespace: Trailing whitespace detected. + C: 5: 3: [Corrected] Layout/IndentationWidth: Use 2 (not 6) spaces for indentation. W: 5: 22: [Corrected] Lint/RedundantCopEnableDirective: Unnecessary enabling of Metrics/MethodLength. W: 7: 54: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Style/For. W: 9: 5: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Style/ClassVars. - 1 file inspected, 9 offenses detected, 8 offenses corrected + 1 file inspected, 8 offenses detected, 7 offenses corrected RESULT corrected = <<~RUBY # frozen_string_literal: true diff --git a/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb b/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb index b076b420ba0..49a9af60eb1 100644 --- a/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb +++ b/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb @@ -12,7 +12,6 @@ expect_correction(<<~RUBY) foo - RUBY end @@ -43,7 +42,6 @@ expect_correction(<<~RUBY) foo - # rubocop:enable bar RUBY end @@ -85,7 +83,6 @@ bar - bar RUBY end @@ -100,7 +97,6 @@ expect_correction(<<~RUBY) foo - RUBY end @@ -179,4 +175,42 @@ RUBY end end + + context 'when all cops are unnecessarily enabled' do + context 'on the same line' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + foo + # rubocop:enable Layout/LineLength, Metrics/AbcSize, Lint/Debugger + ^^^^^^^^^^^^^ Unnecessary enabling of Lint/Debugger. + ^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/AbcSize. + ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Layout/LineLength. + RUBY + + expect_correction(<<~RUBY) + foo + RUBY + end + end + + context 'on separate lines' do + it 'registers an offense and corrects when there is extra white space' do + expect_offense(<<~RUBY) + foo + # rubocop:enable Metrics/ModuleSize + ^^^^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/ModuleSize. + # rubocop:enable Layout/LineLength, Metrics/ClassSize + ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Layout/LineLength. + ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/ClassSize. + # rubocop:enable Metrics/AbcSize, Lint/Debugger + ^^^^^^^^^^^^^ Unnecessary enabling of Lint/Debugger. + ^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/AbcSize. + RUBY + + expect_correction(<<~RUBY) + foo + RUBY + end + end + end end diff --git a/spec/rubocop/directive_comment_spec.rb b/spec/rubocop/directive_comment_spec.rb new file mode 100644 index 00000000000..65e17b8efa5 --- /dev/null +++ b/spec/rubocop/directive_comment_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::DirectiveComment do + subject(:directive_comment) { described_class.new(comment) } + + let(:comment) { instance_double(Parser::Source::Comment, text: text) } + let(:comment_cop_names) { 'all' } + let(:text) { "#rubocop:enable #{comment_cop_names}" } + + describe '#cops' do + subject(:cops) { directive_comment.cops } + + context 'all' do + let(:comment_cop_names) { 'all' } + + it 'returns [all]' do + expect(cops).to eq(%w[all]) + end + end + + context 'single cop' do + let(:comment_cop_names) { 'Metrics/AbcSize' } + + it 'returns [Metrics/AbcSize]' do + expect(cops).to eq(%w[Metrics/AbcSize]) + end + end + + context 'single cop duplicated' do + let(:comment_cop_names) { 'Metrics/AbcSize,Metrics/AbcSize' } + + it 'returns [Metrics/AbcSize]' do + expect(cops).to eq(%w[Metrics/AbcSize]) + end + end + + context 'multiple cops' do + let(:comment_cop_names) { 'Style/Not, Metrics/AbcSize' } + + it 'returns the cops in alphabetical order' do + expect(cops).to eq(%w[Metrics/AbcSize Style/Not]) + end + end + end + + describe '#match?' do + subject(:match) { directive_comment.match?(cop_names) } + + let(:comment_cop_names) { 'Metrics/AbcSize, Metrics/PerceivedComplexity, Style/Not' } + + context 'no comment_cop_names' do + let(:cop_names) { [] } + + it 'returns false' do + expect(match).to eq(false) + end + end + + context 'same cop names as in the comment' do + let(:cop_names) { %w[Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } + + it 'returns true' do + expect(match).to eq(true) + end + end + + context 'same cop names as in the comment in a different order' do + let(:cop_names) { %w[Style/Not Metrics/AbcSize Metrics/PerceivedComplexity] } + + it 'returns true' do + expect(match).to eq(true) + end + end + + context 'subset of names' do + let(:cop_names) { %w[Metrics/AbcSize Style/Not] } + + it 'returns false' do + expect(match).to eq(false) + end + end + + context 'superset of names' do + let(:cop_names) { %w[Lint/Void Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } + + it 'returns false' do + expect(match).to eq(false) + end + end + + context 'duplicate names' do + let(:cop_names) { %w[Metrics/AbcSize Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } + + it 'returns true' do + expect(match).to eq(true) + end + end + end +end