Request: EmptyIterator.remove() should throw IllegalStateException

[GoogleCodeExporter]

I propose to remove the EmptyIterator class from the UnmodifiableIterator
class hierarchy and have the remove() method throw an
IllegalStateException. Here is why:

The Javadoc for Iterator#remove() allows for both
UnsupportedOperationException and IllegalStateException. I believe it is
more appropriate to throw the latter, because the Iterator could
hypothetically support the remove() operation, if there was a legal way to
successfully call next() first without a NoSuchElementException.

From the Iterator's point of view, (at any time) the next() method has not
been called successfully, which constitues an illegal state in the sence of
the javadoc for the remove() method. In essence, the remove() method will
never be called legally, since the Iterator will always be in that
inappropriate state. You could argument, that there is no "state" that
could be "illegal", to justify the current UnsupportedOperationException.
However, there may not be an explicit state but from the callers
perspective an Iterator always has an initial implicit state, which happens
to be illegal for the remove() method to be called. In case of an
EmptyIterator this state simply never changes (i.e. it is "unmodifiable"),
which is why it is not explicitly displayed by some member but encoded into
the behavior. In fact, its extending from UnmodifiableIterator is actually
wrong, since the unmodifiability of an EmptyIterator is not based on a
"desired" behavior but on the unique unmodifiable state this iterator
happens to be in. However, the super-class __implies__ to __always__ throw
the __optional__ UnsupportedOperationException, which legalizes it to be
thrown by the EmptyIterator sub-class, which I don't agree with (I'm not
concerned with the remaining hierarchy, though).

In general, the illegal state where the next() method has not successfully
been called prior to the remove() method outweights any other exceptional
case (actually, including the fact that the remove() operation might not be
supported). By contract any iterator is supposed to check for a legal state
before the execution of the removal. I don't see, why an EmptyIterator
would be an exception to that rule. I believe, the user would rather be
informed about the illegal state (i.e. wrong calling semantics somewhere)
than the "unfortunate coincitence" of using an UnmodifiableIterator where a
modifiable one would be expected (which he would still realize, once he got
rid of the illegal state). The use of any kind of Interator is much more
common than that of unmodifiable ones -- the latter are simply a "special
case" of the former.

Here is my sample use case:

I have this hierarchical iterator for some kind of tree, which wraps two
internal iterators for the "width" and "depth" iteration at each node. The
width-iterator will iterate the branches, while the depth-iterator will
constantly be replaced with recursive iterators into each branch. To avoid
a constant null-check the depth-iterator field is initialized with an
Iterators.emptyIterator(). The remove() method delegates straight to the
depth-iterator of the current branch. Since the wrapping iterator is
supposedly modifiable, the client (ie. my unit-test) wouldn't expect an
UnsupportedOperationException, if it falsely called for the remove() method
first.

Finally, so that noone suggests to implement my own class, I'd like to
mention that I already did that. This report is just noticing an issue... ;)

cheers

Original issue reported on code.google.com by Robin...@gmail.com on 19 Aug 2009 at 7:50

[GoogleCodeExporter]

I'm sorry, the EmptyIterator class does not actually exist, of course. It is an
anonymous class in the Iterators utils. You got the idea, though.

Original comment by Robin...@gmail.com on 19 Aug 2009 at 8:23

[GoogleCodeExporter]

I think the exception is correct. UnsupportedOperationException means there is 
no 
possible state or argument for which this operation could work, on any instance 
of 
this concrete type.  IllegalStateException means that there exists some state 
that an 
instance of this concrete type could be in for which the operation would 
succeed. Your 
test should simply tolerate either type.

Original comment by kevin...@gmail.com on 19 Aug 2009 at 11:40

• Changed state: WontFix

[GoogleCodeExporter]

I agree that UOE is inconvenient for your sample use case and similar cases.  
Another
example: a mutable collection that stores its contents in a lazily allocated 
array. 
Once the array is allocated, a call to iterator() returns an iterator over the 
array.
 But before then, it makes sense to use emptyIterator().  However, this means that
the result of iterator().remove() on an empty collection has two different 
results,
depending upon whether the collection ever contained elements:

