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

Update Style/RedundantSort to be unsafe #10122

Merged
merged 3 commits into from Sep 28, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Sep 23, 2021

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.

Honestly, we should probably revert #10062 as well because that adds a special case for size that doesn't really exist in reality. In that issue, size was the catalyst, but the exact same issue would happen with length, or any other transformation that results in multiple elements having the same value. If we revert #10062, then #10119 can be closed as well. If this is desired, I'll update this PR to include the revert and add the test case from #10119 which is still valueable.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@dvandersluis
Copy link
Member Author

@koic are you ok with reverting your change like I proposed?

@dvandersluis
Copy link
Member Author

@bbatsov any thoughts?

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 27, 2021

I'm fine with your proposal and reverting the extra commit that attempted to fix this before.

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.
@dvandersluis
Copy link
Member Author

@bbatsov the special case for size is now reverted in addition to the cop being made 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.
@bbatsov bbatsov merged commit 14ca0ce into rubocop:master Sep 28, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 28, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants