Check for nulls eagerly in parameters etc. more consistently

[jbduncan]

Basically, use Objects.requireNonNull and Preconditions.checkNotNull more consistently.

If we don't want NPE to be thrown when encountering a variable with null, or if null is considered a valid value, then we should annotate it with @Nullable.

This may get tricky though in cases where we're dealing with e.g. a non-null list containing nullable elements. In that case, we'd need to annotate it like so:

List<@Nullable String> listOfNullableStrings = ...

[jbduncan]

Relevant comments:

1. Extend FileSignature to be sensitive to file order and duplicates. #85 (comment)
2. Extend FileSignature to be sensitive to file order and duplicates. #85 (comment)

[nedtwigg]

I have tried to contribute to projects before where they were in the middle of a code-style change. I copied the style of the project as it existed, and had to rewrite it to suit the new style they were moving to. Frustrating experience - I got some stuff merged but mostly gave up.

The point of spotless is to get rid of these kinds of arguments. In the spirit of spotless, I don't think we should burden our contributors with any stylistic burdens that the tools can't enforce on their own.

If you feel strongly about this @jbduncan, you might to look at guava's testlib. Here's a usage example:

https://github.com/diffplug/durian-rx/blob/master/test/com/diffplug/common/rx/PackageSanityTests.java

It calls every public method and confirms that it obeys the @nonnull / @nullable contracts.

[jbduncan]

Sounds like a reasonable compromise to me @nedtwigg. I'll look into PackageSanityTests as soon as I've the time. :)

[jbduncan]

Hi @nedtwigg, I wonder if I may ask for your help with diagnosing (what I hope is just) a misunderstanding on my part of AbstractPackageSanityTests?

I've pushed a new branch to https://github.com/diffplug/spotless/tree/issue-86 which has my experiments so far for testing the conformance of Spotless with its @Nonnull/@nullable contracts.

However, the PackageSanityTests.java tests that I've written don't seem to be running against any class within the Spotless codebase at all, so they pass (go "green") erroneously.

So far, I've tried running IntelliJ IDEA's debugger against plugin-gradle\src\test\java\com\diffplug\gradle\spotless\PackageSanityTests.java, which seems to confirm that AbstractPackageSanityTests::testNulls (the method actually responsible for doing the null testing) is being called. But when I step through it and allow the debugger to go into loadClassesInPackage(), I notice that it's not able to find any classes in the package com.diffplug.gradle.spotless (which are the classes I want to test) on the classpath.

Do you have the time to help me look into this? If not, then I'd be happy to put this all to one side for someone to tackle again in the future.

[nedtwigg]

I've got a couple ideas:

1. Maybe you need to call publicApiOnly()? See example.
2. The code is in the lib project, but the testcode is in testlib. It might be that in order for PackageSanityTests to work, the sanity tests will need to be in the same project. You can definitely add a test folder to lib - the only reason that all of libs tests are in testlib is to solve the dependency puzzle so that we can reuse test harnessing tools for the lib and testlib tests.

[jbduncan]

Hi @nedtwigg, many thanks for the advice. Unfortunately, it seems that no combination of your suggestions seem to work, so I've resorted to using NullPointerTester directly, which is actually proving to be quite a lot of work!

I don't know when I'll be able to send a PR, as I've been focusing on my university studies over the last few weeks, but I suspect that when I am ready I'll need to send multiple sub-PRs, since my unpublished results so far have touched many files in the codebase.

If you can think of anything else that I could try, then I would be very happy to listen! Otherwise, I'm more than happy to just continue plugging at this when I have the time to work on it again.

[nedtwigg]

I don't have any more tricks up my sleeve :)

I'm happy to add enhanced null-checking, but I don't want to add a ton of boilerplate test code which makes it harder for people to submit new features in the future. This change is valuable, but minor. If it requires a lot of code, I'm not convinced it's worth the cost.

[jbduncan]

Okay, that makes a great deal of sense to me.

Since using NullPointerTester produces a lot of boilerplate test code, and I don't want to inflict the pain of writing tests with it onto future contributors, I'll submit just the enhanced null-checks themselves when I'm ready to do so.

Thanks @nedtwigg!

[jbduncan]

Finished as of #101.