Consider adding the uber/NullAway error-prone plugin

[iamdanfox]

After removing Findbugs from the tritium repo in palantir/tritium#76, it seems like we might actually now be exposed to a few more bugs.

Apparently https://github.com/uber/NullAway catches silly mistakes like:

static void log(Object x) {
    System.out.println(x.toString());
}
static void foo() {
    log(null);
}

cc @schlosna

Other things worth investigating:

• Consider supporting JSR 305 annotations google/error-prone#44
• Create more annotations like @ParametersAreNonnullByDefault google/error-prone#280
• https://checkerframework.org/

[stale]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[iamdanfox]

I've just tried NullAway on a couple of internal repos (template-wc and a larger build-related one) - it was incredibly easy to add and seems to have flagged legitimate cases where the type system cannot prove something won't be null.

    plugins.withId('com.palantir.baseline-error-prone', {
        dependencies {
            compileOnly 'com.google.code.findbugs:jsr305'
            annotationProcessor 'com.uber.nullaway:nullaway'
        }

        tasks.withType(JavaCompile) {
            options.errorprone.errorproneArgs += ['-XepOpt:NullAway:AnnotatedPackages=com.palantir']
        }
    })

I think a remaining question of how we could roll this out everywhere would be how should that AnnotatedPackages=com.palantir thing be specified - in theory we could just bake it into baseline and ask non-palantir users to override it?

There are plenty of ways you can suppress things too if you get annoyed by this plugin (https://github.com/uber/NullAway/wiki/Suppressing-Warnings)

[stale]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[iamdanfox]

Closing out because after trying this on the large build repo, it seemed that readability of map accesses was reduced. Each individual repo is still free to add this snippet and I think we'll continue to experiment with this on certain libraries (e.g. tritium).

[ash211]

Hi @iamdanfox, enabling this nullaway plugin on an internal repo would have found a bug that hit our users, but sadly it wasn't already enabled. I'd like to revisit turning on findbugs by default, or at least making it an easy opt-in as suggested by @carterkozak

it seemed that readability of map accesses was reduced.

From the above thread it looks like this was the only stated issue with enabling nullaway broadly. Could you please expand on what the problem is with map accesses?

[iamdanfox]

@ash211 here are the illustrative PRs from when I have tried enabling nullaway on large repos:

• https://internal-github/foundry/foundry-build2/pull/1993
• https://internal-github/foundry/apollo/pull/10768

In both cases, it seemed like there is a very pervasive pattern of having multiple Maps or Sets where there is an invariant that the keysets are exactly equal…. so once you’ve called .get on one, in reality all of the other .get() calls will succeed but static analysis doesn’t know about this invariant... so it forces you to add a bunch of explicit checking.

INTERESTINGLY another repo didn't seem to have this same pattern, so I was able to adopt nullaway without too much friction: https://internal-github/foundry/adjudication-service/pull/2766