Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a simple nullaway wrapper plugin #2382

Merged
merged 13 commits into from
Sep 15, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions baseline-null-away/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This module acts as a thin shim around NullAway, allowing us to manage the NullAway dependency via baseline.
10 changes: 10 additions & 0 deletions baseline-null-away/baseline-class-uniqueness.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Danger! Multiple jars contain identically named classes. This may cause different behaviour depending on classpath ordering.
# Run ./gradlew checkClassUniqueness --write-locks to update this file

## runtimeClasspath
[org.checkerframework:checker-qual, org.checkerframework:dataflow-nullaway]
- org.checkerframework.dataflow.qual.Deterministic
- org.checkerframework.dataflow.qual.Pure
- org.checkerframework.dataflow.qual.Pure$Kind
- org.checkerframework.dataflow.qual.SideEffectFree
- org.checkerframework.dataflow.qual.TerminatesExecution
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

7 changes: 7 additions & 0 deletions baseline-null-away/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
apply plugin: 'java-library'
apply plugin: 'com.palantir.external-publish-jar'

dependencies {
runtimeOnly 'com.uber.nullaway:nullaway'
runtimeOnly 'org.checkerframework:dataflow-nullaway'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to confirm as my gradle-fu is not as knowledgeable here and I haven't dug into the classfiles produced -- this only puts these on the baseline-null-away gradle plugin runtime classpath when the plugin is applied and does not trigger a transitive runtime dependency to any modules that apply the plugin itself (i.e. we won't produce any class files that assume dataflow-nullaway will be on a service's runtime classpath), is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, although nothing depends directly on this module. We produce this library so that we can control the version of nullaway via the gradle-baseline versions.props and standard excavation, and add the baseline-null-away dependency to the errorprone checks dependency configuration using the current baseline version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I attempted to avoid a new module by checking the package implementationVersion of a nullaway class, however that resulted in a larger plugin classpath (not a huge issue) and unfortunately the nullaway jars don't include an Implementaiton-Version in their jar manifest, so I couldn't query the version directly. Ultimately I think the solution in this PR is cleaner and more maintainable.

}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2382.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Implement a simple nullaway wrapper plugin `com.palantir.baseline-null-away`
links:
- https://github.com/palantir/gradle-baseline/pull/2382
1 change: 1 addition & 0 deletions gradle-baseline-java/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ dependencies {

tasks.test.dependsOn tasks.findByPath(':gradle-baseline-java-config:publishToMavenLocal')
tasks.test.dependsOn tasks.findByPath(':baseline-error-prone:publishToMavenLocal')
tasks.test.dependsOn tasks.findByPath(':baseline-null-away:publishToMavenLocal')
tasks.test.dependsOn tasks.findByPath(':baseline-refaster-javac-plugin:publishToMavenLocal')
tasks.test.dependsOn tasks.findByPath(':baseline-refaster-rules:publishToMavenLocal')
tasks.test.dependsOn tasks.publishToMavenLocal
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline.plugins;

import java.util.Optional;
import net.ltgt.gradle.errorprone.ErrorProneOptions;
import org.gradle.api.Action;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.logging.Logger;
import org.gradle.api.logging.Logging;
import org.gradle.api.plugins.ExtensionAware;
import org.gradle.api.tasks.compile.JavaCompile;

public final class BaselineNullAway implements Plugin<Project> {

private static final Logger log = Logging.getLogger(BaselineNullAway.class);

// This nullaway dependency in our plugin allows dependency upgrades on the baseline
// project to ensure nullaway remains up to date for all consumers.
private static final String NULLAWAY_VERSION = "0.10.1";

carterkozak marked this conversation as resolved.
Show resolved Hide resolved
@Override
public void apply(Project project) {
project.getPluginManager().withPlugin("com.palantir.baseline-error-prone", _unused0 -> {
project.getPluginManager().withPlugin("java", _unused1 -> {
applyToProject(project);
});
});
}

private void applyToProject(Project project) {
String version = Optional.ofNullable(BaselineNullAway.class.getPackage().getImplementationVersion())
.orElseGet(() -> {
log.warn("BaselineNullAway is using 'latest.release' - "
+ "beware this compromises build reproducibility");
return "latest.release";
Copy link
Contributor

@CRogers CRogers Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this ever happen? How is nullaway applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@carterkozak carterkozak Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
project.getDependencies().add("errorprone", "com.palantir.baseline:baseline-null-away:" + version);
configureErrorProneOptions(project, new Action<ErrorProneOptions>() {
@Override
public void execute(ErrorProneOptions options) {
options.option("NullAway:AnnotatedPackages", "com.palantir");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

options.option("NullAway:CheckOptionalEmptiness", "true");
}
});
}

private static void configureErrorProneOptions(Project proj, Action<ErrorProneOptions> action) {
proj.afterEvaluate(new Action<Project>() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Override
public void execute(Project project) {
project.getTasks().withType(JavaCompile.class).configureEach(new Action<JavaCompile>() {
@Override
public void execute(JavaCompile javaCompile) {
((ExtensionAware) javaCompile.getOptions())
.getExtensions()
.configure(ErrorProneOptions.class, action);
}
});
}
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#
# Copyright 2022 Palantir Technologies, Inc. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
implementation-class=com.palantir.baseline.plugins.BaselineNullAway
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline


import org.gradle.testkit.runner.BuildResult
import org.gradle.testkit.runner.TaskOutcome
import spock.lang.Unroll

class BaselineNullAwayIntegrationTest extends AbstractPluginTest {

def standardBuildFile = '''
plugins {
id 'java'
id 'com.palantir.baseline-error-prone'
id 'com.palantir.baseline-null-away'
id 'com.palantir.baseline-java-versions'
}
repositories {
mavenLocal()
mavenCentral()
}
javaVersions {
libraryTarget = 17
}
tasks.withType(JavaCompile).configureEach {
options.compilerArgs += ['-Werror']
}
'''.stripIndent()

def validJavaFile = '''
package com.palantir.test;
public class Test { void test() {} }
'''.stripIndent()

def invalidJavaFile = '''
package com.palantir.test;
public class Test {
int test(Throwable throwable) {
// uh-oh, getMessage may be null!
return throwable.getMessage().hashCode();
}
}
'''.stripIndent()

def 'Can apply plugin'() {
when:
buildFile << standardBuildFile

then:
with('compileJava', '--info').build()
}

def 'compileJava fails when null-away finds errors'() {
when:
buildFile << standardBuildFile
file('src/main/java/com/palantir/test/Test.java') << invalidJavaFile

then:
BuildResult result = with('compileJava').buildAndFail()
result.task(":compileJava").outcome == TaskOutcome.FAILED
result.output.contains("[NullAway] dereferenced expression throwable.getMessage() is @Nullable")
}

def 'compileJava succeeds when null-away finds no errors'() {
when:
buildFile << standardBuildFile
file('src/main/java/com/palantir/test/Test.java') << validJavaFile

then:
BuildResult result = with('compileJava').build()
result.task(":compileJava").outcome == TaskOutcome.SUCCESS
}
}
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
rootProject.name = "gradle-baseline"

include "baseline-error-prone"
include "baseline-null-away"
include "baseline-refaster-javac-plugin"
include "baseline-refaster-rules"
include "baseline-refaster-testing"
Expand Down
4 changes: 3 additions & 1 deletion versions.lock
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ com.google.errorprone:error_prone_test_helpers:2.15.0 (1 constraints: 3a053e3b)
com.google.errorprone:error_prone_type_annotations:2.15.0 (1 constraints: 251151c9)
com.google.googlejavaformat:google-java-format:1.13.0 (1 constraints: 8b149d75)
com.google.guava:failureaccess:1.0.1 (1 constraints: 140ae1b4)
com.google.guava:guava:31.1-jre (13 constraints: 3cdc2b02)
com.google.guava:guava:31.1-jre (14 constraints: d6e9749b)
com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava (1 constraints: bd17c918)
com.google.inject:guice:4.2.2 (1 constraints: e40b61f3)
com.google.j2objc:j2objc-annotations:1.3 (1 constraints: b809eda0)
Expand All @@ -40,6 +40,7 @@ com.palantir.javaformat:palantir-java-format-spi:1.1.0 (1 constraints: 711560be)
com.palantir.safe-logging:preconditions:3.0.0 (6 constraints: 76574831)
com.palantir.safe-logging:safe-logging:3.0.0 (8 constraints: 8777131f)
com.palantir.tritium:tritium-registry:0.50.0 (1 constraints: 3705333b)
com.uber.nullaway:nullaway:0.10.1 (1 constraints: 3405243b)
commons-io:commons-io:2.11.0 (2 constraints: 1426005d)
commons-lang:commons-lang:2.6 (1 constraints: ac04232c)
io.dropwizard.metrics:metrics-core:4.1.1 (1 constraints: 901088a5)
Expand Down Expand Up @@ -70,6 +71,7 @@ org.apache.maven.shared:maven-shared-utils:3.3.4 (1 constraints: e60b61f3)
org.assertj:assertj-core:3.23.1 (3 constraints: e42af49e)
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

org.codehaus.groovy:groovy:3.0.10 (3 constraints: e32879d6)
org.codehaus.groovy:groovy-xml:3.0.10 (1 constraints: 791161da)
org.codehaus.plexus:plexus-cipher:2.0 (1 constraints: 641174c7)
Expand Down
2 changes: 2 additions & 0 deletions versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

com.palantir.gradle.utils:* = 0.1.0
com.uber.nullaway:nullaway = 0.10.1
org.checkerframework:dataflow-nullaway = 3.25.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# test deps
com.fasterxml.jackson.*:* = 2.13.3
Expand Down