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

mutable ArrayBuffer constructor throws on negative size #11482

Open
martijnhoekstra opened this Issue Apr 12, 2019 · 9 comments

Comments

Projects
None yet
4 participants
@martijnhoekstra
Copy link

commented Apr 12, 2019

new scala.collection.mutable.ArrayBuffer(-1) throws NegativeArraySizeException in 2.13.0-RC1, but runs fine in 2.12.x

@SethTisue

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

seems like a progression rather than a regression?

@martijnhoekstra

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

Not sure -- I couldn't find documentation of the change or its reasoning. It seems marginally likelier to me that it'll break peoples code than that it'll catch bugs. 2.12 sets the minimum size of the underlying array to at least 1. It has been that way at least since the old collections were new.

Maybe the bug is that the constructor is undocumented?

@som-snytt

This comment has been minimized.

Copy link

commented Apr 12, 2019

It ought to throw IllegalArgumentException.

@Ichoran

This comment has been minimized.

Copy link

commented Apr 12, 2019

There is no reason an advisory parameter like this should throw an exception rather than implementing the most sane behavior under the circumstances. Something like take(-1) is much more likely to be an error of logic than this is, yet we advocate for the safety of take.

So this should be regarded, unreservedly, as a bug. And fixed.

@som-snytt

This comment has been minimized.

Copy link

commented Apr 12, 2019

WWJD? ArrayList throws.

@Ichoran

This comment has been minimized.

Copy link

commented Apr 12, 2019

WWJD? ArrayList throws.

This is entirely within character for Java, as ArrayList consists almost entirely of brittle low-level methods.

@som-snytt

This comment has been minimized.

Copy link

commented Apr 13, 2019

Brittle as in peanut?

scala> val buf = new ArrayBuffer
buf: scala.collection.mutable.ArrayBuffer[Nothing] = ArrayBuffer()

scala> buf.remove(0, -1)
java.lang.IllegalArgumentException: removing negative number of elements: -1
  at scala.collection.mutable.ArrayBuffer.remove(ArrayBuffer.scala:204)
  ... 29 elided

Normally, I would suggest "fluent" API wants to be forgiving, but Unit methods and certainly constructors should be inclined to fail. Constructors especially should result in a well-defined state, but is new ArrayBuffer(-1) sized to zero or the default?

scala> val buf = new ArrayBuffer(42)
buf: scala.collection.mutable.ArrayBuffer[Nothing] = ArrayBuffer()

scala> buf.remove(-1)
java.lang.IndexOutOfBoundsException: -1 is out of bounds (min 0, max -1)
  at scala.collection.mutable.ArrayBuffer.checkWithinBounds(ArrayBuffer.scala:91)
  at scala.collection.mutable.ArrayBuffer.remove(ArrayBuffer.scala:188)
  ... 29 elided
@martijnhoekstra

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

The 2.12 collections set the underlying array size to max(arg, 1), which seems like a reasonable size, but I have no strong preference between 0, 1 or default (or, indeed throwing an IllegalArgumentException)

The remove(-1) behaviour sounds correct to me, though that out of bounds message is confusing, and probably needs to be special-cased for when the collection is empty.

The remove(0, -1) behaviour sounds like something that could be argued about, throwing andnot removing anything both seem plausible things to do.

These are both significantly different from the constructor argument in that the constructor argument is only a bookkeeping argument for which an unexpected value shouldn't impede normal handling. I guess you could argue that if the argument is important enough for the constructor to exist, out of range values should be important enough to be errors, and if you don't care enough about the value you pass in out of bounds values, maybe the entire things shouldn't exist/be used.

I'm not going to argue any of those points, I'm just reporting the change in behaviour

@SethTisue SethTisue added this to the 2.13.0-RC2 milestone Apr 13, 2019

@Ichoran

This comment has been minimized.

Copy link

commented Apr 13, 2019

Normally, I would suggest "fluent" API wants to be forgiving, but Unit methods and certainly constructors should be inclined to fail. Constructors especially should result in a well-defined state, but is new ArrayBuffer(-1) sized to zero or the default?

The array-expanding math is simplest if zero is forbidden, so it would be 1.

Constructors and Unit methods have a responsibility to do the very best job they can to do something sane, and throw exceptions only as a last resort. Sensible defaults, unless used in a context where it essential to die if doing anything other than exactly what you asked for, are one way to accomplish sanity. Alternatively, you can divide your API in to a "brittle low-level" part that throws exceptions if any i is missing its dot, and a "forgiving high-level" part that does as close to the right thing as practical. Overwhelmingly the collections library is the latter type, so in ambiguous cases I'd advocate for the latter behavior.

You can't remove and return back something that isn't there--so trying should be an exception, yes--but asking to remove less than nothing should, almost surely, just be interpreted as a request to remove nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.