[BEAM-10402] Enable checkerframework globally #13134

kennknowles · 2020-10-17T03:12:10Z

Instead of opting out whole modules now only existing classes are opted out of type checking. This has the following benefits:

New code will be checked, even in modules that are not yet passing.
Code won't as easily backslide in modules that were partially fixed.
It will produce much more manageable incremental work, and limitless starter bugs!

I produced the needed warnings (merged in other PRs) by removing the flag and then repeating the following, more or less:

./gradlew compileJava compileTestJava --continue 2>&1 | tee ~/tmp/gradle.log
 
cat ~/tmp/gradle.log \
    | grep -e "$PWD"'.*error:.*' \
    | cut -d : -f 1 \
    | sort -u \
    | xargs grep -L -e '^@SuppressWarnings' \
    | xargs sed -E -i '' 's/^(public +)?((abstract|final) +)?(class|interface)/@SuppressWarnings("nullness") &/'
  
./gradlew spotlessApply

There are two ways to still suppress type checking, arguments to applyJavaNature:

generatedClassPatterns to exclude various generated code that is not annotated with nullness types
classesTriggerCheckerBugs a map from classes which cannot be analyzed to their checkerframework bug URL

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

kennknowles · 2020-10-22T03:26:43Z

@TheNeuralBit I don't know why I think you would enjoy / be amused by this, but I chose arbitrarily. @jayendra13 may also be interested.

% find . -name '*.java' -or -name 'build.gradle' | wc -l
    5424

So this involved suppression or tweaking on about 2k out of 5.5k files. You can image how it is important to get this done before we grow even bigger...

kennknowles · 2020-10-22T03:27:32Z

Oh now I remember - there was some runtime ramification of enabling checker for certain modules. Do you recall?

jayendra13 · 2020-10-22T03:33:48Z

I am definitely interested in this, but got stuck with my own work. Will resume soon may be in a week or two 🙂 .

kennknowles · 2020-10-22T03:36:03Z

New files since I ran it, of course. It may not be possible to review without that happening during review. It doesn't mean much for the review of the concept, I think.

kennknowles · 2020-10-22T17:41:18Z

The NPE reproduced on InMemoryJobServiceTest when mockito tried to mock it. I believe it is a bug in the JDK. Annotations that are not present on the classpath should be "as if they were not there" but instead the JDK NPEs. Maybe they should use checker to eliminate those null errors :-)

Since the annotations are MIT licensed they can be compile scope instead of compileOnly scope. That fixed InMemoryJobServiceTest.

TheNeuralBit

The concept looks good. Do you think you could change it so all of the @SuppressWarnings annotations are added in a separate commit? Then we'd just be left with the gradle changes, and any other java changes that make sense to review manually.

TheNeuralBit · 2020-10-23T00:33:53Z

examples/java/src/test/java/org/apache/beam/examples/complete/TrafficRoutesIT.java

@@ -42,6 +42,7 @@

 /** End-to-end tests of TrafficRoutes. */
 @RunWith(JUnit4.class)
+@SuppressWarnings("nullness")


Could we add a TODO(BEAM-10402) on these?

kennknowles · 2020-10-23T04:27:05Z

Great ideas.

I learned a bit from:

In particular, I did not know about git grep or git diff-index at all. Now I do, I guess? I never did figure out exactly how to do what you suggest in one go. Perhaps the best way to check that I got it is to separate PRs...

The enabling (somewhat of a leap of faith?) which I will later rebase -i to come after the rest.

git reset HEAD^
git status | grep build.gradle | cut -d : -f 2 | xargs git add
git commit -m 'Enable checkerframework globally'

I figured out I could commit everything that wasn't a new suppression (interactively, somewhat tedious but far less than 2k files)

git add -p $(git diff-index -G ' .*Nullable.*' HEAD | cut -d ' ' -f 5 | cut -f 2)

And the API surface checks all include a new line of pruningPattern

git diff-index -SpruningPattern HEAD

(needed to tweak interactively)

git commit -m 'Allow checkerframework on API surfaces'

Confirmation that the only things remaining are suppressions at the top level:

git diff -U0 | grep '^[+][^+]' | grep -v '@SuppressWarnings'

(oops there were a few more, which I isolated and committed)

kennknowles · 2020-10-23T04:29:29Z

Gya, I botched the API surface patches versus moving nullable annotation patches

kennknowles · 2020-10-23T04:43:39Z

OK. The commits look right to me. Let me know if it helps to split things any other way. I had hoped this would be a pretty silly PR but it got slightly involved. I would say two things to check are:

all except the monster commit make sense
the monster commit contains nothing other than suppressions

Yea?

kennknowles · 2020-10-23T16:25:49Z

Run Java PreCommit

kennknowles · 2020-10-23T16:31:11Z

Hmm this might slow the build down enough that it has to be separated from the CI build.

kennknowles · 2020-10-23T16:32:24Z

Run Python_PVR_Flink PreCommit

kennknowles · 2020-10-23T16:32:37Z

Run Java_Examples_Dataflow PreCommit

TheNeuralBit

LGTM, thank you! This will be much better, hopefully seeing the SuppressWarnings annotations will inspire some more crowdsourced fixes.

I did a quick check over the monster commit to make sure it was just SuppressWarnings, and looked over the other one more closely.

I'm assuming you'll have to re-generate the monster commit to sync with master, can you add TODOs at the same time?

TheNeuralBit · 2020-10-23T16:30:54Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

          publish: configuration.publish,
+          generatedClassPatterns: [
+            '^org\\.apache\\.beam\\..*'
+          ],


This looked concerning until I looked around and realized the "portability" nature means its for making modules that generate protobuf/grpc code. The comment above "applyPortabilityNature should only be applied to projects that want to use vendored gRPC / protobuf dependencies" isn't very helpful. Maybe I'll send you a PR to document these natures.

Yea. I could be .model except it is also used by the windmill proto generation. I have changed it so that it is passed in and the default is the model.

TheNeuralBit · 2020-10-23T16:38:19Z

sdks/java/extensions/protobuf/build.gradle

+    '^org\\.apache\\.beam\\.sdk\\.extensions\\.protobuf\\.Proto2CoderTestMessages',
+    '^org\\.apache\\.beam\\.sdk\\.extensions\\.protobuf\\.Proto2SchemaMessages',
+    '^org\\.apache\\.beam\\.sdk\\.extensions\\.protobuf\\.Proto3SchemaMessages',
+    '^org\\.apache\\.beam\\.sdk\\.extensions\\.protobuf\\.Proto3SchemaOptions',


Is it possible to use slashy strings in our gradle files? That'd clean these arrays up a bit.

Didn't know about slashy strings. Nice. Done.

Nice! I was unaware of them before, but did a check to see if there was some kind of regex literal we could use. I guess ~"pattern" is an option as well, but produces Pattern instances.

TheNeuralBit · 2020-10-23T16:38:51Z

sdks/java/extensions/sql/hcatalog/build.gradle

