From b0cb09f76d8aeebf5f15da4e92bf090dc7f3051b Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Mon, 27 Sep 2021 12:13:26 -0400 Subject: [PATCH 1/3] Revert changes from #10061. These changes added an exception for `size` but the same issue can occur regardless of what method is used, as long as it results in multiple values having the same resulting value. Instead, this cop is being made unsafe. --- lib/rubocop/cop/style/redundant_sort.rb | 18 ------------------ spec/rubocop/cop/style/redundant_sort_spec.rb | 10 ---------- 2 files changed, 28 deletions(-) diff --git a/lib/rubocop/cop/style/redundant_sort.rb b/lib/rubocop/cop/style/redundant_sort.rb index e4af9691c17..bf31a01c9fb 100644 --- a/lib/rubocop/cop/style/redundant_sort.rb +++ b/lib/rubocop/cop/style/redundant_sort.rb @@ -76,12 +76,8 @@ class RedundantSort < Base 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 @@ -92,20 +88,6 @@ def on_send(node) private - def use_size_method_in_block?(sort_node) - 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) 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 From 71de8c23a0f5f6204ca4e563e8a869e3ca96149a Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Thu, 23 Sep 2021 16:00:33 -0400 Subject: [PATCH 2/3] Update `Style/RedundantSort` to be unsafe. In an array where there are multiple elements where `a <=> b` == 0, or where the transformation done by the block has the same result, there will be a different element returned between `sort.last` and `max` (or `sort_by.last` and `max_by`), because `sort.last` will take the last element but `max` will return the first element. As such, this is not a safe change, as the specific element returned will change. --- ..._update_styleredundantsort_to_be_unsafe.md | 1 + config/default.yml | 3 ++- lib/rubocop/cop/style/redundant_sort.rb | 27 +++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 changelog/change_update_styleredundantsort_to_be_unsafe.md 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 bf31a01c9fb..d0b0c6af758 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 From ad5186f2b58c0d149f9512bac34852962d85b21b Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Mon, 27 Sep 2021 12:37:55 -0400 Subject: [PATCH 3/3] Minor refactor of `Style/RedundantSort`. --- lib/rubocop/cop/style/redundant_sort.rb | 40 +++++++++++++------------ 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/lib/rubocop/cop/style/redundant_sort.rb b/lib/rubocop/cop/style/redundant_sort.rb index d0b0c6af758..59a1e43e491 100644 --- a/lib/rubocop/cop/style/redundant_sort.rb +++ b/lib/rubocop/cop/style/redundant_sort.rb @@ -102,37 +102,39 @@ class RedundantSort < Base MATCHER def on_send(node) - if (sort_node, sorter, accessor = redundant_sort?(node.parent)) - ancestor = node.parent - elsif (sort_node, sorter, accessor = redundant_sort?(node.parent&.parent)) - 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 register_offense(ancestor, sort_node, sorter, accessor) - message = message(ancestor, sorter, accessor) - - add_offense(offense_range(sort_node, ancestor), message: message) do |corrector| - autocorrect(corrector, ancestor, sort_node, sorter, accessor) + 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 + + nil 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)) + def register_offense(node, sort_node, sorter, accessor) + message = message(node, 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)) - # Replace "sort" or "sort_by" with the appropriate min/max method. - corrector.replace(sort_node.loc.selector, suggestion(sorter, accessor, arg_value(node))) + # 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 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)