- fresh empty collection: UOE
- cleared collection: ISE

It's certainly true that the iterator doesn't have its own "state," and it 
would have
been defensible (if impractical) for Java to demand that all empty iterators 
throw
UOE instead of ISE, but that's not how the Java collections behave.  Perhaps it 
makes
sense to argue that the "state" is the state of the underlying collection at 
the time
of the iterator() call, but I'm not sure I buy that, myself.  More convincing 
to me
is the consistency argument.

Original comment by cpov...@google.com on 20 Aug 2009 at 12:48

[GoogleCodeExporter]

It is certainly correct for any subclass of UnmodifiableIterator to conform to 
its
superclass interface and thus always throw an UnsupportedOperationException. My
argument is that the EmptyIterator shouldn't be a descendent of 
UnmodifiableIterator
in the first place.

> ... UnsupportedOperationException means there is no 
> possible state or argument for which this operation could work ...

Kind of correct -- at least for some sane and well defined state, which 
complies with
the expected preconditions of the respective operation, so that invariants and
postconditions of that same operation can hold (even if that operation was 
"panic" or
"chaos"). An IllegalStateException would be the one and only correct indicator 
for
non-compliance.

> ... IllegalStateException means that there exists some state that an 
> instance of this concrete type could be in for which the operation would 
succeed ...

This is certainly incorrect. An IllegalStateException does not imply the 
existence of
a legal state or its producibility -- just like an 
UnsupportedOperationException does
not imply there is a supported alternative.

The one state where this operation could succeed can never be, since no 
instance has
that state nor can it ever change to it -- a fact, bound to the concrete
implementation rather than the interface, and a random decision.

Another argument:

If I had some kind of Iterator over an arbitrarily sized collection, I wouldn't 
want
that Iterator to throw an UnsupportedOperationException just because I missed 
to call
next() before remove(), regardless of whether the collection is empty. In fact, 
I
wouldn't want that iterator to consider the size of the collection at all. This
stipulation for the explicit EmptyIterator to always throw the
UnsupportedOperationException is introduced by the inheritence from
UnmodifiableIterator and for no sufficient reasen. It is only legal for this
particular hierarchy and only in concrete cases where the client is aware of the
implementation. In any other case, I consider this a bug... fortunately an
itsy-bitsy-tiny one, so never mind.

Original comment by Robin...@gmail.com on 20 Aug 2009 at 2:56

[GoogleCodeExporter]

The correct emptyIterator.remove() behavior, as I see it, varies based on the
iterator the same collection would return if nonempty.  If the nonempty iterator
would be immutable, then remove() should throw UOE.  If the nonempty iterator 
would
be mutable, then remove() should throw ISE.  Consistency über alles.

This doesn't address the question of whether many Google Collections users are
implementing their own mutable collection classes with special-case iterators 
for
empty.  If not, two separate public empty-iterator methods may not be worth the 
API
bloat.

Original comment by cpov...@google.com on 20 Aug 2009 at 3:08

[GoogleCodeExporter]

> ... An IllegalStateException would be the one and only correct indicator for
non-compliance.

We're talking about Iterator#remove() here, which may only be called once per
successful call to Iterator#next() -- anything else resembles non-compliance.

Original comment by Robin...@gmail.com on 20 Aug 2009 at 3:10

[GoogleCodeExporter]

IllegalStateException would even be the correct throw for UnmodifiableIterators,
since remove() can never be called if next() cannot be called.

Original comment by Robin...@gmail.com on 20 Aug 2009 at 3:14

[GoogleCodeExporter]

For an empty iterator spawned from an immutable collection, I'm not sure I'd 
argue
against either exception.  UOE feels like the "stronger" exception to me, the 
one
that suggests "even with a different instance of this class, remove() wouldn't 
work."
 Thus, I prefer it.  But I admit that that's as much opinion as fact.  Hence, I've
focused on the question of "Is it a good use of API space to have two methods?"

In short, I can see your argument that ISE is always acceptable, and I can see
Kevin's that UOE is always acceptable.  In principle, I'd rather be able to 
choose,
but I don't know that the two methods would pull their weight.

Original comment by cpov...@google.com on 20 Aug 2009 at 3:33