Normalize @NonNull / checkNotNull usage

[JakeWharton]

Figure out if we want actual non-null verification or just nullability annotations.

[dlew]

My vote is on checkNotNull because the annotations are non-binding.

[JakeWharton]

To play devil's advocate to that, annotations show in the IDE for consumers
of the library whereas checkNotNull does not.

On Wed, Apr 29, 2015, 1:21 PM Daniel Lew notifications@github.com wrote:

My vote is on checkNotNull because the annotations are non-binding.

—
Reply to this email directly or view it on GitHub
#20 (comment)
.

[dlew]

[DrewCarlson]

Both seems like a good solution.

[vanvlack]

Using both allows the consumer to easily seeing can something be null/non-null and enforces a semi-fail fast contract.

Note that modern IDEs can give warnings, but there are people in the world that use simple text editors.

public @Nonnull String hello(@Nonnull String name) {
  return "hello " + Objects.requireNonNulll(name, "name == null");
}

Above assumes JDK7+ (or in the case of android, api 19+) and the use of the java.util.Objects handler for non-null, which throws an NPE on null.

[LalitMaganti]

Another positive for NonNull annotation is the support of null typing by the Kotlin compiler in the upcoming milestone.

[JakeWharton]

@vanvlack we have Preconditions.checkNotNull which is fairly standard in libraries.

My biggest problem with both is that it causes a ton of warnings in the library projects for "needlessly" checking a @NonNull value against null. It can be suppressed, and perhaps that's the cost we pay, but it's just annoying.

[vanvlack]

@JakeWharton Yea, I'm a fan of using what the standard JDK gives us. If it's there, why use/create our own just to do it.

The nice thing about @Nonnull annotations is it gives the caller/user a contract to follow. If I see a method signature that has these annotations (whether @Nonnull or @Nullable), I know with certainty how to handle the call or results of a call.

Of course, this all assumes the implementer is following the contract that it set upon itself, thus the fail-fast requireNonNull on passed variables and making sure return values are non-null, either by using some Optional mechanism or guaranteeing the existence of values upon object creation.

My biggest problem with both is that it causes a ton of warnings in the library projects for "needlessly" checking a @nonnull value against null. It can be suppressed, and perhaps that's the cost we pay, but it's just annoying.

I wouldn't say they are needless checking. To me, moving the check to one spot (object creation or value setting) is easier than checking for null through-out my code all over.

[JakeWharton]

The nice thing about @Nonnull annotations is it gives the caller/user a contract to follow. If I see a method signature that has these annotations (whether @Nonnull or @Nullable), I know with certainty how to handle the call or results of a call.

But the number of people reading the documentation that closely can be rounded down to zero. As far as I'm concerned the only value of these annotations is making IntelliJ show warnings and static analysis tools like findbugs and error-prone fail your build when you pass an unexpected value.

Of course, this only works when the nullability of the argument can be statically determined. Sometimes it's ambiguous leading you to unknowingly pass null to the method which leads me to my next point...

I wouldn't say they are needless checking. To me, moving the check to one spot (object creation or value setting) is easier than checking for null through-out my code all over.

It's absolutely needless checking because of its redundancy, not its location. In fact, you mention this in your point above:

this all assumes the implementer is following the contract that it set upon itself, thus the fail-fast requireNonNull on passed variables

IntelliJ flags the null check as redundant because of the annotation:

This makes it annoying on the persons developing the library since they need to now suppress that warning to ensure the ambiguously null argument does not make it through for an ambiguous NullPointerException down the road rather than the eager one up front.

I'm not opposed to this as I said above. It's just an annoying burden pushed onto library implementations that want absolute guarantees.

[grodin]

Do you still get the warning when using Preconditions.checkNotNull()? I use both annotations and a null check with that method and hadn't noticed any warnings. It's possible I turned off that inspection at some point though.

I've just checked and IntelliJ (or Android Studio at least) doesn't even show the annotations in either the JavaDoc popup or the popup for the parameters method, the value of the annotations is much less than I'd thought it was.

I personally have found them slightly useful when writing code, as it at least gives me some instant statically checked feedback if I've accidentally passed null somewhere, but given that you only get the warning when the static checking can prove that you have passed a null, it's not as useful as it could be.

So I don't really care that much about annotations. As long as it's documented rigorously in the JavaDoc that nulls are not accepted, and we fail fast on them, beyond that it's a vaguely-nice-to-have, but not essential.

Ultimately, it's a bit of a bike-sheddy issue, and we should probably just commit to some choice fairly quickly and move on.

[artem-zinnatullin]

IntelliJ flags the null check as redundant because of the annotation:
This makes it annoying on the persons developing the library since they need to now suppress that warning to ensure the ambiguously null argument does not make it through for an ambiguous NullPointerException down the road rather than the eager one up front.

IntelliJ flags only direct check of nullability, but it won't flag indirect check via method call:

We use both @NonNull/@Nullable and checkNotNull() in our library and we don't have problems with suppressing that warning, so I'd vote for both annotations and assertions: some-safety at compile time and safety at runtime.

Btw, I'm thinking about renaming checkNotNull() to assertNotNull() or verifyNotNull() in our code, because in fact, "check" should return boolean/etc, but not throw an exception (or I have wrong expectations). Agree/disagree?

[JakeWharton]

Yes, yes I'm glossing over that fact. My point still stands about the redundancy.

Agree/disagree?

Disagree. Also, who cares ¯_(ツ)_/¯.

[pakoito]

A vote for the redundant behaviour of annotations plus checker redundancy.

[artem-zinnatullin]

Disagree. Also, who cares ¯_(ツ)_/¯.

Yeah, but Guava's design is sometimes questionable too :)

I have last argument about profit from annotations: Kotlin and probably other languages.

TL;TR:

// Java
class Hey {

  @Nullable String yo(@NonNull String whoCares) {
    if (whoCares.lenght() > 0) {
      return "yo, whoCares = " + whoCares;
    } else {
      return null;
    }
  }
}

// Kotlin
{
  Hey hey = Hey();

  hey.yo(nullableString); // won't compile!
  hey.yo(nullableString ?: "nobody"); // will compile.

  val yoResult : String  = hey.yo("me"); // won't compile!
  val yoResult : String? = hey.yo("J");  // will compile.
}

http://blog.jetbrains.com/kotlin/2015/04/upcoming-change-more-null-safety-for-java/

The details are described in this spec-document, but the overall idea is as follows: whenever we encounter nullability annotations in Java, and they do not conflict with anything around them (like overridden declarations in supertypes), we use precise types.

Use the Power Both, Luke!

[JakeWharton]

Yes, we were always going to use annotations. The argument was more whether the redundant explicit checks were also necessary.

[vanvlack]

As long as it's documented rigorously in the JavaDoc that nulls are not accepted, and we fail fast on them, beyond that it's a vaguely-nice-to-have, but not essential

Agree, documentation is still needed. I'm not advocating for one over the other, just stating my opinion that both can/do help.

Note that the default in IntelliJ is to display a warning on null setting to a @Nonnull parameter, but this can also be configured to be an error or ignored.

I believe as of now, the annotations by themselves are only useful for warnings when you have it setup in an IDE. You could bring in something like the Checker Framework, which will analyze your code at compile time and determine what is breaking that nonnull contract.

As long as there is consistency, whether that be annotations, runtime checks, documentation or a combination of those, that is really what matters.

[JakeWharton]

👍 👍

error-prone and findbugs also fail builds on violation of nullability annotation contracts. I'm a fan.

[vanvlack]

I've just checked and IntelliJ (or Android Studio at least) doesn't even show the annotations in either the JavaDoc popup or the popup for the parameters method, the value of the annotations is much less than I'd thought it was.

I'm using IntelliJ 14 and I not only see @Nonnull annotations in javadoc, but generating javadoc also includes the annotations. Unsure what Android Studio is built on though. The only thing that is not included is actual javadoc comment related info.

[JakeWharton]

Those are @Documented versions from (I'm guessing?) the Checker Framework. The platform annotations are not @Documented versions, sadly, so they won't show up.

[JakeWharton]

http://r.android.com/151379

[vanvlack]

@JakeWharton likely due to me using:

<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>

Which seems to have @Documented annotations.

[vanvlack]

@JakeWharton another option, is to only annotate those parameters that are Nullable in your code. You can then state that all parameters are expected to be Nonnull, unless otherwise noted.

You should then only need to check for null on methods, vs doing explicit Nonnull annotations.

Just a thought.

[JakeWharton]

That seems the least best option in terms of utility to the consumer of the
library. The IDE won't warn when they pass null to a non-Nullable-annotated
parameter nor will the library fail that value eagerly.

On Mon, Jun 1, 2015 at 5:56 PM Chris notifications@github.com wrote:

@JakeWharton https://github.com/JakeWharton another option, is to only
annotate those parameters that are Nullable in your code. You can then
state that all parameters are expected to be Nonnull, unless otherwise
noted.

You should then only need to check for null on methods, vs doing explicit
Nonnull annotations.

Just a thought.

—
Reply to this email directly or view it on GitHub
#20 (comment)
.