From 4a097d6828a47641de96f0deb3711cfe307f2bba Mon Sep 17 00:00:00 2001 From: Aleksandr Lossenko Date: Fri, 27 Sep 2019 17:46:19 +0200 Subject: [PATCH 1/8] cop to forbid enabling or disabling cops within source code --- CHANGELOG.md | 2 + config/default.yml | 6 +++ lib/rubocop.rb | 1 + ...sable_cops_within_source_code_directive.rb | 49 +++++++++++++++++++ manual/cops.md | 1 + manual/cops_style.md | 24 +++++++++ ..._cops_within_source_code_directive_spec.rb | 29 +++++++++++ 7 files changed, 112 insertions(+) create mode 100644 lib/rubocop/cop/style/disable_cops_within_source_code_directive.rb create mode 100644 spec/rubocop/cop/style/disable_cops_within_source_code_directive_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b5d0661ea4..c74292b2a96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * [#7823](https://github.com/rubocop-hq/rubocop/pull/7823): Add `IgnoredMethods` configuration in `Metrics/AbcSize`, `Metrics/CyclomaticComplexity`, and `Metrics/PerceivedComplexity` cops. ([@drenmi][]) * [#7816](https://github.com/rubocop-hq/rubocop/pull/7816): Support Ruby 2.7's numbered parameter for `Style/Lambda`. ([@koic][]) * [#7829](https://github.com/rubocop-hq/rubocop/issues/7829): Fix an error for `Style/OneLineConditional` when one of the branches contains `next` keyword. ([@koic][]) +* [#7384](https://github.com/rubocop-hq/rubocop/pull/7384): Add new `Style/DisableCopsWithinSourceCodeDirective` cop. ([@egze][]) ### Bug fixes @@ -4424,3 +4425,4 @@ [@nikitasakov]: https://github.com/nikitasakov [@dmolesUC]: https://github.com/dmolesUC [@yuritomanek]: https://github.com/yuritomanek +[@egze]: https://github.com/egze diff --git a/config/default.yml b/config/default.yml index e0a4f83a298..3544924dfb8 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2616,6 +2616,12 @@ Style/Dir: Enabled: true VersionAdded: '0.50' +Style/DisableCopsWithinSourceCodeDirective: + Description: >- + Forbids disabling/enabling cops within source code. + Enabled: false + VersionAdded: '0.75' + Style/Documentation: Description: 'Document classes and non-namespace modules.' Enabled: true diff --git a/lib/rubocop.rb b/lib/rubocop.rb index d9f4d8d6706..7809089beec 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -423,6 +423,7 @@ require_relative 'rubocop/cop/style/date_time' require_relative 'rubocop/cop/style/def_with_parentheses' require_relative 'rubocop/cop/style/dir' +require_relative 'rubocop/cop/style/disable_cops_within_source_code_directive' require_relative 'rubocop/cop/style/documentation_method' require_relative 'rubocop/cop/style/documentation' require_relative 'rubocop/cop/style/double_cop_disable_directive' diff --git a/lib/rubocop/cop/style/disable_cops_within_source_code_directive.rb b/lib/rubocop/cop/style/disable_cops_within_source_code_directive.rb new file mode 100644 index 00000000000..55fb434c0a1 --- /dev/null +++ b/lib/rubocop/cop/style/disable_cops_within_source_code_directive.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +# rubocop:disable Lint/RedundantCopDisableDirective + +module RuboCop + module Cop + module Style + # Detects comments to enable/disable RuboCop. + # This is useful if want to make sure that every RuboCop error gets fixed + # and not quickly disabled with a comment. + # + # @example + # # bad + # # rubocop:disable Metrics/AbcSize + # def f + # end + # # rubocop:enable Metrics/AbcSize + # + # # good + # def fixed_method_name_and_no_rubocop_comments + # end + # + class DisableCopsWithinSourceCodeDirective < Cop + # rubocop:enable Lint/RedundantCopDisableDirective + MSG = 'Comment to disable/enable RuboCop.' + + def investigate(processed_source) + processed_source.comments.each do |comment| + next unless rubocop_directive_comment?(comment) + + add_offense(comment) + end + end + + def autocorrect(comment) + lambda do |corrector| + corrector.replace(comment.loc.expression, '') + end + end + + private + + def rubocop_directive_comment?(comment) + comment.text =~ CommentConfig::COMMENT_DIRECTIVE_REGEXP + end + end + end + end +end diff --git a/manual/cops.md b/manual/cops.md index ae10d5af5d6..aafcfc2968e 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -334,6 +334,7 @@ In the following section you find all available cops: * [Style/DateTime](cops_style.md#styledatetime) * [Style/DefWithParentheses](cops_style.md#styledefwithparentheses) * [Style/Dir](cops_style.md#styledir) +* [Style/DisableCopsWithinSourceCodeDirective](cops_style.md#styledisablecopswithinsourcecodedirective) * [Style/Documentation](cops_style.md#styledocumentation) * [Style/DocumentationMethod](cops_style.md#styledocumentationmethod) * [Style/DoubleCopDisableDirective](cops_style.md#styledoublecopdisabledirective) diff --git a/manual/cops_style.md b/manual/cops_style.md index ea135748be3..856d78d29c8 100644 --- a/manual/cops_style.md +++ b/manual/cops_style.md @@ -1334,6 +1334,30 @@ path = File.dirname(File.realpath(__FILE__)) path = __dir__ ``` +## Style/DisableCopsWithinSourceCodeDirective + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Disabled | Yes | Yes | 0.75 | - + +Detects comments to enable/disable RuboCop. +This is useful if want to make sure that every RuboCop error gets fixed +and not quickly disabled with a comment. + +### Examples + +```ruby +# bad +# rubocop:disable Metrics/AbcSize +def f +end +# rubocop:enable Metrics/AbcSize + +# good +def fixed_method_name_and_no_rubocop_comments +end +``` + ## Style/Documentation Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/style/disable_cops_within_source_code_directive_spec.rb b/spec/rubocop/cop/style/disable_cops_within_source_code_directive_spec.rb new file mode 100644 index 00000000000..19eb00b4326 --- /dev/null +++ b/spec/rubocop/cop/style/disable_cops_within_source_code_directive_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::DisableCopsWithinSourceCodeDirective do + subject(:cop) { described_class.new } + + it 'registers an offense for disabled cop within source code' do + expect_offense(<<~RUBY) + def choose_move(who_to_move)# rubocop:disable Metrics/CyclomaticComplexity + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Comment to disable/enable RuboCop. + end + RUBY + expect_correction(<<~RUBY) + def choose_move(who_to_move) + end + RUBY + end + + it 'registers an offense for enabled cop within source code' do + expect_offense(<<~RUBY) + def choose_move(who_to_move)# rubocop:enable Metrics/CyclomaticComplexity + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Comment to disable/enable RuboCop. + end + RUBY + expect_correction(<<~RUBY) + def choose_move(who_to_move) + end + RUBY + end +end From 8167c384ca5f326289cdc636cdc67dfff1be2f59 Mon Sep 17 00:00:00 2001 From: Aleksandr Lossenko Date: Sun, 29 Mar 2020 19:33:41 +0200 Subject: [PATCH 2/8] remove DisabledLinesFormatter --- lib/rubocop.rb | 1 - .../formatter/disabled_lines_formatter.rb | 57 ------------- lib/rubocop/formatter/formatter_set.rb | 1 - .../disabled_lines_formatter_spec.rb | 83 ------------------- spec/rubocop/options_spec.rb | 1 - 5 files changed, 143 deletions(-) delete mode 100644 lib/rubocop/formatter/disabled_lines_formatter.rb delete mode 100644 spec/rubocop/formatter/disabled_lines_formatter_spec.rb diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 7809089beec..60717410ba9 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -586,7 +586,6 @@ # relies on simple text require_relative 'rubocop/formatter/clang_style_formatter' require_relative 'rubocop/formatter/disabled_config_formatter' -require_relative 'rubocop/formatter/disabled_lines_formatter' require_relative 'rubocop/formatter/emacs_style_formatter' require_relative 'rubocop/formatter/file_list_formatter' require_relative 'rubocop/formatter/fuubar_style_formatter' diff --git a/lib/rubocop/formatter/disabled_lines_formatter.rb b/lib/rubocop/formatter/disabled_lines_formatter.rb deleted file mode 100644 index 5e9e0017247..00000000000 --- a/lib/rubocop/formatter/disabled_lines_formatter.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -module RuboCop - module Formatter - # A basic formatter that displays the lines disabled - # inline comments. - class DisabledLinesFormatter < BaseFormatter - include PathUtil - include Colorizable - - attr_reader :cop_disabled_line_ranges - - def started(_target_files) - @cop_disabled_line_ranges = {} - end - - def file_started(file, options) - return unless options[:cop_disabled_line_ranges] - - @cop_disabled_line_ranges[file] = - options[:cop_disabled_line_ranges] - end - - def finished(_inspected_files) - cops_disabled_in_comments_summary - end - - private - - def cops_disabled_in_comments_summary - summary = "\nCops disabled line ranges:\n\n" - - @cop_disabled_line_ranges.each do |file, disabled_cops| - disabled_cops.each do |cop, line_ranges| - line_ranges.each do |line_range| - file = cyan(smart_path(file)) - summary += "#{file}:#{line_range}: #{cop}\n" - end - end - end - - output.puts summary - end - - def smart_path(path) - # Ideally, we calculate this relative to the project root. - base_dir = Dir.pwd - - if path.start_with? base_dir - relative_path(path, base_dir) - else - path - end - end - end - end -end diff --git a/lib/rubocop/formatter/formatter_set.rb b/lib/rubocop/formatter/formatter_set.rb index 08e83de3357..f85bf17acd3 100644 --- a/lib/rubocop/formatter/formatter_set.rb +++ b/lib/rubocop/formatter/formatter_set.rb @@ -11,7 +11,6 @@ class FormatterSet < Array BUILTIN_FORMATTERS_FOR_KEYS = { '[a]utogenconf' => AutoGenConfigFormatter, '[c]lang' => ClangStyleFormatter, - '[d]isabled' => DisabledLinesFormatter, '[e]macs' => EmacsStyleFormatter, '[fi]les' => FileListFormatter, '[fu]ubar' => FuubarStyleFormatter, diff --git a/spec/rubocop/formatter/disabled_lines_formatter_spec.rb b/spec/rubocop/formatter/disabled_lines_formatter_spec.rb deleted file mode 100644 index d10c01c3349..00000000000 --- a/spec/rubocop/formatter/disabled_lines_formatter_spec.rb +++ /dev/null @@ -1,83 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe RuboCop::Formatter::DisabledLinesFormatter do - subject(:formatter) { described_class.new(output) } - - let(:output) { StringIO.new } - - let(:files) do - %w[lib/rubocop.rb spec/spec_helper.rb exe/rubocop].map do |path| - File.expand_path(path) - end - end - - let(:file_started) do - formatter.file_started(files.first, cop_disabled_line_ranges) - end - - describe '#file_started' do - before { formatter.started(files) } - - context 'when no disable cop comments are detected' do - let(:cop_disabled_line_ranges) { {} } - - it 'does not add to cop_disabled_line_ranges' do - expect { file_started }.not_to( - change(formatter, :cop_disabled_line_ranges) - ) - end - end - - context 'when any disable cop comments are detected' do - let(:cop_disabled_line_ranges) do - { cop_disabled_line_ranges: { 'LineLength' => [1..1] } } - end - - it 'merges the changes into cop_disabled_line_ranges' do - expect { file_started }.to( - change(formatter, :cop_disabled_line_ranges) - ) - end - end - end - - describe '#finished' do - context 'when there disabled cops detected' do - let(:cop_disabled_line_ranges) do - { - cop_disabled_line_ranges: { - 'LineLength' => [1..1], - 'ClassLength' => [1..Float::INFINITY] - } - } - end - let(:offenses) do - [ - RuboCop::Cop::Offense.new(:convention, location, 'Class too long.', - 'ClassLength', :disabled), - RuboCop::Cop::Offense.new(:convention, location, 'Line too long.', - 'LineLength', :uncorrected) - ] - end - let(:location) { OpenStruct.new(line: 3) } - - before do - formatter.started(files) - formatter.file_started('lib/rubocop.rb', cop_disabled_line_ranges) - formatter.file_finished('lib/rubocop.rb', offenses) - end - - it 'lists disabled cops by file' do - formatter.finished(files) - expect(output.string) - .to eq(<<~OUTPUT) - - Cops disabled line ranges: - - lib/rubocop.rb:1..1: LineLength - lib/rubocop.rb:1..Infinity: ClassLength - OUTPUT - end - end - end -end diff --git a/spec/rubocop/options_spec.rb b/spec/rubocop/options_spec.rb index 505367cf633..04f96de1da5 100644 --- a/spec/rubocop/options_spec.rb +++ b/spec/rubocop/options_spec.rb @@ -71,7 +71,6 @@ def abs(path) [p]rogress is used by default [a]utogenconf [c]lang - [d]isabled [e]macs [fi]les [fu]ubar From d44e62e9f42e1d67e5fa599a8e87ec2ee7f11ae4 Mon Sep 17 00:00:00 2001 From: shekhar-patil Date: Wed, 1 Apr 2020 19:54:17 +0530 Subject: [PATCH 3/8] Generate valid code if comment present before closing-brace Add changes in change log file --- CHANGELOG.md | 6 +++ .../multiline_literal_brace_corrector.rb | 22 +++++++++++ spec/rubocop/cli/cli_autocorrect_spec.rb | 38 +++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 426ba4fcda5..f0a51d9b355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## master (unreleased) +### Bug fixes + +* [#7777](https://github.com/rubocop-hq/rubocop/issues/7777): Fix crash for `Layout/MultilineArrayBraceLayout` when comment is present after last element. ([@shekhar-patil][]) +* [#7776](https://github.com/rubocop-hq/rubocop/issues/7776): Fix crash for `Layout/MultilineMethodCallBraceLayout` when comment is present before closing braces. ([@shekhar-patil][]) + ## 0.81.0 (2020-04-01) ### New features @@ -4426,3 +4431,4 @@ [@nikitasakov]: https://github.com/nikitasakov [@dmolesUC]: https://github.com/dmolesUC [@yuritomanek]: https://github.com/yuritomanek +[@shekhar-patil]: https://github.com/shekhar-patil diff --git a/lib/rubocop/cop/correctors/multiline_literal_brace_corrector.rb b/lib/rubocop/cop/correctors/multiline_literal_brace_corrector.rb index 35e8fc7b248..e361e632135 100644 --- a/lib/rubocop/cop/correctors/multiline_literal_brace_corrector.rb +++ b/lib/rubocop/cop/correctors/multiline_literal_brace_corrector.rb @@ -42,8 +42,30 @@ def correct_next_line_brace(corrector) corrector.insert_before( last_element_range_with_trailing_comma(node).end, + closing_brace_content(corrector, node) + ) + end + + def closing_brace_content(corrector, node) + range = range_with_surrounding_space( + range: children(node).last.source_range, + side: :right + ).end.resize(1) + if range.source == '#' + trailing_content_of_closing_brace(corrector, node) + else node.loc.end.source + end + end + + def trailing_content_of_closing_brace(corrector, node) + range = range_between( + node.loc.end.begin_pos, + range_by_whole_lines(node.loc.expression).end.end_pos ) + + corrector.remove(range) + range.source end def last_element_range_with_trailing_comma(node) diff --git a/spec/rubocop/cli/cli_autocorrect_spec.rb b/spec/rubocop/cli/cli_autocorrect_spec.rb index 059402a37a1..c40586cf09b 100644 --- a/spec/rubocop/cli/cli_autocorrect_spec.rb +++ b/spec/rubocop/cli/cli_autocorrect_spec.rb @@ -945,6 +945,44 @@ class Bar RUBY end + it 'correct multiline array brace without crashing' do + create_file('example.rb', <<~RUBY) + hash = { + first_key: [ 'value a', # comment 1 + 'value b' # comment 2 + ], + second_key: 'value c' + } + RUBY + expect(cli.run(['--auto-correct'])).to eq(1) + expect(IO.read('example.rb')) + .to eq(<<~RUBY) + # frozen_string_literal: true + + hash = { + first_key: ['value a', # comment 1 + 'value b'], # comment 2 + second_key: 'value c' + } + RUBY + end + + it 'correct multiline literal brace without crashing' do + create_file('example.rb', <<~RUBY) + super(foo(bar, + 'hash' => {'key' => 'value'} # comment + )) + RUBY + expect(cli.run(['--auto-correct'])).to eq(0) + expect(IO.read('example.rb')) + .to eq(<<~RUBY) + # frozen_string_literal: true + + super(foo(bar, + 'hash' => { 'key' => 'value' })) # comment + RUBY + end + it 'can change block comments and indent them' do create_file('example.rb', <<~RUBY) module Foo From 639dacdcfd050966680475797a0f36d0d2d76ce0 Mon Sep 17 00:00:00 2001 From: Yuji Hanamura Date: Thu, 2 Apr 2020 17:34:11 +0900 Subject: [PATCH 4/8] Fix typo in CHANGELOG and relnotes --- CHANGELOG.md | 2 +- relnotes/v0.81.0.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 426ba4fcda5..012f4730c30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ ### Changes * [#7797](https://github.com/rubocop-hq/rubocop/pull/7797): Allow unicode-display_width dependency version 1.7.0. ([@yuritomanek][]) -* [#7779](https://github.com/rubocop-hq/rubocop/issues/7779): Change `AllowComments` option of `Lint/SuppressedException` to true by default. ([@koic][]) +* [#7805](https://github.com/rubocop-hq/rubocop/pull/7805): Change `AllowComments` option of `Lint/SuppressedException` to true by default. ([@koic][]) * [#7320](https://github.com/rubocop-hq/rubocop/issues/7320): `Naming/MethodName` now flags `attr_reader/attr_writer/attr_accessor/attr`. ([@denys281][]) * [#7813](https://github.com/rubocop-hq/rubocop/issues/7813): **(Breaking)** Remove `Lint/EndInMethod` cop. ([@tejasbubane][]) diff --git a/relnotes/v0.81.0.md b/relnotes/v0.81.0.md index b46763e8e02..975bff1a930 100644 --- a/relnotes/v0.81.0.md +++ b/relnotes/v0.81.0.md @@ -36,7 +36,7 @@ ### Changes * [#7797](https://github.com/rubocop-hq/rubocop/pull/7797): Allow unicode-display_width dependency version 1.7.0. ([@yuritomanek][]) -* [#7779](https://github.com/rubocop-hq/rubocop/issues/7779): Change `AllowComments` option of `Lint/SuppressedException` to true by default. ([@koic][]) +* [#7805](https://github.com/rubocop-hq/rubocop/pull/7805): Change `AllowComments` option of `Lint/SuppressedException` to true by default. ([@koic][]) * [#7320](https://github.com/rubocop-hq/rubocop/issues/7320): `Naming/MethodName` now flags `attr_reader/attr_writer/attr_accessor/attr`. ([@denys281][]) * [#7813](https://github.com/rubocop-hq/rubocop/issues/7813): **(Breaking)** Remove `Lint/EndInMethod` cop. ([@tejasbubane][]) From eda5183d809a04341935b510c5af78296656938d Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 3 Apr 2020 14:06:31 +0900 Subject: [PATCH 5/8] [#7842] Fix a false positive for `Lint/RaiseException` Resolve part of #7842. This PR fixes a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. `Exception` belonging to a namespace is expected to inherit `StandardError`. This PR makes `Lint/RaiseException` aware of the following differences: ```ruby Gem::Exception.new.is_a?(StandardError) # => true Exception.new.is_a?(StandardError) # => false ``` On the other hand, the following case have not been resolved by this PR. ```ruby module Gem def self.foo raise Exception end end Gem.foo #=> Gem::Exception ``` The above case will be resolved separately from this PR. --- CHANGELOG.md | 4 ++++ lib/rubocop/cop/lint/raise_exception.rb | 4 ++-- spec/rubocop/cop/lint/raise_exception_spec.rb | 7 +++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 426ba4fcda5..7f2f31117af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### Bug fixes + +* [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. ([@koic][]) + ## 0.81.0 (2020-04-01) ### New features diff --git a/lib/rubocop/cop/lint/raise_exception.rb b/lib/rubocop/cop/lint/raise_exception.rb index fed736c57e8..e49175198d4 100644 --- a/lib/rubocop/cop/lint/raise_exception.rb +++ b/lib/rubocop/cop/lint/raise_exception.rb @@ -16,12 +16,12 @@ class RaiseException < Cop MSG = 'Use `StandardError` over `Exception`.' def_node_matcher :exception?, <<~PATTERN - (send nil? ${:raise :fail} (const _ :Exception) ... ) + (send nil? ${:raise :fail} (const {cbase nil?} :Exception) ... ) PATTERN def_node_matcher :exception_new_with_message?, <<~PATTERN (send nil? ${:raise :fail} - (send (const _ :Exception) :new ... )) + (send (const {cbase nil?} :Exception) :new ... )) PATTERN def on_send(node) diff --git a/spec/rubocop/cop/lint/raise_exception_spec.rb b/spec/rubocop/cop/lint/raise_exception_spec.rb index 4433e55b9d6..5e3e9a310b4 100644 --- a/spec/rubocop/cop/lint/raise_exception_spec.rb +++ b/spec/rubocop/cop/lint/raise_exception_spec.rb @@ -80,4 +80,11 @@ it 'does not register an offense for `fail` without arguments' do expect_no_offenses('fail') end + + it 'does not register an offense when raising Exception with explicit ' \ + 'namespace' do + expect_no_offenses(<<~RUBY) + raise Foo::Exception + RUBY + end end From 831a99e00e6ea20f441a464dbe3b2c1ee3cb3a61 Mon Sep 17 00:00:00 2001 From: Tejas Bubane Date: Wed, 1 Apr 2020 21:25:27 +0530 Subject: [PATCH 6/8] Fix `Lint/UriRegexp` to register offense with array arguments Closes #7834 --- CHANGELOG.md | 1 + lib/rubocop/cop/lint/uri_regexp.rb | 6 ++-- spec/rubocop/cop/lint/uri_regexp_spec.rb | 36 ++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dce4c7eca0..f97844e9535 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug fixes * [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. ([@koic][]) +* [#7834](https://github.com/rubocop-hq/rubocop/issues/7834): Fix `Lint/UriRegexp` to register offense with array arguments. ([@tejasbubane][]) ## 0.81.0 (2020-04-01) diff --git a/lib/rubocop/cop/lint/uri_regexp.rb b/lib/rubocop/cop/lint/uri_regexp.rb index dbefc8d8579..2c17b7f869b 100644 --- a/lib/rubocop/cop/lint/uri_regexp.rb +++ b/lib/rubocop/cop/lint/uri_regexp.rb @@ -21,7 +21,7 @@ class UriRegexp < Cop def_node_matcher :uri_regexp_with_argument?, <<~PATTERN (send (const ${nil? cbase} :URI) :regexp - (str $_)) + ${(str _) (array ...)}) PATTERN def_node_matcher :uri_regexp_without_argument?, <<~PATTERN @@ -32,7 +32,7 @@ class UriRegexp < Cop def on_send(node) uri_regexp_with_argument?(node) do |double_colon, arg| register_offense( - node, top_level: double_colon ? '::' : '', arg: "('#{arg}')" + node, top_level: double_colon ? '::' : '', arg: "(#{arg.source})" ) end @@ -51,7 +51,7 @@ def autocorrect(node) double_colon, arg = captured_values top_level = double_colon ? '::' : '' - argument = arg ? "('#{arg}')" : '' + argument = arg ? "(#{arg.source})" : '' corrector.replace( node.loc.expression, diff --git a/spec/rubocop/cop/lint/uri_regexp_spec.rb b/spec/rubocop/cop/lint/uri_regexp_spec.rb index 259d5cee5fd..990e4c76c71 100644 --- a/spec/rubocop/cop/lint/uri_regexp_spec.rb +++ b/spec/rubocop/cop/lint/uri_regexp_spec.rb @@ -49,4 +49,40 @@ ::URI::DEFAULT_PARSER.make_regexp RUBY end + + context 'array argument' do + it 'registers an offense and corrects using `URI.regexp` with '\ + 'literal arrays' do + expect_offense(<<~RUBY) + URI.regexp(['http', 'https']) + ^^^^^^ `URI.regexp(['http', 'https'])` is obsolete and should not be used. Instead, use `URI::DEFAULT_PARSER.make_regexp(['http', 'https'])`. + RUBY + + expect_correction(<<~RUBY) + URI::DEFAULT_PARSER.make_regexp(['http', 'https']) + RUBY + end + + it 'registers an offense and corrects using `URI.regexp` with %w arrays' do + expect_offense(<<~RUBY) + URI.regexp(%w[http https]) + ^^^^^^ `URI.regexp(%w[http https])` is obsolete and should not be used. Instead, use `URI::DEFAULT_PARSER.make_regexp(%w[http https])`. + RUBY + + expect_correction(<<~RUBY) + URI::DEFAULT_PARSER.make_regexp(%w[http https]) + RUBY + end + + it 'registers an offense and corrects using `URI.regexp` with %i arrays' do + expect_offense(<<~RUBY) + URI.regexp(%i[http https]) + ^^^^^^ `URI.regexp(%i[http https])` is obsolete and should not be used. Instead, use `URI::DEFAULT_PARSER.make_regexp(%i[http https])`. + RUBY + + expect_correction(<<~RUBY) + URI::DEFAULT_PARSER.make_regexp(%i[http https]) + RUBY + end + end end From 6169ce336342aeae23d1564ddbeebb67fa4e203f Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 2 Apr 2020 18:53:39 +0900 Subject: [PATCH 7/8] [Fix #7841] Fix an error for `Style/TrailingCommaInBlockArgs` cop Fixes #7841. This PR fixes an error for `Style/TrailingCommaInBlockArgs` cop when lambda literal (`->`) has multiple arguments. It will not be checked in the case of lambda literal because lambda literal (`->`) never have a block arguments. ```console # Valid syntax % ruby -ce '-> (foo, bar) { do_something(foo, bar) }' Syntax OK # Syntax error % ruby -ce '-> { |foo| do_something }' -e:1: syntax error, unexpected '|' -> { |foo| do_something } ^ # Also syntax error % ruby -ce '-> (foo, bar,) { do_something(foo, bar) }' -e:1: syntax error, unexpected ')' -> (foo, bar,) { do_something(foo, bar) } ^ -e:1: syntax error, unexpected '}', expecting end-of-input r,) { do_something(foo, bar) } ^ ``` --- CHANGELOG.md | 1 + .../cop/style/trailing_comma_in_block_args.rb | 3 ++ .../trailing_comma_in_block_args_spec.rb | 28 +++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f97844e9535..faf8b748a8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. ([@koic][]) * [#7834](https://github.com/rubocop-hq/rubocop/issues/7834): Fix `Lint/UriRegexp` to register offense with array arguments. ([@tejasbubane][]) +* [#7841](https://github.com/rubocop-hq/rubocop/issues/7841): Fix an error for `Style/TrailingCommaInBlockArgs` when lambda literal (`->`) has multiple arguments. ([@koic][]) ## 0.81.0 (2020-04-01) diff --git a/lib/rubocop/cop/style/trailing_comma_in_block_args.rb b/lib/rubocop/cop/style/trailing_comma_in_block_args.rb index ea0ed46e006..72f12358e2d 100644 --- a/lib/rubocop/cop/style/trailing_comma_in_block_args.rb +++ b/lib/rubocop/cop/style/trailing_comma_in_block_args.rb @@ -44,6 +44,9 @@ class TrailingCommaInBlockArgs < Cop MSG = 'Useless trailing comma present in block arguments.' def on_block(node) + # lambda literal (`->`) never has block arguments. + return if node.send_node.lambda_literal? + return unless useless_trailing_comma?(node) add_offense(node, location: last_comma(node).pos) diff --git a/spec/rubocop/cop/style/trailing_comma_in_block_args_spec.rb b/spec/rubocop/cop/style/trailing_comma_in_block_args_spec.rb index 0d01b242b71..91e79a352fe 100644 --- a/spec/rubocop/cop/style/trailing_comma_in_block_args_spec.rb +++ b/spec/rubocop/cop/style/trailing_comma_in_block_args_spec.rb @@ -139,4 +139,32 @@ RUBY end end + + context 'when `->` has multiple arguments' do + it 'does not registers an offense' do + expect_no_offenses(<<~RUBY) + -> (foo, bar) { do_something(foo, bar) } + RUBY + end + end + + context 'when `lambda` has multiple arguments' do + it 'does not register an offense when more than one argument is ' \ + 'present with no trailing comma' do + expect_no_offenses(<<~RUBY) + lambda { |foo, bar| do_something(foo, bar) } + RUBY + end + + it "registers an offense and corrects when a trailing comma isn't needed" do + expect_offense(<<~RUBY) + lambda { |foo, bar,| do_something(foo, bar) } + ^ Useless trailing comma present in block arguments. + RUBY + + expect_correction(<<~RUBY) + lambda { |foo, bar| do_something(foo, bar) } + RUBY + end + end end From 9e47db3a62b969b61a982d14ab8b92b3f11d155f Mon Sep 17 00:00:00 2001 From: Tomek Urban Date: Sat, 14 Mar 2020 20:45:07 +0100 Subject: [PATCH 8/8] [Fix #6963] Check if iterating through Enumerator created with value returning method --- CHANGELOG.md | 2 + lib/rubocop/ast/node.rb | 25 +++++++++++ lib/rubocop/cop/lint/void.rb | 17 ++++++++ manual/cops_lint.md | 9 ++++ spec/rubocop/cop/lint/void_spec.rb | 69 ++++++++++++++++++++++++++++++ 5 files changed, 122 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index faf8b748a8d..99b640562e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. ([@koic][]) * [#7834](https://github.com/rubocop-hq/rubocop/issues/7834): Fix `Lint/UriRegexp` to register offense with array arguments. ([@tejasbubane][]) * [#7841](https://github.com/rubocop-hq/rubocop/issues/7841): Fix an error for `Style/TrailingCommaInBlockArgs` when lambda literal (`->`) has multiple arguments. ([@koic][]) +* [#6963](https://github.com/rubocop-hq/rubocop/issues/6963): Check if iterating through Enumerator created with value returning method. ([@tomurb][]) ## 0.81.0 (2020-04-01) @@ -4434,3 +4435,4 @@ [@dmolesUC]: https://github.com/dmolesUC [@yuritomanek]: https://github.com/yuritomanek [@egze]: https://github.com/egze +[@tomurb]: https://github.com/tomurb diff --git a/lib/rubocop/ast/node.rb b/lib/rubocop/ast/node.rb index 64b6ddb5049..03249adf6b0 100644 --- a/lib/rubocop/ast/node.rb +++ b/lib/rubocop/ast/node.rb @@ -290,6 +290,18 @@ def source_length ## Destructuring + def_node_matcher :lvasgn_return, <<~PATTERN + `(lvasgn $_ (send _ $_)) + PATTERN + + def_node_matcher :enumerated_method, <<~PATTERN + `(send (send (send _ :array) $_) :each) + PATTERN + + def_node_matcher :enumerated, <<~PATTERN + ^`(send (lvar $_) :each) + PATTERN + def_node_matcher :receiver, <<~PATTERN {(send $_ ...) ({block numblock} (send $_ ...) ...)} PATTERN @@ -356,6 +368,10 @@ def empty_source? {assignment? (send _recv :<< ...)} PATTERN + def enumerating?(method) + enumerating_assigned?(method) || enumerating_chained?(method) + end + def literal? LITERALS.include?(type) end @@ -559,6 +575,15 @@ def visit_descendants(types, &block) private + def enumerating_chained?(method) + method == parent.enumerated_method + end + + def enumerating_assigned?(method) + lvasgn_name, lvasgn_value = parent.parent&.lvasgn_return + lvasgn_value == method && !enumerated.nil? && enumerated == lvasgn_name + end + def visit_ancestors(types) last_node = self diff --git a/lib/rubocop/cop/lint/void.rb b/lib/rubocop/cop/lint/void.rb index fd6f0b60c50..99c22c705d1 100644 --- a/lib/rubocop/cop/lint/void.rb +++ b/lib/rubocop/cop/lint/void.rb @@ -25,6 +25,10 @@ module Lint # do_something(some_array) # end # + # def some_method(some_array) + # some_array.each(&:some_other_method) + # end + # # # good # def some_method # do_something @@ -40,6 +44,11 @@ module Lint # some_array.sort! # do_something(some_array) # end + # + # def some_method(some_array) + # some_array = some_array.map + # some_array.map(&:some_other_method) + # end class Void < Cop OP_MSG = 'Operator `%s` used in void context.' VAR_MSG = 'Variable `%s` used in void context.' @@ -60,6 +69,10 @@ class Void < Cop shuffle slice sort sort_by squeeze strip sub succ swapcase tr tr_s transform_values unicode_normalize uniq upcase].freeze + VALUE_RETURNING_METHODS = %i[collect collect! delete_if drop_while + filter! find_index index keep_if map map! + reject reject! rindex select select! sort + sort! take_while uniq uniq!].freeze def on_block(node) return unless node.body && !node.body.begin_type? @@ -143,6 +156,10 @@ def in_void_context?(node) return false unless parent && parent.children.last == node + return false if VALUE_RETURNING_METHODS.any? do |method| + node.enumerating?(method) + end + VOID_CONTEXT_TYPES.include?(parent.type) && parent.void_context? end end diff --git a/manual/cops_lint.md b/manual/cops_lint.md index a1e3b3f4791..04490ceb503 100644 --- a/manual/cops_lint.md +++ b/manual/cops_lint.md @@ -3044,6 +3044,10 @@ def some_method(some_array) do_something(some_array) end +def some_method(some_array) + some_array.each(&:some_other_method) +end + # good def some_method do_something @@ -3059,6 +3063,11 @@ def some_method(some_array) some_array.sort! do_something(some_array) end + +def some_method(some_array) + some_array = some_array.map + some_array.map(&:some_other_method) +end ``` ### Configurable attributes diff --git a/spec/rubocop/cop/lint/void_spec.rb b/spec/rubocop/cop/lint/void_spec.rb index 7b85bb8a3d6..bcef22427ae 100644 --- a/spec/rubocop/cop/lint/void_spec.rb +++ b/spec/rubocop/cop/lint/void_spec.rb @@ -180,6 +180,75 @@ def foo=(rhs) RUBY end + context 'object is an Enumerator instance' do + context 'Enumerator instance created with #each' do + it 'registers an offense' do + expect_offense(<<~RUBY) + array.each.each { |_item| 42 } + ^^ Literal `42` used in void context. + RUBY + end + end + + context 'Enumerator instance created with #map' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + array.map.each { |_item| 42 } + RUBY + end + end + + context 'Enumerator instance is assigned to variable' do + context 'Enumerator instance created with #each' do + it 'registers an offense' do + expect_offense(<<~RUBY) + enum = array.each + enum.each { |_item| 42 } + ^^ Literal `42` used in void context. + RUBY + end + end + + context 'Enumerator instance created with #map' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + enum = array.map + enum.each { |_item| 42 } + RUBY + end + end + + context 'node is in nested scope' do + context 'Enumerator instance created with #each' do + it 'registers an offense' do + expect_offense(<<~RUBY) + def enum + array.each + end + def abc + enum.each { |_item| 42 } + ^^ Literal `42` used in void context. + end + RUBY + end + end + + context 'Enumerator instance created with #map' do + it "doesn't register an offense" do + expect_no_offenses(<<~RUBY) + def enum + array.map + end + def abc + enum.each { |_item| asdf } + end + RUBY + end + end + end + end + end + it 'handles `#each` block with single expression' do expect_offense(<<~RUBY) array.each do |_item|