RFR [15] 8161558: ListIterator should not discard cause on exception

[forax at univ-mlv.fr]

----- Mail original -----
> De: "Stuart Marks" <stuart.marks at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "Martin Buchholz" <martinrb at google.com>, "Kiran Ravikumar" <kiran.sidhartha.ravikumar at oracle.com>, "core-libs-dev"
> <core-libs-dev at openjdk.java.net>
> Envoyé: Mardi 11 Février 2020 00:57:52
> Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception

> Hi Remi,
> 
> The bug report originally requested that a bunch of different exceptions include
> a cause. I don't think the cause should be added to all of them. The cases that
> Kiran is adding are ones where I thought that adding a cause does have value.
> 
> If someone is using a ListIterator (or a plain Iterator for that matter) they
> might have an incorrect model for what the index value is after a certain
> operation (remove, for example), and they might get an NSEE unexpectedly. They
> might reasonably wonder what the state of the iterator is that resulted in that
> exception. Without a cause, NSEE doesn't have that information. Chaining the
> IOOBE will usually include the index that caused the problem, which I think is
> useful in such circumstances.
> 

I don't disagree with the fact that having the index may help,
I disagree with the fact that chaining the IOOBE to the NSEE is the right way to expose that index, crafting an error message with the index is IMO a better idea because you have no idea if the exception thrown by all implementation of List.get() will provide this index (and only this index).

> The iterators in AbstractList all keep track of indexes and call get() for
> access to the appropriate element. I don't think they should do a bounds check
> and throw an exception on that basis, because the bounds could change between
> the time they're checked and the call to get(). Thus, the iterators would have
> to catch the exception from get() even if the bounds are checked in advance,
> making the bounds check redundant.

It depends how List.get() is implemented, if there is a bound check before accessing the value in the implementation, you have the same issue, one turtle down :)

> 
> s'marks

Rémi

> 
> On 2/5/20 2:05 PM, Remi Forax wrote:
>> Stuart, Martin, Kiran,
>> I think this "bug" should not be fixed because it's one of the cases where
>> providing more information is actually bad from a user POV.
>> 
>> The current code throws NoSuchElementException when the iterator reach the end
>> so from the user POV, this is the right exception because of the right issue,
>> so from the user POV there is no need to change the actual code.
>> If we chain the exception,
>> - it's less clear from a user POV
>> - the user may think that there is an error in the AbstractList implementation
>> but it's not the case, it's just that AbstractList iterators next method is
>> implemented weirdly, it prefers to go out of bound instead of checking the
>> bound.
>> 
>> I'm okay with NoSuchElementException having a new constructor that takes a
>> chained exception but i really think that in this specific case, it's a bad
>> idea(TM) to use it to propagate an exception to the user that it should not
>> care about.
>> 
>> BTW, perhaps, those method next() should be re-written to test the bound instead
>> of catching the IOOBE because i'm not sure this "optimization" make sense
>> nowadays.
>> 
>> regards,
>> Rémi
>> 
>> ----- Mail original -----
>>> De: "Kiran Ravikumar" <kiran.sidhartha.ravikumar at oracle.com>
>>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>>> Envoyé: Mercredi 5 Février 2020 20:49:09
>>> Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception
>> 
>>> Thanks Stuart and Martin,
>>>
>>>
>>> Here is an updated webrev with the changes.
>>>
>>> http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8161558
>>>
>>>
>>> Thanks,
>>>
>>> Kiran
>>>
>>>
>>> On 15/01/2020 12:46, Martin Buchholz wrote:
>>>> Hi Kiran,
>>>>
>>>> Looks good to me, but I always nitpick ...
>>>>
>>>> Stray semicolon?
>>>>         var iterator = list.listIterator(list.size());; // position at end
>>>>
>>>> I would have documented whitebox test assumptions: that nCopies
>>>> iterators are implemented via AbstractList, and that
>>>> AbstractList's list iterator inherits behavior from iterator.
>>>>
>>>> I probably would have added a plain iterator test, and might have
>>>> refactored the code that tests the exception.
>>>>
>>>>
>>>> On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar
>>>> <kiran.sidhartha.ravikumar at oracle.com
>>>> <mailto:kiran.sidhartha.ravikumar at oracle.com>> wrote:
>>>>
>>>>      Hi Guys,
>>>>
>>>>
>>>>      Could someone please review my fix to add missing standard
>>>>      constructor
>>>>      overloads to NoSuchElementException class and update the AbstractList
>>>>      class to use them.
>>>>
>>>>
>>>>      A CSR was filed and approved. Along with the code change a new
>>>>      test is
>>>>      added to verify the behavior.
>>>>
>>>>
>>>>      Please find the webrev at  -
>>>>
>>>>
>>>>      http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/
>>>>
>>>>
>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8161558
>>>>
>>>>
>>>>      Thanks,
>>>>
> >>>      Kiran