+applyJavaNature(
+  automaticModuleName: 'org.apache.beam.sdk.extensions.sql.meta.provider.hcatalog',
+  classesTriggerCheckerBugs: [
+    'HCatalogTable': '',


not yet filed or TODO

TheNeuralBit · 2020-10-23T16:40:42Z

sdks/java/io/hadoop-format/build.gradle

@@ -20,7 +20,7 @@ import groovy.json.JsonOutput

 plugins { id 'org.apache.beam.module' }
 applyJavaNature(
-  enableChecker:false,
+


nit: some of these are left with unnecessary whitespace (not a big deal if its a pain to fix)

TheNeuralBit · 2020-10-23T16:41:09Z

sdks/java/io/kudu/build.gradle

  ignoreRawtypeErrors:true,
+  classesTriggerCheckerBugs: [
+    'KuduTestUtils': '',
+    'KuduIOIT': '',


not yet filed or TODO

TheNeuralBit · 2020-10-23T16:43:00Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

-          '-Werror'
+          '-Werror',
+          '-Xmaxerrs',
+          '10000'


Is this a typo for maxerrors?

I'm assuming you added this to make it quicker to run, can/should we drop it now?

I did this so that it would expose all the errors and I could grep the log to pass the right files through sed. Presumably in mostly-functioning code you would be at 0 or a handful anyhow so the number doesn't matter. But I'll remove it because might as well leave it at default.

kennknowles · 2020-10-23T17:00:57Z

Seeing really wonky errors on Jenkins that I cannot repro locally. Could be checkerframework bugs related to particular JDKs?

kennknowles · 2020-10-23T17:01:01Z

Run Java PreCommit

kennknowles · 2020-10-23T20:08:40Z

Run Java PreCommit

kennknowles · 2020-10-29T20:35:19Z

The re-run was 38 minutes, pretty normal. Many things pulled from cache. https://scans.gradle.com/s/wyh5rgkbd3rjq/timeline?sort=longest

"2463 tasks executed in 116 projects in 33m 22.938s, with 173 avoided tasks saving 4h 12m 58.349s"

kennknowles · 2020-10-29T20:41:05Z

Here are some samples of runs against commits and phrases:

It is hard to determine the real likely slowdown in practice. The tasks that were the longest and most problematic in the bad run are almost always FROM-CACHE.

kennknowles · 2020-10-29T23:21:37Z

On my idle desktop machine:

git checkout github/master && ./gradlew :javaPreCommit --continue --no-build-cache --rerun-tasks --scan https://scans.gradle.com/s/cdfrii6qsopii/timeline?sort=longest
git checkout checkerframework-classlevel && ./gradlew :javaPreCommit --continue --no-build-cache --rerun-tasks --scan https://scans.gradle.com/s/2r65rr5mflsro/timeline?sort=longest

Unfortunate that the build seems very unhealthy on master so I am just looking at particular tasks. It does look like compile tasks are perhaps 5x as slow but it does not affect the overall runtime very much. One option is to cease analyzing test java. These seem to have a much greater slowdown factor. I wonder if incorrect types are more expensive than correct ones...

jayendra13 · 2020-11-04T02:52:53Z

We have fixed some of the modules, so I think creating a map somewhere denoting modules fixed and not fixed would help for better tracking this issue. What do you think? I am soon going to start this again.

TheNeuralBit · 2020-11-04T19:42:52Z

I think creating a map somewhere denoting modules fixed and not fixed would help for better tracking this issue

Are you thinking something like a spreadsheet that we would keep up-to-date, or would this be a part of the build?

With the change in #13191 we could identify fixed vs. not fixed classes and/or modules by grepping for @SuppressWarnings("nullness") // TODO(https://issues.apache.org/jira/browse/BEAM-10402). Maybe that's enough for tracking? We could put a bash one-liner in the BEAM-10402 comments

This commit is independently useful, since checkerframework annotations are helpful for users. We should preserve them at runtime.

jayendra13 · 2020-11-05T13:40:46Z

Are you thinking something like a spreadsheet that we would keep up-to-date, or would this be a part of the build?

Not part of a build, but some where publicly accessible like kanban board on jira or on github .

kennknowles · 2020-11-05T16:58:10Z

OK this is finally healthy and ran in a normal amount of time (precommits tend to be in the 30-60 minute range and this was 30). Only the tests for SDK core are skipped. Hopefully we won't have to skip them forever, since carelessness with nulls in tests is an annoying source of false issues. Going to merge it!

kennknowles · 2020-11-05T17:05:18Z

Confirmed that there are no breakages introduced in master since the tests ran here.

kennknowles · 2020-11-05T17:10:32Z

Since there are 2000+ classes with issues, probably this is too many Jiras and too much for a kanban? I don't know. Actually each class can be enough of a challenge for a Jira. But maybe Jiras for the module level is easier to manage.

A spreadsheet containing the output of a bash script that finds the TODO might make a lot of sense for tracking the burndown.

kennknowles mentioned this pull request Oct 17, 2020

Crash report: IndexOutOfBoundsException typetools/checker-framework#3794

Closed

kennknowles force-pushed the checkerframework-classlevel branch 3 times, most recently from 75a9fe8 to 78f9fb0 Compare October 22, 2020 03:22

kennknowles marked this pull request as ready for review October 22, 2020 03:24

kennknowles requested a review from TheNeuralBit October 22, 2020 03:24

kennknowles force-pushed the checkerframework-classlevel branch 2 times, most recently from a9342ef to fa57261 Compare October 22, 2020 17:26

kennknowles force-pushed the checkerframework-classlevel branch from fa57261 to 9af1770 Compare October 22, 2020 23:18

TheNeuralBit reviewed Oct 23, 2020

View reviewed changes

kennknowles force-pushed the checkerframework-classlevel branch from 9af1770 to 48ba7a1 Compare October 23, 2020 04:27

kennknowles force-pushed the checkerframework-classlevel branch 2 times, most recently from b9d51f4 to 21dbf7a Compare October 23, 2020 04:40

kennknowles force-pushed the checkerframework-classlevel branch from 21dbf7a to 4b4b8fc Compare October 23, 2020 13:51

TheNeuralBit approved these changes Oct 23, 2020

View reviewed changes

kennknowles force-pushed the checkerframework-classlevel branch from ecd1e0a to 00aa127 Compare October 29, 2020 20:41

kennknowles force-pushed the checkerframework-classlevel branch from 00aa127 to 8ef235f Compare October 30, 2020 21:14

kennknowles force-pushed the checkerframework-classlevel branch 3 times, most recently from 4ebfe54 to 7c1127c Compare November 4, 2020 18:05

kennknowles force-pushed the checkerframework-classlevel branch 5 times, most recently from 64f2f07 to 249643f Compare November 5, 2020 04:18

kennknowles added 6 commits November 4, 2020 20:43

Suppress nullness errors in new files since last round of suppressions

d01aa67

Fix position of @nullable annotations since last round

234ef1b

Exclude nonexistent org.checkerframework:jdk8 from all configurations

54c52e1

Fix nullness error in Kotlin WriteOneFilePerWindow

75dcc08

Allow checkerframework on API surfaces

22bc31a

This commit is independently useful, since checkerframework annotations are helpful for users. We should preserve them at runtime.

Enable checkerframework globally

c5d5b9a

kennknowles force-pushed the checkerframework-classlevel branch from 249643f to c5d5b9a Compare November 5, 2020 04:44

kennknowles merged commit 38599d1 into apache:master Nov 5, 2020

kennknowles deleted the checkerframework-classlevel branch November 5, 2020 17:05

udim mentioned this pull request Nov 11, 2020

[DO NOT MERGE] Start snapshot build for release process. #13274

Closed

HoussemNasri mentioned this pull request Feb 19, 2023

Eliminate nullability errors from :sdks:java:core #20507

Open

[BEAM-10402] Enable checkerframework globally #13134

[BEAM-10402] Enable checkerframework globally #13134

Conversation

kennknowles commented Oct 17, 2020 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

kennknowles commented Oct 22, 2020

kennknowles commented Oct 22, 2020

jayendra13 commented Oct 22, 2020

kennknowles commented Oct 22, 2020

kennknowles commented Oct 22, 2020

TheNeuralBit left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

TheNeuralBit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 23, 2020

kennknowles commented Oct 29, 2020

kennknowles commented Oct 29, 2020

kennknowles commented Oct 29, 2020

jayendra13 commented Nov 4, 2020 • edited

TheNeuralBit commented Nov 4, 2020 • edited

jayendra13 commented Nov 5, 2020 • edited

kennknowles commented Nov 5, 2020

kennknowles commented Nov 5, 2020

kennknowles commented Nov 5, 2020

kennknowles commented Oct 17, 2020 •

edited

TheNeuralBit left a comment •

edited

jayendra13 commented Nov 4, 2020 •

edited

TheNeuralBit commented Nov 4, 2020 •

edited

jayendra13 commented Nov 5, 2020 •

edited