NullPointerException in Multimap.put, putAll, and replaceValues

[GoogleCodeExporter]

com.google.common.collect.Multimap
line 107, 126, and 150

In Multimap, put, putAll, and replaceValues have @Nullable annotations.

 boolean put(@Nullable K key, @Nullable V value);
 boolean putAll(@Nullable K key, Iterable<? extends V> values);
 Collection<V> replaceValues(@Nullable K key, Iterable<? extends V> values);

These annotations indicate that a Multimap should always allow
@Nullable keys and @Nullable values.  In other words, the following
methods should never throw a NullPointerException:

 private void multimapPut(Multimap<Object, Object> mm) {
     mm.put(null, null); // should never throw a NullPointerException
 }

 private void multimapPutAll(Multimap<Object, Object> mm,
Iterable<Object> values) {
     mm.putAll(null, values); // should never throw a NullPointerException
 }

 public void multimapReplaceValues(Multimap<Object, Object> mm,
Iterable<Object> values) {
     mm.replaceValues(null, values); // should never throw a
NullPointerException
 }

However, the following unit tests fail by throwing a NullPointerException:

 public void applyTest1() {
     multimapPut(ImmutableListMultimap.of());
 }

 public void applyTest2() {
     ArrayList<Object> foo = new ArrayList<Object>();
     foo.add(null);
     multimapPutAll(ImmutableListMultimap.of(), null, foo);
 }

 public void applyTest3() {
     ArrayList<Object> foo = new ArrayList<Object>();
     foo.add(null);
     multimapReplaceValues(ImmutableListMultimap.of(), foo);
 }

The Javadoc for ImmutableListMultimap, which is implements Multimap,
indicates the following on lines 31 and 32:

 * An immutable {@link ListMultimap} with reliable user-specified key and value
 * iteration order. Does not permit null keys or values.

This is inconsistent with the specification of MultiMap, as expressed
by MultiMap's annotations.

This problem also shows up in one other file, StandardListMultimap, on line 70:

@Override public boolean put(@Nullable K key, @Nullable V value)

This is an overridden method from Multimap, and needs to be changed as well.

Proposed fix:  Remove the @Nullable annotations on put, putAll, and
replaceValues.  Then, the Multimap annotations correctly reflect the
fact that a client cannot count on null definitely being a legal key
and/or value.
             In addition, remove the @Nullable annotation on put in
StandardListMultimap on line 70.

A patch for the fix is in MultimapFix.patch

Original issue reported on code.google.com by mala...@gmail.com on 18 Sep 2009 at 11:03

Attachments:

• MultimapFix.patch

[GoogleCodeExporter]

Surprisingly, @Nullable doesn't mean that null is always a valid argument to 
the method. It only means that it 
could be a valid argument.

Here's the breakdown of JSR-305 for parameters, as far as I understand it:

@Nonnull: passing a null value is always an error
@CheckForNull: passing a null is never an error
@Nullable: passing a null value may be legal

And some examples of each from the reference platform:
  @Nonnull: new HashMap(null)
    One cannot create a HashMap prepopulated by null

  @CheckForNull: map.containsKey(null);
     Map.containsKey must always cope with a null parameter, even if the implementation itself doesn't permit 
nulls

  @Nullable: map.put(null, "foo");
     ConcurrentHashMaps and naturally-ordered TreeMaps do not permit null keys

Original comment by limpbizkit on 19 Sep 2009 at 6:31

• Changed state: Invalid

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

> Surprisingly, @Nullable doesn't mean that null is always a valid argument
> to the method. It only means that it could be a valid argument.

I agree that it is surprising.  The @Nullable annotation used in the Google
Collections has no defined semantics (for instance, there is no Javadoc).
This does not seem acceptable, especially for annotations that appear in
public Javadoc.  At the very least, an outcome of this bug report should be
to document the meaning of @Nullable.

You can't rely on JSR 305 for the semantics, because the @Nullable
annotation of JSR 305 also has no defined semantics.  For example, there is
no Javadoc (see
http://code.google.com/p/jsr-305/source/browse/trunk/ri/src/main/java/javax/anno
tation/Nullable.java
or
http://jsr-305.googlecode.com/svn/trunk/javadoc/javax/annotation/Nullable.html?i
s-external=true).
Since JSR 305 is inactive (http://jcp.org/en/jsr/detail?id=305), there
seems to be little hope for rectifying this situation.

-------------

> @Nullable: passing a null value may be legal

It's a bit odd that when applied to a formal parameter, @NonNull and
@Nullable mean exactly the same thing to a client:

 If the client passes a null value, then the method may fail because of
 dereferencing the parameter (but you have to read the Javadoc to know
 whether that is possible).

A semantics that makes @NonNull and @Nullable mean different things would
be clearer.  I believe the most useful semantics would be:

 * @NonNull indicates that if a client passes null, then the code may
   fail due to dereferencing the parameter
 * @Nullable indicates that if a client passes null, then the code won't
   fail due to dereferencing the parameter

This permits programmers to use annotations to reason about their code,
rather than requiring them to read the Javadoc.  It's also consistent with
every definition of these annotations (except that of FindBugs, which is
different than all the others).

-------------

> [Warning suppression]

The FindBugs @Nullable annotation is not documented as suppressing warnings
(see URLs above).  The FindBugs *tool* happens to use @Nullable for warning
suppression.  This treatment of @Nullable is both un-documented in the
Javadoc and confusing to programmers.

Warning suppression is often necessary.  However, the choice of the name
"@Nullable" to mean "suppress warnings" was poor choice in FindBugs and
should not be propagated to JSR305 or to the Google Collections.

Suppressing warnings is a different matter than specifying whether a value
is null, and should be treated separately.  Furthermore, whether warnings
are suppressed in the body of a method should not appear in the public Javadoc,
as the @Nullable annotations currently does.

--------------

> @CheckForNull: passing a null is never an error

Google Collections never uses @CheckForNull, nor is @CheckForNull
documented.  There are many places in the codebase where passing null is
never an error, such as the parameter to equals(Object).  Google
Collections uses @Nullable annotation here, not @CheckForNull.  Since even
the Google engineers find @Nullable to be the most intuitive name for this 
concept, I
suggest using this name.

Original comment by mala...@gmail.com on 16 Nov 2009 at 5:11