Skip to content

Commit

Permalink
@nonnull by default in core packages.
Browse files Browse the repository at this point in the history
  • Loading branch information
@nedtwigg
nedtwigg committed Dec 1, 2016
1 parent 8a59495 commit 4d6f9b9
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 0 deletions.
4 changes: 4 additions & 0 deletions lib-extra/src/main/java/com/diffplug/spotless/extra/package-info.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@ParametersAreNonnullByDefault
package com.diffplug.spotless.extra;

import javax.annotation.ParametersAreNonnullByDefault;
4 changes: 4 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/generic/package-info.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@ParametersAreNonnullByDefault
package com.diffplug.spotless.generic;

import javax.annotation.ParametersAreNonnullByDefault;
4 changes: 4 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/java/package-info.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@ParametersAreNonnullByDefault
package com.diffplug.spotless.java;

import javax.annotation.ParametersAreNonnullByDefault;
4 changes: 4 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/package-info.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@ParametersAreNonnullByDefault
package com.diffplug.spotless;

import javax.annotation.ParametersAreNonnullByDefault;

10 comments on commit 4d6f9b9

@jbduncan
Copy link
Member

@jbduncan jbduncan commented on 4d6f9b9 Dec 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a couple of suggestions regarding this commit.

  1. We should probably go as far as introducing and using custom @ReturnValuesAreNonnullByDefault and @FieldsAreNonnullByDefault annotations (as described in http://stackoverflow.com/questions/11776302/how-to-indicate-that-member-fields-are-nonnull-by-default). This is what I'm doing in my university dissertation. (I'd hesitate to make @EverythingIsNonnullByDefault though, as I doubt the benefit of being able to analyse local vars would outstrip the awkwardness of having to use @Nullable on nullable vars).
  2. I'd strongly suggest using a different tool to FindBugs for null analysis, as it interprets @Nullable completely differently to other tools (according to section 3.7.2 of the Checker Framework Manual).

@nedtwigg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Have a recommended tool?

CheckerFramework seems pretty heavyweight, but I'm not against a PR for it:
https://types.cs.washington.edu/checker-framework/current/checker-framework-manual.html#gradle

@jbduncan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I'd 100% recommend it yet, but I'm personally using Google's error-prone in my dissertation, so if we decide to use it, then I'd be in a better position to help figure out any Gradle config issues.

Its main benefit is it catches a whole range of things at compile-time (which is rather useful!), but it comes with the drawback that compilation must be done through a custom JVM distributed with error-prone itself. This is so that error-prone can hook into the relevant parts of the Java compilation process to perform its checks. (But then again, the Checker Framework also imposes this 'drawback' of requiring a custom JVM, so it may be a moot point).

@nedtwigg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's good enough for Google, it's good enough for Spotless, I think :) My preference is for the custom JVM build to be optional. e.g. gradlew jar test wouldn't need to suck down an entire JVM, but gradlew check would. I'm not immovable on this point, however.

@jbduncan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my preference is for the custom JVM to be optional as well, but it's not clear to me if this is possible with error-prone. Might be worth raising an issue over over at error-prone's Github repo?

I hope to be able to look into creating a PR for this tomorrow. I'm currently thinking of following Caffeine's example on configuring and using FindBugs and error-prone in the same build, and to apply this config as needed to each of Spotless's subprojects.

@nedtwigg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I think this is a great-to-have but not a must-have - it's not blocking anything.

@jbduncan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just to correct myself, I think error-prone doesn't actually depend on a JVM, but rather on a custom version of javac. So AFAICT it's not that bad, but still against my personal preferences.) :)

@jbduncan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, after doing some investigation, I've come to the conclusion that although error-prone does have some nullness checking built-in, those checks are "Experimental" and aren't enabled by default. As I don't (yet) know how to enable Experimental checks via the gradle plugin, I'm tempted to leave this for another time.

@nedtwigg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me :)

Think you'll have time to work on freshmark or any of these?

#56 (comment)

@jbduncan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nedtwigg, I'm sorry to say not feeling too bright this evening, so whereas I'm happy to have a go, please don't wait on me to complete any particular ones, and feel free to have a go at them yourself if you think I'm going too slowly. :)

Please sign in to comment.