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 all 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.

}
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-2382.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: improvement
improvement:
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.
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,71 @@
/*
* (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 com.google.common.collect.ImmutableSet;
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);

/** We may add a gradle extension in a future release allowing custom additional packages. */
private static final ImmutableSet<String> DEFAULT_ANNOTATED_PACKAGES = ImmutableSet.of("com.palantir");

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

Choose a reason for hiding this comment

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

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

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", String.join(",", DEFAULT_ANNOTATED_PACKAGES));
options.option("NullAway:CheckOptionalEmptiness", "true");
}
});
}

private static void configureErrorProneOptions(Project project, Action<ErrorProneOptions> action) {
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
8 changes: 5 additions & 3 deletions 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 @@ -68,8 +69,9 @@ org.apache.maven.resolver:maven-resolver-util:1.6.3 (3 constraints: 5930fd65)
org.apache.maven.shared:maven-dependency-analyzer:1.12.0 (1 constraints: 36052f3b)
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:checker-qual:3.25.0 (3 constraints: 08256088)
org.checkerframework:dataflow-errorprone:3.25.0 (4 constraints: fc3da85d)
org.checkerframework:dataflow-nullaway:3.25.0 (2 constraints: 6911b8f1)
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
3 changes: 2 additions & 1 deletion versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@ com.google.auto.service:auto-service = 1.0.1
com.google.errorprone:error_prone_* = 2.15.0
com.google.guava:guava = 31.1-jre
com.palantir.safe-logging:* = 3.0.0
com.uber.nullaway:nullaway = 0.10.1
commons-lang:commons-lang = 2.6
org.apache.maven.shared:maven-dependency-analyzer = 1.12.0
org.apache.maven:maven-core = 3.8.5
org.checkerframework:* = 3.25.0
org.inferred:freebuilder = 1.14.6
org.jooq:jooq = 3.17.2
org.slf4j:* = 1.7.36
org.immutables:* = 2.8.8
org.ow2.asm:asm = 9.3
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

# test deps
Expand Down