diff --git a/changelog/change_update_styleredundantsort_to_be_unsafe.md b/changelog/change_update_styleredundantsort_to_be_unsafe.md new file mode 100644 index 00000000000..ec4722dab88 --- /dev/null +++ b/changelog/change_update_styleredundantsort_to_be_unsafe.md @@ -0,0 +1 @@ +* [#10122](https://github.com/rubocop/rubocop/pull/10122): Update `Style/RedundantSort` to be unsafe, and revert the special case for `size` from [#10061](https://github.com/rubocop/rubocop/pull/10061). ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index 6e99897a826..9b9e61be563 100644 --- a/config/default.yml +++ b/config/default.yml @@ -4160,7 +4160,6 @@ Style/NumericLiteralPrefix: - zero_with_o - zero_only - Style/NumericLiterals: Description: >- Add underscores to large numeric literals to improve their @@ -4481,6 +4480,8 @@ Style/RedundantSort: `max_by` instead of `sort_by...last`, etc. Enabled: true VersionAdded: '0.76' + VersionChanged: <> + Safe: false Style/RedundantSortBy: Description: 'Use `sort` instead of `sort_by { |x| x }`.' diff --git a/lib/rubocop/cop/style/redundant_sort.rb b/lib/rubocop/cop/style/redundant_sort.rb index e4af9691c17..59a1e43e491 100644 --- a/lib/rubocop/cop/style/redundant_sort.rb +++ b/lib/rubocop/cop/style/redundant_sort.rb @@ -12,6 +12,33 @@ module Style # `Enumerable#max_by` can replace `Enumerable#sort_by` calls # after which only the first or last element is used. # + # @safety + # This cop is unsafe, because `sort...last` and `max` may not return the + # same element in all cases. + # + # In an enumerable where there are multiple elements where `a <=> b == 0`, + # or where the transformation done by the `sort_by` block has the + # same result, `sort.last` and `max` (or `sort_by.last` and `max_by`) + # will return different elements. `sort.last` will return the last + # element but `max` will return the first element. + # + # For example: + # + # [source,ruby] + # ---- + # class MyString < String; end + # strings = [MyString.new('test'), 'test'] + # strings.sort.last.class #=> String + # strings.max.class #=> MyString + # ---- + # + # [source,ruby] + # ---- + # words = %w(dog horse mouse) + # words.sort_by { |word| word.length }.last #=> 'mouse' + # words.max_by { |word| word.length } #=> 'horse' + # ---- + # # @example # # bad # [2, 1, 3].sort.first @@ -75,55 +102,39 @@ class RedundantSort < Base MATCHER def on_send(node) - if (sort_node, sorter, accessor = redundant_sort?(node.parent)) - return if use_size_method_in_block?(sort_node) - - ancestor = node.parent - elsif (sort_node, sorter, accessor = redundant_sort?(node.parent&.parent)) - return if use_size_method_in_block?(sort_node) - - ancestor = node.parent.parent - else - return - end + ancestor, sort_node, sorter, accessor = + find_redundant_sort(node.parent, node.parent&.parent) + return unless ancestor register_offense(ancestor, sort_node, sorter, accessor) end private - def use_size_method_in_block?(sort_node) - return true if block_calls_send?(sort_node) - return false unless sort_node.block_argument? + def find_redundant_sort(*nodes) + nodes.each do |node| + if (sort_node, sorter, accessor = redundant_sort?(node)) + return [node, sort_node, sorter, accessor] + end + end - sort_node.last_argument.children.first.value == :size + nil 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(node, sort_node, sorter, accessor) + message = message(node, sorter, accessor) - def register_offense(ancestor, sort_node, sorter, accessor) - message = message(ancestor, sorter, accessor) + add_offense(offense_range(sort_node, node), message: message) do |corrector| + # Remove accessor, e.g. `first` or `[-1]`. + corrector.remove(range_between(accessor_start(node), node.loc.expression.end_pos)) - add_offense(offense_range(sort_node, ancestor), message: message) do |corrector| - autocorrect(corrector, ancestor, sort_node, sorter, accessor) + # Replace "sort" or "sort_by" with the appropriate min/max method. + corrector.replace(sort_node.loc.selector, suggestion(sorter, accessor, arg_value(node))) end end - def autocorrect(corrector, node, sort_node, sorter, accessor) - # Remove accessor, e.g. `first` or `[-1]`. - corrector.remove(range_between(accessor_start(node), node.loc.expression.end_pos)) - - # Replace "sort" or "sort_by" with the appropriate min/max method. - corrector.replace(sort_node.loc.selector, suggestion(sorter, accessor, arg_value(node))) - end - - def offense_range(sort_node, ancestor) - range_between(sort_node.loc.selector.begin_pos, ancestor.loc.expression.end_pos) + def offense_range(sort_node, node) + range_between(sort_node.loc.selector.begin_pos, node.loc.expression.end_pos) end def message(node, sorter, accessor) diff --git a/spec/rubocop/cop/style/redundant_sort_spec.rb b/spec/rubocop/cop/style/redundant_sort_spec.rb index da7eb3bdce6..f69bd6009c5 100644 --- a/spec/rubocop/cop/style/redundant_sort_spec.rb +++ b/spec/rubocop/cop/style/redundant_sort_spec.rb @@ -231,16 +231,6 @@ expect_no_offenses('[[1, 2], [3, 4]].first.sort') end - # `[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 - - # `[2, 1, 3].sort_by { |i| i.size }.last` is not equivalent to `[2, 1, 3].max_by { |i| i.size }`. - 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 - # `[2, 1, 3].sort_by(&:size).first` is not equivalent to `[2, 1, 3].first`, but this # cop would "correct" it to `[2, 1, 3].min_by`. it 'does not register an offense when sort_by is not given a block' do