On 06/26/2018 08:11 AM, Zheka Kozlov wrote:

I agree with Ivan. Isn't it better to create a standalone implementation of Iterator and return its instance in iterator()? Implementing of Iterator for ImmutableList doesn't look like a big problem.

Also, performance would be better if it was a separate class. It could be a superclass of ListItr so that common logic is not duplicated. @Stable annotation on private final boolean isListIterator does not help here unless the returned Iterator object is assigned to a static final field and used from it. Hardly a common use case for Iterator(s) I guess. Another thing about @Stable is that it never has effect on fields that hold a default value. Ok, when isListIterator == false, we don't need speed as we're throwing UOE anyway. The same arguments hold for other two @Stable fields in this class. I don't think those three @Stable annotations have any effect on real Iterator use cases. Regards, Peter

2018-06-26 6:14 GMT+07:00 Ivan Gerasimov <ivan.gerasimov@oracle.com>:

...

Hi Stuart!

Out of curiosity. What if someone does something like

if (it instanceof ListIterator) { // optimized for bidirectional access } else { // slower algorithm }

previous(), nextIndex() and previousIndex() methods are not declared to be optional, so is it appropriate to throw UnsupportedOperationException from them?

Someone may assume that if an object can be cast to ListIterator interface, non-optional methods should not throw UOE.

With kind regards,

Ivan

On 6/25/18 3:06 PM, Stuart Marks wrote:

...

Hi all,

Please review this small changeset that ensures that calling iterator() on an unmodifiable List (from List.of, et. al.) returns an instance that cannot be downcast to ListIterator and used as such.

Bug:

https://bugs.openjdk.java.net/browse/JDK-8203670

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8203670/webrev.0/

Thanks,

s'marks

-- With kind regards, Ivan Gerasimov

On 6/26/18 9:57 AM, Ivan Gerasimov wrote:

On the first place, I'd like to understand why it is bad, if List.of().iterator() in fact returns ListIterator? What kind of problems it may cause?

Basically it leaks information. Somebody might get an iterator, partially advance it, and hand it off to another piece of code, assuming that it can only be advanced. But if that instance can be downcast and used as a ListIterator, that assumption would be violated. I admit it's a narrow case, but it's nonetheless a possibility.

Out of curiosity. What if someone does something like

if (it instanceof ListIterator) { // optimized for bidirectional access } else { // slower algorithm }

previous(), nextIndex() and previousIndex() methods are not declared to be optional, so is it appropriate to throw UnsupportedOperationException from them?

Someone may assume that if an object can be cast to ListIterator interface, non-optional methods should not throw UOE.

I think this usage is out of bounds for the List API. The List specification is: Iterator<E> iterator(); ListIterator<E> listIterator(); ListIterator<E> listIterator(int); That is, you get an Iterator from the iterator() method, and you get a ListIterator from the listIterator() methods. This implementation fulfils all the contracts for the interfaces declared as the return types. In particular, if you ask for a ListIterator, you get a ListIterator implementation that implements all the methods. However, if you call iterator() and get an Iterator, and do an 'instanceof' and downcast, then all bets are off. It's reasonable to rely on the thing returned by iterator() having any behaviors other than those specified by Iterator. Peter Levart wrote:

Also, performance would be better if it was a separate class. It could be a superclass of ListItr so that common logic is not duplicated.

This isn't at all obvious. Certainly it's not obvious enough to embark on a refactoring without doing some benchmarking. However, I want to fix this bug in JDK 11. If somebody wants to do some benchmarking, they're welcome to do so, but I don't want to consider this as part of this changeset.

@Stable annotation on private final boolean isListIterator does not help here unless the returned Iterator object is assigned to a static final field and used from it. Hardly a common use case for Iterator(s) I guess.

The exact effect of @Stable is Hotspot-specific and changes over time. Here we're using it as a declaration that this final field really can be treated as final, and that it won't be modified via reflection or a varhandle, thus enabling VM more aggressive optimizations. s'marks

Okay. Thanks for explaining! With kind regards, Ivan On 6/26/18 10:51 AM, Stuart Marks wrote:

On 6/26/18 9:57 AM, Ivan Gerasimov wrote:

...

On the first place, I'd like to understand why it is bad, if List.of().iterator() in fact returns ListIterator? What kind of problems it may cause?

Basically it leaks information. Somebody might get an iterator, partially advance it, and hand it off to another piece of code, assuming that it can only be advanced. But if that instance can be downcast and used as a ListIterator, that assumption would be violated. I admit it's a narrow case, but it's nonetheless a possibility.

...

Out of curiosity. What if someone does something like

if (it instanceof ListIterator) { // optimized for bidirectional access } else { // slower algorithm }

previous(), nextIndex() and previousIndex() methods are not declared to be optional, so is it appropriate to throw UnsupportedOperationException from them?

Someone may assume that if an object can be cast to ListIterator interface, non-optional methods should not throw UOE.

I think this usage is out of bounds for the List API. The List specification is:

Iterator<E> iterator(); ListIterator<E> listIterator(); ListIterator<E> listIterator(int);

That is, you get an Iterator from the iterator() method, and you get a ListIterator from the listIterator() methods. This implementation fulfils all the contracts for the interfaces declared as the return types. In particular, if you ask for a ListIterator, you get a ListIterator implementation that implements all the methods.

However, if you call iterator() and get an Iterator, and do an 'instanceof' and downcast, then all bets are off. It's reasonable to rely on the thing returned by iterator() having any behaviors other than those specified by Iterator.

Peter Levart wrote:

...

Also, performance would be better if it was a separate class. It could be a superclass of ListItr so that common logic is not duplicated.

This isn't at all obvious. Certainly it's not obvious enough to embark on a refactoring without doing some benchmarking.

However, I want to fix this bug in JDK 11. If somebody wants to do some benchmarking, they're welcome to do so, but I don't want to consider this as part of this changeset.

...

@Stable annotation on private final boolean isListIterator does not help here unless the returned Iterator object is assigned to a static final field and used from it. Hardly a common use case for Iterator(s) I guess.

The exact effect of @Stable is Hotspot-specific and changes over time. Here we're using it as a declaration that this final field really can be treated as final, and that it won't be modified via reflection or a varhandle, thus enabling VM more aggressive optimizations.

s'marks

-- With kind regards, Ivan Gerasimov