Implement a simple nullaway wrapper plugin #2382

carterkozak · 2022-09-14T15:03:52Z

This plugin can be applied with the baseline-error-prone plugin, it adds a standardized nullaway configuration to the project with centrally managed nullaway versioning via gralde-baseline. Note that Nullaway is applied at WARNING rather than ERROR such that projects that already enforce warnings will fail builds on nullaway, however the nullaway warnings will only produce console churn otherwise.

==COMMIT_MSG==
Implement a simple nullaway wrapper plugin com.palantir.baseline-null-away
==COMMIT_MSG==

This plugin can be applied with the baseline-error-prone plugin, it adds a standardized nullaway configuration to the project with centrally managed nullaway versioning via gralde-baseline. Note that Nullaway is applied at `WARNING` rather than `ERROR` such that projects that already enforce warnings will fail builds on nullaway, however the nullaway warnings will only produce console churn otherwise.

changelog-app · 2022-09-14T15:03:55Z

Generate changelog in `changelog/@unreleased`

Type

Description

Implement a simple nullaway wrapper plugin com.palantir.baseline-null-away which registers the NullAway check at WARNING. Projects which fail on warnings will require this to pass pre-merge.

Check the box to generate changelog(s)

Generate changelog entry

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

…gins/BaselineNullAway.java

...e-baseline-java/src/test/groovy/com/palantir/baseline/BaselineNullAwayIntegrationTest.groovy

…elineNullAwayIntegrationTest.groovy

carterkozak · 2022-09-14T16:57:09Z

baseline-null-away/baseline-class-uniqueness.lock

+  - org.checkerframework.dataflow.qual.Pure
+  - org.checkerframework.dataflow.qual.Pure$Kind
+  - org.checkerframework.dataflow.qual.SideEffectFree
+  - org.checkerframework.dataflow.qual.TerminatesExecution


This roughly matches the baseline-class-uniqueness failures in baseline-error-prone. Seems fine, especially for build-only code.

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

…gins/BaselineNullAway.java

carterkozak · 2022-09-14T17:21:32Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

