Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

SI-6634 Fixes data corruption issue in ListBuffer#remove

This is the cut-down version with minimally invasive changes,
e. g. keeping the "auto-correcting" bounds algorithm.
  • Loading branch information...
commit 2c23acf39e810fd43f9ce5af0a4c3e4d952f2081 1 parent c264898
@soc soc authored
View
5 src/library/scala/collection/mutable/ListBuffer.scala
@@ -249,7 +249,12 @@ final class ListBuffer[A]
* @param n the index which refers to the first element to remove.
* @param count the number of elements to remove.
*/
+ @annotation.migration("Invalid input values will be rejected in future releases.", "2.11")
@jozic
jozic added a note

Should't this be something like "Invalid input values are rejected now"?
I mean the code is already here

@paulp
paulp added a note

Also, where is the documentation on what behavior to expect on invalid values? It's arguably worse than doing nothing to modify the behavior without giving people a clear indication of what to expect. I know the entire collections are woefully underdocumented in this regard, but that's all the more reason to start.

@soc
soc added a note

@jozic: Not all "probably" invalid values are rejected with this patch, just the ones which would have caused a corrupted data structure, as far as I remember.

@paulp I mentioned on the mailing list that the whole collection library is underspecified in this regard and needs an overhaul.
Because fixing it once-and-for-all will probably not work in the 2.10.x releases, I'm targeting 2.11 for everything more than the most pressing stop-gap measures. Looking at individual methods doesn't make that much sense anyway, so it should better go into 2.11. What's needed is a holistic approach which considers the collection API as a whole (and related APIs like Slick, ...) and existing precedents like Haskell's implementation.

I think once we can use macros for testing it might get a lot easier to say "invoke all the collection API's methods for each implementation with the following arguments: ..."

@paulp
paulp added a note

"I think once we can use macros for testing it might get a lot easier to say "invoke all the collection API's methods for each implementation with the following arguments: ..."

There are a number of ways you could do this already, possibly most easily with reflection.

@jozic
jozic added a note

@soc
I'm just saying that the text just looks a little bit inconsistent with other usages of migration annotation

For example,
@migration("The behavior of scanRight has changed. The previous behavior can be reproduced with scanRight.reverse.", "2.9.0")
@migration("Double linked list now removes the current node from the list.", "2.9.0")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
override def remove(n: Int, count: Int) {
+ if (n >= len)
+ return
+ if (count < 0)
+ throw new IllegalArgumentException(s"removing negative number ($count) of elements")
if (exported) copy()
val n1 = n max 0
val count1 = count min (len - n1)
View
31 test/files/run/t6634.check
@@ -0,0 +1,31 @@
+Trying lb0 ...
+Checking ...
+String OK.
+Length OK.
+
+Trying lb1 ...
+Checking ...
+String OK.
+Length OK.
+
+Trying lb2 ...
+Checking ...
+String OK.
+Length OK.
+
+Trying lb3 ...
+Checking ...
+String OK.
+Length OK.
+
+Trying lb4 ...
+Checking ...
+String OK.
+Length OK.
+
+Trying lb5 ...
+java.lang.IllegalArgumentException: removing negative number (-1) of elements
+Checking ...
+String OK.
+Length OK.
+
View
80 test/files/run/t6634.scala
@@ -0,0 +1,80 @@
+import collection.mutable.ListBuffer
+
+object Test extends App {
+ def newLB = ListBuffer('a, 'b, 'c, 'd, 'e)
+
+ val lb0 = newLB
+ println("Trying lb0 ...")
+ try {
+ lb0.remove(5, 0)
+ } catch {
+ // Not thrown in 2.10, will be thrown in 2.11
+ case ex: IndexOutOfBoundsException => println(ex)
+ }
+ checkNotCorrupted(lb0)
+
+ val lb1 = newLB
+ println("Trying lb1 ...")
+ try {
+ lb1.remove(6, 6)
+ } catch {
+ // Not thrown in 2.10, will be thrown in 2.11
+ case ex: IndexOutOfBoundsException => println(ex)
+ }
+ checkNotCorrupted(lb1)
+
+ val lb2 = newLB
+ println("Trying lb2 ...")
+ try {
+ lb2.remove(99, 6)
+ } catch {
+ // Not thrown in 2.10, will be thrown in 2.11
+ case ex: IndexOutOfBoundsException => println(ex)
+ }
+ checkNotCorrupted(lb2)
+
+ val lb3 = newLB
+ println("Trying lb3 ...")
+ try {
+ lb3.remove(1, 9)
+ } catch {
+ // Not thrown in 2.10, will be thrown in 2.11
+ case ex: IllegalArgumentException => println(ex)
+ }
+ checkNotCorrupted(lb3, "ListBuffer('a)", 1)
+
+ val lb4 = newLB
+ println("Trying lb4 ...")
+ try {
+ lb4.remove(-1, 1)
+ } catch {
+ // Not thrown in 2.10, will be thrown in 2.11
+ case ex: IndexOutOfBoundsException => println(ex)
+ }
+ checkNotCorrupted(lb4, "ListBuffer('b, 'c, 'd, 'e)", 4)
+
+ val lb5 = newLB
+ println("Trying lb5 ...")
+ try {
+ lb5.remove(1, -1)
+ } catch {
+ case ex: IllegalArgumentException => println(ex)
+ }
+ checkNotCorrupted(lb5)
+
+ // buffer should neither be changed nor corrupted after calling remove with invalid arguments
+ def checkNotCorrupted(
+ lb: ListBuffer[Symbol],
+ expectedString: String = "ListBuffer('a, 'b, 'c, 'd, 'e)",
+ expectedLength: Int = 5) = {
+ println("Checking ...")
+ val replStr = scala.runtime.ScalaRunTime.replStringOf(lb, 100)
+ if (replStr == expectedString + "\n") println("String OK.")
+ else println("!!! replStringOf FAILED: " + replStr)
+
+ val len = lb.length
+ if (len == expectedLength) println("Length OK.")
+ else println("!!! length FAILED: " + len)
+ println()
+ }
+}
Please sign in to comment.
Something went wrong with that request. Please try again.