Breaking Changeset: 4802647: Throw required NPEs from removeAll()/retainAll()

[Ali Ebrahimi]

What we this:

previous result: before changeset
collection.removeAll(null) do nothing if collection is empty
collection.removeAll(null) NullPointerException if collection is not empty

current result: after changeset
collection.removeAll(null) NullPointerException if collection is empty
collection.removeAll(null) NullPointerException if collection is not empty

solutions:
solution1: backout/revert changeset

solution2: patch changeset

    public boolean removeAll(Collection<?> c) {
        if(!isEmpty()) Objects.requireNonNull(c);
        boolean modified = false;
        Iterator<?> it = iterator();
        while (it.hasNext()) {
            if (c.contains(it.next())) {
                it.remove();
                modified = true;
            }
        }
        return modified;
    }

    public boolean retainAll(Collection<?> c) {
        if(!isEmpty()) Objects.requireNonNull(c);
        boolean modified = false;
        Iterator<E> it = iterator();
        while (it.hasNext()) {
            if (!c.contains(it.next())) {
                it.remove();
                modified = true;
            }
        }
        return modified;
    }
this is behaviorally compatible change.



On Tue, Jun 4, 2013 at 2:08 PM, Alan Bateman <Alan.Bateman at oracle.com>wrote:

> On 04/06/2013 10:14, Ali Ebrahimi wrote:
>
>> :
>>
>>
>> the cause of this error is this new changeset:
>>
>> 4802647: Throw required NPEs from removeAll()/retainAll()
>>
>> current code assume that collection.removeAll(null) doesn't do anything.
>> but with this changeset produces NullPointerException that doesn't
>> handled.
>>
>> following is part of source code org/antlr/tool/**CompositeGrammar.java
>> (see********
>> )
>>
>>    217          /** Get delegates below direct delegates of g */
>>    218          public List<Grammar>  getIndirectDelegates(Grammar g) {
>>    219                  List<Grammar>  direct = getDirectDelegates(g);
>>    220                  List<Grammar>  delegates = getDelegates(g);  221
>>                 delegates.removeAll(direct);**********
>>    222                  return delegates;
>>    223          }
>>    224
>>    225          /** Return list of delegate grammars from root down to g.
>>    226           *  Order is root, ..., g.parent.  (g not included).
>>    227           */
>>    228          public List<Grammar>  getDelegators(Grammar g) {
>>    229                  if ( g==delegateGrammarTreeRoot.**grammar ) {
>>    230                          return null;**********  231             }
>>    232                  List<Grammar>  grammars = new ArrayList();
>>    233                  CompositeGrammarTree t = delegateGrammarTreeRoot.
>> **findNode(g);
>>    234                  // walk backwards to root, collecting grammars
>>    235                  CompositeGrammarTree p = t.parent;
>>    236                  while ( p!=null ) {
>>    237                          grammars.add(0, p.grammar); // add to
>> head so in order later
>>    238                          p = p.parent;
>>    239                  }
>>    240                  return grammars;  241           }
>>
>>
>> So this changeset at least breaks 'antlr' third-party library and any apps
>> depends on.
>>
> Thanks for the bug report. It does seem to have exposed a bug in antlr.
> Kevin Rushforth (FX) and Mike Duigou have been looking into the same thing
> via 8015656.
>
> -Alan.
>
>