ArrayList.removeAll()/retainAll()

[Mike Duigou]

The condition that is causing the problem is not a collection containing null, which is allowed, but that the provided collection "c" is itself null. 

The problem was is a consequence of the following implementation:

Iterator<E> iterator = iterator();

while(iterator.hasNext()) {
 if(c.contains(iterator.next()) {
    iterator.remove();
 }
}

This implementation had an inconsistent behaviour. If a collection contains elements then the first iteration of the while loops will cause a NullPointerException if "c" is null. If, however, the collection is empty then c is never referenced and no NPE is thrown. The change in java 8 which adds an explicit Objects.requireNonNull(c) ensures the behaviour is consistent whether the collection is empty or not. Passing null as the parameter for removeAll and retainAll has never been valid. What's changed is that it's now consistently checked.

It's unfortunate that this particular fix tickled a bug in Antlr 3. It appears that it's simply a bug that Antlr doesn't check the result of it's method which returns null. (https://github.com/antlr/antlr3/blob/master/tool/src/main/java/org/antlr/tool/CompositeGrammar.java#L226) The fix would seem to be relatively straightforward to either have getDirectDelegates() return Collections.<Grammar>emptyList() or add a check for null. My personal preference would be for the former as it allows for uniform handling of the result wherever the method is called without having to remember that the result might be null.

Mike

On Feb 4 2014, at 09:00 , Benedict Elliott Smith <belliottsmith at datastax.com> wrote:

> Hi,
> 
> I notice this (or a related issue) has been mentioned
> before<http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017663.html>on
> this list, but I'm not convinced the correct resolution was reached.
> We
> are seeing this problem thrown by antlr, but rather than a bug in antlr, as
> surmised on the previous exchange, it looks to me that ArrayList is
> imposing a new constraint that is neither declared by itself nor
> Collection, and is unnecessary. ArrayList happily supports null elements,
> so requiring that the provided collection has no null elements is surely a
> bug?
> 
> I've pasted the two declarations below for ease of reference. Neither
> javadocs describe the constraint that is imposed.
> 
> ArrayList:
>    /**
>     * Removes from this list all of its elements that are contained in the
>     * specified collection.
>     *
>     * @param c collection containing elements to be removed from this list
>     * @return {@code true} if this list changed as a result of the call
>     * @throws ClassCastException if the class of an element of this list
>     *         is incompatible with the specified collection
>     * (<a href="Collection.html#optional-restrictions">optional</a>)
>     * @throws NullPointerException if this list contains a null element
> and the
>     *         specified collection does not permit null elements
>     * (<a href="Collection.html#optional-restrictions">optional</a>),
>     *         or if the specified collection is null
>     * @see Collection#contains(Object)
>     */
>    public boolean removeAll(Collection<?> c) {
>        Objects.requireNonNull(c);
>        return batchRemove(c, false);
>    }
> 
> Collection:
>    /**
>     * Removes all of this collection's elements that are also contained in
> the
>     * specified collection (optional operation).  After this call returns,
>     * this collection will contain no elements in common with the specified
>     * collection.
>     *
>     * @param c collection containing elements to be removed from this
> collection
>     * @return <tt>true</tt> if this collection changed as a result of the
>     *         call
>     * @throws UnsupportedOperationException if the <tt>removeAll</tt>
> method
>     *         is not supported by this collection
>     * @throws ClassCastException if the types of one or more elements
>     *         in this collection are incompatible with the specified
>     *         collection
>     *         (<a href="#optional-restrictions">optional</a>)
>     * @throws NullPointerException if this collection contains one or more
>     *         null elements and the specified collection does not support
>     *         null elements
>     *         (<a href="#optional-restrictions">optional</a>),
>     *         or if the specified collection is null
>     * @see #remove(Object)
>     * @see #contains(Object)
>     */
>    boolean removeAll(Collection<?> c);