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é: Vendredi 14 Février 2020 18:25:14
> Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception

> On 2/12/20 2:04 PM, forax at univ-mlv.fr wrote:
>> 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).
> 
> It /seems/ like AbstractList has enough information to do the checks itself and
> to formulate a reasonable error message, but it really doesn't. It does need to
> delegate to the subclass's get() implementation to do the actual work. The get()
> method has only two choices: it can return a valid element, or it can throw
> IOOBE. AbstractList can guess at why get() failed, but the knowledge of that
> belongs to the subclass, and it's the subclass's responsibility to put that
> information into the IOOBE.

Yes, the implementation of next() has to call get() with the index, and yes, it's up to the subclass to put the right information in the IOOBE,
but the IOOBE is an implementation details, it should not bubble up to the user of next().

So i was thinking to a code like this:

public E next() {
    checkForComodification();
    try {
        int i = cursor;
        E next = get(i);
        lastRet = i;
        cursor = i + 1;
        return next;
    } catch (IndexOutOfBoundsException e) {
        checkForComodification();
        throw new NoSuchElementException("index out of bounds " + i);
    }
} 

> 
> Of course it's possible that some subclass hasn't provided useful information in
> the exception, but that shouldn't be a reason to penalize subclasses that have
> done so.

The thing is that inside the iterator, you already have the right information, so you don't have to pray to have the right info.

> 
>> 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 :)
> 
> It's a subclass implementation choice as to whether it wants to do a bounds
> check and throw explicitly or to let the VM generate the exception implicitly.
> Either way results in an IOOBE (or some subtype). Which of these is done is
> irrelevant to AbstractList; all it knows is that get() reported an error by
> throwing an exception of this type, so it should be chained into the resulting
> NSEE.

I just disagree on the conclusion, chaining exception is great when you bubble up an information that make sense to the end-user,
knowing that next() is implemented using get() that why you get an IOOBE as cause of the NSEE is just noise for a end-user.

> 
> s'marks

Rémi

> 
>> 
>>>
>>> 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