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

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][])

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: '<<next>>'
+  SafeAutoCorrect: false
+  RequireParenthesesForMethodChains: false
+
 Lint/AmbiguousRegexpLiteral:
   Description: >-
                  Checks for ambiguous regexp literals in the first argument of

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'

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

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