cushon / issues-import

0 stars 0 forks source link

Null checks for values that can't possibly be null (Multimap.get, protocol buffer accessors) #85

Open cushon opened 10 years ago

cushon commented 10 years ago

Original issue created by cpovirk@google.com on 2013-02-12 at 02:49 AM


I see this from time to time:

if (multimap.get(foo) == null) { ... }

But Multimap.get always returns a collection (possibly empty), never null:

http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/collect/Multimap.html#get(K)

A quick search shows that this call appears a few dozen times in Google's codebase.

Another place that I see such null checks is with protocol buffers:

if (proto.getFoo() == null) { ... }

But proto fields aren't null; they have default values.

You could catch even more of these by looking for isNullOrEmpty calls:

if (Strings.isNullOrEmpty(proto.getFoo())) { ... }

But isNullOrEmpty is less likely to be a bug, since it will still fire (as the coder apparently expects) for a default value of "".

Are there other such methods? No doubt, but those are the two that spring to mind. I wonder whether there's an annotation that could identify them? JSR305 (covered in issue 44) has @Nonnull, but I think I've seen it mainly with parameters, and there's no documentation to say whether it's supposed to be used with return types.

cushon commented 10 years ago

Original comment posted by cpovirk@google.com on 2013-02-12 at 02:51 AM


(adding a comment so that I get an email so that I can reply internally)

cushon commented 10 years ago

Original comment posted by cpovirk@google.com on 2013-02-12 at 02:53 AM


(Of course, there are probably other Multimap.get callers who assign the result to a variable and then check for null. I don't know how much harder that is to catch.)

cushon commented 10 years ago

Original comment posted by eaftan@google.com on 2013-06-14 at 06:24 PM


Felix (flx) is working on this for protos.


Status: Started