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

[Fix #4182] Add Lint/AmbiguousRange cop to check for ranges with ambiguous boundaries #9986

Merged
merged 1 commit into from Aug 12, 2021

Conversation

dvandersluis
Copy link
Member

Adds a new cop Lint/AmbiguousRange to detect range boundaries that are ambiguous. An ambiguous boundary is one that is not a basic literal, single variable/constant/method reference, or method chain (depending on the value of RequireParenthesesForMethodChains, default false). Originally I was intending to have it only accept unparenthesized basic literals, but it would require too many ranges in rubocop's code to be changed that I re-evaluated.

This change will allow 1..2 without parens but require the parens in (x || 1)..10 or (x || 1)..(y || 10). Having the cop act on each boundary individually is the clearest approach IMO because (x || 1..y || 10) is just as ambiguous as without the parentheses.

In order to avoid the ambiguity of 1..2.to_a, method calls with a basic literal receiver always require parentheses, regardless of RequireParenthesesForMethodChains.

Autocorrection is straightforward, wrapping any unacceptable boundaries in parens. However, I have marked this cop as SafeAutocorrect: false because it is likely that the intended code is not the same as the actual behaviour of the code. In other words, 1..2.to_a is equivalent to 1..(2.to_a), which is what the cop will correct to, but the programmer almost definitely intended on (1..2).to_a. The autocorrection will make it clear how ruby will actually evaluate this code, however.

This seemed like it was going to be simple until it became complicated 😁

Fixes #4182.


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 dvandersluis changed the title [Fix #4182] Add Lint/AmbiguousRange cop to check for ranges with am… [Fix #4182] Add Lint/AmbiguousRange cop to check for ranges with ambiguous boundaries Aug 7, 2021
@dvandersluis dvandersluis requested a review from koic August 10, 2021 18:53
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 12, 2021

Looks pretty solid to me. Great work!

@bbatsov bbatsov merged commit b3f6e50 into rubocop:master Aug 12, 2021
@dvandersluis dvandersluis deleted the issue/4182 branch September 14, 2021 16:04
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.

New cop for .. operator precedence
2 participants