Range#step: restore legacy behavior for String ranges

Merged: https://github.com/ruby/ruby/pull/11454

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 if (!NIL_P(sb) && FIXNUM_P(step)) {
-    else {
+        // 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) {

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
-        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 }
-          eval("('A'..)").step { |x| break if x > "D"; ScratchPad << x }
+        ScratchPad.recorded.should == ["A", "B", "C", "D"]
-          ScratchPad.recorded.should == ["A", "B", "C", "D"]
 
-          ScratchPad.record []
+        ScratchPad.record []
-          eval("('A'...)").step { |x| break if x > "D"; ScratchPad << x }
+        eval("('A'...)").step { |x| break if x > "D"; ScratchPad << x }
-          ScratchPad.recorded.should == ["A", "B", "C", "D"]
+        ScratchPad.recorded.should == ["A", "B", "C", "D"]
-        end
+      end
 
-        it "yields String values incremented by #succ called Integer step times" do
+      it "yields String values incremented by #succ called Integer step times" do
-          eval("('A'..)").step(2) { |x| break if x > "F"; ScratchPad << x }
+        eval("('A'..)").step(2) { |x| break if x > "F"; ScratchPad << x }
-          ScratchPad.recorded.should == ["A", "C", "E"]
+        ScratchPad.recorded.should == ["A", "C", "E"]
 
-          ScratchPad.record []
+        ScratchPad.record []
-          eval("('A'...)").step(2) { |x| break if x > "F"; ScratchPad << x }
+        eval("('A'...)").step(2) { |x| break if x > "F"; ScratchPad << x }
-          ScratchPad.recorded.should == ["A", "C", "E"]
+        ScratchPad.recorded.should == ["A", "C", "E"]
-        end
+      end
 
-        it "raises a TypeError when passed a Float step" do
+      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)
-          -> { eval("('A'...)").step(2.0) { } }.should raise_error(TypeError)
+        -> { eval("('A'...)").step(2.0) { } }.should raise_error(TypeError)
-        end
       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([]) { } }.should raise_error(TypeError)
-          -> { eval("('A'...)").step(2) { } }.should raise_error(TypeError)
+          -> { eval("('A'...)").step([]) { } }.should raise_error(TypeError)
         end
       end
     end

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) { (Time.new(2022)...Time.new(2023)).step('a') {} }
-    assert_raise(TypeError) { ('a'...'aaaa').step(1).to_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))