Update Style/RedundantSort to be unsafe

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][])

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: <<next>>
+  Safe: false
 
 Style/RedundantSortBy:
   Description: 'Use `sort` instead of `sort_by { |x| x }`.'

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)

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