search

MedwynLiang / google-collections

Automatically exported from code.google.com/p/google-collections
Apache License 2.0
0 stars 0 forks source link

Missing @Nullable annotations #249

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The Javadoc for Object.equals states:
* For any non-null reference value x, x.equals(null) should return false.

In most places, the Google Collections properly annotates equals as follows:

 public boolean equals(@Nullable other)

However, in a few places the @Nullable annotation is missing.

Based on the Google Collections style, a programmer seeing a version
that omits the @Nullable annotation might believe the implementation
forbids null as an argument.  The missing annotations also hinder code
analysis tools.  And, the inconsistency is bad in and of itself.

The Javadoc for Object.equals states:
* For any non-null reference value x, x.equals(null) should return false.

In most places, the Google Collections properly annotates equals as follows:

  public boolean equals(@Nullable other)

However, in a few places the @Nullable annotation is missing.

The inconsistency is bad by itself.  Furthermore, programmer seeing a
version that omits the @Nullable annotation might be confused and
conclude that that implementation forbids null as an argument.  The
missing annotations also hinder code analysis tools.

There are similar problems with the following methods:

Collection.contains
Collection.remove
Map.containsKey
Map.containsValue
List.indexOf
List.lastIndexOf
Multimap.containsEntry

In addition, the following methods are inconsistent with their implementation:

com.google.common.collect.ConcurrentHashMultiset
 private static int unbox(Integer i) {
   return (i == null) ? 0 : i;
 }

com.google.common.collect.StandardBiMap
 private V removeFromBothMaps(Object key) {
   V oldValue = delegate.remove(key); //Remove allows @Nullable
entries, this method is to remove from both maps.
   removeFromInverseMap(oldValue);
   return oldValue;
 }

 private void removeFromInverseMap(V oldValue) {
   inverse.delegate.remove(oldValue);  //RemoveFromBothMaps calls
this method, possible with a null reference.
 }

com.google.common.collect.StandardMultimaps
 public boolean containsEntry(Object key, Object value) {  //Some
entries can contain null Objects
   Collection<V> collection = map.get(key);
   return collection != null && collection.contains(value);
 }

Proposed fix:  add the missing @Nullable annotations

All changes are in NullableAnnotationsFix.patch

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

Attachments:

GoogleCodeExporter commented 9 years ago 
Our policy is to include @Nullable only when it would appear in the standard 
Javadoc:
when the method is public or protected and belongs to a public class.

Have you found missing @Nullable annotations of that sort?

Original comment by jared.l....@gmail.com on 18 Sep 2009 at 11:26

GoogleCodeExporter commented 9 years ago 
In fact, the presence and absence of @Nullable in our code has been validated 
by 
findbugs.  Still let us know if you find another problem.

Original comment by kevin...@gmail.com on 21 Sep 2009 at 5:50

GoogleCodeExporter commented 9 years ago 
> Our policy is to include @Nullable only when it would appear in the
> standard Javadoc:  when the method is public or protected and belongs to
> a public class.

Is there any place where this decision is documented?  Absent any
documentation, the inconsistency from class to class is confusing.

Also, can you explain the reasoning behind this?  As far as I can tell, it
would not hurt to add annotations to private classes and methods.  While
they might not appear in the Javadoc, they serve as useful documentation.

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

GoogleCodeExporter commented 9 years ago 
The "reasoning" is trivial:  we added what was necessary to make findbugs 
happy. In 
the future it looks like we may be moving to the JSR-308 checker framework, 
which is 
more powerful but seems to perhaps have subtle differences. And when we do 
that, we'll 
add or change what's necessary to make it happy. As long as the tools support 
the 
correct application of these things, it's something we can keep working 
correctly, but 
beyond that, it's not worth our extensive time and thought to think everything 
over 
across the whole library.

Original comment by kevin...@gmail.com on 16 Nov 2009 at 5:19

GoogleCodeExporter commented 9 years ago 
Private methods and classes make up a large portion of the code base which is 
just as
likely to contain bugs.  It seems undesirable to leave such a part of your code 
base unannotated, and therefore unchecked.

My patch contains  annotations for the private methods of the google 
collections, 
so applying the patch would not be too burdensome.

If you are considering migrating over to a different system, I could annotate 
the 
collections with JSR-308 annotations for comparison.  Let me know if you want 
me 
to do this.

I don't think you have addressed my point about documentation of the 
annotations. 
While a future move to a more documented system may address the problem, the 
current annotations are undocumented and confusing.  Some potential solutions 
are 
to add documentation, or to remove them from the code base for the final 
release.  
A half-solution would be to mark the annotations as undocumented, so they don't 
show up in the Javadoc.

Original comment by mala...@gmail.com on 29 Nov 2009 at 5:51