Implement a simple nullaway wrapper plugin #2382
There are no files selected for viewing
| 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. |
| 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 | ||
| 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' | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 |
||
| } | ||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /* | ||
| * (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); | ||
|
|
||
| private static final ImmutableSet<String> DEFAULT_ANNOTATED_PACKAGES = ImmutableSet.of( | ||
| "com.palantir", | ||
| // guava | ||
| "com.google.common"); | ||
|
|
||
| @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"; | ||
|
There was a problem hiding this comment. When does this ever happen? How is nullaway applied? There was a problem hiding this comment. 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 There was a problem hiding this comment. Do most jars like google's own error prone core jar have this manifest usually, or is this something we publish? There was a problem hiding this comment. No, this is used so that we can publishToMavenLocal prior to running tests, then resolve our own checks from the |
||
| }); | ||
| 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 proj, Action<ErrorProneOptions> action) { | ||
| proj.afterEvaluate(new Action<Project>() { | ||
|
There was a problem hiding this comment. 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+) There was a problem hiding this comment. Does errorprone also use There was a problem hiding this comment. 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 | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. I think we have, but the two definition locations are merged, and likely out of sync. This is allowed due to gradle-baseline/gradle-baseline-java/build.gradle Lines 129 to 131 in 2399039 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.