From c1032dc332a89434edf75f3c3dddbb567631e82d Mon Sep 17 00:00:00 2001 From: Bozhidar Batsov Date: Tue, 15 Oct 2024 12:16:17 +0200 Subject: [PATCH 01/49] Reset the docs version --- docs/antora.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/antora.yml b/docs/antora.yml index a27586bb6ffe..9c897ea03d1d 100644 --- a/docs/antora.yml +++ b/docs/antora.yml @@ -2,6 +2,6 @@ name: rubocop title: RuboCop # We always provide version without patch here (e.g. 1.1), # as patch versions should not appear in the docs. -version: '1.67' +version: ~ nav: - modules/ROOT/nav.adoc From 1d828d0fdffb941d0ea056c160dc4f2492f6ab87 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Tue, 15 Oct 2024 13:17:02 +0200 Subject: [PATCH 02/49] Fix test failures caused by changes to the rubocop version These hashses are tied to the rubocop version, which means that a version bump will change these as well. The first test previously passes since it relies on a changed hash, which it was anyways. For the second test the hash must match when computed in the server. --- spec/rubocop/server/rubocop_server_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/rubocop/server/rubocop_server_spec.rb b/spec/rubocop/server/rubocop_server_spec.rb index 748f42300eed..8d15e0d1606c 100644 --- a/spec/rubocop/server/rubocop_server_spec.rb +++ b/spec/rubocop/server/rubocop_server_spec.rb @@ -32,8 +32,10 @@ options = '--server --only Style/FrozenStringLiteralComment,Style/StringLiterals' expect(`ruby -I . "#{rubocop}" #{options}`).to start_with('RuboCop server starting on') - # Emulating the SHA1 value of Gemfile.lock, .rubocop.yml, and .rubocop_todo.yml. - RuboCop::Server::Cache.write_version_file('615dfedabfe01face6557b935510309ae550f74f') + + # Emulate the server starting with an older RuboCop version. + stub_const('RuboCop::Version::STRING', '0.0.1') + RuboCop::Server::Cache.write_version_file(RuboCop::Server::Cache.restart_key) expect(`ruby -I . "#{rubocop}" --server-status`).to match(/RuboCop server .* is running/) _stdout, stderr, _status = Open3.capture3("ruby -I . \"#{rubocop}\" #{options}") @@ -218,8 +220,8 @@ match(/RuboCop server .* is running/), debug_output.join("\n") ) - # Emulating the SHA1 value of Gemfile.lock and .rubocop.yml. - RuboCop::Server::Cache.write_version_file('f63c8f29b26472dae07bfa80c13a6f658361893d') + # Recompute the cache key with the modified .rubocop.yml content. + RuboCop::Server::Cache.write_version_file(RuboCop::Server::Cache.restart_key) message = expect(`ruby -I . "#{rubocop}" --server`) message.not_to start_with('RuboCop server starting on '), debug_output.join("\n") From d629f01d01e0e127ca39ac0bd15e7186ef91fe02 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Mon, 7 Oct 2024 00:08:45 +0300 Subject: [PATCH 03/49] Add new `Style/KeywordArgumentsMerging` cop --- ...new_style_keyword_arguments_merging_cop.md | 1 + config/default.yml | 8 ++ lib/rubocop.rb | 1 + .../cop/style/keyword_arguments_merging.rb | 67 ++++++++++++++ .../style/keyword_arguments_merging_spec.rb | 92 +++++++++++++++++++ 5 files changed, 169 insertions(+) create mode 100644 changelog/new_style_keyword_arguments_merging_cop.md create mode 100644 lib/rubocop/cop/style/keyword_arguments_merging.rb create mode 100644 spec/rubocop/cop/style/keyword_arguments_merging_spec.rb diff --git a/changelog/new_style_keyword_arguments_merging_cop.md b/changelog/new_style_keyword_arguments_merging_cop.md new file mode 100644 index 000000000000..00de4468e506 --- /dev/null +++ b/changelog/new_style_keyword_arguments_merging_cop.md @@ -0,0 +1 @@ +* [#13252](https://github.com/rubocop/rubocop/pull/13252): Add new `Style/KeywordArgumentsMerging` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index ab536a1f3211..a211e0de86a5 100644 --- a/config/default.yml +++ b/config/default.yml @@ -4256,6 +4256,14 @@ Style/IpAddresses: - '**/gems.rb' - '**/*.gemspec' +Style/KeywordArgumentsMerging: + Description: >- + When passing an existing hash as keyword arguments, provide additional arguments + directly rather than using `merge`. + StyleGuide: '#merging-keyword-arguments' + Enabled: pending + VersionAdded: '<>' + Style/KeywordParametersOrder: Description: 'Enforces that optional keyword parameters are placed at the end of the parameters list.' StyleGuide: '#keyword-parameters-order' diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 7d116d506b4a..234e367159a8 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -559,6 +559,7 @@ require_relative 'rubocop/cop/style/inline_comment' require_relative 'rubocop/cop/style/invertible_unless_condition' require_relative 'rubocop/cop/style/ip_addresses' +require_relative 'rubocop/cop/style/keyword_arguments_merging' require_relative 'rubocop/cop/style/keyword_parameters_order' require_relative 'rubocop/cop/style/lambda' require_relative 'rubocop/cop/style/lambda_call' diff --git a/lib/rubocop/cop/style/keyword_arguments_merging.rb b/lib/rubocop/cop/style/keyword_arguments_merging.rb new file mode 100644 index 000000000000..c91c53cc302a --- /dev/null +++ b/lib/rubocop/cop/style/keyword_arguments_merging.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # When passing an existing hash as keyword arguments, provide additional arguments + # directly rather than using `merge`. + # + # Providing arguments directly is more performant, than using `merge`, and + # also leads to a shorter and simpler code. + # + # @example + # # bad + # some_method(**opts.merge(foo: true)) + # some_method(**opts.merge(other_opts)) + # + # # good + # some_method(**opts, foo: true) + # some_method(**opts, **other_opts) + # + class KeywordArgumentsMerging < Base + extend AutoCorrector + + MSG = 'Provide additional arguments directly rather than using `merge`.' + + # @!method merge_kwargs?(node) + def_node_matcher :merge_kwargs?, <<~PATTERN + (send _ _ + ... + (hash + (kwsplat + $(send $_ :merge $...)) + ...)) + PATTERN + + def on_kwsplat(node) + return unless (ancestor = node.parent&.parent) + + merge_kwargs?(ancestor) do |merge_node, hash_node, other_hash_node| + add_offense(merge_node) do |corrector| + autocorrect(corrector, node, hash_node, other_hash_node) + end + end + end + + private + + def autocorrect(corrector, kwsplat_node, hash_node, other_hash_node) + other_hash_node_replacement = + other_hash_node.map do |node| + if node.hash_type? + if node.braces? + node.source[1...-1] + else + node.source + end + else + "**#{node.source}" + end + end.join(', ') + + corrector.replace(kwsplat_node, "**#{hash_node.source}, #{other_hash_node_replacement}") + end + end + end + end +end diff --git a/spec/rubocop/cop/style/keyword_arguments_merging_spec.rb b/spec/rubocop/cop/style/keyword_arguments_merging_spec.rb new file mode 100644 index 000000000000..6868d1c1f487 --- /dev/null +++ b/spec/rubocop/cop/style/keyword_arguments_merging_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::KeywordArgumentsMerging, :config do + it 'registers an offense and corrects when using `merge` with keyword arguments' do + expect_offense(<<~RUBY) + foo(x, **options.merge(y: 1)) + ^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, y: 1) + RUBY + end + + it 'registers an offense and corrects when using `merge` with non-keyword arguments' do + expect_offense(<<~RUBY) + foo(x, **options.merge(other_options)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, **other_options) + RUBY + end + + it 'registers an offense and corrects when using `merge` with keyword and non-keyword arguments' do + expect_offense(<<~RUBY) + foo(x, **options.merge(y: 1, **other_options, z: 2)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, y: 1, **other_options, z: 2) + RUBY + end + + it 'registers an offense and corrects when using `merge` with hash with braces' do + expect_offense(<<~RUBY) + foo(x, **options.merge({ y: 1 })) + ^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, y: 1 ) + RUBY + end + + it 'registers an offense and corrects when using `merge` with complex argument' do + expect_offense(<<~RUBY) + foo(x, **options.merge(other_options, y: 1, **yet_another_options)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, **other_options, y: 1, **yet_another_options) + RUBY + end + + it 'registers an offense and corrects when using `merge` with multiple hash arguments' do + expect_offense(<<~RUBY) + foo(x, **options.merge(other_options, yet_another_options)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, **other_options, **yet_another_options) + RUBY + end + + it 'registers an offense and corrects when using a chain of `merge`s' do + expect_offense(<<~RUBY) + foo(x, **options.merge(other_options).merge(more_options)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Provide additional arguments directly rather than using `merge`. + RUBY + + expect_correction(<<~RUBY) + foo(x, **options, **other_options, **more_options) + RUBY + end + + it 'does not register an offense when not using `merge`' do + expect_no_offenses(<<~RUBY) + foo(x, **options) + RUBY + end + + it 'does not register an offense when using `merge!`' do + expect_no_offenses(<<~RUBY) + foo(x, **options.merge!(other_options)) + RUBY + end +end From 6d54052da5c25c0a0fd29790ac1a1c3aff2bc0c8 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 16 Oct 2024 01:16:34 +0900 Subject: [PATCH 04/49] [Fix #13337] Fix false positives for `Style/RedundantLineContinuation` Fixes #13337. This PR fixes false positives for `Style/RedundantLineContinuation` when required line continuations for `&&` is used with an assignment after a line break. --- ...lse_positives_for_style_redundant_line_continuation.md | 1 + lib/rubocop/cop/style/redundant_line_continuation.rb | 5 ++++- .../rubocop/cop/style/redundant_line_continuation_spec.rb | 8 ++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changelog/fix_false_positives_for_style_redundant_line_continuation.md diff --git a/changelog/fix_false_positives_for_style_redundant_line_continuation.md b/changelog/fix_false_positives_for_style_redundant_line_continuation.md new file mode 100644 index 000000000000..cbeb3eb0bef6 --- /dev/null +++ b/changelog/fix_false_positives_for_style_redundant_line_continuation.md @@ -0,0 +1 @@ +* [#13337](https://github.com/rubocop/rubocop/issues/13337): Fix false positives for `Style/RedundantLineContinuation` when required line continuations for `&&` is used with an assignment after a line break. ([@koic][]) diff --git a/lib/rubocop/cop/style/redundant_line_continuation.rb b/lib/rubocop/cop/style/redundant_line_continuation.rb index 4ccc3c7d93f4..e52c9d887df7 100644 --- a/lib/rubocop/cop/style/redundant_line_continuation.rb +++ b/lib/rubocop/cop/style/redundant_line_continuation.rb @@ -125,7 +125,10 @@ def redundant_line_continuation?(range) return true unless (node = find_node_for_line(range.last_line)) return false if argument_newline?(node) - source = node.parent ? node.parent.source : node.source + source = node.source + while (node = node.parent) + source = node.source + end parse(source.gsub("\\\n", "\n")).valid_syntax? end diff --git a/spec/rubocop/cop/style/redundant_line_continuation_spec.rb b/spec/rubocop/cop/style/redundant_line_continuation_spec.rb index 6abfb299425d..7c4d4ee603c2 100644 --- a/spec/rubocop/cop/style/redundant_line_continuation_spec.rb +++ b/spec/rubocop/cop/style/redundant_line_continuation_spec.rb @@ -206,6 +206,14 @@ def foo RUBY end + it 'does not register an offense when required line continuations for `&&` is used with an assignment after a line break' do + expect_no_offenses(<<~'RUBY') + if foo \ + && (bar = baz) + end + RUBY + end + it 'does not register an offense when required line continuations for multiline leading dot method chain with an empty line' do expect_no_offenses(<<~'RUBY') obj From 386801409fef1ade0ac9c17192c0fb66103d1ccf Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 16 Oct 2024 03:27:20 +0900 Subject: [PATCH 05/49] [Fix #13341] Fix false positives for `Lint/SafeNavigationChain` Fixes #13341. This PR fixes false positives for `Lint/SafeNavigationChain` when a safe navigation operator is used with a method call as the RHS operand of `&&` for the same receiver. --- ...positves_for_lint_safe_navigation_chain.md | 1 + lib/rubocop/cop/lint/safe_navigation_chain.rb | 9 +++ .../cop/lint/safe_navigation_chain_spec.rb | 62 +++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 changelog/fix_false_positves_for_lint_safe_navigation_chain.md diff --git a/changelog/fix_false_positves_for_lint_safe_navigation_chain.md b/changelog/fix_false_positves_for_lint_safe_navigation_chain.md new file mode 100644 index 000000000000..cbdf35ad7a05 --- /dev/null +++ b/changelog/fix_false_positves_for_lint_safe_navigation_chain.md @@ -0,0 +1 @@ +* [#13341](https://github.com/rubocop/rubocop/issues/13341): Fix false positives for `Lint/SafeNavigationChain` when a safe navigation operator is used with a method call as the RHS operand of `&&` for the same receiver. ([@koic][]) diff --git a/lib/rubocop/cop/lint/safe_navigation_chain.rb b/lib/rubocop/cop/lint/safe_navigation_chain.rb index da4c44a89706..e9ad7ad2cb18 100644 --- a/lib/rubocop/cop/lint/safe_navigation_chain.rb +++ b/lib/rubocop/cop/lint/safe_navigation_chain.rb @@ -38,6 +38,8 @@ class SafeNavigationChain < Base PATTERN def on_send(node) + return unless require_safe_navigation?(node) + bad_method?(node) do |safe_nav, method| return if nil_methods.include?(method) || PLUS_MINUS_METHODS.include?(node.method_name) @@ -52,6 +54,13 @@ def on_send(node) private + def require_safe_navigation?(node) + parent = node.parent + return true unless parent&.and_type? + + parent.rhs != node || parent.lhs.receiver != parent.rhs.receiver + end + # @param [Parser::Source::Range] offense_range # @param [RuboCop::AST::SendNode] send_node # @return [String] diff --git a/spec/rubocop/cop/lint/safe_navigation_chain_spec.rb b/spec/rubocop/cop/lint/safe_navigation_chain_spec.rb index b6cbbaf07a37..693ac865aeed 100644 --- a/spec/rubocop/cop/lint/safe_navigation_chain_spec.rb +++ b/spec/rubocop/cop/lint/safe_navigation_chain_spec.rb @@ -171,6 +171,68 @@ RUBY end + it 'registers an offense when a safe navigation operator is used with a method call as both the LHS and RHS operands of `&&` for the same receiver' do + expect_offense(<<~RUBY) + x&.foo.bar && x&.foo.baz + ^^^^ Do not chain ordinary method call after safe navigation operator. + RUBY + + expect_correction(<<~RUBY) + x&.foo&.bar && x&.foo.baz + RUBY + end + + it 'does not register an offense when a safe navigation operator is used with a method call as the RHS operand of `&&` for the same receiver' do + expect_no_offenses(<<~RUBY) + x&.foo&.bar && x&.foo.baz + RUBY + end + + it 'registers an offense when a safe navigation operator is used with a method call as the RHS operand of `&&` for a different receiver' do + expect_offense(<<~RUBY) + x&.foo&.bar && y&.foo.baz + ^^^^ Do not chain ordinary method call after safe navigation operator. + RUBY + + expect_correction(<<~RUBY) + x&.foo&.bar && y&.foo&.baz + RUBY + end + + it 'registers an offense when a safe navigation operator is used with a method call as both the LHS and RHS operands of `||` for the same receiver' do + expect_offense(<<~RUBY) + x&.foo.bar || x&.foo.baz + ^^^^ Do not chain ordinary method call after safe navigation operator. + ^^^^ Do not chain ordinary method call after safe navigation operator. + RUBY + + expect_correction(<<~RUBY) + x&.foo&.bar || x&.foo&.baz + RUBY + end + + it 'registers an offense when a safe navigation operator is used with a method call as the RHS operand of `||` for the same receiver' do + expect_offense(<<~RUBY) + x&.foo&.bar || x&.foo.baz + ^^^^ Do not chain ordinary method call after safe navigation operator. + RUBY + + expect_correction(<<~RUBY) + x&.foo&.bar || x&.foo&.baz + RUBY + end + + it 'registers an offense when a safe navigation operator is used with a method call as the RHS operand of `||` for a different receiver' do + expect_offense(<<~RUBY) + x&.foo&.bar || y&.foo.baz + ^^^^ Do not chain ordinary method call after safe navigation operator. + RUBY + + expect_correction(<<~RUBY) + x&.foo&.bar || y&.foo&.baz + RUBY + end + it 'registers an offense for safe navigation with a method call as an expression of `&&` operand' do expect_offense(<<~RUBY) do_something && x&.foo.bar From c68385b714d47b6794a8b529db6cade54b960e1a Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 16 Oct 2024 10:56:04 -0400 Subject: [PATCH 06/49] [Fix #13343] Prevent `Layout/LineLength` from breaking up a method with arguments chained onto a heredoc delimiter. --- ...t_layout_line_length_from_breaking_up_a.md | 1 + lib/rubocop/cop/mixin/check_line_breakable.rb | 10 ++++++ spec/rubocop/cop/layout/line_length_spec.rb | 35 +++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 changelog/fix_prevent_layout_line_length_from_breaking_up_a.md diff --git a/changelog/fix_prevent_layout_line_length_from_breaking_up_a.md b/changelog/fix_prevent_layout_line_length_from_breaking_up_a.md new file mode 100644 index 000000000000..42d653a284ca --- /dev/null +++ b/changelog/fix_prevent_layout_line_length_from_breaking_up_a.md @@ -0,0 +1 @@ +* [#13343](https://github.com/rubocop/rubocop/issues/13343): Prevent `Layout/LineLength` from breaking up a method with arguments chained onto a heredoc delimiter. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/mixin/check_line_breakable.rb b/lib/rubocop/cop/mixin/check_line_breakable.rb index edbeb82223dc..72b90dd1f046 100644 --- a/lib/rubocop/cop/mixin/check_line_breakable.rb +++ b/lib/rubocop/cop/mixin/check_line_breakable.rb @@ -44,6 +44,8 @@ module Cop module CheckLineBreakable def extract_breakable_node(node, max) if node.send_type? + return if chained_to_heredoc?(node) + args = process_args(node.arguments) return extract_breakable_node_from_elements(node, args, max) elsif node.def_type? @@ -222,6 +224,14 @@ def already_on_multiple_lines?(node) !node.single_line? end + + def chained_to_heredoc?(node) + while (node = node.receiver) + return true if (node.str_type? || node.dstr_type? || node.xstr_type?) && node.heredoc? + end + + false + end end end end diff --git a/spec/rubocop/cop/layout/line_length_spec.rb b/spec/rubocop/cop/layout/line_length_spec.rb index e622ea2bb6bf..6bd482b6186b 100644 --- a/spec/rubocop/cop/layout/line_length_spec.rb +++ b/spec/rubocop/cop/layout/line_length_spec.rb @@ -1204,6 +1204,41 @@ def baz(bar) expect_no_corrections end end + + context 'when HEREDOC start delimiter has a chained method with arguments that go over limit' do + it 'adds offense and does not autocorrect' do + expect_offense(<<~RUBY) + str = <<~HEREDOC.do_something.with_args(foo: '', bar: '', baz: '') + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Line is too long. [66/40] + text + HEREDOC + RUBY + + expect_no_corrections + end + + it 'adds offense and does not autocorrect for `dstr`' do + expect_offense(<<~'RUBY') + str = <<~HEREDOC.do_something.with_args(foo: '', bar: '', baz: '') + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Line is too long. [66/40] + #{text} + HEREDOC + RUBY + + expect_no_corrections + end + + it 'adds offense and does not autocorrect for `xstr`' do + expect_offense(<<~RUBY) + str = <<~`HEREDOC`.do_something.with_args(foo: '', bar: '', baz: '') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Line is too long. [68/40] + text + HEREDOC + RUBY + + expect_no_corrections + end + end end context 'comments' do From 2de40111c00d62aa10a3e3334c12d2e7e569b1c9 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 16 Oct 2024 14:26:40 -0400 Subject: [PATCH 07/49] [Fix #13348] Ensure `Style/BlockDelimiters` autocorrection does not move other code between the block and comment. --- ...le_block_delimiters_autocorrection_does.md | 1 + lib/rubocop/cop/style/block_delimiters.rb | 19 +++++++++++++++++-- .../cop/style/block_delimiters_spec.rb | 19 ++++++++++++++++--- 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 changelog/fix_ensure_style_block_delimiters_autocorrection_does.md diff --git a/changelog/fix_ensure_style_block_delimiters_autocorrection_does.md b/changelog/fix_ensure_style_block_delimiters_autocorrection_does.md new file mode 100644 index 000000000000..c1a0f086d835 --- /dev/null +++ b/changelog/fix_ensure_style_block_delimiters_autocorrection_does.md @@ -0,0 +1 @@ +* [#13348](https://github.com/rubocop/rubocop/issues/13348): Ensure `Style/BlockDelimiters` autocorrection does not move other code between the block and comment. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/style/block_delimiters.rb b/lib/rubocop/cop/style/block_delimiters.rb index 7048b57db7be..9f05009c4735 100644 --- a/lib/rubocop/cop/style/block_delimiters.rb +++ b/lib/rubocop/cop/style/block_delimiters.rb @@ -303,13 +303,28 @@ def whitespace_after?(range, length = 1) def move_comment_before_block(corrector, comment, block_node, closing_brace) range = block_node.chained? ? end_of_chain(block_node.parent).source_range : closing_brace + + # It is possible that there is code between the block and the comment + # which needs to be preserved and trimmed. + pre_comment_range = source_range_before_comment(range, comment) + corrector.remove(range_with_surrounding_space(comment.source_range, side: :right)) - remove_trailing_whitespace(corrector, range, comment) - corrector.insert_after(range, "\n") + remove_trailing_whitespace(corrector, pre_comment_range, comment) + corrector.insert_after(pre_comment_range, "\n") corrector.insert_before(block_node, "#{comment.text}\n") end + def source_range_before_comment(range, comment) + range = range.end.join(comment.source_range.begin) + + # End the range before any whitespace that precedes the comment + trailing_whitespace_count = range.source[/\s+\z/]&.length + range = range.adjust(end_pos: -trailing_whitespace_count) if trailing_whitespace_count + + range + end + def end_of_chain(node) return end_of_chain(node.block_node) if with_block?(node) return node unless node.chained? diff --git a/spec/rubocop/cop/style/block_delimiters_spec.rb b/spec/rubocop/cop/style/block_delimiters_spec.rb index 6f833a99d5e7..1645d903bcf2 100644 --- a/spec/rubocop/cop/style/block_delimiters_spec.rb +++ b/spec/rubocop/cop/style/block_delimiters_spec.rb @@ -455,11 +455,10 @@ }] # comment RUBY - expect_correction(<<~RUBY.chop) + expect_correction(<<~RUBY) [# comment foo do - end - ]#{trailing_whitespace} + end] RUBY end @@ -630,6 +629,20 @@ }, arg3: :another_value RUBY end + + it 'handles a multiline {} block with trailing comment' do + expect_offense(<<~RUBY) + my_method { |x| + ^ Avoid using `{...}` for multi-line blocks. + x.foo } unless bar # comment + RUBY + + expect_correction(<<~RUBY) + # comment + my_method do |x| + x.foo end unless bar + RUBY + end end context 'with a single line do-end block with an inline `rescue`' do From 1e1b9f986524c9d3cd61cc33e497fda988686b5e Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Wed, 16 Oct 2024 20:04:30 +0200 Subject: [PATCH 08/49] Clearly distinguish between the version string and verbose version output This method really does two things. While it is marked as private api, some external code uses it as an alias for `Version::STRING`. Additionally, standardrb uses it to programatically output the same verbose output that RuboCop does. --- lib/rubocop/cli/command/execute_runner.rb | 2 +- lib/rubocop/cli/command/version.rb | 4 ++-- lib/rubocop/formatter/disabled_config_formatter.rb | 2 +- lib/rubocop/target_ruby.rb | 2 +- lib/rubocop/version.rb | 6 ++++++ rubocop.gemspec | 2 +- spec/rubocop/version_spec.rb | 4 ++++ 7 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cli/command/execute_runner.rb b/lib/rubocop/cli/command/execute_runner.rb index a1eaac1f5461..d379d87d6959 100644 --- a/lib/rubocop/cli/command/execute_runner.rb +++ b/lib/rubocop/cli/command/execute_runner.rb @@ -78,7 +78,7 @@ def display_error_summary(errors) Please, report your problems to RuboCop's issue tracker. #{bug_tracker_uri} Mention the following information in the issue report: - #{RuboCop::Version.version(debug: true)} + #{RuboCop::Version.verbose} WARNING end diff --git a/lib/rubocop/cli/command/version.rb b/lib/rubocop/cli/command/version.rb index d097a31acfef..02fcd376284f 100644 --- a/lib/rubocop/cli/command/version.rb +++ b/lib/rubocop/cli/command/version.rb @@ -9,8 +9,8 @@ class Version < Base self.command_name = :version def run - puts RuboCop::Version.version(debug: false) if @options[:version] - puts RuboCop::Version.version(debug: true, env: env) if @options[:verbose_version] + puts RuboCop::Version::STRING if @options[:version] + puts RuboCop::Version.verbose(env: env) if @options[:verbose_version] end end end diff --git a/lib/rubocop/formatter/disabled_config_formatter.rb b/lib/rubocop/formatter/disabled_config_formatter.rb index 2ad83b23719a..2b08e31b57da 100644 --- a/lib/rubocop/formatter/disabled_config_formatter.rb +++ b/lib/rubocop/formatter/disabled_config_formatter.rb @@ -10,7 +10,7 @@ class DisabledConfigFormatter < BaseFormatter HEADING = <<~COMMENTS # This configuration was generated by # `%s` - # %susing RuboCop version #{Version.version}. + # %susing RuboCop version #{Version::STRING}. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new diff --git a/lib/rubocop/target_ruby.rb b/lib/rubocop/target_ruby.rb index f144de8be56e..94595f9ce218 100644 --- a/lib/rubocop/target_ruby.rb +++ b/lib/rubocop/target_ruby.rb @@ -273,7 +273,7 @@ def supported? def rubocop_version_with_support if supported? - RuboCop::Version.version + RuboCop::Version::STRING else OBSOLETE_RUBIES[version] end diff --git a/lib/rubocop/version.rb b/lib/rubocop/version.rb index 7365e91c9f1f..0c9306fe4820 100644 --- a/lib/rubocop/version.rb +++ b/lib/rubocop/version.rb @@ -18,6 +18,7 @@ module Version 'rubocop-md' => 'markdown', 'rubocop-factory_bot' => 'factory_bot' }.freeze + # NOTE: Marked as private but used by gems like standard. # @api private def self.version(debug: false, env: nil) if debug @@ -41,6 +42,11 @@ def self.version(debug: false, env: nil) end end + # @api private + def self.verbose(env: nil) + version(debug: true, env: env) + end + # @api private def self.parser_version config_path = ConfigFinder.find_config_path(Dir.pwd) diff --git a/rubocop.gemspec b/rubocop.gemspec index 84808ea503ad..c56e2a430d1b 100644 --- a/rubocop.gemspec +++ b/rubocop.gemspec @@ -24,7 +24,7 @@ Gem::Specification.new do |s| s.metadata = { 'homepage_uri' => 'https://rubocop.org/', - 'changelog_uri' => "https://github.com/rubocop/rubocop/releases/tag/v#{RuboCop::Version.version}", + 'changelog_uri' => "https://github.com/rubocop/rubocop/releases/tag/v#{RuboCop::Version::STRING}", 'source_code_uri' => 'https://github.com/rubocop/rubocop/', 'documentation_uri' => "https://docs.rubocop.org/rubocop/#{RuboCop::Version.document_version}/", 'bug_tracker_uri' => 'https://github.com/rubocop/rubocop/issues', diff --git a/spec/rubocop/version_spec.rb b/spec/rubocop/version_spec.rb index 62b772cee277..d6ee439afa76 100644 --- a/spec/rubocop/version_spec.rb +++ b/spec/rubocop/version_spec.rb @@ -18,6 +18,10 @@ it { is_expected.to match(/\d+\.\d+\.\d+ \(using Parser/) } end + + it 'is the gem version when called without arguments' do + expect(described_class.version).to eq(described_class::STRING) + end end describe '.extension_versions', :isolated_environment, :restore_registry do From d204188df752acf8fdb9fb1855fc09c71bd62485 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Wed, 16 Oct 2024 20:11:17 +0200 Subject: [PATCH 09/49] Show Ruby version of the current dir for `rubocop -V` Followup to #13310 --- .../change_rubocop_-V-show_ruby_of_pwd.md | 1 + lib/rubocop/version.rb | 27 ++++++++++++++----- spec/rubocop/cli/options_spec.rb | 16 ++++++++++- 3 files changed, 36 insertions(+), 8 deletions(-) create mode 100644 changelog/change_rubocop_-V-show_ruby_of_pwd.md diff --git a/changelog/change_rubocop_-V-show_ruby_of_pwd.md b/changelog/change_rubocop_-V-show_ruby_of_pwd.md new file mode 100644 index 000000000000..042436667cee --- /dev/null +++ b/changelog/change_rubocop_-V-show_ruby_of_pwd.md @@ -0,0 +1 @@ +* [#13347](https://github.com/rubocop/rubocop/pull/13347): When running `rubocop -V`, show the analysis Ruby version of the current directory. ([@earlopain][]) diff --git a/lib/rubocop/version.rb b/lib/rubocop/version.rb index 0c9306fe4820..e251a291a69d 100644 --- a/lib/rubocop/version.rb +++ b/lib/rubocop/version.rb @@ -24,7 +24,7 @@ def self.version(debug: false, env: nil) if debug verbose_version = format(MSG, version: STRING, parser_version: parser_version, rubocop_ast_version: RuboCop::AST::Version::STRING, - target_ruby_version: TargetRuby.new(Config.new).version, + target_ruby_version: target_ruby_version(env), ruby_engine: RUBY_ENGINE, ruby_version: RUBY_VERSION, server_mode: server_mode, ruby_platform: RUBY_PLATFORM) @@ -64,12 +64,7 @@ def self.parser_version # @api private def self.extension_versions(env) - features = Util.silence_warnings do - # Suppress any config issues when loading the config (ie. deprecations, - # pending cops, etc.). - env.config_store.unvalidated.for_pwd.loaded_features.sort - end - + features = config_for_pwd(env).loaded_features.sort features.filter_map do |loaded_feature| next unless (match = loaded_feature.match(/rubocop-(?.*)/)) @@ -91,6 +86,24 @@ def self.extension_versions(env) end end + # @api private + def self.target_ruby_version(env) + if env + config_for_pwd(env).target_ruby_version + else + TargetRuby.new(Config.new).version + end + end + + # @api private + def self.config_for_pwd(env) + Util.silence_warnings do + # Suppress any config issues when loading the config (ie. deprecations, + # pending cops, etc.). + env.config_store.unvalidated.for_pwd + end + end + # Returns feature version in one of two ways: # # * Find by RuboCop core version style (e.g. rubocop-performance, rubocop-rspec) diff --git a/spec/rubocop/cli/options_spec.rb b/spec/rubocop/cli/options_spec.rb index 9ed2ceddaf0e..389c9f008352 100644 --- a/spec/rubocop/cli/options_spec.rb +++ b/spec/rubocop/cli/options_spec.rb @@ -293,7 +293,7 @@ expect($stdout.string).to include(RuboCop::Version::STRING) expect($stdout.string).to match(/Parser \d+\.\d+\.\d+/) expect($stdout.string).to match(/rubocop-ast \d+\.\d+\.\d+/) - expect($stdout.string).to match(/analyzing as Ruby \d+\.\d+/) + expect($stdout.string).to match(/analyzing as Ruby #{RuboCop::TargetRuby::DEFAULT_VERSION}/o) end context 'when requiring extension cops' do @@ -422,6 +422,20 @@ class SomeCop < Base expect($stdout.string).to include(RuboCop::Version::STRING) end end + + context 'when run in a subfolder with `.rubocop.yml` specifying `TargetRubyVersion`' do + before do + create_file('subdir/.rubocop.yml', <<~YAML) + AllCops: + TargetRubyVersion: 3.0 + YAML + end + + it 'shows that Ruby version in the output' do + Dir.chdir('subdir') { cli.run ['-V'] } + expect($stdout.string).to match(/analyzing as Ruby 3\.0/) + end + end end describe '--only' do From 68bfccbf83901a396801533c1e40c2c31c478f74 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 16 Oct 2024 12:27:47 -0400 Subject: [PATCH 10/49] [Fix #13324] Fix `--disable-uncorrectable` to not insert a comment inside a string continuation. --- ...e_uncorrectable_to_not_insert_a_comment.md | 1 + lib/rubocop/cop/autocorrect_logic.rb | 24 +++++++- .../rubocop/cli/disable_uncorrectable_spec.rb | 56 +++++++++++++++++++ 3 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 changelog/fix_fix_disable_uncorrectable_to_not_insert_a_comment.md diff --git a/changelog/fix_fix_disable_uncorrectable_to_not_insert_a_comment.md b/changelog/fix_fix_disable_uncorrectable_to_not_insert_a_comment.md new file mode 100644 index 000000000000..fdc678970c6a --- /dev/null +++ b/changelog/fix_fix_disable_uncorrectable_to_not_insert_a_comment.md @@ -0,0 +1 @@ +* [#13324](https://github.com/rubocop/rubocop/issues/13324): Fix `--disable-uncorrectable` to not insert a comment inside a string continuation. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/autocorrect_logic.rb b/lib/rubocop/cop/autocorrect_logic.rb index 6afab6b39899..a10b716c4518 100644 --- a/lib/rubocop/cop/autocorrect_logic.rb +++ b/lib/rubocop/cop/autocorrect_logic.rb @@ -49,7 +49,9 @@ def autocorrect_enabled? private def disable_offense(offense_range) - range = surrounding_heredoc(offense_range) || surrounding_percent_array(offense_range) + range = surrounding_heredoc(offense_range) || + surrounding_percent_array(offense_range) || + string_continuation(offense_range) if range disable_offense_before_and_after(range_by_lines(range)) @@ -88,10 +90,28 @@ def surrounding_percent_array(offense_range) end percent_array.map(&:source_range).find do |range| - offense_range.begin_pos > range.begin_pos && range.overlaps?(offense_range) + range_overlaps_offense?(offense_range, range) end end + def string_continuation(offense_range) + return nil if offense_range.empty? + + string_continuation_nodes = processed_source.ast.each_descendant.filter_map do |node| + range_by_lines(node.source_range) if string_continuation?(node) + end + + string_continuation_nodes.find { |range| range_overlaps_offense?(offense_range, range) } + end + + def range_overlaps_offense?(offense_range, range) + offense_range.begin_pos > range.begin_pos && range.overlaps?(offense_range) + end + + def string_continuation?(node) + (node.str_type? || node.dstr_type? || node.xstr_type?) && node.source.match?(/\\\s*$/) + end + def range_of_first_line(range) begin_of_first_line = range.begin_pos - range.column end_of_first_line = begin_of_first_line + range.source_line.length diff --git a/spec/rubocop/cli/disable_uncorrectable_spec.rb b/spec/rubocop/cli/disable_uncorrectable_spec.rb index 35cea4009656..650956a80119 100644 --- a/spec/rubocop/cli/disable_uncorrectable_spec.rb +++ b/spec/rubocop/cli/disable_uncorrectable_spec.rb @@ -343,6 +343,62 @@ def foo # rubocop:todo Metrics/MethodLength RUBY end end + + context 'and the offense is on a line containing a string continuation' do + it 'adds before-and-after disable statement around the string continuation' do + create_file('.rubocop.yml', <<~YAML) + AllCops: + DisabledByDefault: true + Lint/DuplicateHashKey: + Enabled: true + YAML + create_file('example.rb', <<~'RUBY') + { + key1: 'something', + key1: 'whatever'\ + 'something else' + } + RUBY + expect(exit_code).to eq(0) + expect(File.read('example.rb')).to eq(<<~'RUBY') + { + key1: 'something', + # rubocop:todo Lint/DuplicateHashKey + key1: 'whatever'\ + 'something else' + # rubocop:enable Lint/DuplicateHashKey + } + RUBY + end + end + + context 'and the offense is on a different line than a string continuation' do + it 'adds a single one-line disable statement' do + create_file('.rubocop.yml', <<~YAML) + AllCops: + DisabledByDefault: true + Lint/DuplicateHashKey: + Enabled: true + YAML + create_file('example.rb', <<~'RUBY') + x = 'something'\ + 'something else' + { + key1: 'foo', + key1: 'bar' + } + RUBY + expect(exit_code).to eq(0) + expect(File.read('example.rb')).to eq(<<~'RUBY') + x = 'something'\ + 'something else' + { + key1: 'foo', + key1: 'bar' # rubocop:todo Lint/DuplicateHashKey + } + RUBY + end + end end context 'when exist offense for Layout/SpaceInsideArrayLiteralBrackets' do From 76383e7380103481801c7b658a558fde42a606d3 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Tue, 15 Oct 2024 10:03:14 -0400 Subject: [PATCH 11/49] [Fix #13336] Update `Style/SafeNavigation` to not autocorrect if the RHS of an `and` node is an `or` node. --- ...e_safe_navigation_to_not_autocorrect_if.md | 1 + lib/rubocop/cop/style/safe_navigation.rb | 12 +++++++ .../rubocop/cop/style/safe_navigation_spec.rb | 31 +++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 changelog/fix_update_style_safe_navigation_to_not_autocorrect_if.md diff --git a/changelog/fix_update_style_safe_navigation_to_not_autocorrect_if.md b/changelog/fix_update_style_safe_navigation_to_not_autocorrect_if.md new file mode 100644 index 000000000000..dcccbda968ec --- /dev/null +++ b/changelog/fix_update_style_safe_navigation_to_not_autocorrect_if.md @@ -0,0 +1 @@ +* [#13336](https://github.com/rubocop/rubocop/issues/13336): Update `Style/SafeNavigation` to not autocorrect if the RHS of an `and` node is an `or` node. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/style/safe_navigation.rb b/lib/rubocop/cop/style/safe_navigation.rb index 4b12a08dea0c..65d59776c872 100644 --- a/lib/rubocop/cop/style/safe_navigation.rb +++ b/lib/rubocop/cop/style/safe_navigation.rb @@ -21,6 +21,11 @@ module Style # We have limited the cop to not register an offense for method chains # that exceed this option's value. # + # NOTE: This cop will recognize offenses but not autocorrect code when the + # right hand side (RHS) of the `&&` statement is an `||` statement + # (eg. `foo && (foo.bar? || foo.baz?)`). It can be corrected + # manually by removing the `foo &&` and adding `&.` to each `foo` on the RHS. + # # @safety # Autocorrection is unsafe because if a value is `false`, the resulting # code will have different behavior or raise an error. @@ -121,6 +126,9 @@ class SafeNavigation < Base # rubocop:disable Metrics/ClassLength } PATTERN + # @!method and_with_rhs_or?(node) + def_node_matcher :and_with_rhs_or?, '(and _ {or (begin or)})' + # @!method not_nil_check?(node) def_node_matcher :not_nil_check?, '(send (send $_ :nil?) :!)' @@ -172,6 +180,10 @@ def on_and(node) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity def report_offense(node, rhs, rhs_receiver, *removal_ranges, offense_range: node) add_offense(offense_range) do |corrector| + # If the RHS is an `or` we cannot safely autocorrect because in order to remove + # the non-nil check we need to add safe-navs to all clauses where the receiver is used + next if and_with_rhs_or?(node) + removal_ranges.each { |range| corrector.remove(range) } yield corrector if block_given? diff --git a/spec/rubocop/cop/style/safe_navigation_spec.rb b/spec/rubocop/cop/style/safe_navigation_spec.rb index 96a3e606b5f9..065e978db113 100644 --- a/spec/rubocop/cop/style/safe_navigation_spec.rb +++ b/spec/rubocop/cop/style/safe_navigation_spec.rb @@ -1396,6 +1396,37 @@ def foobar RUBY end + it 'registers an offense but does not autocorrect when the RHS of `and` is an `or` node containing an `and`' do + expect_offense(<<~RUBY) + foo && (foo.bar? || (foo.baz? && foo.quux?)) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use safe navigation (`&.`) instead [...] + RUBY + + expect_no_corrections + end + + it 'registers an offense when the RHS of `and` is a nested `and` node' do + expect_offense(<<~RUBY) + foo && (foo.bar? && (foo.baz? && foo.quux?)) + ^^^^^^^^^^^^^^^^ Use safe navigation (`&.`) instead [...] + RUBY + + expect_correction(<<~RUBY) + (foo&.bar? && (foo.baz? && foo.quux?)) + RUBY + end + + it 'registers an offense when the RHS of `and` is an `and` node containing `or`' do + expect_offense(<<~RUBY) + foo && (foo.bar? && (foo.baz? || foo.quux?)) + ^^^^^^^^^^^^^^^^ Use safe navigation (`&.`) instead [...] + RUBY + + expect_correction(<<~RUBY) + (foo&.bar? && (foo.baz? || foo.quux?)) + RUBY + end + context 'respond_to?' do it 'allows method calls safeguarded by a respond_to check' do expect_no_offenses('foo.bar if foo.respond_to?(:bar)') From bfc974d0206f7c3c7cc97d202873466a2d40e1b7 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Tue, 15 Oct 2024 14:39:07 +0200 Subject: [PATCH 12/49] Add a basic test for the docs generator Nothing much, just checking that it runs without errors on all ruby versions tested in CI Followup to #13332 --- lib/rubocop/cops_documentation_generator.rb | 3 ++- .../cops_documentation_generator_spec.rb | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 spec/rubocop/cops_documentation_generator_spec.rb diff --git a/lib/rubocop/cops_documentation_generator.rb b/lib/rubocop/cops_documentation_generator.rb index 895f748eb45c..d24b40518ba2 100644 --- a/lib/rubocop/cops_documentation_generator.rb +++ b/lib/rubocop/cops_documentation_generator.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'fileutils' +require 'yard' # Class for generating documentation of all cops departments # @api private @@ -295,7 +296,7 @@ def print_cops_of_department(department) HEADER selected_cops.each { |cop| content << print_cop_with_doc(cop) } content << footer_for_department(department) - file_name = "#{docs_path}/#{department_to_basename(department)}.adoc" + file_name = "#{docs_path}#{department_to_basename(department)}.adoc" File.open(file_name, 'w') do |file| puts "* generated #{file_name}" file.write("#{content.strip}\n") diff --git a/spec/rubocop/cops_documentation_generator_spec.rb b/spec/rubocop/cops_documentation_generator_spec.rb new file mode 100644 index 000000000000..0edcaeba6efb --- /dev/null +++ b/spec/rubocop/cops_documentation_generator_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'rubocop/cops_documentation_generator' + +RSpec.describe CopsDocumentationGenerator, :isolated_environment do + around do |example| + new_global = RuboCop::Cop::Registry.new([RuboCop::Cop::Style::HashSyntax]) + RuboCop::Cop::Registry.with_temporary_global(new_global) { example.run } + end + + it 'generates docs without errors' do + generator = described_class.new(departments: %w[Style]) + expect do + generator.call + end.to output(%r{generated .*docs/modules/ROOT/pages/cops_style.adoc}).to_stdout + end +end From ce37499b63827f142ebefd88b01c0b08b66218b1 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 2 Oct 2024 12:37:16 -0400 Subject: [PATCH 13/49] [Fix #12988] Add new `Style/AmbiguousEndlessMethodDefinition` cop. --- ...tyle_endless_method_operator_precedence.md | 1 + config/default.yml | 5 ++ lib/rubocop.rb | 2 + .../cop/mixin/endless_method_rewriter.rb | 24 ++++++ .../ambiguous_endless_method_definition.rb | 79 +++++++++++++++++++ lib/rubocop/cop/style/endless_method.rb | 15 +--- ...mbiguous_endless_method_definition_spec.rb | 70 ++++++++++++++++ 7 files changed, 182 insertions(+), 14 deletions(-) create mode 100644 changelog/new_add_new_style_endless_method_operator_precedence.md create mode 100644 lib/rubocop/cop/mixin/endless_method_rewriter.rb create mode 100644 lib/rubocop/cop/style/ambiguous_endless_method_definition.rb create mode 100644 spec/rubocop/cop/style/ambiguous_endless_method_definition_spec.rb diff --git a/changelog/new_add_new_style_endless_method_operator_precedence.md b/changelog/new_add_new_style_endless_method_operator_precedence.md new file mode 100644 index 000000000000..c623f2543bd6 --- /dev/null +++ b/changelog/new_add_new_style_endless_method_operator_precedence.md @@ -0,0 +1 @@ +* [#12988](https://github.com/rubocop/rubocop/issues/12988): Add new `Style/AmbiguousEndlessMethodDefinition` cop. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index a211e0de86a5..424af19af725 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3143,6 +3143,11 @@ Style/Alias: - prefer_alias - prefer_alias_method +Style/AmbiguousEndlessMethodDefinition: + Description: 'Checks for endless methods inside operators of lower precedence.' + Enabled: pending + VersionAdded: '<>' + Style/AndOr: Description: 'Use &&/|| instead of and/or.' StyleGuide: '#no-and-or-or' diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 234e367159a8..63c4ef14b11a 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -90,6 +90,7 @@ require_relative 'rubocop/cop/mixin/empty_lines_around_body' # relies on range require_relative 'rubocop/cop/mixin/empty_parameter' require_relative 'rubocop/cop/mixin/end_keyword_alignment' +require_relative 'rubocop/cop/mixin/endless_method_rewriter' require_relative 'rubocop/cop/mixin/enforce_superclass' require_relative 'rubocop/cop/mixin/first_element_line_break' require_relative 'rubocop/cop/mixin/frozen_string_literal' @@ -460,6 +461,7 @@ require_relative 'rubocop/cop/style/access_modifier_declarations' require_relative 'rubocop/cop/style/accessor_grouping' require_relative 'rubocop/cop/style/alias' +require_relative 'rubocop/cop/style/ambiguous_endless_method_definition' require_relative 'rubocop/cop/style/and_or' require_relative 'rubocop/cop/style/arguments_forwarding' require_relative 'rubocop/cop/style/array_coercion' diff --git a/lib/rubocop/cop/mixin/endless_method_rewriter.rb b/lib/rubocop/cop/mixin/endless_method_rewriter.rb new file mode 100644 index 000000000000..6db30afd2710 --- /dev/null +++ b/lib/rubocop/cop/mixin/endless_method_rewriter.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for rewriting endless methods to normal method definitions + module EndlessMethodRewriter + def correct_to_multiline(corrector, node) + replacement = <<~RUBY.strip + def #{node.method_name}#{arguments(node)} + #{node.body.source} + end + RUBY + + corrector.replace(node, replacement) + end + + private + + def arguments(node, missing = '') + node.arguments.any? ? node.arguments.source : missing + end + end + end +end diff --git a/lib/rubocop/cop/style/ambiguous_endless_method_definition.rb b/lib/rubocop/cop/style/ambiguous_endless_method_definition.rb new file mode 100644 index 000000000000..dbabcef7540e --- /dev/null +++ b/lib/rubocop/cop/style/ambiguous_endless_method_definition.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # Looks for endless methods inside operations of lower precedence (`and`, `or`, and + # modifier forms of `if`, `unless`, `while`, `until`) that are ambiguous due to + # lack of parentheses. This may lead to unexpected behavior as the code may appear + # to use these keywords as part of the method but in fact they modify + # the method definition itself. + # + # In these cases, using a normal method definition is more clear. + # + # @example + # + # # bad + # def foo = true if bar + # + # # good - using a non-endless method is more explicit + # def foo + # true + # end if bar + # + # # ok - method body is explicit + # def foo = (true if bar) + # + # # ok - method definition is explicit + # (def foo = true) if bar + class AmbiguousEndlessMethodDefinition < Base + extend TargetRubyVersion + extend AutoCorrector + include EndlessMethodRewriter + include RangeHelp + + minimum_target_ruby_version 3.0 + + MSG = 'Avoid using `%s` statements with endless methods.' + + # @!method ambiguous_endless_method_body(node) + def_node_matcher :ambiguous_endless_method_body, <<~PATTERN + ^${ + (if _ ) + ({and or} def _) + ({while until} _ def) + } + PATTERN + + def on_def(node) + return unless node.endless? + + operation = ambiguous_endless_method_body(node) + return unless operation + + return unless modifier_form?(operation) + + add_offense(operation, message: format(MSG, keyword: keyword(operation))) do |corrector| + correct_to_multiline(corrector, node) + end + end + + private + + def modifier_form?(operation) + return true if operation.and_type? || operation.or_type? + + operation.modifier_form? + end + + def keyword(operation) + if operation.respond_to?(:keyword) + operation.keyword + else + operation.operator + end + end + end + end + end +end diff --git a/lib/rubocop/cop/style/endless_method.rb b/lib/rubocop/cop/style/endless_method.rb index abbcbe718225..7217cc1a75c4 100644 --- a/lib/rubocop/cop/style/endless_method.rb +++ b/lib/rubocop/cop/style/endless_method.rb @@ -48,6 +48,7 @@ module Style # class EndlessMethod < Base include ConfigurableEnforcedStyle + include EndlessMethodRewriter extend TargetRubyVersion extend AutoCorrector @@ -81,20 +82,6 @@ def handle_disallow_style(node) add_offense(node) { |corrector| correct_to_multiline(corrector, node) } end - - def correct_to_multiline(corrector, node) - replacement = <<~RUBY.strip - def #{node.method_name}#{arguments(node)} - #{node.body.source} - end - RUBY - - corrector.replace(node, replacement) - end - - def arguments(node, missing = '') - node.arguments.any? ? node.arguments.source : missing - end end end end diff --git a/spec/rubocop/cop/style/ambiguous_endless_method_definition_spec.rb b/spec/rubocop/cop/style/ambiguous_endless_method_definition_spec.rb new file mode 100644 index 000000000000..644b9ae24967 --- /dev/null +++ b/spec/rubocop/cop/style/ambiguous_endless_method_definition_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::AmbiguousEndlessMethodDefinition, :config do + context 'Ruby >= 3.0', :ruby30 do + it 'does not register an offense for a non endless method' do + expect_no_offenses(<<~RUBY) + def foo + end + RUBY + end + + %i[and or if unless while until].each do |operator| + context "with #{operator}" do + it "does not register an offense for a non endless method followed by `#{operator}`" do + expect_no_offenses(<<~RUBY) + def foo + end #{operator} bar + RUBY + end + + it 'does not register an offense when the operator is already wrapped in parens' do + expect_no_offenses(<<~RUBY) + def foo = (true #{operator} bar) + RUBY + end + + it 'does not register an offense when the method definition is already wrapped in parens' do + expect_no_offenses(<<~RUBY) + (def foo = true) #{operator} bar + RUBY + end + + unless %i[and or].include?(operator) + it "does not register an offense for non-modifier `#{operator}`" do + expect_no_offenses(<<~RUBY) + #{operator} bar + def foo = true + end + RUBY + end + end + + it "registers and offense and corrects an endless method followed by `#{operator}`" do + expect_offense(<<~RUBY, operator: operator) + def foo = true #{operator} bar + ^^^^^^^^^^^^^^^^{operator}^^^^ Avoid using `#{operator}` statements with endless methods. + RUBY + + expect_correction(<<~RUBY) + def foo + true + end #{operator} bar + RUBY + end + end + end + + it 'does not register an offense for `&&`' do + expect_no_offenses(<<~RUBY) + def foo = true && bar + RUBY + end + + it 'does not register an offense for `||`' do + expect_no_offenses(<<~RUBY) + def foo = true || bar + RUBY + end + end +end From c0026e77b6415d99359756f7d72b15fe877520a7 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 18 Oct 2024 01:33:41 +0900 Subject: [PATCH 14/49] Add style guide URL to `Style/AmbiguousEndlessMethodDefinition` Follow up https://github.com/rubocop/ruby-style-guide/pull/949 and https://github.com/rubocop/rubocop/pull/13288. --- config/default.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/default.yml b/config/default.yml index 424af19af725..3f76299b5083 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3145,6 +3145,7 @@ Style/Alias: Style/AmbiguousEndlessMethodDefinition: Description: 'Checks for endless methods inside operators of lower precedence.' + StyleGuide: '#ambiguous-endless-method-defintions' Enabled: pending VersionAdded: '<>' From fda24499fd2b831f9352f8ce63c478e862f54d24 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 18 Oct 2024 02:47:14 +0900 Subject: [PATCH 15/49] [Fix #13353] Fix an incorrect autocorrect for `Style/BlockDelimiters` Fixes #13353. This PR fixes an incorrect autocorrect for `Style/BlockDelimiters` when `EnforcedStyle: semantic` is set and used with `Layout/SpaceInsideBlockBraces`. --- ...tocorrect_for_style_block_delimiters_cop.md | 1 + .../cop/layout/space_inside_block_braces.rb | 4 ++++ spec/rubocop/cli/autocorrect_spec.rb | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+) create mode 100644 changelog/fix_incorrect_autocorrect_for_style_block_delimiters_cop.md diff --git a/changelog/fix_incorrect_autocorrect_for_style_block_delimiters_cop.md b/changelog/fix_incorrect_autocorrect_for_style_block_delimiters_cop.md new file mode 100644 index 000000000000..321df8822904 --- /dev/null +++ b/changelog/fix_incorrect_autocorrect_for_style_block_delimiters_cop.md @@ -0,0 +1 @@ +* [#13353](https://github.com/rubocop/rubocop/issues/13353): Fix an incorrect autocorrect for `Style/BlockDelimiters` when `EnforcedStyle: semantic` is set and used with `Layout/SpaceInsideBlockBraces`. ([@koic][]) diff --git a/lib/rubocop/cop/layout/space_inside_block_braces.rb b/lib/rubocop/cop/layout/space_inside_block_braces.rb index c8a1310219c8..bb3729cad962 100644 --- a/lib/rubocop/cop/layout/space_inside_block_braces.rb +++ b/lib/rubocop/cop/layout/space_inside_block_braces.rb @@ -82,6 +82,10 @@ class SpaceInsideBlockBraces < Base include RangeHelp extend AutoCorrector + def self.autocorrect_incompatible_with + [Style::BlockDelimiters] + end + def on_block(node) return if node.keywords? diff --git a/spec/rubocop/cli/autocorrect_spec.rb b/spec/rubocop/cli/autocorrect_spec.rb index 2bbfe8a504ca..7d3510e61331 100644 --- a/spec/rubocop/cli/autocorrect_spec.rb +++ b/spec/rubocop/cli/autocorrect_spec.rb @@ -301,6 +301,24 @@ def batch RUBY end + it 'corrects `EnforcedStyle: semantic` of `Style/BlockDelimiters` with `Layout/SpaceInsideBlockBraces`' do + create_file('.rubocop.yml', <<~YAML) + Style/BlockDelimiters: + EnforcedStyle: semantic + YAML + source = <<~RUBY + File.open('a', 'w') { } + RUBY + create_file('example.rb', source) + expect(cli.run([ + '--autocorrect-all', + '--only', 'Style/BlockDelimiters,Layout/SpaceInsideBlockBraces' + ])).to eq(0) + expect(File.read('example.rb')).to eq(<<~RUBY) + File.open('a', 'w') do end + RUBY + end + it 'corrects `EnforcedStyle: require_parentheses` of `Style/MethodCallWithArgsParentheses` with `Style/NestedParenthesizedCalls`' do create_file('.rubocop.yml', <<~YAML) Style/MethodCallWithArgsParentheses: From 8247ca79d718e2458235efaa917856c28c70d625 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Fri, 18 Oct 2024 10:37:22 +0200 Subject: [PATCH 16/49] [Fix #13356] Fix a false positive for `Layout/SpaceBeforeBrackets` when there is a dot before `[]=` In addition to checking `[]`, `[]=` must also be considered --- changelog/fix_false_positive_space_before_brackets.md | 1 + lib/rubocop/cop/layout/space_before_brackets.rb | 10 +++++----- spec/rubocop/cop/layout/space_before_brackets_spec.rb | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 changelog/fix_false_positive_space_before_brackets.md diff --git a/changelog/fix_false_positive_space_before_brackets.md b/changelog/fix_false_positive_space_before_brackets.md new file mode 100644 index 000000000000..65351fc1c0f6 --- /dev/null +++ b/changelog/fix_false_positive_space_before_brackets.md @@ -0,0 +1 @@ +* [#13356](https://github.com/rubocop/rubocop/issues/13356): Fix a false positive for `Layout/SpaceBeforeBrackets` when there is a dot before `[]=`. ([@earlopain][]) diff --git a/lib/rubocop/cop/layout/space_before_brackets.rb b/lib/rubocop/cop/layout/space_before_brackets.rb index fc455729762b..1e8a206a1a23 100644 --- a/lib/rubocop/cop/layout/space_before_brackets.rb +++ b/lib/rubocop/cop/layout/space_before_brackets.rb @@ -33,12 +33,12 @@ def on_send(node) private def offense_range(node, begin_pos) - if reference_variable_with_brackets?(node) - receiver_end_pos = node.receiver.source_range.end_pos - selector_begin_pos = node.loc.selector.begin_pos - return if receiver_end_pos >= selector_begin_pos - return if dot_before_brackets?(node, receiver_end_pos, selector_begin_pos) + receiver_end_pos = node.receiver.source_range.end_pos + selector_begin_pos = node.loc.selector.begin_pos + return if receiver_end_pos >= selector_begin_pos + return if dot_before_brackets?(node, receiver_end_pos, selector_begin_pos) + if reference_variable_with_brackets?(node) range_between(receiver_end_pos, selector_begin_pos) elsif node.method?(:[]=) offense_range_for_assignment(node, begin_pos) diff --git a/spec/rubocop/cop/layout/space_before_brackets_spec.rb b/spec/rubocop/cop/layout/space_before_brackets_spec.rb index 1e837993da54..1432b1e38652 100644 --- a/spec/rubocop/cop/layout/space_before_brackets_spec.rb +++ b/spec/rubocop/cop/layout/space_before_brackets_spec.rb @@ -108,6 +108,12 @@ @@collection.[](index_or_key) RUBY end + + it 'does not register an offense when call desugared `Hash#[]=`' do + expect_no_offenses(<<~RUBY) + collection.[]=(index_or_key, value) + RUBY + end end context 'when assigning' do From 7e82e44afdf65d585603a2b615e497eefa5b02a9 Mon Sep 17 00:00:00 2001 From: Takeshi KOMIYA Date: Sat, 19 Oct 2024 01:19:00 +0900 Subject: [PATCH 17/49] Add AllowSteepAnnotation option to Layout/LeadingCommentSpace [Steep](https://github.com/soutaro/steep) is a type checker for Ruby. The new option; `AllowSteepAnnotation` for `Layout/LeadingCommentSpace` allows to use Steep's annotation comments in the Ruby code. refs: https://github.com/soutaro/steep/wiki/Release-Note-1.3#add-type-assertion-and-type-application-syntax --- .../new_allow_steep_annotation_option.md | 1 + config/default.yml | 1 + .../cop/layout/leading_comment_space.rb | 30 +++++++++++++- .../cop/layout/leading_comment_space_spec.rb | 40 +++++++++++++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 changelog/new_allow_steep_annotation_option.md diff --git a/changelog/new_allow_steep_annotation_option.md b/changelog/new_allow_steep_annotation_option.md new file mode 100644 index 000000000000..8574d0c1daf4 --- /dev/null +++ b/changelog/new_allow_steep_annotation_option.md @@ -0,0 +1 @@ +* [#13360](https://github.com/rubocop/rubocop/pull/13360): Add `AllowSteepAnnotation` config option to `Layout/LeadingCommentSpace`. ([@tk0miya][]) diff --git a/config/default.yml b/config/default.yml index 3f76299b5083..324011e9ef4d 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1038,6 +1038,7 @@ Layout/LeadingCommentSpace: AllowDoxygenCommentStyle: false AllowGemfileRubyComment: false AllowRBSInlineAnnotation: false + AllowSteepAnnotation: false Layout/LeadingEmptyLines: Description: Check for unnecessary blank lines at the beginning of a file. diff --git a/lib/rubocop/cop/layout/leading_comment_space.rb b/lib/rubocop/cop/layout/leading_comment_space.rb index 23d3b5d716b3..068fd869fadf 100644 --- a/lib/rubocop/cop/layout/leading_comment_space.rb +++ b/lib/rubocop/cop/layout/leading_comment_space.rb @@ -67,19 +67,39 @@ module Layout # attr_reader :name #: String # attr_reader :age #: Integer? # + # @example AllowSteepAnnotation: false (default) + # + # # bad + # [1, 2, 3].each_with_object([]) do |n, list| #$ Array[Integer] + # list << n + # end + # + # name = 'John' #: String + # + # @example AllowSteepAnnotation: true + # + # # good + # + # [1, 2, 3].each_with_object([]) do |n, list| #$ Array[Integer] + # list << n + # end + # + # name = 'John' #: String + # class LeadingCommentSpace < Base include RangeHelp extend AutoCorrector MSG = 'Missing space after `#`.' - def on_new_investigation # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity + def on_new_investigation # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity processed_source.comments.each do |comment| next unless /\A(?!#\+\+|#--)(#+[^#\s=])/.match?(comment.text) next if comment.loc.line == 1 && allowed_on_first_line?(comment) next if doxygen_comment_style?(comment) next if gemfile_ruby_comment?(comment) next if rbs_inline_annotation?(comment) + next if steep_annotation?(comment) add_offense(comment) do |corrector| expr = comment.source_range @@ -142,6 +162,14 @@ def allow_rbs_inline_annotation? def rbs_inline_annotation?(comment) allow_rbs_inline_annotation? && comment.text.start_with?(/#:|#\[.+\]/) end + + def allow_steep_annotation? + cop_config['AllowSteepAnnotation'] + end + + def steep_annotation?(comment) + allow_steep_annotation? && comment.text.start_with?(/#[$:]/) + end end end end diff --git a/spec/rubocop/cop/layout/leading_comment_space_spec.rb b/spec/rubocop/cop/layout/leading_comment_space_spec.rb index 0e32ba0b98d4..7aedc0c0eee9 100644 --- a/spec/rubocop/cop/layout/leading_comment_space_spec.rb +++ b/spec/rubocop/cop/layout/leading_comment_space_spec.rb @@ -259,4 +259,44 @@ end end end + + describe 'Steep annotation' do + context 'when config option is disabled' do + let(:cop_config) { { 'AllowSteepAnnotation' => false } } + + it 'registers an offense and corrects using Steep annotation' do + expect_offense(<<~RUBY) + [1, 2, 3].each_with_object([]) do |n, list| #$ Array[Integer] + ^^^^^^^^^^^^^^^^^ Missing space after `#`. + list << n + end + + name = 'John' #: String + ^^^^^^^^^ Missing space after `#`. + RUBY + + expect_correction(<<~RUBY) + [1, 2, 3].each_with_object([]) do |n, list| # $ Array[Integer] + list << n + end + + name = 'John' # : String + RUBY + end + end + + context 'when config option is enabled' do + let(:cop_config) { { 'AllowSteepAnnotation' => true } } + + it 'does not register an offense when using Steep annotation' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].each_with_object([]) do |n, list| #$ Array[Integer] + list << n + end + + name = 'John' #: String + RUBY + end + end + end end From c6826941ce4bf45105cfc8a42b354501167db540 Mon Sep 17 00:00:00 2001 From: Vlad Pisanov Date: Fri, 18 Oct 2024 17:42:48 -0400 Subject: [PATCH 18/49] Fix false positives for `Style/RedundantInterpolationUnfreeze` and `Style/RedundantFreeze` when strings contain interpolated global, instance, and class variables --- .../fix_interpolated_string_detection.md | 1 + .../cop/mixin/frozen_string_literal.rb | 4 ++- .../cop/style/redundant_freeze_spec.rb | 30 +++++++++++++++++ .../redundant_interpolation_unfreeze_spec.rb | 33 +++++++++++++++++++ 4 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 changelog/fix_interpolated_string_detection.md diff --git a/changelog/fix_interpolated_string_detection.md b/changelog/fix_interpolated_string_detection.md new file mode 100644 index 000000000000..50ecff9cf2ca --- /dev/null +++ b/changelog/fix_interpolated_string_detection.md @@ -0,0 +1 @@ +* [#13361](https://github.com/rubocop/rubocop/issues/13361): Fix false positives for `Style/RedundantInterpolationUnfreeze` and `Style/RedundantFreeze` when strings contain interpolated global, instance, and class variables. ([@vlad-pisanov][]) diff --git a/lib/rubocop/cop/mixin/frozen_string_literal.rb b/lib/rubocop/cop/mixin/frozen_string_literal.rb index 315880708bd6..e3402d0b2ad6 100644 --- a/lib/rubocop/cop/mixin/frozen_string_literal.rb +++ b/lib/rubocop/cop/mixin/frozen_string_literal.rb @@ -29,7 +29,9 @@ def frozen_string_literal?(node) end def uninterpolated_string?(node) - node.str_type? || (node.dstr_type? && node.each_descendant(:begin).none?) + node.str_type? || ( + node.dstr_type? && node.each_descendant(:begin, :ivar, :cvar, :gvar).none? + ) end def uninterpolated_heredoc?(node) diff --git a/spec/rubocop/cop/style/redundant_freeze_spec.rb b/spec/rubocop/cop/style/redundant_freeze_spec.rb index fff65e44a3df..e4751e7ea701 100644 --- a/spec/rubocop/cop/style/redundant_freeze_spec.rb +++ b/spec/rubocop/cop/style/redundant_freeze_spec.rb @@ -38,6 +38,9 @@ it_behaves_like 'mutable objects', '{ a: 1, b: 2 }' it_behaves_like 'mutable objects', "'str'" it_behaves_like 'mutable objects', '"top#{1 + 2}"' + it_behaves_like 'mutable objects', '"top#@foo"' + it_behaves_like 'mutable objects', '"top#@@foo"' + it_behaves_like 'mutable objects', '"top#$foo"' it_behaves_like 'mutable objects', "('a' + 'b')" it_behaves_like 'mutable objects', "('a' * 20)" it_behaves_like 'mutable objects', '(a + b)' @@ -103,18 +106,27 @@ context 'when the frozen string literal comment is missing' do it_behaves_like 'immutable objects', '"#{a}"' + it_behaves_like 'immutable objects', '"#@a"' + it_behaves_like 'immutable objects', '"#@@a"' + it_behaves_like 'immutable objects', '"#$a"' end context 'when the frozen string literal comment is true' do let(:prefix) { '# frozen_string_literal: true' } it_behaves_like 'immutable objects', '"#{a}"' + it_behaves_like 'immutable objects', '"#@a"' + it_behaves_like 'immutable objects', '"#@@a"' + it_behaves_like 'immutable objects', '"#$a"' end context 'when the frozen string literal comment is false' do let(:prefix) { '# frozen_string_literal: false' } it_behaves_like 'immutable objects', '"#{a}"' + it_behaves_like 'immutable objects', '"#@a"' + it_behaves_like 'immutable objects', '"#@@a"' + it_behaves_like 'immutable objects', '"#$a"' end end end @@ -122,36 +134,54 @@ context 'Ruby 3.0 or higher', :ruby30 do context 'when the frozen string literal comment is missing' do it_behaves_like 'mutable objects', '"#{a}"' + it_behaves_like 'mutable objects', '"#@a"' + it_behaves_like 'mutable objects', '"#@@a"' + it_behaves_like 'mutable objects', '"#$a"' end context 'when the frozen string literal comment is true' do let(:prefix) { '# frozen_string_literal: true' } it_behaves_like 'mutable objects', '"#{a}"' + it_behaves_like 'mutable objects', '"#@a"' + it_behaves_like 'mutable objects', '"#@@a"' + it_behaves_like 'mutable objects', '"#$a"' end context 'when the frozen string literal comment is false' do let(:prefix) { '# frozen_string_literal: false' } it_behaves_like 'mutable objects', '"#{a}"' + it_behaves_like 'mutable objects', '"#@a"' + it_behaves_like 'mutable objects', '"#@@a"' + it_behaves_like 'mutable objects', '"#$a"' end end context 'Ruby 2.7 or lower', :ruby27, unsupported_on: :prism do context 'when the frozen string literal comment is missing' do it_behaves_like 'mutable objects', '"#{a}"' + it_behaves_like 'mutable objects', '"#@a"' + it_behaves_like 'mutable objects', '"#@@a"' + it_behaves_like 'mutable objects', '"#$a"' end context 'when the frozen string literal comment is true' do let(:prefix) { '# frozen_string_literal: true' } it_behaves_like 'immutable objects', '"#{a}"' + it_behaves_like 'immutable objects', '"#@a"' + it_behaves_like 'immutable objects', '"#@@a"' + it_behaves_like 'immutable objects', '"#$a"' end context 'when the frozen string literal comment is false' do let(:prefix) { '# frozen_string_literal: false' } it_behaves_like 'mutable objects', '"#{a}"' + it_behaves_like 'mutable objects', '"#@a"' + it_behaves_like 'mutable objects', '"#@@a"' + it_behaves_like 'mutable objects', '"#$a"' end end diff --git a/spec/rubocop/cop/style/redundant_interpolation_unfreeze_spec.rb b/spec/rubocop/cop/style/redundant_interpolation_unfreeze_spec.rb index 918d886a7149..5a6efd745e13 100644 --- a/spec/rubocop/cop/style/redundant_interpolation_unfreeze_spec.rb +++ b/spec/rubocop/cop/style/redundant_interpolation_unfreeze_spec.rb @@ -52,6 +52,39 @@ RUBY end + it 'registers an offense for interpolated strings with global variables' do + expect_offense(<<~'RUBY') + +"#$var" + ^ Don't unfreeze interpolated strings as they are already unfrozen. + RUBY + + expect_correction(<<~'RUBY') + "#$var" + RUBY + end + + it 'registers an offense for strings with interpolated instance variables' do + expect_offense(<<~'RUBY') + +"#@var" + ^ Don't unfreeze interpolated strings as they are already unfrozen. + RUBY + + expect_correction(<<~'RUBY') + "#@var" + RUBY + end + + it 'registers an offense for strings with interpolated class variables' do + expect_offense(<<~'RUBY') + +"#@@var" + ^ Don't unfreeze interpolated strings as they are already unfrozen. + RUBY + + expect_correction(<<~'RUBY') + "#@@var" + RUBY + end + it 'registers an offense for interpolated heredoc with `dup`' do expect_offense(<<~'RUBY') foo(<<~MSG.dup) From 512b032ac5bc4832800d3b2cfdd882b9770ab488 Mon Sep 17 00:00:00 2001 From: Vlad Pisanov Date: Sun, 22 Sep 2024 10:12:15 -0400 Subject: [PATCH 19/49] Fix false negatives for `Style/MapIntoArray` when using non-splatted arguments --- ...alse_negatives_for_style_map_into_array.md | 1 + lib/rubocop/cop/style/map_into_array.rb | 7 +- spec/rubocop/cop/style/map_into_array_spec.rb | 132 ++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 changelog/fix_false_negatives_for_style_map_into_array.md diff --git a/changelog/fix_false_negatives_for_style_map_into_array.md b/changelog/fix_false_negatives_for_style_map_into_array.md new file mode 100644 index 000000000000..2ae9e93f06f6 --- /dev/null +++ b/changelog/fix_false_negatives_for_style_map_into_array.md @@ -0,0 +1 @@ +* [#13255](https://github.com/rubocop/rubocop/pull/13255): Fix false negatives for `Style/MapIntoArray` when using non-splatted arguments. ([@vlad-pisanov][]) diff --git a/lib/rubocop/cop/style/map_into_array.rb b/lib/rubocop/cop/style/map_into_array.rb index 1c5eff8a2bf0..0b716a63632e 100644 --- a/lib/rubocop/cop/style/map_into_array.rb +++ b/lib/rubocop/cop/style/map_into_array.rb @@ -53,12 +53,17 @@ class MapIntoArray < Base MSG = 'Use `%s` instead of `each` to map elements into an array.' + # @!method suitable_argument_node?(node) + def_node_matcher :suitable_argument_node?, <<-PATTERN + !{splat forwarded-restarg forwarded-args (hash (forwarded-kwrestarg)) (block-pass nil?)} + PATTERN + # @!method each_block_with_push?(node) def_node_matcher :each_block_with_push?, <<-PATTERN [ ^({begin kwbegin} ...) ({block numblock} (send !{nil? self} :each) _ - (send (lvar _) {:<< :push :append} {send lvar begin})) + (send (lvar _) {:<< :push :append} #suitable_argument_node?)) ] PATTERN diff --git a/spec/rubocop/cop/style/map_into_array_spec.rb b/spec/rubocop/cop/style/map_into_array_spec.rb index 6a675084880d..f7685524922c 100644 --- a/spec/rubocop/cop/style/map_into_array_spec.rb +++ b/spec/rubocop/cop/style/map_into_array_spec.rb @@ -65,6 +65,102 @@ RUBY end + it 'registers an offense and corrects when using an if statement' do + expect_offense(<<~RUBY) + dest = [] + src.each do |e| + ^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest << if cond? + e + else + e * 2 + end + end + RUBY + + expect_correction(<<~RUBY) + dest = src.map do |e| + if cond? + e + else + e * 2 + end + end + RUBY + end + + it 'registers an offense and corrects when using a case statement' do + expect_offense(<<~RUBY) + dest = [] + src.each do |e| + ^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest << case foo + when 1 + e + else + e * 2 + end + end + RUBY + + expect_correction(<<~RUBY) + dest = src.map do |e| + case foo + when 1 + e + else + e * 2 + end + end + RUBY + end + + it 'registers an offense and corrects when using a begin block' do + expect_offense(<<~RUBY) + dest = [] + src.each do |e| + ^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest << begin + foo + e * 2 + end + end + RUBY + + expect_correction(<<~RUBY) + dest = src.map do |e| + begin + foo + e * 2 + end + end + RUBY + end + + it 'registers an offense and corrects when using an array literal' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << [1, e] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| [1, e] } + RUBY + end + + it 'registers an offense and corrects when using a literal' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << true } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| true } + RUBY + end + it 'registers an offense and corrects when using a numblock' do expect_offense(<<~RUBY) dest = [] @@ -215,6 +311,42 @@ RUBY end + it 'does not register an offense when pushing anonymously-forwarded block', :ruby31 do + expect_no_offenses(<<~RUBY) + def foo(&) + dest = [] + src.each { |e| dest.push(&) } + end + RUBY + end + + it 'does not register an offense when pushing anonymously-forwarded restarg', :ruby32 do + expect_no_offenses(<<~RUBY) + def foo(*) + dest = [] + src.each { |e| dest.push(*) } + end + RUBY + end + + it 'does not register an offense when pushing anonymously-forwarded kwrestarg', :ruby32, unsupported_on: :prism do + expect_no_offenses(<<~RUBY) + def foo(**) + dest = [] + src.each { |e| dest.push(**) } + end + RUBY + end + + it 'does not register an offense when pushing anonymously-forwarded args' do + expect_no_offenses(<<~RUBY) + def foo(...) + dest = [] + src.each { |e| dest.push(...) } + end + RUBY + end + context 'destination initializer' do shared_examples 'corrects' do |initializer:| it 'registers an offense and corrects' do From 0553bcfb26bec56d39d705415ea1ac30528b8514 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 18 Jul 2024 00:56:32 +0900 Subject: [PATCH 20/49] [Fix #13050] Add new `Style/BitwisePredicate` cop Closes #13050. This PR adds new `Style/BitwisePredicates` cop, which checks for usage of `&` operator in bitwise operations involving comparisons. ```ruby # bad - checks any set bits (variable & flags).positive? # good variable.anybits?(flags) # bad - checks all set bits (variable & flags) == flags # good variable.allbits?(flags) # bad - checks no set bits (variable & flags).zero? (variable & flags) == 0 # good variable.nobits?(flags) ``` I've opened a proposal in the style guide at https://github.com/rubocop/ruby-style-guide/pull/944. --- ...new_add_new_style_bitwise_predicate_cop.md | 1 + config/default.yml | 7 + lib/rubocop.rb | 1 + lib/rubocop/cop/style/bitwise_predicate.rb | 100 +++++++++++ .../cop/style/bitwise_predicate_spec.rb | 163 ++++++++++++++++++ 5 files changed, 272 insertions(+) create mode 100644 changelog/new_add_new_style_bitwise_predicate_cop.md create mode 100644 lib/rubocop/cop/style/bitwise_predicate.rb create mode 100644 spec/rubocop/cop/style/bitwise_predicate_spec.rb diff --git a/changelog/new_add_new_style_bitwise_predicate_cop.md b/changelog/new_add_new_style_bitwise_predicate_cop.md new file mode 100644 index 000000000000..f49cd563fe5b --- /dev/null +++ b/changelog/new_add_new_style_bitwise_predicate_cop.md @@ -0,0 +1 @@ +* [#13050](https://github.com/rubocop/rubocop/issues/13050): Add new `Style/BitwisePredicate` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 324011e9ef4d..ca04a15d88e7 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3257,6 +3257,13 @@ Style/BisectedAttrAccessor: Enabled: true VersionAdded: '0.87' +Style/BitwisePredicate: + Description: 'Prefer bitwise predicate methods over direct comparison operations.' + StyleGuide: '#bitwise-predicate-methods' + Enabled: pending + Safe: false + VersionAdded: '<>' + Style/BlockComments: Description: 'Do not use block comments.' StyleGuide: '#no-block-comments' diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 63c4ef14b11a..0fbff1ad6b4a 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -474,6 +474,7 @@ require_relative 'rubocop/cop/style/bare_percent_literals' require_relative 'rubocop/cop/style/begin_block' require_relative 'rubocop/cop/style/bisected_attr_accessor' +require_relative 'rubocop/cop/style/bitwise_predicate' require_relative 'rubocop/cop/style/block_comments' require_relative 'rubocop/cop/style/block_delimiters' require_relative 'rubocop/cop/style/case_equality' diff --git a/lib/rubocop/cop/style/bitwise_predicate.rb b/lib/rubocop/cop/style/bitwise_predicate.rb new file mode 100644 index 000000000000..5331c79431cb --- /dev/null +++ b/lib/rubocop/cop/style/bitwise_predicate.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # Prefer bitwise predicate methods over direct comparison operations. + # + # @safety + # This cop is unsafe, as it can produce false positives if the receiver + # is not an `Integer` object. + # + # @example + # + # # bad - checks any set bits + # (variable & flags).positive? + # + # # good + # variable.anybits?(flags) + # + # # bad - checks all set bits + # (variable & flags) == flags + # + # # good + # variable.allbits?(flags) + # + # # bad - checks no set bits + # (variable & flags).zero? + # + # # good + # variable.nobits?(flags) + # + class BitwisePredicate < Base + extend AutoCorrector + extend TargetRubyVersion + + MSG = 'Replace with `%s` for comparison with bit flags.' + RESTRICT_ON_SEND = %i[!= == > >= positive? zero?].freeze + + minimum_target_ruby_version 2.5 + + # @!method anybits?(node) + def_node_matcher :anybits?, <<~PATTERN + { + (send #bit_operation? :positive?) + (send #bit_operation? :> (int 0)) + (send #bit_operation? :>= (int 1)) + (send #bit_operation? :!= (int 0)) + } + PATTERN + + # @!method allbits?(node) + def_node_matcher :allbits?, <<~PATTERN + { + (send (begin (send _ :& _flags)) :== _flags) + (send (begin (send _flags :& _)) :== _flags) + } + PATTERN + + # @!method nobits?(node) + def_node_matcher :nobits?, <<~PATTERN + { + (send #bit_operation? :zero?) + (send #bit_operation? :== (int 0)) + } + PATTERN + + # @!method bit_operation?(node) + def_node_matcher :bit_operation?, <<~PATTERN + (begin + (send _ :& _)) + PATTERN + + def on_send(node) + return unless node.receiver.begin_type? + return unless (preferred_method = preferred_method(node)) + + bit_operation = node.receiver.children.first + lhs, _operator, rhs = *bit_operation + preferred = "#{lhs.source}.#{preferred_method}(#{rhs.source})" + + add_offense(node, message: format(MSG, preferred: preferred)) do |corrector| + corrector.replace(node, preferred) + end + end + + private + + def preferred_method(node) + if anybits?(node) + 'anybits?' + elsif allbits?(node) + 'allbits?' + elsif nobits?(node) + 'nobits?' + end + end + end + end + end +end diff --git a/spec/rubocop/cop/style/bitwise_predicate_spec.rb b/spec/rubocop/cop/style/bitwise_predicate_spec.rb new file mode 100644 index 000000000000..cb83ec847f18 --- /dev/null +++ b/spec/rubocop/cop/style/bitwise_predicate_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::BitwisePredicate, :config do + context 'when checking any set bits' do + context 'when Ruby >= 2.5', :ruby25 do + it 'registers an offense when using `&` in conjunction with `predicate` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags).positive? + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.anybits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.anybits?(flags) + RUBY + end + + it 'registers an offense when using `&` in conjunction with `> 0` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags) > 0 + ^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.anybits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.anybits?(flags) + RUBY + end + + it 'registers an offense when using `&` in conjunction with `>= 1` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags) >= 1 + ^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.anybits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.anybits?(flags) + RUBY + end + + it 'registers an offense when using `&` in conjunction with `!= 0` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags) != 0 + ^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.anybits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.anybits?(flags) + RUBY + end + + it 'does not register an offense when using `anybits?` method' do + expect_no_offenses(<<~RUBY) + variable.anybits?(flags) + RUBY + end + + it 'does not register an offense when using `&` in conjunction with `> 1` for comparisons' do + expect_no_offenses(<<~RUBY) + (variable & flags) > 1 + RUBY + end + + it 'does not register an offense when comparing with no parentheses' do + expect_no_offenses(<<~RUBY) + foo == bar + RUBY + end + end + + context 'when Ruby <= 2.4', :ruby24, unsupported_on: :prism do + it 'does not register an offense when using `&` in conjunction with `predicate` for comparisons' do + expect_no_offenses(<<~RUBY) + (variable & flags).positive? + RUBY + end + end + end + + context 'when checking all set bits' do + context 'when Ruby >= 2.5', :ruby25 do + it 'registers an offense when using `&` with RHS flags in conjunction with `==` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags) == flags + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.allbits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.allbits?(flags) + RUBY + end + + it 'registers an offense when using `&` with LHS flags in conjunction with `==` for comparisons' do + expect_offense(<<~RUBY) + (flags & variable) == flags + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Replace with `flags.allbits?(variable)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + flags.allbits?(variable) + RUBY + end + + it 'does not register an offense when using `allbits?` method' do + expect_no_offenses(<<~RUBY) + variable.allbits?(flags) + RUBY + end + + it 'does not register an offense when flag variable names are mismatched' do + expect_no_offenses(<<~RUBY) + (flags & variable) == flagments + RUBY + end + end + + context 'when Ruby <= 2.4', :ruby24, unsupported_on: :prism do + it 'does not register an offense when using `&` with RHS flags in conjunction with `==` for comparisons' do + expect_no_offenses(<<~RUBY) + (variable & flags) == flags + RUBY + end + end + end + + context 'when checking no set bits' do + context 'when Ruby >= 2.5', :ruby25 do + it 'registers an offense when using `&` in conjunction with `zero?` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags).zero? + ^^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.nobits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.nobits?(flags) + RUBY + end + + it 'registers an offense when using `&` in conjunction with `== 0` for comparisons' do + expect_offense(<<~RUBY) + (variable & flags) == 0 + ^^^^^^^^^^^^^^^^^^^^^^^ Replace with `variable.nobits?(flags)` for comparison with bit flags. + RUBY + + expect_correction(<<~RUBY) + variable.nobits?(flags) + RUBY + end + + it 'does not register an offense when using `nobits?` method' do + expect_no_offenses(<<~RUBY) + variable.nobits?(flags) + RUBY + end + end + + context 'when Ruby <= 2.4', :ruby24, unsupported_on: :prism do + it 'does not register an offense when using `&` in conjunction with `zero?` for comparisons' do + expect_no_offenses(<<~RUBY) + (variable & flags).zero? + RUBY + end + end + end +end From 0ee219d9a7fe1893c1712f57698f3782798090d2 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 6 Sep 2024 02:23:48 +0300 Subject: [PATCH 21/49] Add new `IgnoreDuplicateElseBranch` option to `Lint/DuplicateBranch` --- ...ew_duplicate_branch_ignore_elses_option.md | 1 + config/default.yml | 1 + lib/rubocop/cop/lint/duplicate_branch.rb | 43 +++++++- .../rubocop/cop/lint/duplicate_branch_spec.rb | 99 +++++++++++++++++++ 4 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 changelog/new_duplicate_branch_ignore_elses_option.md diff --git a/changelog/new_duplicate_branch_ignore_elses_option.md b/changelog/new_duplicate_branch_ignore_elses_option.md new file mode 100644 index 000000000000..3f87e6838cb9 --- /dev/null +++ b/changelog/new_duplicate_branch_ignore_elses_option.md @@ -0,0 +1 @@ +* [#13146](https://github.com/rubocop/rubocop/issues/13146): Add new `IgnoreDuplicateElseBranch` option to `Lint/DuplicateBranch`. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index ca04a15d88e7..7ae1205e5a2c 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1786,6 +1786,7 @@ Lint/DuplicateBranch: VersionChanged: '1.7' IgnoreLiteralBranches: false IgnoreConstantBranches: false + IgnoreDuplicateElseBranch: false Lint/DuplicateCaseCondition: Description: 'Do not repeat values in case conditionals.' diff --git a/lib/rubocop/cop/lint/duplicate_branch.rb b/lib/rubocop/cop/lint/duplicate_branch.rb index cb626d64b41a..681d5b18637c 100644 --- a/lib/rubocop/cop/lint/duplicate_branch.rb +++ b/lib/rubocop/cop/lint/duplicate_branch.rb @@ -15,6 +15,9 @@ module Lint # With `IgnoreConstantBranches: true`, branches are not registered # as offenses if they return a constant value. # + # With `IgnoreDuplicateElseBranch: true`, in conditionals with multiple branches, + # duplicate 'else' branches are not registered as offenses. + # # @example # # bad # if foo @@ -83,21 +86,37 @@ module Lint # else MEDIUM_SIZE # end # + # @example IgnoreDuplicateElseBranch: true + # # good + # if foo + # do_foo + # elsif bar + # do_bar + # else + # do_foo + # end + # class DuplicateBranch < Base MSG = 'Duplicate branch body detected.' def on_branching_statement(node) - branches(node).each_with_object(Set.new) do |branch, previous| - next unless consider_branch?(branch) + branches = branches(node) + branches.each_with_object(Set.new) do |branch, previous| + next unless consider_branch?(branches, branch) add_offense(offense_range(branch)) unless previous.add?(branch) end end - alias on_if on_branching_statement alias on_case on_branching_statement alias on_case_match on_branching_statement alias on_rescue on_branching_statement + def on_if(node) + # Ignore 'elsif' nodes, because we don't want to check them separately whether + # the 'else' branch is duplicated. We want to check only on the outermost conditional. + on_branching_statement(node) unless node.elsif? + end + private def offense_range(duplicate_branch) @@ -118,10 +137,14 @@ def branches(node) node.branches.compact end - def consider_branch?(branch) + def consider_branch?(branches, branch) return false if ignore_literal_branches? && literal_branch?(branch) return false if ignore_constant_branches? && const_branch?(branch) + if ignore_duplicate_else_branches? && duplicate_else_branch?(branches, branch) + return false + end + true end @@ -133,6 +156,10 @@ def ignore_constant_branches? cop_config.fetch('IgnoreConstantBranches', false) end + def ignore_duplicate_else_branches? + cop_config.fetch('IgnoreDuplicateElseBranch', false) + end + def literal_branch?(branch) # rubocop:disable Metrics/CyclomaticComplexity return false if !branch.literal? || branch.xstr_type? return true if branch.basic_literal? @@ -147,6 +174,14 @@ def literal_branch?(branch) # rubocop:disable Metrics/CyclomaticComplexity def const_branch?(branch) branch.const_type? end + + def duplicate_else_branch?(branches, branch) + return false unless (parent = branch.parent) + + branches.size > 2 && + branch.equal?(branches.last) && + parent.respond_to?(:else?) && parent.else? + end end end end diff --git a/spec/rubocop/cop/lint/duplicate_branch_spec.rb b/spec/rubocop/cop/lint/duplicate_branch_spec.rb index 1137c04ad1b5..3bf70c0973ee 100644 --- a/spec/rubocop/cop/lint/duplicate_branch_spec.rb +++ b/spec/rubocop/cop/lint/duplicate_branch_spec.rb @@ -438,4 +438,103 @@ end end end + + context 'with IgnoreDuplicateElseBranch: true' do + let(:cop_config) { { 'IgnoreDuplicateElseBranch' => true } } + + it 'allows duplicate `else` branch for `if-elsif`' do + expect_no_offenses(<<~RUBY) + if x + foo + elsif y + bar + else + bar + end + RUBY + end + + it 'registers an offense for duplicate `if-else`' do + expect_offense(<<~RUBY) + if x + foo + else + ^^^^ Duplicate branch body detected. + foo + end + RUBY + end + + it 'allows duplicate `else` branch for `case` with multiple `when` branches' do + expect_no_offenses(<<~RUBY) + case x + when foo + do_foo + when bar + do_bar + else + do_bar + end + RUBY + end + + it 'registers an offense for duplicate `else` branch for `case` with single `when` branch' do + expect_offense(<<~RUBY) + case x + when foo + do_foo + else + ^^^^ Duplicate branch body detected. + do_foo + end + RUBY + end + + it 'allows duplicate `else` branch for `case` with multiple `match` branches' do + expect_no_offenses(<<~RUBY) + case x + in foo then do_foo + in bar then do_bar + else do_bar + end + RUBY + end + + it 'registers an offense for duplicate `else` branch for `case` with single `match` branch' do + expect_offense(<<~RUBY) + case x + in foo then do_foo + else do_foo + ^^^^ Duplicate branch body detected. + end + RUBY + end + + it 'allows duplicate `else` branch for `rescue` with multiple `resbody` branches' do + expect_no_offenses(<<~RUBY) + begin + x + rescue FooError + foo + rescue BarError + bar + else + bar + end + RUBY + end + + it 'registers an offense for duplicate `else` branch for `rescue` with single `resbody` branch' do + expect_offense(<<~RUBY) + begin + x + rescue FooError + foo + else + ^^^^ Duplicate branch body detected. + foo + end + RUBY + end + end end From 2e2fe0eef5f6bd631697ef7babdcd57d1f8a0aa7 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Thu, 5 Sep 2024 00:04:07 +0300 Subject: [PATCH 22/49] Fix false positive in `Style/MultipleComparison` when `ComparisonsThreshold` exceeds 2 Co-authored-by: Vlad Pisanov --- changelog/fix_style_multiple_comparison.md | 1 + lib/rubocop/cop/style/multiple_comparison.rb | 67 ++++++++----------- .../cop/style/multiple_comparison_spec.rb | 33 +++++++++ 3 files changed, 62 insertions(+), 39 deletions(-) create mode 100644 changelog/fix_style_multiple_comparison.md diff --git a/changelog/fix_style_multiple_comparison.md b/changelog/fix_style_multiple_comparison.md new file mode 100644 index 000000000000..a85e818ab355 --- /dev/null +++ b/changelog/fix_style_multiple_comparison.md @@ -0,0 +1 @@ +* [#13193](https://github.com/rubocop/rubocop/issues/13193): Fix false positive in `Style/MultipleComparison` when `ComparisonsThreshold` exceeds 2. ([@fatkodima][],[@vlad-pisanov][]) diff --git a/lib/rubocop/cop/style/multiple_comparison.rb b/lib/rubocop/cop/style/multiple_comparison.rb index 1205880b040e..9cf50a1d11d7 100644 --- a/lib/rubocop/cop/style/multiple_comparison.rb +++ b/lib/rubocop/cop/style/multiple_comparison.rb @@ -55,25 +55,19 @@ class MultipleComparison < Base MSG = 'Avoid comparing a variable with multiple items ' \ 'in a conditional, use `Array#include?` instead.' - def on_new_investigation - reset_comparison - end - def on_or(node) root_of_or_node = root_of_or_node(node) - return unless node == root_of_or_node - return unless nested_variable_comparison?(root_of_or_node) - return if @allowed_method_comparison - return if @compared_elements.size < comparisons_threshold + return unless nested_comparison?(node) + + return unless (variable, values = find_offending_var(node)) + return if values.size < comparisons_threshold add_offense(node) do |corrector| - elements = @compared_elements.join(', ') - prefer_method = "[#{elements}].include?(#{variables_in_node(node).first})" + elements = values.map(&:source).join(', ') + prefer_method = "[#{elements}].include?(#{variable_name(variable)})" corrector.replace(node, prefer_method) - - reset_comparison end end @@ -92,32 +86,25 @@ def on_or(node) (send $_ :== $lvar) PATTERN - def nested_variable_comparison?(node) - return false unless nested_comparison?(node) - - variables_in_node(node).count == 1 - end - - def variables_in_node(node) + # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + def find_offending_var(node, variables = Set.new, values = []) if node.or_type? - node.node_parts.flat_map { |node_part| variables_in_node(node_part) }.uniq - else - variables_in_simple_node(node) - end - end + find_offending_var(node.lhs, variables, values) + find_offending_var(node.rhs, variables, values) + elsif simple_double_comparison?(node) + return + elsif (var, obj = simple_comparison?(node)) + return if allow_method_comparison? && obj.send_type? - def variables_in_simple_node(node) - simple_double_comparison?(node) do |var1, var2| - return [variable_name(var1), variable_name(var2)] - end - if (var, obj = simple_comparison_lhs?(node)) || (obj, var = simple_comparison_rhs?(node)) - @allowed_method_comparison = true if allow_method_comparison? && obj.send_type? - @compared_elements << obj.source - return [variable_name(var)] + variables << var + return if variables.size > 1 + + values << obj end - [] + [variables.first, values] if variables.any? end + # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def variable_name(node) node.children[0] @@ -132,7 +119,14 @@ def nested_comparison?(node) end def comparison?(node) - simple_comparison_lhs?(node) || simple_comparison_rhs?(node) || nested_comparison?(node) + simple_comparison?(node) || nested_comparison?(node) + end + + def simple_comparison?(node) + if (var, obj = simple_comparison_lhs?(node)) || + (obj, var = simple_comparison_rhs?(node)) + [var, obj] + end end def root_of_or_node(or_node) @@ -145,11 +139,6 @@ def root_of_or_node(or_node) end end - def reset_comparison - @compared_elements = [] - @allowed_method_comparison = false - end - def allow_method_comparison? cop_config.fetch('AllowMethodComparison', true) end diff --git a/spec/rubocop/cop/style/multiple_comparison_spec.rb b/spec/rubocop/cop/style/multiple_comparison_spec.rb index 9bf3e7c0a059..b60042590ecb 100644 --- a/spec/rubocop/cop/style/multiple_comparison_spec.rb +++ b/spec/rubocop/cop/style/multiple_comparison_spec.rb @@ -259,11 +259,44 @@ def do_something(foo, bar) context 'when `ComparisonsThreshold`: 3' do let(:cop_config) { { 'ComparisonsThreshold' => 3 } } + it 'registers an offense and corrects when `a` is compared thrice' do + expect_offense(<<~RUBY) + a = "a" + foo if a == "a" || a == "b" || a == "c" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid comparing a variable with multiple items in a conditional, use `Array#include?` instead. + RUBY + + expect_correction(<<~RUBY) + a = "a" + foo if ["a", "b", "c"].include?(a) + RUBY + end + it 'does not register an offense when `a` is compared twice' do expect_no_offenses(<<~RUBY) a = "a" foo if a == "a" || a == "b" RUBY end + + it 'does not register an offense when `a` is compared twice in multiple expressions' do + expect_no_offenses(<<~RUBY) + a = "a" + foo if a == "a" || a == "b" + bar if a == "a" || a == "b" + RUBY + end + + it 'does not register an offense when `a` is compared twice in different contexts expressions' do + expect_no_offenses(<<~RUBY) + def foo(a) + a == "a" || a == "b" + end + + def bar(a) + a == "a" || a == "b" + end + RUBY + end end end From 756d6478cde254220bb103265feadf09a7d727bf Mon Sep 17 00:00:00 2001 From: masato-bkn <37011138+masato-bkn@users.noreply.github.com> Date: Sun, 20 Oct 2024 16:14:27 +0900 Subject: [PATCH 23/49] [Doc] Fix the broken example in the `Metrics/CyclomaticComplexity` document --- lib/rubocop/cop/metrics/cyclomatic_complexity.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/rubocop/cop/metrics/cyclomatic_complexity.rb b/lib/rubocop/cop/metrics/cyclomatic_complexity.rb index 6207785ba1cd..d079399ab7b4 100644 --- a/lib/rubocop/cop/metrics/cyclomatic_complexity.rb +++ b/lib/rubocop/cop/metrics/cyclomatic_complexity.rb @@ -14,11 +14,14 @@ module Metrics # and ||/or is shorthand for a sequence of ifs, so they also add one. # Loops can be said to have an exit condition, so they add one. # Blocks that are calls to builtin iteration methods - # (e.g. `ary.map{...}) also add one, others are ignored. + # (e.g. `ary.map{...}`) also add one, others are ignored. + # + # @example # # def each_child_node(*types) # count begins: 1 # unless block_given? # unless: +1 # return to_enum(__method__, *types) + # end # # children.each do |child| # each{}: +1 # next unless child.is_a?(Node) # unless: +1 From eb73a9eb31416425cab5b912b2ed104f43332942 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 22 Oct 2024 00:31:09 +0900 Subject: [PATCH 24/49] [Fix #13368] Fix an incorrect autocorrect for `Naming/BlockForwarding` Fixes #13368. This PR fixes an incorrect autocorrect for `Naming/BlockForwarding` with `Style/ExplicitBlockArgument`. --- ...autocorrect_for_naming_block_forwarding.md | 1 + lib/rubocop/cop/naming/block_forwarding.rb | 2 +- spec/rubocop/cli/autocorrect_spec.rb | 24 +++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 changelog/fix_an_incorrect_autocorrect_for_naming_block_forwarding.md diff --git a/changelog/fix_an_incorrect_autocorrect_for_naming_block_forwarding.md b/changelog/fix_an_incorrect_autocorrect_for_naming_block_forwarding.md new file mode 100644 index 000000000000..dccdc33b280b --- /dev/null +++ b/changelog/fix_an_incorrect_autocorrect_for_naming_block_forwarding.md @@ -0,0 +1 @@ +* [#13386](https://github.com/rubocop/rubocop/issues/13386): Fix an incorrect autocorrect for `Naming/BlockForwarding` with `Style/ExplicitBlockArgument`. ([@koic][]) diff --git a/lib/rubocop/cop/naming/block_forwarding.rb b/lib/rubocop/cop/naming/block_forwarding.rb index 2b36857f6ca3..016e61594d8b 100644 --- a/lib/rubocop/cop/naming/block_forwarding.rb +++ b/lib/rubocop/cop/naming/block_forwarding.rb @@ -48,7 +48,7 @@ class BlockForwarding < Base MSG = 'Use %