Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #10118] Fix crash with Style/RedundantSort when the block doesn't only contain a single send node #10119

Merged
merged 1 commit into from Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/fix_fix_crash_with_styleredundantsort_when.md
@@ -0,0 +1 @@
* [#10118](https://github.com/rubocop/rubocop/issues/10118): Fix crash with `Style/RedundantSort` when the block doesn't only contain a single `send` node. ([@dvandersluis][])
9 changes: 8 additions & 1 deletion lib/rubocop/cop/style/redundant_sort.rb
Expand Up @@ -93,12 +93,19 @@ def on_send(node)
private

def use_size_method_in_block?(sort_node)
return true if sort_node.send_type? && sort_node.block_node&.body&.method?(:size)
return true if block_calls_send?(sort_node)
return false unless sort_node.block_argument?

sort_node.last_argument.children.first.value == :size
end

def block_calls_send?(node)
return false unless node.send_type? && node.block_node
return false unless node.block_node.body.send_type?

node.block_node.body.method?(:size)
end

def register_offense(ancestor, sort_node, sorter, accessor)
message = message(ancestor, sorter, accessor)

Expand Down
15 changes: 13 additions & 2 deletions spec/rubocop/cop/style/redundant_sort_spec.rb
Expand Up @@ -231,12 +231,12 @@
expect_no_offenses('[[1, 2], [3, 4]].first.sort')
end

# `[3, 1, 2].sort_by(&:size).last` is equivalent to `[3, 1, 2].max_by(&:size)`.
# `[2, 1, 3].sort_by(&:size).last` is not equivalent to `[2, 1, 3].max_by(&:size)`.
it 'does not register an offense when using `sort_by(&:size).last`' do
expect_no_offenses('[2, 1, 3].sort_by(&:size).last')
end

# `[3, 1, 2].sort_by { |i| i.size }.last` is equivalent to `[3, 1, 2].max_by { |i| i.size }`.
# `[2, 1, 3].sort_by { |i| i.size }.last` is not equivalent to `[2, 1, 3].max_by { |i| i.size }`.
Comment on lines -234 to +239
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that these comments had a typo so I figured I'd fix them while I'm looking at it.

it 'does not register an offense when using `sort_by { |i| i.size }.last`' do
expect_no_offenses('[2, 1, 3].sort_by { |i| i.size }.last')
end
Expand All @@ -247,6 +247,17 @@
expect_no_offenses('[2, 1, 3].sort_by.first')
end

it 'registers an offense with `sort_by { a || b }`' do
expect_offense(<<~RUBY)
x.sort_by { |y| y.foo || bar }.last
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `max_by` instead of `sort_by...last`.
RUBY

expect_correction(<<~RUBY)
x.max_by { |y| y.foo || bar }
RUBY
end

context 'when not taking first or last element' do
it 'does not register an offense when [1] is called on sort' do
expect_no_offenses('[1, 2, 3].sort[1]')
Expand Down