diff --git a/range.c b/range.c index 536568ba76..a52418d2cb 100644 --- a/range.c +++ b/range.c @@ -309,6 +309,34 @@ range_each_func(VALUE range, int (*func)(VALUE, VALUE), VALUE arg) } } +// NB: Two functions below (step_i_iter and step_i) are used only to maintain the +// backward-compatible behavior for string ranges with integer steps. If that branch +// will be removed from range_step, these two can go, too. +static bool +step_i_iter(VALUE arg) +{ + VALUE *iter = (VALUE *)arg; + + if (FIXNUM_P(iter[0])) { + iter[0] -= INT2FIX(1) & ~FIXNUM_FLAG; + } + else { + iter[0] = rb_funcall(iter[0], '-', 1, INT2FIX(1)); + } + if (iter[0] != INT2FIX(0)) return false; + iter[0] = iter[1]; + return true; +} + +static int +step_i(VALUE i, VALUE arg) +{ + if (step_i_iter(arg)) { + rb_yield(i); + } + return 0; +} + static int discrete_object_p(VALUE obj) { @@ -430,9 +458,18 @@ range_step_size(VALUE range, VALUE args, VALUE eobj) * * For non-Numeric ranges, step absence is an error: * - * ('a'..'z').step { p _1 } + * (Time.utc(2022, 3, 1)..Time.utc(2022, 2, 24)).step { p _1 } * # raises: step is required for non-numeric ranges (ArgumentError) * + * For backward compatibility reasons, String ranges support the iteration both with + * string step and with integer step. In the latter case, the iteration is performed + * by calculating the next values with String#succ: + * + * ('a'..'e').step(2) { p _1 } + * # Prints: a, c, e + * ('a'..'e').step { p _1 } + * # Default step 1; prints: a, b, c, d, e + * */ static VALUE range_step(int argc, VALUE *argv, VALUE range) @@ -445,11 +482,15 @@ range_step(int argc, VALUE *argv, VALUE range) const VALUE b_num_p = rb_obj_is_kind_of(b, rb_cNumeric); const VALUE e_num_p = rb_obj_is_kind_of(e, rb_cNumeric); + // For backward compatibility reasons (conforming to behavior before 3.4), String supports + // both old behavior ('a'..).step(1) and new behavior ('a'..).step('a') + // Hence the additional conversion/addional checks. + const VALUE sb = rb_check_string_type(b); if (rb_check_arity(argc, 0, 1)) step = argv[0]; else { - if (b_num_p || (NIL_P(b) && e_num_p)) + if (b_num_p || !NIL_P(sb) || (NIL_P(b) && e_num_p)) step = INT2FIX(1); else rb_raise(rb_eArgError, "step is required for non-numeric ranges"); @@ -520,8 +561,19 @@ range_step(int argc, VALUE *argv, VALUE range) } else if (b_num_p && step_num_p && ruby_float_step(b, e, step, EXCL(range), TRUE)) { /* done */ - } - else { + } else if (!NIL_P(sb) && FIXNUM_P(step)) { + // backwards compatibility behavior for String only, when no step/Integer step is passed + // See discussion in https://bugs.ruby-lang.org/issues/18368 + + VALUE iter[2] = {INT2FIX(1), step}; + + if (NIL_P(e)) { + rb_str_upto_endless_each(sb, step_i, (VALUE)iter); + } + else { + rb_str_upto_each(sb, e, EXCL(range), step_i, (VALUE)iter); + } + } else { v = b; if (!NIL_P(e)) { if (b_num_p && step_num_p && r_less(step, INT2FIX(0)) < 0) { diff --git a/spec/ruby/core/range/step_spec.rb b/spec/ruby/core/range/step_spec.rb index f858a6a6bb..31cfd400cc 100644 --- a/spec/ruby/core/range/step_spec.rb +++ b/spec/ruby/core/range/step_spec.rb @@ -175,21 +175,21 @@ describe "Range#step" do end describe "and String values" do + it "yields String values incremented by #succ and less than or equal to end when not passed a step" do + ("A".."E").step { |x| ScratchPad << x } + ScratchPad.recorded.should == ["A", "B", "C", "D", "E"] + end + + it "yields String values incremented by #succ called Integer step times" do + ("A".."G").step(2) { |x| ScratchPad << x } + ScratchPad.recorded.should == ["A", "C", "E", "G"] + end + + it "raises a TypeError when passed a Float step" do + -> { ("A".."G").step(2.0) { } }.should raise_error(TypeError) + end + ruby_version_is ""..."3.4" do - it "yields String values incremented by #succ and less than or equal to end when not passed a step" do - ("A".."E").step { |x| ScratchPad << x } - ScratchPad.recorded.should == ["A", "B", "C", "D", "E"] - end - - it "yields String values incremented by #succ called Integer step times" do - ("A".."G").step(2) { |x| ScratchPad << x } - ScratchPad.recorded.should == ["A", "C", "E", "G"] - end - - it "raises a TypeError when passed a Float step" do - -> { ("A".."G").step(2.0) { } }.should raise_error(TypeError) - end - it "calls #succ on begin and each element returned by #succ" do obj = mock("Range#step String start") obj.should_receive(:<=>).exactly(3).times.and_return(-1, -1, -1, 0) @@ -201,17 +201,13 @@ describe "Range#step" do end ruby_version_is "3.4" do - it "raises an ArgumentError when not passed a step" do - -> { ("A".."E").step { } }.should raise_error(ArgumentError) - end - it "yields String values adjusted by step and less than or equal to end" do ("A".."AAA").step("A") { |x| ScratchPad << x } ScratchPad.recorded.should == ["A", "AA", "AAA"] end it "raises a TypeError when passed an incompatible type step" do - -> { ("A".."G").step(2) { } }.should raise_error(TypeError) + -> { ("A".."G").step([]) { } }.should raise_error(TypeError) end it "calls #+ on begin and each element returned by #+" do @@ -397,17 +393,13 @@ describe "Range#step" do end ruby_version_is "3.4" do - it "raises an ArgumentError when not passed a step" do - -> { ("A".."E").step { } }.should raise_error(ArgumentError) - end - it "yields String values adjusted by step and less than or equal to end" do ("A"..."AAA").step("A") { |x| ScratchPad << x } ScratchPad.recorded.should == ["A", "AA"] end it "raises a TypeError when passed an incompatible type step" do - -> { ("A".."G").step(2) { } }.should raise_error(TypeError) + -> { ("A".."G").step([]) { } }.should raise_error(TypeError) end end end @@ -482,36 +474,30 @@ describe "Range#step" do end describe "and String values" do - ruby_version_is ""..."3.4" do - it "yields String values incremented by #succ and less than or equal to end when not passed a step" do - eval("('A'..)").step { |x| break if x > "D"; ScratchPad << x } - ScratchPad.recorded.should == ["A", "B", "C", "D"] + it "yields String values incremented by #succ and less than or equal to end when not passed a step" do + eval("('A'..)").step { |x| break if x > "D"; ScratchPad << x } + ScratchPad.recorded.should == ["A", "B", "C", "D"] - ScratchPad.record [] - eval("('A'...)").step { |x| break if x > "D"; ScratchPad << x } - ScratchPad.recorded.should == ["A", "B", "C", "D"] - end + ScratchPad.record [] + eval("('A'...)").step { |x| break if x > "D"; ScratchPad << x } + ScratchPad.recorded.should == ["A", "B", "C", "D"] + end - it "yields String values incremented by #succ called Integer step times" do - eval("('A'..)").step(2) { |x| break if x > "F"; ScratchPad << x } - ScratchPad.recorded.should == ["A", "C", "E"] + it "yields String values incremented by #succ called Integer step times" do + eval("('A'..)").step(2) { |x| break if x > "F"; ScratchPad << x } + ScratchPad.recorded.should == ["A", "C", "E"] - ScratchPad.record [] - eval("('A'...)").step(2) { |x| break if x > "F"; ScratchPad << x } - ScratchPad.recorded.should == ["A", "C", "E"] - end + ScratchPad.record [] + eval("('A'...)").step(2) { |x| break if x > "F"; ScratchPad << x } + ScratchPad.recorded.should == ["A", "C", "E"] + end - it "raises a TypeError when passed a Float step" do - -> { eval("('A'..)").step(2.0) { } }.should raise_error(TypeError) - -> { eval("('A'...)").step(2.0) { } }.should raise_error(TypeError) - end + it "raises a TypeError when passed a Float step" do + -> { eval("('A'..)").step(2.0) { } }.should raise_error(TypeError) + -> { eval("('A'...)").step(2.0) { } }.should raise_error(TypeError) end ruby_version_is "3.4" do - it "raises an ArgumentError when not passed a step" do - -> { ("A"..).step { } }.should raise_error(ArgumentError) - end - it "yields String values adjusted by step" do eval("('A'..)").step("A") { |x| break if x > "AAA"; ScratchPad << x } ScratchPad.recorded.should == ["A", "AA", "AAA"] @@ -522,8 +508,8 @@ describe "Range#step" do end it "raises a TypeError when passed an incompatible type step" do - -> { eval("('A'..)").step(2) { } }.should raise_error(TypeError) - -> { eval("('A'...)").step(2) { } }.should raise_error(TypeError) + -> { eval("('A'..)").step([]) { } }.should raise_error(TypeError) + -> { eval("('A'...)").step([]) { } }.should raise_error(TypeError) end end end diff --git a/test/ruby/test_range.rb b/test/ruby/test_range.rb index b7541424d1..6c591b3155 100644 --- a/test/ruby/test_range.rb +++ b/test/ruby/test_range.rb @@ -427,11 +427,11 @@ class TestRange < Test::Unit::TestCase assert_raise(ArgumentError) { (...'aaa').step('a') } # step is not provided - assert_raise(ArgumentError) { ('a'...'aaaa').step } + assert_raise(ArgumentError) { (Time.new(2022)...Time.new(2023)).step } # step is incompatible - assert_raise(TypeError) { ('a'...'aaaa').step(1) {} } - assert_raise(TypeError) { ('a'...'aaaa').step(1).to_a } + assert_raise(TypeError) { (Time.new(2022)...Time.new(2023)).step('a') {} } + assert_raise(TypeError) { (Time.new(2022)...Time.new(2023)).step('a').to_a } # step is compatible, but shouldn't convert into numeric domain: a = [] @@ -467,6 +467,58 @@ class TestRange < Test::Unit::TestCase assert_equal([], a) end + def test_step_string_legacy + # finite + a = [] + ('a'..'g').step(2) { a << _1 } + assert_equal(%w[a c e g], a) + + assert_kind_of(Enumerator, ('a'..'g').step(2)) + assert_equal(%w[a c e g], ('a'..'g').step(2).to_a) + + a = [] + ('a'...'g').step(2) { a << _1 } + assert_equal(%w[a c e], a) + + assert_kind_of(Enumerator, ('a'...'g').step(2)) + assert_equal(%w[a c e], ('a'...'g').step(2).to_a) + + # endless + a = [] + ('a'...).step(2) { a << _1; break if a.size == 3 } + assert_equal(%w[a c e], a) + + assert_kind_of(Enumerator, ('a'...).step(2)) + assert_equal(%w[a c e], ('a'...).step(2).take(3)) + + # beginless + assert_raise(ArgumentError) { (...'g').step(2) {} } + assert_raise(ArgumentError) { (...'g').step(2) } + + # step is not provided + a = [] + ('a'..'d').step { a << _1 } + assert_equal(%w[a b c d], a) + + assert_kind_of(Enumerator, ('a'..'d').step) + assert_equal(%w[a b c d], ('a'..'d').step.to_a) + + a = [] + ('a'...'d').step { a << _1 } + assert_equal(%w[a b c], a) + + assert_kind_of(Enumerator, ('a'...'d').step) + assert_equal(%w[a b c], ('a'...'d').step.to_a) + + # endless + a = [] + ('a'...).step { a << _1; break if a.size == 3 } + assert_equal(%w[a b c], a) + + assert_kind_of(Enumerator, ('a'...).step) + assert_equal(%w[a b c], ('a'...).step.take(3)) + end + def test_step_bug15537 assert_equal([10.0, 9.0, 8.0, 7.0], (10 ..).step(-1.0).take(4)) assert_equal([10.0, 9.0, 8.0, 7.0], (10.0 ..).step(-1).take(4))