Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
…biguous boundaries.
- Loading branch information
1 parent
07c8ef8
commit b3f6e50
Showing
5 changed files
with
288 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#4182](https://github.com/rubocop/rubocop/issues/4182): Add `Lint/AmbiguousRange` cop to check for ranges with ambiguous boundaries. ([@dvandersluis][]) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Lint | ||
# This cop checks for ambiguous ranges. | ||
# | ||
# Ranges have quite low precedence, which leads to unexpected behaviour when | ||
# using a range with other operators. This cop avoids that by making ranges | ||
# explicit by requiring parenthesis around complex range boundaries (anything | ||
# that is not a basic literal: numerics, strings, symbols, etc.). | ||
# | ||
# NOTE: The cop auto-corrects by wrapping the entire boundary in parentheses, which | ||
# makes the outcome more explicit but is possible to not be the intention of the | ||
# programmer. For this reason, this cop's auto-correct is marked as unsafe (it | ||
# will not change the behaviour of the code, but will not necessarily match the | ||
# intent of the program). | ||
# | ||
# This cop can be configured with `RequireParenthesesForMethodChains` in order to | ||
# specify whether method chains (including `self.foo`) should be wrapped in parens | ||
# by this cop. | ||
# | ||
# NOTE: Regardless of this configuration, if a method receiver is a basic literal | ||
# value, it will be wrapped in order to prevent the ambiguity of `1..2.to_a`. | ||
# | ||
# @example | ||
# # bad | ||
# x || 1..2 | ||
# (x || 1..2) | ||
# 1..2.to_a | ||
# | ||
# # good, unambiguous | ||
# 1..2 | ||
# 'a'..'z' | ||
# :bar..:baz | ||
# MyClass::MIN..MyClass::MAX | ||
# @min..@max | ||
# a..b | ||
# -a..b | ||
# | ||
# # good, ambiguity removed | ||
# x || (1..2) | ||
# (x || 1)..2 | ||
# (x || 1)..(y || 2) | ||
# (1..2).to_a | ||
# | ||
# @example RequireParenthesesForMethodChains: false (default) | ||
# # good | ||
# a.foo..b.bar | ||
# (a.foo)..(b.bar) | ||
# | ||
# @example RequireParenthesesForMethodChains: true | ||
# # bad | ||
# a.foo..b.bar | ||
# | ||
# # good | ||
# (a.foo)..(b.bar) | ||
# | ||
class AmbiguousRange < Base | ||
extend AutoCorrector | ||
|
||
MSG = 'Wrap complex range boundaries with parentheses to avoid ambiguity.' | ||
|
||
def on_irange(node) | ||
each_boundary(node) do |boundary| | ||
next if acceptable?(boundary) | ||
|
||
add_offense(boundary) do |corrector| | ||
corrector.wrap(boundary, '(', ')') | ||
end | ||
end | ||
end | ||
alias on_erange on_irange | ||
|
||
private | ||
|
||
def each_boundary(range) | ||
yield range.begin if range.begin | ||
yield range.end if range.end | ||
end | ||
|
||
def acceptable?(node) | ||
node.begin_type? || | ||
node.basic_literal? || | ||
node.variable? || node.const_type? || | ||
node.call_type? && acceptable_call?(node) | ||
end | ||
|
||
def acceptable_call?(node) | ||
return true if node.unary_operation? | ||
|
||
# Require parentheses when making a method call on a literal | ||
# to avoid the ambiguity of `1..2.to_a`. | ||
return false if node.receiver&.basic_literal? | ||
|
||
require_parentheses_for_method_chain? || node.receiver.nil? | ||
end | ||
|
||
def require_parentheses_for_method_chain? | ||
!cop_config['RequireParenthesesForMethodChains'] | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Lint::AmbiguousRange, :config do | ||
{ 'irange' => '..', 'erange' => '...' }.each do |node_type, operator| | ||
context "for an #{node_type}" do | ||
it 'registers an offense and corrects when not parenthesized' do | ||
expect_offense(<<~RUBY) | ||
x || 1#{operator}2 | ||
^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(x || 1)#{operator}2 | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and corrects when the entire range is parenthesized but contains complex boundaries' do | ||
expect_offense(<<~RUBY) | ||
(x || 1#{operator}2) | ||
^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
((x || 1)#{operator}2) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and corrects when there are clauses on both sides' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
x || 1#{operator}y || 2 | ||
_{operator}^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(x || 1)#{operator}(y || 2) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and corrects when one side is parenthesized but the other is not' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
(x || 1)#{operator}y || 2 | ||
_{operator}^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(x || 1)#{operator}(y || 2) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense if the range is parenthesized' do | ||
expect_no_offenses(<<~RUBY) | ||
x || (1#{operator}2) | ||
(x || 1)#{operator}2 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense if the range is composed of basic literals' do | ||
expect_no_offenses(<<~RUBY) | ||
1#{operator}2 | ||
'a'#{operator}'z' | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a variable' do | ||
expect_no_offenses(<<~RUBY) | ||
@a#{operator}@b | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a constant' do | ||
expect_no_offenses(<<~RUBY) | ||
Foo::MIN#{operator}Foo::MAX | ||
RUBY | ||
end | ||
|
||
it 'can handle an endless range', :ruby26 do | ||
expect_offense(<<~RUBY) | ||
x || 1#{operator} | ||
^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(x || 1)#{operator} | ||
RUBY | ||
end | ||
|
||
it 'can handle a beginningless range', :ruby27 do | ||
expect_offense(<<~RUBY, operator: operator) | ||
#{operator}y || 1 | ||
_{operator}^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
#{operator}(y || 1) | ||
RUBY | ||
end | ||
|
||
context 'method calls' do | ||
shared_examples_for 'common behavior' do | ||
it 'does not register an offense for a non-chained method call' do | ||
expect_no_offenses(<<~RUBY) | ||
a#{operator}b | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a unary +' do | ||
expect_no_offenses(<<~RUBY) | ||
+a#{operator}10 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a unary -' do | ||
expect_no_offenses(<<~RUBY) | ||
-a#{operator}10 | ||
RUBY | ||
end | ||
|
||
it 'requires parens when calling a method on a basic literal' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
1#{operator}2.to_a | ||
_{operator}^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
1#{operator}(2.to_a) | ||
RUBY | ||
end | ||
end | ||
|
||
context 'with RequireParenthesesForMethodChains: true' do | ||
let(:cop_config) { { 'RequireParenthesesForMethodChains' => true } } | ||
|
||
it_behaves_like 'common behavior' | ||
|
||
it 'registers an offense for a chained method call without parens' do | ||
expect_offense(<<~RUBY) | ||
foo.bar#{operator}10 | ||
^^^^^^^ Wrap complex range boundaries with parentheses to avoid ambiguity. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
(foo.bar)#{operator}10 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a chained method call with parens' do | ||
expect_no_offenses(<<~RUBY) | ||
(foo.bar)#{operator}10 | ||
RUBY | ||
end | ||
end | ||
|
||
context 'with RequireParenthesesForMethodChains: false' do | ||
let(:cop_config) { { 'RequireParenthesesForMethodChains' => false } } | ||
|
||
it_behaves_like 'common behavior' | ||
|
||
it 'does not register an offense for a chained method call without parens' do | ||
expect_no_offenses(<<~RUBY) | ||
foo.bar#{operator}10 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for a chained method call with parens' do | ||
expect_no_offenses(<<~RUBY) | ||
(foo.bar)#{operator}10 | ||
RUBY | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |