-
Notifications
You must be signed in to change notification settings - Fork 971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize @NonNull / checkNotNull usage #20
Comments
My vote is on checkNotNull because the annotations are non-binding. |
To play devil's advocate to that, annotations show in the IDE for consumers On Wed, Apr 29, 2015, 1:21 PM Daniel Lew notifications@github.com wrote:
|
Both seems like a good solution. |
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 |
Another positive for NonNull annotation is the support of null typing by the Kotlin compiler in the upcoming milestone. |
@vanvlack we have My biggest problem with both is that it causes a ton of warnings in the library projects for "needlessly" checking a |
@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 Of course, this all assumes the implementer is following the contract that it set upon itself, thus the fail-fast
I wouldn't say they are |
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
It's absolutely needless checking because of its redundancy, not its location. In fact, you mention this in your point above:
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 I'm not opposed to this as I said above. It's just an annoying burden pushed onto library implementations that want absolute guarantees. |
Do you still get the warning when using 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 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. |
Yes, yes I'm glossing over that fact. My point still stands about the redundancy.
Disagree. Also, who cares ¯_(ツ)_/¯. |
A vote for the redundant behaviour of annotations plus checker redundancy. |
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/
Use the |
Yes, we were always going to use annotations. The argument was more whether the redundant explicit checks were also necessary. |
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 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. |
👍 👍 error-prone and findbugs also fail builds on violation of nullability annotation contracts. I'm a fan. |
I'm using IntelliJ 14 and I not only see |
Those are |
@JakeWharton likely due to me using: <groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId> Which seems to have |
@JakeWharton another option, is to only annotate those parameters that are You should then only need to check for Just a thought. |
That seems the least best option in terms of utility to the consumer of the On Mon, Jun 1, 2015 at 5:56 PM Chris notifications@github.com wrote:
|
Figure out if we want actual non-null verification or just nullability annotations.
The text was updated successfully, but these errors were encountered: