NPE in CompositeGrammer.getIndirectDelegates in Java 8

[druidsquirrel]

In the class org.antlr.tool.CompositeGrammar, in the method getIndirectDelegates, there is a call to java.util.ArrayList.removeAll with an argument that might be null.

In Java 7, this can sometimes be okay: ArrayList.removeAll(null) is a no-op if the list you are removing from is empty. But starting with Java 8, it's never okay: ArrayList.removeAll(null) will always throw NullPointerException. When I upgraded my build to use Java 8, it started failing with this NPE.

I suggest that a check should be added to getIndirectDelegates so that the call to removeAll (line 226, in the source I'm seeing) only happens if direct is not null. And, for that matter, if delegates is also not null.

I get this in antlr versions 3.2 and 3.5.1.

Here's a partial stack trace:

java.lang.NullPointerException
at java.util.Objects.requireNonNull(Objects.java:203)
at java.util.ArrayList.removeAll(ArrayList.java:674)
at org.antlr.tool.CompositeGrammar.getIndirectDelegates(CompositeGrammar.java:226)
at org.antlr.tool.Grammar.getIndirectDelegates(Grammar.java:2722)

[druidsquirrel]

Also see the java issue where this problem was discussed. They considered backing out the change (so that it would continue to be safe for antlr3 to pass null to removeAll), but decided against it.

http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8015656

[sharwell]

What they should have done is report the issue here, so it would get fixed. Wow.

[druidsquirrel]

Yeah I was stunned when I saw that no one had bothered to report it here. I thought for sure I had just missed it, and had to look back through all the reported issues here to make sure I was not filing a duplicate.

[druidsquirrel]

The fix here is trivial, but I figure I might as well provide a patch anyway. Patch to be applied to CompositeGrammar.java:

226c226,227
<       delegates.removeAll(direct);
---
>       if (direct != null)
>           delegates.removeAll(direct);

My guess is that when Java 8 comes out (on March 18), other people will start to run into this.

[parrt]

makes sense. fixed.