Adding Static Analysis Checks for @Nullable via NullAway

[Andrew Grieve]

I've been looking at the feasibility of adding NullAway (https://issues.chromium.org/issues/40657579), and am at the point now where I feel pretty confident that:

1. Adding nullness annotations improves our code (by making it always apparent when null checks are required).
2. NullAway is a solid tool to use for enforcement.

There's a lot more context in this doc, but the proposal is:

1. Use annotations as per: https://jspecify.dev/docs/user-guide/, in addition to others supported by NullAway:
1. @RequiresNonNull / @EnsuresNonNull{If}
   
2. @Initializer
3. @Contract
4. assumeNonNull() (not an annotation, but a core piece of the puzzle)
   
   
2. Enable checking on a per-file basis, by annotating classes with @NullMarked
3. Use chromium-specific copies of all annotations (to make Cronet's life easier)
4. Avoid excessive GN deps by making //build/android:build_java a default dep of ~all targets

Everything I've described is already implemented, and we've added @NullMarked to >500 classes so far. I didn't want to propose this until I was confident it would work well for us, and I couldn't do that without first trying it out :).

While we've annotated a lot of classes already, there are still a huge number that remain. To get to a fully-annotated state, I propose:

1. We give a window of ~3 months for teams to run the auto-annotating tool on their code, fixing up any suppressions it adds.
2. We then run the tool on the entire codebase, and leave suppressions to be cleaned up over time.
3. Add a PRESUBMIT.py to ensure new files add @NullMarked

Please let me know if you have any concerns / questions with this plan. If not, I'll follow-up with a style guide update proposal.

[Henrique Nakashima]

I believe NullAway is a net positive and that we should go forward with the migration.

Positives:

- It will catch possible/rare NPEs earlier (compile-time vs CQ/CI/Production). It's hard to be sure to what extent because we've been converting code that has already been through the latter phases and has stood the test of time, so we didn't find many impactful bugs; However, we still found a good number of potential bugs (which were marked as assumeNonNull() since that's what the code currently does).

- Fails early (ErrorProne-time, so right after compile).

- Seems solid in terms of correctness. It triggered in places you'd expect it to.

- The annotation tool makes it feasible to annotate the whole codebase.

Negatives:

- It's an extra thing to learn that developers writing Clank code need to know.

- @Nullable annotations are ugly sometimes (for arrays and nested classes). Android Studio refactor tools don't handle it properly.

- Even though the annotation tool helps, it's on the order of magnitude of 3 eng*month to annotate the whole codebase.

We probably don't want to be in the middle of this migration for long, so +1 should actively migrate it

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1VoT1f4AJ7jZxOvfzYrBY5FW-%2B3f8nEh6jXNkJ8Vfs4uQ%40mail.gmail.com.

[Henrique Nakashima]

* +1 to actively migrate it and finish in Q1.

[Peter Conn]

> We give a window of ~3 months for teams to run the auto-annotating tool on their code, fixing up any suppressions it adds.

I was wondering if you could give a bit more detail about this - is there going to be a code-drive or outreach to teams?

The reason I ask is that I'd like to get //android_webview/ annotated, but at the moment we're blocked on a whole bunch of our dependencies. Running the auto-annotation tool on android_webview:browser_java caused modifications to 1 file in //android_webview and 48 files across //content and //components (specific files). We don't want to block the overall process, but are ourselves blocked on other teams.

[Andrew Grieve]

We've got a team rotation for the quarter to take turns running the annotator over the codebase, and a script to check our progress:

https://source.chromium.org/chromium/chromium/src/+/main:tools/android/nullaway/java_file_stats.py

At the end of the quarter, we'll see where we are, and there's a good chance we'll just run the annotator over the entire codebase and commit the suppressions to be fixed up as-needed.