Skip to content

Commit 29372c8

Browse files
headiusheadius
committed
Align Range#step logic with CRuby 3.4
Most of the logic was already here but there's new bits for non- numeric ranges or ranges stepping with non-numeric values.
1 parent 9f70c5a commit 29372c8

File tree

2 files changed

+133
-106
lines changed

2 files changed

+133
-106
lines changed

‎core/src/main/java/org/jruby/RubyNumeric.java‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1166,32 +1166,6 @@ private static void fixnumStep(ThreadContext context, RubyFixnum from, IRubyObje
11661166
}
11671167
}
11681168

1169-
static void floatStep(ThreadContext context, Ruby runtime, IRubyObject from, IRubyObject to, IRubyObject step, boolean excl, boolean allowEndless, Block block) {
1170-
double beg = num2dbl(context, from);
1171-
double end = allowEndless && to.isNil() ? RubyFloat.INFINITY : num2dbl(context, to);
1172-
double unit = num2dbl(context, step);
1173-
1174-
double n = floatStepSize(beg, end, unit, excl);
1175-
1176-
if (Double.isInfinite(unit)) {
1177-
/* if unit is infinity, i*unit+beg is NaN */
1178-
if (n != 0) block.yield(context, asFloat(context, beg));
1179-
} else if (unit == 0) {
1180-
RubyFloat value = asFloat(context, beg);
1181-
for (;;) {
1182-
block.yield(context, value);
1183-
context.pollThreadEvents();
1184-
}
1185-
} else {
1186-
for (long i=0; i<n; i++) {
1187-
double d = i*unit+beg;
1188-
if (unit >= 0 ? end < d : d < end) d = end;
1189-
block.yield(context, asFloat(context, d));
1190-
context.pollThreadEvents();
1191-
}
1192-
}
1193-
}
1194-
11951169
private static void duckStep(ThreadContext context, IRubyObject from, IRubyObject to, IRubyObject step, boolean inf, boolean desc, Block block) {
11961170
IRubyObject i = from;
11971171

‎core/src/main/java/org/jruby/RubyRange.java‎

Lines changed: 133 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import org.jruby.runtime.Signature;
5959
import org.jruby.runtime.ThreadContext;
6060

61+
import static org.jruby.RubyEnumerator.enumeratorize;
6162
import static org.jruby.RubyEnumerator.enumeratorizeWithSize;
6263
import static org.jruby.RubyNumeric.*;
6364
import static org.jruby.api.Convert.*;
@@ -755,31 +756,11 @@ public IRubyObject step(final ThreadContext context, final Block block) {
755756

756757
@JRubyMethod(name = "step")
757758
public IRubyObject step(final ThreadContext context, IRubyObject step, final Block block) {
758-
String method = "step";
759-
if (!block.isGiven()) {
760-
return stepEnumeratorize(context, step, method);
761-
}
762-
763-
step = checkStepDomain(context, step, method);
764-
765759
return stepCommon(context, step, block);
766760
}
767761

768-
private IRubyObject checkStepDomain(ThreadContext context, IRubyObject step, String method) {
769-
if (!(step instanceof RubyNumeric)) step = step.convertToInteger("to_int");
770-
if (((RubyNumeric) step).isNegative(context)) throw argumentError(context, method + " can't be negative");
771-
if (((RubyNumeric) step).isZero(context)) throw argumentError(context, method + " can't be 0");
772-
773-
return step;
774-
}
775-
776762
private IRubyObject stepEnumeratorize(ThreadContext context, IRubyObject step, String method) {
777-
if (!step.isNil() && !(step instanceof RubyNumeric)) step = step.convertToInteger("to_int");
778-
if ((step instanceof RubyNumeric num) && num.isZero(context)) throw argumentError(context, "step can't be 0");
779-
780-
if ((begin instanceof RubyNumeric && (end.isNil() || end instanceof RubyNumeric)) ||
781-
(end instanceof RubyNumeric && begin.isNil())) {
782-
763+
if (step instanceof RubyNumeric && (begin instanceof RubyNumeric && (end.isNil() || end instanceof RubyNumeric)) || (begin.isNil() && end instanceof RubyNumeric)) {
783764
return RubyArithmeticSequence.newArithmeticSequence(
784765
context,
785766
this,
@@ -791,9 +772,14 @@ private IRubyObject stepEnumeratorize(ThreadContext context, IRubyObject step, S
791772
isExclusive ? context.tru : context.fals);
792773
}
793774

775+
// ...but generic Enumerator from beginless range is useless and probably an error.
776+
if (begin.isNil()) {
777+
throw argumentError(context, "#step for non-numeric beginless ranges is meaningless");
778+
}
779+
794780
return !step.isNil() ?
795-
enumeratorizeWithSize(context, this, method, new IRubyObject[]{step}, RubyRange::stepSize) :
796-
enumeratorizeWithSize(context, this, method, RubyRange::stepSize);
781+
enumeratorize(context.runtime, this, method, step) :
782+
enumeratorize(context.runtime, this, method);
797783
}
798784

799785
@JRubyMethod(name = "%")
@@ -802,81 +788,148 @@ public IRubyObject op_mod(final ThreadContext context, IRubyObject step) {
802788
}
803789

804790
private IRubyObject stepCommon(ThreadContext context, IRubyObject step, Block block) {
805-
Ruby runtime = context.runtime;
806-
if (begin instanceof RubyFixnum && end.isNil() && step instanceof RubyFixnum) {
807-
long i = begin.convertToInteger().getLongValue();
808-
long unit = step.convertToInteger().getLongValue();
809-
while (i < Long.MAX_VALUE) {
810-
block.yield(context, asFixnum(context, i));
811-
i += unit;
812-
}
813-
IRubyObject b = asFixnum(context, i);
814-
for (;; b = ((RubyInteger) b).op_plus(context, step)) {
815-
block.yield(context, b);
791+
boolean beginIsNumeric = begin instanceof RubyNumeric;
792+
boolean endIsNumeric = end instanceof RubyNumeric;
793+
// For backward compatibility reasons (conforming to behavior before 3.4), String/Symbol
794+
// supports both old behavior ('a'..).step(1) and new behavior ('a'..).step('a')
795+
// Hence the additional conversion/additional checks.
796+
IRubyObject strBegin = begin.checkStringType();
797+
IRubyObject symBegin = begin instanceof RubySymbol symbol ? symbol.to_s(context) : context.nil;
798+
799+
if (step.isNil()) {
800+
if (beginIsNumeric || !strBegin.isNil() || !symBegin.isNil() || (begin.isNil() && endIsNumeric)) {
801+
step = asFixnum(context, 1);
802+
} else {
803+
throw argumentError(context, "step is required for non-numeric ranges");
816804
}
805+
}
806+
807+
boolean stepIsNumeric = step instanceof RubyNumeric;
808+
809+
if (stepIsNumeric && beginIsNumeric && step.op_eqq(context, asFixnum(context, 0)).isTrue()) {
810+
throw argumentError(context, "step can't be 0");
811+
}
812+
813+
if (!block.isGiven()) {
814+
return stepEnumeratorize(context, step, "step");
815+
}
816+
817+
if (begin.isNil()) {
818+
throw argumentError(context, "#step iteration for beginless ranges is meaningless");
819+
}
820+
821+
if (begin instanceof RubyFixnum && end.isNil() && step instanceof RubyFixnum) {
822+
fixnumEndlessStep(context, step, block);
817823
} else if (begin instanceof RubyFixnum && end instanceof RubyFixnum && step instanceof RubyFixnum) {
818824
fixnumStep(context, ((RubyFixnum) step).getLongValue(), block);
819-
} else if (begin instanceof RubyFloat || end instanceof RubyFloat || step instanceof RubyFloat) {
820-
RubyNumeric.floatStep(context, runtime, begin, end, step, isExclusive, isEndless, block);
821-
} else if (begin instanceof RubySymbol && (end.isNil() || end instanceof RubySymbol)) { /* symbols are special */
822-
RubyString b = begin.asString();
823-
SymbolStepBlockCallBack callback = new SymbolStepBlockCallBack(block, RubyFixnum.one(runtime), step);
824-
Block blockCallback = CallBlock.newCallClosure(context, this, Signature.ONE_ARGUMENT, callback);
825-
if (end.isNil()) {
826-
b.uptoEndless(context, blockCallback);
827-
} else {
828-
b.uptoCommon(context, end.asString(), isExclusive, blockCallback);
829-
}
830-
} else if (begin instanceof RubyNumeric
831-
|| !checkToInteger(context, begin).isNil()
832-
|| !checkToInteger(context, end).isNil()) {
833-
numericStep(context, step, block);
825+
} else if (beginIsNumeric && endIsNumeric && floatStep(context, begin, end, step, isExclusive, isEndless, block)) {
826+
/* done */
827+
} else if (!strBegin.isNil() && step instanceof RubyFixnum) {
828+
// backwards compatibility behavior for String only, when no step/Integer step is passed
829+
// See discussion in https://bugs.ruby-lang.org/issues/18368
830+
stringStep(context, step, block, (RubyString) strBegin);
831+
} else if (!symBegin.isNil() && step instanceof RubyFixnum) {
832+
// same as above: backward compatibility for symbols
833+
symbolStep(context, step, block, (RubyString) symBegin);
834834
} else {
835-
IRubyObject tmp = begin.checkStringType();
836-
if (!tmp.isNil()) {
837-
StepBlockCallBack callback = new StepBlockCallBack(block, RubyFixnum.one(runtime), step);
838-
Block blockCallback = CallBlock.newCallClosure(context, this, Signature.ONE_ARGUMENT, callback);
839-
if (end.isNil()) {
840-
((RubyString) tmp).uptoEndless(context, blockCallback);
835+
int c, dir;
836+
IRubyObject v = begin;
837+
if (!end.isNil()) {
838+
if (beginIsNumeric && stepIsNumeric && rangeLess(context, step, asFixnum(context, 0)) < 0) {
839+
// iterate backwards, for consistency with ArithmeticSequence
840+
if (isExclusive) {
841+
for (; rangeLess(context, end, v) < 0; v = v.callMethod(context, "+", step)) {
842+
block.yield(context, v);
843+
}
844+
} else {
845+
for (; (c = rangeLess(context, end, v)) <= 0; v = v.callMethod(context, "+", step)) {
846+
block.yield(context, v);
847+
if (c == 0) break;
848+
}
849+
}
850+
841851
} else {
842-
((RubyString) tmp).uptoCommon(context, end, isExclusive, blockCallback);
852+
// Direction of the comparison. We use it as a comparison operator in cycle:
853+
// if begin < end, the cycle performs while value < end (iterating forward)
854+
// if begin > end, the cycle performs while value > end (iterating backward with
855+
// a negative step)
856+
dir = rangeLess(context, begin, end);
857+
// One preliminary addition to check the step moves iteration in the same direction as
858+
// from begin to end; otherwise, the iteration should be empty.
859+
if (rangeLess(context, begin, begin.callMethod(context, "+", step)) == dir) {
860+
if (isExclusive) {
861+
for (; rangeLess(context, v, end) == dir; v = v.callMethod(context, "+", step)) {
862+
block.yield(context, v);
863+
}
864+
} else {
865+
for (; (c = rangeLess(context, v, end)) == dir || c == 0; v = v.callMethod(context, "+", step)) {
866+
block.yield(context, v);
867+
if (c == 0) break;
868+
}
869+
}
870+
}
843871
}
844872
} else {
845-
if (!begin.respondsTo("succ")) throw typeError(context, "can't iterate from ", begin, "");
846-
847-
// range_each_func(range, step_i, b, e, args);
848-
rangeEach(context, new StepBlockCallBack(block, RubyFixnum.one(runtime), step));
873+
for (; ; v = v.callMethod(context, "+", step)) {
874+
block.yield(context, v);
875+
}
849876
}
850877
}
851878
return this;
852879
}
853880

881+
private void fixnumEndlessStep(ThreadContext context, IRubyObject step, Block block) {
882+
long i = begin.convertToInteger().getLongValue();
883+
long unit = step.convertToInteger().getLongValue();
884+
while (i < Long.MAX_VALUE) {
885+
block.yield(context, asFixnum(context, i));
886+
i += unit;
887+
}
888+
IRubyObject b = asFixnum(context, i);
889+
for (;; b = ((RubyInteger) b).op_plus(context, step)) {
890+
block.yield(context, b);
891+
}
892+
}
893+
854894
private void fixnumStep(ThreadContext context, long step, Block block) {
855-
// We must avoid integer overflows.
856-
// Any method calling this method must ensure that "step" is greater than 0.
857-
long to = ((RubyFixnum) end).getLongValue();
858-
if (isExclusive) {
859-
if (to == Long.MIN_VALUE) return;
860-
to--;
895+
long end = fix2long(this.end);
896+
long i, unit = step;
897+
if (unit < 0) {
898+
if (!isExclusive)
899+
end -= 1;
900+
i = fix2long(begin);
901+
while (i > end) {
902+
block.yield(context, asFixnum(context, i));
903+
i += unit;
904+
}
905+
} else {
906+
if (!isExclusive)
907+
end += 1;
908+
i = fix2long(begin);
909+
while (i < end) {
910+
block.yield(context, asFixnum(context, i));
911+
i += unit;
912+
}
861913
}
862-
long tov = Long.MAX_VALUE - step;
863-
if (to < tov) tov = to;
914+
}
864915

865-
long i;
866-
for (i = ((RubyFixnum) begin).getLongValue(); i <= tov; i += step) {
867-
block.yield(context, asFixnum(context, i));
916+
private void stringStep(ThreadContext context, IRubyObject step, Block block, RubyString strBegin) {
917+
StepBlockCallBack callback = new StepBlockCallBack(block, RubyFixnum.one(context.runtime), step);
918+
Block blockCallback = CallBlock.newCallClosure(context, this, Signature.ONE_ARGUMENT, callback);
919+
if (end.isNil()) {
920+
strBegin.uptoEndless(context, blockCallback);
921+
} else {
922+
strBegin.uptoCommon(context, end, isExclusive, blockCallback);
868923
}
869-
if (i <= to) block.yield(context, asFixnum(context, i));
870924
}
871925

872-
private void numericStep(ThreadContext context, IRubyObject step, Block block) {
873-
final String method = isExclusive ? "<" : "<=";
874-
IRubyObject beg = begin;
875-
long i = 0;
876-
while (beg.callMethod(context, method, end).isTrue()) {
877-
block.yield(context, beg);
878-
i++;
879-
beg = begin.callMethod(context, "+", asFixnum(context, i).callMethod(context, "*", step));
926+
private void symbolStep(ThreadContext context, IRubyObject step, Block block, RubyString symBegin) {
927+
SymbolStepBlockCallBack callback = new SymbolStepBlockCallBack(block, RubyFixnum.one(context.runtime), step);
928+
Block blockCallback = CallBlock.newCallClosure(context, this, Signature.ONE_ARGUMENT, callback);
929+
if (end.isNil()) {
930+
symBegin.uptoEndless(context, blockCallback);
931+
} else {
932+
symBegin.uptoCommon(context, end.asString(), isExclusive, blockCallback);
880933
}
881934
}
882935

0 commit comments

Comments
 (0)