Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range.contains is wrong #6736

Closed
scabug opened this issue Nov 30, 2012 · 10 comments
Closed

Range.contains is wrong #6736

scabug opened this issue Nov 30, 2012 · 10 comments

Comments

@scabug
Copy link

scabug commented Nov 30, 2012

scala> (Int.MinValue until 0).contains(4)
res18: Boolean = true
scala> (0 to Int.MaxValue).contains(4)
res21: Boolean = false
@scabug
Copy link
Author

scabug commented Nov 30, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6736?orig=1
Reporter: @Atry
Affected Versions: 2.10.0-RC3

@scabug
Copy link
Author

scabug commented Nov 30, 2012

Daniel Vashchilenko (skydan) said:
Scala 2.9.2 is not affected as it won't evaluate:

java.lang.IllegalArgumentException: -2147483648 until 0 by 1: seqs cannot contain more than Int.MaxValue elements.

@scabug
Copy link
Author

scabug commented Dec 1, 2012

@soc said:
It could be possible that the issue was introduced when Range was made more lazy to allow “too large” start/end inputs where by is used afterwards to make the size less than Int.MaxValue again, like {{{Int.MinValue to Int.MaxValue by 10}}}.

contains probably doesn't check the inputs, because it still assumes that an invalid Range would have thrown en exception earlier already as it has been the case in older versions.

@scabug
Copy link
Author

scabug commented Dec 1, 2012

Daniel Vashchilenko (skydan) said (edited on Dec 1, 2012 4:52:32 PM UTC):
contains seems to be good to me. It uses step-aware isWithinBoundaries and then checks that the value is n*step away from the start.

final def contains(x: Int) = isWithinBoundaries(x) && ((x - start) % step == 0)

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@soc said:
I'm not sure that's the issue here. If not otherwise specified, step is always 1. In the given examples the range is ascending anyways, so the check just needs to get fixed properly. See also #6747. Or am I missing something?

@scabug
Copy link
Author

scabug commented Dec 13, 2012

Sergii Zashchelkin (sergz72) said:
Problem is in isWithinBoundaries function:

  // Tests whether a number is within the endpoints, without testing
  // whether it is a member of the sequence (i.e. when step > 1.)
  private def isWithinBoundaries(elem: Int) = !isEmpty && (
    (step > 0 && start <= elem && elem <= last ) ||
    (step < 0 &&  last <= elem && elem <= start)
  )
``

this function uses Range.last function which defined as follows:

```scala
  override def last = if (isEmpty) Nil.last else lastElement

and lastElement function have following definition:

  final val lastElement     = start + (numRangeElements - 1) * step

and numRangeElements have following definition:

  final val numRangeElements: Int = {
    if (step == 0) throw new IllegalArgumentException("step cannot be 0.")
    else if (isEmpty) 0
    else {
      val len = longLength
      if (len > scala.Int.MaxValue) -1
      else len.toInt
    }
  }

so if length of Range object > scala.Int.MaxValue it returns -1

and lastElement will be calculated as start + (-1 - 1) * step and this equals (step = 1) to start - 2

I propose to use Range.end instead of Range.last in the isWithinBoundaries function

@scabug
Copy link
Author

scabug commented Dec 13, 2012

Daniel Vashchilenko (skydan) said:
isWithinBoundaries returns true if the number is between the start and the last element. It does not check that the number is a part of the Range. This is the expected behavior as far as I understand.

@scabug
Copy link
Author

scabug commented Jan 23, 2013

@adriaanm said:
Alex, could you have a look?

@scabug
Copy link
Author

scabug commented May 20, 2013

@JamesIry said:
2.10.2 is about to be cut. Kicking down the road and un-assigning to foster work stealing.

@scabug
Copy link
Author

scabug commented Feb 1, 2014

@adriaanm said:
scala/scala#3437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants