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

Mark Style/SlicingWithRange as safe #8890

Conversation

koic
Copy link
Member

@koic koic commented Oct 13, 2020

Follow #7921.

This PR marks Style/SlicingWithRange as safe.


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@koic koic force-pushed the mark_unsafe_autocorrect_for_slicing_with_range branch 2 times, most recently from 9e669d8 to 69909ec Compare October 15, 2020 03:44
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 15, 2020

Let's also add a note in the cop description explaning why the auto-correct is unsafe, so it's clearer to the end users.

@koic koic changed the title Mark Style/SlicingWithRange as unsafe auto-correction Mark Style/SlicingWithRange as safe Oct 15, 2020
@koic koic force-pushed the mark_unsafe_autocorrect_for_slicing_with_range branch from 69909ec to bd5a38a Compare October 15, 2020 15:25
@koic
Copy link
Member Author

koic commented Oct 15, 2020

Ah, I misunderstood it as (begin_pos..-1) 💦 I think this cop is safe now, and I updated it that way.

@zverok Please let me know if there is any reason why this cop is unsafe.

@koic koic force-pushed the mark_unsafe_autocorrect_for_slicing_with_range branch from bd5a38a to 389c14b Compare October 15, 2020 15:26
@@ -4116,8 +4116,7 @@ Style/SingleLineMethods:
Style/SlicingWithRange:
Description: 'Checks array slicing is done with endless ranges when suitable.'
Enabled: pending
VersionAdded: '0.83'
Safe: false
VersionAdded: '0.94'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be VersionChanged.

Follow rubocop#7921.

This PR marks `Style/SlicingWithRange` as safe.
@koic koic force-pushed the mark_unsafe_autocorrect_for_slicing_with_range branch from 389c14b to 89cd0e3 Compare October 15, 2020 16:17
@zverok
Copy link
Contributor

zverok commented Oct 15, 2020

I don't believe this cop can be safe. Equivalence of x..-1 and x.. is guaranteed only for Array#[], but as any class might have #[] method (with semantics we do not know from code analysis), generally those two are NOT equivalent. Examples with Ruby core classes:

h = {-3..-1 => 'foo', 0..5 => 'bar'}
h[-3..-1] # => "foo" 
h[-3..] # => nil 

sum = proc { |ary| ary.sum }
sum[-3..-1] # => -6 
sum[-3..] # Hangs forever

While both examples are artificial, they demonstrate the principle: in general, #[] is not expected to treat endless range as range till -1

@koic
Copy link
Member Author

koic commented Oct 19, 2020

Ah, I was missing on that point of view. I think it's possible to add an unsafe reason to the documentation. I'll close this PR for now. @zverok Thank you for your explanation!

@koic koic closed this Oct 19, 2020
@koic koic deleted the mark_unsafe_autocorrect_for_slicing_with_range branch October 19, 2020 02:44
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

3 participants