IndexedSeq#head now throws NoSuchElementException (not IndexOutOfBoundsException)
#10392
Conversation
| override def head: A = | ||
| try apply(0) | ||
| catch { | ||
| case e: IndexOutOfBoundsException => |
There was a problem hiding this comment.
This won't work on Scala.js. Array index out of bounds is undefined behavior over there. Besides, using a try for the happy path is not quite as free as on the JVM.
It seems to me that an explicit isEmpty test would be more appropriate. It's what we have in other collections I believe.
There was a problem hiding this comment.
I waffled on isEmpty because of hidden costs, but maybe that is paranoid for an indexed thing. Looking at it now, I see the PR from last year was that IterableOnce#isEmpty now uses knownSize first and iterator.hasNext second. I haven't looked whether indexed implies size is known, but it seems likely. They have to override something useful if they want a useful collection.
Edit: I haven't internalized that IndexedSeq means "efficient apply and length" and knownSize is just length. Also SeqOps#isEmpty is lengthCompare(0), so probably isEmpty is guaranteed to be fine.
IndexOutOfBoundsException doesn't imply ArrayIndexOutOfBounds; this behavior is only for collections which don't override head. I didn't quite do a survey but picked ArrayDeque as an example, and it happens to do an explicit bounds check.
"Ideal" would be a comprehensive unit and benchmark test for performance and coverage (which collections or wrappers fail to do something specific for head or isEmpty?). I said I didn't benchmark last year, so I'll try that.
Also meant to add, I figured out how to invoke
sbt:root> junit/testOnly scala.collection.IndexedTestImpl.ArrayBufferTest
but the types aren't constrained where you can toss in a test of xs.head, so I gave up quickly. Also in progress is a tweak or update to collections-laws.
There was a problem hiding this comment.
Just to document that the first attempt was new NSEE(ioobe.getMessage, ioobe.getCause) but actually the new exception should have had the NSEE as the cause.
Seq.isEmpty is lengthCompare and IndexedSeq has efficient length aka knownSize. headOption already uses isEmpty.
|
I caved to sjrd's reasonable demands. The code is better but the PR title is much more ordinary. |
som-snytt
changed the title
SethTisue
added
the
library:collections
| @@ -91,11 +91,25 @@ trait IndexedSeqOps[+A, +CC[_], +C] extends Any with SeqOps[A, CC, C] { self => | |||
|
|
|||
| override def slice(from: Int, until: Int): C = fromSpecific(new IndexedSeqView.Slice(this, from, until)) | |||
|
|
|||
| override def head: A = apply(0) | |||
| override def head: A = | |||
There was a problem hiding this comment.
wouldn't this now lead to repeated checks for isEmpty in {head,last}Option? should there be an @inline def headImpl: A = apply(0) and used in both head and headOption?
There was a problem hiding this comment.
My comment on the first commit is that head is fast. headOption is allocating.
|
Release-noting since there are compatibility implications here — for those catching exceptions, and for those extending |
SethTisue
changed the title
IndexedSeq#head throws NoSuchElementException (not IndexOutOfBoundsException as before)
SethTisue
changed the title
IndexedSeq#head throws NoSuchElementException (not IndexOutOfBoundsException as before)IndexedSeq#head now throws NoSuchElementException (not IndexOutOfBoundsException)
Fixes scala/bug#12782
Attempts to use
collectionClassNamefor a helpful exception message, falls back totoString, which should be fine if the collection is in fact empty. (Extra parens at worst.)AvoidsRelishesisEmptyin the happy path.