RFR-8148748: ArrayList.subList().spliterator() is not late-binding

[Tagir F. Valeev]

Hello!

AIOOBE is possible for ArrayList itself as well. E.g.:

    ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
    Spliterator<Integer> spltr = test.spliterator();
    spltr.tryAdvance(System.out::println);
    test.clear();
    test.trimToSize();
    spltr.tryAdvance(System.out::println);

Result (both in Java-8 and Java-9):

1
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 1
    at java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1398)

So this is not subList-specific problem.

Seems that forEachRemaining is not affected by this, due to additional
length check and making the local copy of elementData array. At least
I don't see the way to make forEachRemaining (either of subList
spliterator or main ArrayList spliterator) throwing AIOOBE. Probably
I'm missing something. However it can traverse some unexpected nulls
before throwing CME if it was shrinked:

    ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
    Spliterator<Integer> spltr = test.spliterator();
    spltr.tryAdvance(System.out::println);
    test.clear();
    spltr.forEachRemaining(System.out::println);

Output:

1
null
null
null
Exception in thread "main" java.util.ConcurrentModificationException
    at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1428)

This may unexpectedly throw NPE if forEachRemaining Consumer
unconditionally dereference them as program logic suggests that null
is impossible here. So user will see NPE instead of CME.

Probably it could be logged as separate issue. My patch does not cause
any regression here (at least, I hope so). Probably some other
collections should be checked for similar corner cases as well.

With best regards,
Tagir Valeev.

>> On 4 Mar 2016, at 15:38, Tagir F. Valeev <amaembo at gmail.com> wrote:
>> 
>> Hello!
>> 
>> Thank you for the review!
>> 

PS> Thanks.

PS> I just realised there are some subtleties where if the top-level
PS> list is reduced in size the spliterator of a sublist may on
PS> traversal throw an ArrayIndexOutOfBoundsException rather than ConcurrentModificationException.

PS> This can already occur for a partially traversed top-level list
PS> spliterator, so i am wondering how much we should care. Arguably
PS> in the sublist case there is another level of indirection, where
PS> errors creep in before traversal, so it suggests we should
PS> additionally check the mod count at the start of the traversal
PS> methods, and it’s probably ok on a best-effort basis to do this
PS> for the sublist spliterator and not it’s splits.

PS> Separately, i am not sure why the SubList.iterator has to check
PS> that the offset + 1 is within bounds, since expectedModCount =
PS> ArrayList.this.modCount should be false.

PS> Paul.

>> Here's updated webrev:
>> http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r3/
>> 
>> PS> Looks good. I especially like:
>> 
>> PS>  125             addCollection(l.andThen(list -> list.subList(0, list.size())));
>> 
>> PS> Can you also update SpliteratorTraversingAndSplittingTest?
>> 
>> PS> void addList(Function<Collection<T>, ? extends List<T>> l) {
>> PS>     // @@@ If collection is instance of List then add sub-list tests
>> PS>     addCollection(l);
>> PS> }
>> 
>> Done.
>> 
>>>> Now it's separate anonymous class as you suggested.
>>>> ArrayListSpliterator is untouched. Note that trySplit() can return
>>>> original ArrayListSpliterator as after the binding their behavior is
>>>> compatible.
>>>> 
>> 
>> PS> Very nice, might be worth an extra comment noting that. Up to you.
>> 
>> Short comments added.
>> 
>> With best regards,
>> Tagir Valeev.
>>