diff --git a/changelog/new_add_lintambiguousrange_cop_to_check_for.md b/changelog/new_add_lintambiguousrange_cop_to_check_for.md new file mode 100644 index 00000000000..26a0a85e2e1 --- /dev/null +++ b/changelog/new_add_lintambiguousrange_cop_to_check_for.md @@ -0,0 +1 @@ +* [#4182](https://github.com/rubocop/rubocop/issues/4182): Add `Lint/AmbiguousRange` cop to check for ranges with ambiguous boundaries. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index c4231cde358..06d2b6f2a48 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1434,6 +1434,13 @@ Lint/AmbiguousOperator: VersionAdded: '0.17' VersionChanged: '0.83' +Lint/AmbiguousRange: + Description: Checks for ranges with ambiguous boundaries. + Enabled: pending + VersionAdded: '<>' + SafeAutoCorrect: false + RequireParenthesesForMethodChains: false + Lint/AmbiguousRegexpLiteral: Description: >- Checks for ambiguous regexp literals in the first argument of diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 4b475009723..a4bdbecb540 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -262,6 +262,7 @@ require_relative 'rubocop/cop/lint/ambiguous_assignment' require_relative 'rubocop/cop/lint/ambiguous_block_association' require_relative 'rubocop/cop/lint/ambiguous_operator' +require_relative 'rubocop/cop/lint/ambiguous_range' require_relative 'rubocop/cop/lint/ambiguous_regexp_literal' require_relative 'rubocop/cop/lint/assignment_in_condition' require_relative 'rubocop/cop/lint/big_decimal_new' diff --git a/lib/rubocop/cop/lint/ambiguous_range.rb b/lib/rubocop/cop/lint/ambiguous_range.rb new file mode 100644 index 00000000000..f3ace9674bd --- /dev/null +++ b/lib/rubocop/cop/lint/ambiguous_range.rb @@ -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 diff --git a/spec/rubocop/cop/lint/ambiguous_range_spec.rb b/spec/rubocop/cop/lint/ambiguous_range_spec.rb new file mode 100644 index 00000000000..eb931156ce8 --- /dev/null +++ b/spec/rubocop/cop/lint/ambiguous_range_spec.rb @@ -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