+    }
+
+    private static void configureErrorProneOptions(Project proj, Action<ErrorProneOptions> action) {
+        proj.afterEvaluate(new Action<Project>() {


This is a bit gross, but required due to the way the errorprone plugin configures JavaCompile tasks. At this point it may be simpler to implement the errorprone plugin ourselves (now that the errorprone-javac dep isn't needed, and we support jdk11+)

Does errorprone also use afterEvaluate? If so just note that the order multiple afterEvaluates run is technically undefined.

Hmm, this may not be necessary here, I'm not entirely sure why it was required in another plugin we worked on recently. I'll remove the afterEvaluate.

schlosna

Overall seems reasonable, but have a concern about version pins

schlosna · 2022-09-15T04:10:24Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

+        configureErrorProneOptions(project, new Action<ErrorProneOptions>() {
+            @Override
+            public void execute(ErrorProneOptions options) {
+                options.option("NullAway:AnnotatedPackages", "com.palantir");


I guess we need to provide a default package to make this work out of the box per https://github.com/uber/NullAway/wiki/Configuration#annotated-packages so this seems reasonable, though any non-Palantir projects would need to override this to enable NullAway for their code.

It would be nice to include some other commonly used packages like com.google.common for Guava but that may require more opt-in work for folks adopting this from the get-go per the "Nullness annotations" bits from Guava 31.0 release specifically the NullAway bits mentioning google/guava#6126 (comment)

Happy to add guava by default for now, it's easier to remove packages than add them.

I'd considered introspecting the maven coordinate group, and adding something like the first two segments. So net.ckozak.foo would result in net.ckozak as an annotated package. However, I think there are sufficiently many edge cases that I'd prefer to punt solving this until we need to do so.

I kicked off https://github.com/palantir/tritium/compare/ds/guava-nullaway?expand=1 as a quick check last night and that build failed so I think adding Guava here might cause more false positives than desired by default, so we probably do want just options.option("NullAway:AnnotatedPackages", "com.palantir"); for now. Sorry for the run around on that.

No worries, I appreciate that you called out the idea! I had just tested a snapshot on tritium and found the same thing :-)

palantir/tritium#1570 to fix up tritium's one flagged issue

schlosna · 2022-09-15T04:18:40Z

...e-java/src/main/resources/META-INF/gradle-plugins/com.palantir.baseline-null-away.properties

+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+implementation-class=com.palantir.baseline.plugins.BaselineNullAway


I guess we haven't switched over to the gradle plugin-publish 1.0 yet on this repo, as this would need to go in the gradlePlugin implementationClass

I think we have, but the two definition locations are merged, and likely out of sync. This is allowed due to duplicatesStrategy 'include' declared here:

gradle-baseline/gradle-baseline-java/build.gradle

Lines 129 to 131 in 2399039

tasks.named('processResources').configure {

duplicatesStrategy 'include'

}

I've created an issue to follow-up and migrate to the gradle-plugin definition-site: #2384

schlosna · 2022-09-15T04:52:14Z

versions.props

@@ -15,6 +15,8 @@ com.googlecode.java-diff-utils:diffutils = 1.3.0
 com.puppycrawl.tools:checkstyle = 10.3.2
 org.checkerframework:* = 3.23.0
 com.palantir.gradle.utils:* = 0.1.0
+com.uber.nullaway:nullaway = 0.10.1
+org.checkerframework:dataflow-nullaway = 3.25.0


Do we need this more specific version pin compared to above?

Suggested change

org.checkerframework:dataflow-nullaway = 3.25.0

org.checkerframework:dataflow-nullaway = 3.25.0

schlosna · 2022-09-15T04:52:55Z

versions.props

@@ -15,6 +15,8 @@ com.googlecode.java-diff-utils:diffutils = 1.3.0
 com.puppycrawl.tools:checkstyle = 10.3.2
 org.checkerframework:* = 3.23.0


Should we bump this to 3.25.0 and remove the pin below (and maybe sort these)?

Ah, the dependency upgrade excavator is stuck, good catch! I agree that we shouldn't pin a more specific version, keeping these in sync is helpful.

schlosna · 2022-09-15T04:54:47Z

versions.lock

 org.checkerframework:checker-qual:3.23.0 (3 constraints: 08256088)
 org.checkerframework:dataflow-errorprone:3.23.0 (4 constraints: fa3d685c)
+org.checkerframework:dataflow-nullaway:3.25.0 (2 constraints: 6911b8f1)


See below, but the mix of 3.23.0 and 3.25.0 versions seems potentially problematic.

CRogers · 2022-09-15T13:26:48Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

+                .orElseGet(() -> {
+                    log.warn("BaselineNullAway is using 'latest.release' - "
+                            + "beware this compromises build reproducibility");
+                    return "latest.release";


When does this ever happen? How is nullaway applied?

This is the same logic used in baseline-error-prone. This happens when running integration tests from source, when the classpath is built up of class files rather than a jar with an Implementation-Version in the manifest.

Do most jars like google's own error prone core jar have this manifest usually, or is this something we publish?

No, this is used so that we can publishToMavenLocal prior to running tests, then resolve our own checks from the mavenLocal() repo to test them.

carterkozak · 2022-09-15T13:44:36Z

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java

+    @Override
+    public void apply(Project project) {
+        project.getPluginManager().withPlugin("com.palantir.baseline-error-prone", _unused0 -> {
+            project.getPluginManager().withPlugin("java-base", _unused1 -> {


Updated from java to java-base as I believe that's more general

###### _excavator_ is a bot for automating changes across repositories. Changes produced by the roomba/latest-baseline-oss check. # Release Notes ## 4.164.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | The `CyclomaticComplexity` check is now configured with `switchBlockAsSingleDecisionPoint`. | palantir/gradle-baseline#2383 | ## 4.165.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Implement a simple nullaway wrapper plugin `com.palantir.baseline-null-away` which registers the `NullAway` check at `WARNING`. Projects which fail on warnings will require this to pass pre-merge. | palantir/gradle-baseline#2382 | ## 4.166.0 _Automated release, no documented user facing changes_ To enable or disable this check, please contact the maintainers of Excavator.

carterkozak commented Sep 14, 2022

View reviewed changes

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java Outdated Show resolved Hide resolved

carterkozak and others added 2 commits September 14, 2022 11:04

Update gradle-baseline-java/src/main/groovy/com/palantir/baseline/plu…

3d36b53

…gins/BaselineNullAway.java

Add generated changelog entries

7b8913c

carterkozak commented Sep 14, 2022

View reviewed changes

...e-baseline-java/src/test/groovy/com/palantir/baseline/BaselineNullAwayIntegrationTest.groovy Outdated Show resolved Hide resolved

carterkozak and others added 3 commits September 14, 2022 11:06

Update gradle-baseline-java/src/test/groovy/com/palantir/baseline/Bas…

c0bf34e

…elineNullAwayIntegrationTest.groovy

test with smaller blast radius. target=15 has issues

e1ea88e

same class-uniqueness issue as baseline-error-prone

be84974

carterkozak commented Sep 14, 2022

View reviewed changes

gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineNullAway.java Outdated Show resolved Hide resolved

Update gradle-baseline-java/src/main/groovy/com/palantir/baseline/plu…

f0ad86a

…gins/BaselineNullAway.java

carterkozak requested a review from schlosna September 14, 2022 17:19

carterkozak commented Sep 14, 2022

View reviewed changes

carterkozak requested review from fawind and CRogers September 14, 2022 17:58

carterkozak added merge when ready autorelease labels Sep 14, 2022

Add generated changelog entries

6352a5b

schlosna requested changes Sep 15, 2022

View reviewed changes

carterkozak mentioned this pull request Sep 15, 2022

Migrate gradle-baseline-java plugin definitions from META-INF to gradlePlugin dsl #2384

Open

carterkozak added 2 commits September 15, 2022 09:10

constrain versions

ec79a56

include guava

7c3bb86

CRogers reviewed Sep 15, 2022

View reviewed changes

carterkozak added 3 commits September 15, 2022 09:30

no guava

2bfcbcc

comment

a502e3c

no afterevaluate

4ebcd14

schlosna mentioned this pull request Sep 15, 2022

Enforce NullAway for Guava packages palantir/tritium#1570

Merged

schlosna self-requested a review September 15, 2022 13:43

carterkozak commented Sep 15, 2022

View reviewed changes

svc-excavator-bot mentioned this pull request Sep 15, 2022

Excavator: Upgrades Baseline to the latest version palantir/witchcraft-api#571

Merged

schlosna mentioned this pull request Sep 19, 2022

Remove dependency on net.ltgt.gradle:gradle-errorprone-plugin #2394

Open

svc-excavator-bot mentioned this pull request Sep 30, 2022

Excavator: Upgrades Baseline to the latest version palantir/hadoop-crypto#593

Merged

svc-excavator-bot mentioned this pull request Oct 24, 2022

Excavator: Upgrades Baseline to the latest version palantir/docker-proxy-rule#496

Merged

svc-excavator-bot mentioned this pull request Nov 25, 2022

Excavator: Upgrades Baseline to the latest version palantir/gradle-launch-config-plugin#72

Open

svc-excavator-bot mentioned this pull request Mar 23, 2023

Excavator: Upgrades Baseline to the latest version palantir/docker-compose-rule#685

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a simple nullaway wrapper plugin #2382

Implement a simple nullaway wrapper plugin #2382

carterkozak commented Sep 14, 2022

changelog-app bot commented Sep 14, 2022 •

edited by carterkozak

Loading

carterkozak Sep 14, 2022

carterkozak Sep 14, 2022

CRogers Sep 15, 2022

carterkozak Sep 15, 2022

schlosna left a comment

schlosna Sep 15, 2022

carterkozak Sep 15, 2022

schlosna Sep 15, 2022

carterkozak Sep 15, 2022

schlosna Sep 15, 2022

schlosna Sep 15, 2022

carterkozak Sep 15, 2022

schlosna Sep 15, 2022

schlosna Sep 15, 2022

carterkozak Sep 15, 2022

schlosna Sep 15, 2022

CRogers Sep 15, 2022 •

edited

Loading

carterkozak Sep 15, 2022

brahms Sep 16, 2022

carterkozak Sep 16, 2022 •

edited

Loading

carterkozak Sep 15, 2022

	tasks.named('processResources').configure {
	duplicatesStrategy 'include'
	}

	org.checkerframework:dataflow-nullaway = 3.25.0
	org.checkerframework:dataflow-nullaway = 3.25.0

		@@ -15,6 +15,8 @@ com.googlecode.java-diff-utils:diffutils = 1.3.0
		com.puppycrawl.tools:checkstyle = 10.3.2
		org.checkerframework:* = 3.23.0

Implement a simple nullaway wrapper plugin #2382

Implement a simple nullaway wrapper plugin #2382

Conversation

carterkozak commented Sep 14, 2022

changelog-app bot commented Sep 14, 2022 • edited by carterkozak Loading

Generate changelog in changelog/@unreleased

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

schlosna 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

CRogers Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changelog-app bot commented Sep 14, 2022 •

edited by carterkozak

Loading

Generate changelog in `changelog/@unreleased`

CRogers Sep 15, 2022 •

edited

Loading

carterkozak Sep 16, 2022 •

edited

Loading