From 94a34041f75dc2afa36f1cf8f5425e83d940fd78 Mon Sep 17 00:00:00 2001 From: Alex Mooney Date: Wed, 21 Feb 2024 16:56:40 -0800 Subject: [PATCH] Correct beginless and endless range comparisons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some range comparisons raise `RangeError`, for example: ```ruby > (1..) === (2..) RangeError: cannot get the last element of endless range from …/activesupport-7.1.3/lib/active_support/core_ext/range/compare_range.rb:23:in `last' ``` --- activesupport/CHANGELOG.md | 15 +++++ .../core_ext/range/compare_range.rb | 67 ++++++++++++------- activesupport/test/core_ext/range_ext_test.rb | 30 ++++++++- 3 files changed, 85 insertions(+), 27 deletions(-) diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 23c382a209190..3a515552c59ce 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,18 @@ +* Fix `RangeError` in `Range#===` and `Range#include?` for endless and beginless ranges + + ```ruby + (1..) === (2..) + => true + (2..) === (1..) + => false + (..1) === (..2) + => false + (..2) === (..1) + => true + ``` + + *Alex Mooney* + * Add filename support for `ActiveSupport::Logger.logger_outputs_to?` ```ruby diff --git a/activesupport/lib/active_support/core_ext/range/compare_range.rb b/activesupport/lib/active_support/core_ext/range/compare_range.rb index affbbebb42826..5c7392e38883e 100644 --- a/activesupport/lib/active_support/core_ext/range/compare_range.rb +++ b/activesupport/lib/active_support/core_ext/range/compare_range.rb @@ -11,20 +11,8 @@ module CompareWithRange # The native Range#=== behavior is untouched. # ('a'..'f') === ('c') # => true # (5..9) === (11) # => false - # - # The given range must be fully bounded, with both start and end. - def ===(value) - if value.is_a?(::Range) - is_backwards_op = value.exclude_end? ? :>= : :> - return false if value.begin && value.end && value.begin.public_send(is_backwards_op, value.end) - # 1...10 includes 1..9 but it does not include 1..10. - # 1..10 includes 1...11 but it does not include 1...12. - operator = exclude_end? && !value.exclude_end? ? :< : :<= - value_max = !exclude_end? && value.exclude_end? ? value.max : value.last - super(value.first) && (self.end.nil? || value_max.public_send(operator, last)) - else - super - end + def ===(other) + compare(other) { |arg_for_comparison| super(arg_for_comparison) } end # Extends the default Range#include? to support range comparisons. @@ -36,21 +24,48 @@ def ===(value) # The native Range#include? behavior is untouched. # ('a'..'f').include?('c') # => true # (5..9).include?(11) # => false - # - # The given range must be fully bounded, with both start and end. - def include?(value) - if value.is_a?(::Range) - is_backwards_op = value.exclude_end? ? :>= : :> - return false if value.begin && value.end && value.begin.public_send(is_backwards_op, value.end) + def include?(other) + compare(other) { |arg_for_comparison| super(arg_for_comparison) } + end + + private + def compare(other, &block) + if other.is_a?(::Range) + compare_with_range(other, &block) + else + block.call(other) + end + end + + def compare_with_range(other, &block) + return false if other_is_backwards?(other) + + begin_includes_other_begin?(other, &block) && end_includes_other_end?(other) + end + + def other_is_backwards?(other) + is_backwards_op = other.exclude_end? ? :>= : :> + other.begin && other.end && other.begin.public_send(is_backwards_op, other.end) + end + + def begin_includes_other_begin?(other, &block) + # A beginless range always includes the other range's begin. + self.begin.nil? || block.call(other.begin) + end + + def end_includes_other_end?(other) + # An endless range always includes the other range's end. + return true if self.end.nil? + # If the other range is endless, it's not included. + return false if other.end.nil? + # 1...10 includes 1..9 but it does not include 1..10. # 1..10 includes 1...11 but it does not include 1...12. - operator = exclude_end? && !value.exclude_end? ? :< : :<= - value_max = !exclude_end? && value.exclude_end? ? value.max : value.last - super(value.first) && (self.end.nil? || value_max.public_send(operator, last)) - else - super + # We can't simplify to `max >= other.max` since it raises TypeError for exclusive ranges with float end points. + operator = exclude_end? && !other.exclude_end? ? :< : :<= + value_max = !exclude_end? && other.exclude_end? ? other.max : other.last + value_max.public_send(operator, last) end - end end end diff --git a/activesupport/test/core_ext/range_ext_test.rb b/activesupport/test/core_ext/range_ext_test.rb index 171ed86ffc16d..4caa2f7879ede 100644 --- a/activesupport/test/core_ext/range_ext_test.rb +++ b/activesupport/test/core_ext/range_ext_test.rb @@ -131,6 +131,13 @@ def test_overlap_behaves_like_ruby assert_not_operator((...3), :overlap?, (3..)) end + def test_include_uses_ruby_include + assert((1..3).include?(1.5)) + assert(("a".."d").include?("c")) + + assert_not(("a".."d").include?("cc")) # As opposed to `=== 'cc'` => `true` + end + def test_should_include_identical_inclusive assert((1..10).include?(1..10)) end @@ -143,8 +150,10 @@ def test_should_include_other_with_exclusive_end assert((1..10).include?(1...11)) end - def test_include_returns_false_for_backwards + def test_include_returns_false_for_backwards_ranges assert_not((1..10).include?(5..3)) + assert_not((10..1).include?(3..5)) + assert_not((10..1).include?(5..3)) end # Match quirky plain-Ruby behavior @@ -158,10 +167,12 @@ def test_include_with_endless_range def test_should_include_range_with_endless_range assert((1..).include?(2..4)) + assert_not((2..4).include?(1..)) end def test_should_not_include_range_with_endless_range assert_not((1..).include?(0..4)) + assert_not((0..4).include?(1..)) end def test_include_with_beginless_range @@ -170,10 +181,20 @@ def test_include_with_beginless_range def test_should_include_range_with_beginless_range assert((..2).include?(-1..1)) + assert((..2).include?(..1)) end def test_should_not_include_range_with_beginless_range assert_not((..2).include?(-1..3)) + assert_not((..1).include?(..2)) + assert_not((-1..1).include?(..2)) + end + + def test_case_equality_uses_ruby_case_equality + assert((1..3) === 1.5) + assert(("a".."d") === "c") + + assert(("a".."d") === "cc") # As opposed to `.include?('cc')` => `false` end def test_should_compare_identical_inclusive @@ -199,18 +220,25 @@ def test_compare_returns_false_for_empty_exclusive_end def test_should_compare_range_with_endless_range assert((1..) === (2..4)) + assert((1..) === (1..)) + assert((1..) === (2..)) end def test_should_not_compare_range_with_endless_range assert_not((1..) === (0..4)) + assert_not((0..4) === (1..)) + assert_not((2..) === (1..)) end def test_should_compare_range_with_beginless_range assert((..2) === (-1..1)) + assert((..2) === (..1)) end def test_should_not_compare_range_with_beginless_range assert_not((..2) === (-1..3)) + assert_not((..1) === (..2)) + assert_not((-1..3) === (..2)) end def test_exclusive_end_should_not_include_identical_with_inclusive_end