scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

`NumericRange.length` fails for wide`Long` ranges. #12716

Open AminMal opened 1 year ago

AminMal commented 1 year ago

Reproduction steps

Scala version: 2.13.4, 2.13.8, 2.13.12, 2.13.13

scala> import scala.collection.immutable.NumericRange
import scala.collection.immutable.NumericRange

scala> val r = NumericRange(1L, Long.MinValue, -1L)
val r: collection.immutable.NumericRange.Exclusive[Long] = NumericRange 1 until -9223372036854775808 by -1

scala> r.length
val res0: Int = 1

Problem

Trying to get the length of a wide Long range, somehow 1 is returned, normally, I expected it to throw an IllegalArgumentException much like wide Int or BigInt or BigDecimal. The same exact values but with BigInt is working fine though:

scala> val r = NumericRange(BigInt(1L), BigInt(Long.MinValue), BigInt(-1L)) 
val r: collection.immutable.NumericRange.Exclusive[scala.math.BigInt] = NumericRange 1 until -9223372036854775808 by -1

scala> r.length
java.lang.IllegalArgumentException: More than Int.MaxValue elements.
  at scala.collection.immutable.NumericRange$.check$1(NumericRange.scala:324)
  at scala.collection.immutable.NumericRange$.count(NumericRange.scala:367)
  at scala.collection.immutable.NumericRange.length$lzycompute(NumericRange.scala:75)
  at scala.collection.immutable.NumericRange.length(NumericRange.scala:75)
  ... 32 elided
Ichoran commented 1 year ago

Good catch! There are a lot of edge cases in this code, and apparently you found one that was missed!

Ichoran commented 1 year ago

Looks like (N, Long.MinValue, -1L) always returns N. Also, separately, (Long.MinValue, 0L, 1L) returns 0 instead of an exception.

Ichoran commented 1 year ago

The latter one is caused by

      // If the range crosses zero, it might overflow when subtracted
      val startside = num.sign(start)
      val endside = num.sign(end)
      num.toInt{
        if (num.gteq(num.times(startside, endside), zero)) {

not obeying (later on) the requirement that you can only count on being able to represent -a+1 if a is negative, not -a. This can be restored by making the following change

      num.toInt{
        // If the range crosses from negative to zero or positive,
        // it might overflow when subtracted; otherwise we can compute directly
        if (num.lt(start, zero) == num.lt(end, zero)) {
Ichoran commented 1 year ago

The second one is a bit more subtle. The first waypoint when traveling in the negative direction is 1, and then you go one step, and then you go to the endpoint. The problem is that if the step size is -1, the endpoint is 0, and 0 to MinValue won't fit in a 2's complement integer.

I am pretty sure, but not completely sure, that this will be fixed by replacing

          val startlim  = if (posStep) negone else one

by

          val startlim  = if (posStep) negone else zero
Ichoran commented 1 year ago

I also don't see how this line can be correct:

          val waypointA = if (startq == zero) start else num.plus(start, num.times(startq, step))

I'm pretty sure this would have to be

          val waypointA = if (startq == zero) startlim else num.plus(start, num.times(startq, step))

No, this was silly, I misread what startq means. This is fine.