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

Update to JSpecify 1.0 and use JSpecify annotations in NullAway code #1000

Merged
merged 9 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ dependencies {
// Optional, some source of nullability annotations.
// Not required on Android if you use the support
// library nullability annotations.
compileOnly "com.google.code.findbugs:jsr305:3.0.2"
api "org.jspecify:jspecify:1.0.0"

errorprone "com.google.errorprone:error_prone_core:<Error Prone version>"
}
Expand All @@ -48,7 +48,7 @@ tasks.withType(JavaCompile) {

Let's walk through this script step by step. The `plugins` section pulls in the [Gradle Error Prone plugin](https://github.com/tbroyer/gradle-errorprone-plugin) for Error Prone integration.

In `dependencies`, the first `errorprone` line loads NullAway, and the `compileOnly` line loads a [JSR 305](https://jcp.org/en/jsr/detail?id=305) library which provides a suitable `@Nullable` annotation (`javax.annotation.Nullable`). NullAway allows for any `@Nullable` annotation to be used, so, e.g., `@Nullable` from the Android Support Library or JetBrains annotations is also fine. The second `errorprone` line sets the version of Error Prone is used.
In `dependencies`, the first `errorprone` line loads NullAway, and the `api` line loads the [JSpecify](https://jspecify.dev) library which provides suitable nullability annotations, e.g., `org.jspecify.annotations.Nullable`. NullAway allows for any `@Nullable` annotation to be used, so, e.g., `@Nullable` from the Android Support Library or JetBrains annotations is also fine. The second `errorprone` line sets the version of Error Prone is used.

Finally, in the `tasks.withType(JavaCompile)` section, we pass some configuration options to NullAway. First `check("NullAway", CheckSeverity.ERROR)` sets NullAway issues to the error level (it's equivalent to the `-Xep:NullAway:ERROR` standard Error Prone argument); by default NullAway emits warnings. Then, `option("NullAway:AnnotatedPackages", "com.uber")` (equivalent to the `-XepOpt:NullAway:AnnotatedPackages=com.uber` standard Error Prone argument) tells NullAway that source code in packages under the `com.uber` namespace should be checked for null dereferences and proper usage of `@Nullable` annotations, and that class files in these packages should be assumed to have correct usage of `@Nullable` (see [the docs](https://github.com/uber/NullAway/wiki/Configuration) for more detail). NullAway requires at least the `AnnotatedPackages` configuration argument to run, in order to distinguish between annotated and unannotated code. See [the configuration docs](https://github.com/uber/NullAway/wiki/Configuration) for other useful configuration options. For even simpler configuration of NullAway options, use the [Gradle NullAway plugin](https://github.com/tbroyer/gradle-nullaway-plugin).

Expand All @@ -58,7 +58,7 @@ We recommend addressing all the issues that Error Prone reports, particularly th

Versions 3.0.0 and later of the Gradle Error Prone Plugin [no longer support Android](https://github.com/tbroyer/gradle-errorprone-plugin/releases/tag/v3.0.0). So if you're using a recent version of this plugin, you'll need to add some further configuration to run Error Prone and NullAway. Our [sample app `build.gradle` file](https://github.com/uber/NullAway/blob/master/sample-app/build.gradle) shows one way to do this, but your Android project may require tweaks. Alternately, 2.x versions of the Gradle Error Prone Plugin still support Android and may still work with your project.

Beyond that, compared to the Java configuration, the `com.google.code.findbugs:jsr305:3.0.2` dependency can be removed; you can use the `android.support.annotation.Nullable` annotation from the Android Support library instead.
Beyond that, compared to the Java configuration, the JSpecify dependency can be removed; you can use the `android.support.annotation.Nullable` annotation from the Android Support library instead.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cpovirk are we currently encouraging Android users to switch over from Android support annotations to JSpecify as well? Or will that cause problems for them?

Copy link

@cpovirk cpovirk Jul 19, 2024

Choose a reason for hiding this comment

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

Phrasing is difficult :) We have had similar conversations internally.

On the one hand, it would be nice if we could find a way to avoid implying that the Android annotations (which of course are declaration annotations) are "just as good" as the JSpecify ones.

On the other hand, it would be nice to make clear there there are still tradeoffs, by which I mainly mean "Dagger (if you don't use a recent javac)." (On the plus side, maybe Android users can more upgrade their compiler especially easily, since they don't need to worry about setting --release? Part of me now wants for the NullAway docs to demonstrate configuring a Java 22 toolchain for Android, except that it's not really fair to put that on the critical path for NullAway adopters :))

I was hoping to convince myself that we had an existing doc that covered this, but as you saw, I ended up leaving a comment on a JSpecify issue instead.

The best link at this point is probably https://jspecify.dev/docs/whether/#annotation-processors. Let me see if I can think of a way that I'd phrase that once I get back to my desk after a quick break. I'm hoping to propose a different framing for this sentence.

Copy link

Choose a reason for hiding this comment

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

On second thought: I might be putting too much emphasis on the ability to annotate type arguments, type-parameter bounds, and array-element types yet, since NullAway support there is still developing. (And of course NullAway already offers a way to treat unannotated types as non-nullable when you want it, so there's less reason for @NullMarked, too.)

I think I'd probably just keep this as you have it for now, and we can revisit once we have docs that speak more directly to the tradeoffs. Maybe we'll also tweak the comment in the code block at line 29 at that point.


Hmm, but now that I'm here: Nowadays, people use androidx.annotation.Nullable instead of android.support.annotation.Nullable, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will switch to refer to androidx, thanks!


#### Annotation Processors / Generated Code

Expand Down
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ subprojects { project ->
check("UnusedException", CheckSeverity.ERROR)
check("UnnecessaryFinal", CheckSeverity.ERROR)
check("PreferredInterfaceType", CheckSeverity.ERROR)
check("AnnotationPosition", CheckSeverity.ERROR)
// To enable auto-patching, uncomment the line below, replace [CheckerName] with
// the checker(s) you want to apply patches for (comma-separated), and above, disable
// "-Werror"
Expand Down
6 changes: 3 additions & 3 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ def build = [
guava : "com.google.guava:guava:30.1-jre",
javaparser : "com.github.javaparser:javaparser-core:3.26.0",
javaxValidation : "javax.validation:validation-api:2.0.1.Final",
jspecify : "org.jspecify:jspecify:0.3.0",
jsr305Annotations : "com.google.code.findbugs:jsr305:3.0.2",
jspecify : "org.jspecify:jspecify:1.0.0",
commonsIO : "commons-io:commons-io:2.11.0",
wala : [
"com.ibm.wala:com.ibm.wala.util:${versions.wala}",
Expand Down Expand Up @@ -112,7 +111,8 @@ def test = [
rxjava2 : "io.reactivex.rxjava2:rxjava:2.1.2",
commonsLang3 : "org.apache.commons:commons-lang3:3.8.1",
commonsLang : "commons-lang:commons-lang:2.6",
lombok : "org.projectlombok:lombok:1.18.24",
jsr305Annotations : "com.google.code.findbugs:jsr305:3.0.2",
lombok : "org.projectlombok:lombok:1.18.34",
springBeans : "org.springframework:spring-beans:5.3.7",
springContext : "org.springframework:spring-context:5.3.7",
grpcCore : "io.grpc:grpc-core:1.15.1", // Should upgrade, but this matches our guava version
Expand Down
2 changes: 1 addition & 1 deletion guava-recent-unit-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ dependencies {
testImplementation(deps.build.errorProneTestHelpers) {
exclude group: "junit", module: "junit"
}
testImplementation deps.build.jsr305Annotations
testImplementation deps.test.jsr305Annotations
testImplementation "com.google.guava:guava:31.1-jre"

errorProneOldest deps.build.errorProneCheckApiOld
Expand Down
2 changes: 1 addition & 1 deletion jar-infer/test-java-lib-jarinfer/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ dependencies {
compileOnly deps.apt.autoService
annotationProcessor deps.apt.autoService
compileOnly project(":nullaway")
implementation deps.build.jsr305Annotations
api deps.build.jspecify
}

jar.dependsOn ":jar-infer:jar-infer-cli:assemble"
2 changes: 1 addition & 1 deletion jdk-recent-unit-tests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ dependencies {
testImplementation(deps.build.errorProneTestHelpers) {
exclude group: "junit", module: "junit"
}
testImplementation deps.build.jsr305Annotations
testImplementation deps.test.jsr305Annotations
testModulePath deps.test.cfQual
}

Expand Down
1 change: 1 addition & 0 deletions jmh/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ dependencies {
// use the same version of Error Prone Core that we are compiling NullAway against, so we can
// benchmark against different versions of Error Prone
implementation deps.build.errorProneCoreForApi
api deps.build.jspecify


// Source jars for our desired benchmarks
Expand Down
4 changes: 2 additions & 2 deletions jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import javax.annotation.Nullable;
import javax.tools.DiagnosticListener;
import javax.tools.JavaCompiler;
import javax.tools.JavaFileObject;
import javax.tools.SimpleJavaFileObject;
import javax.tools.StandardJavaFileManager;
import javax.tools.ToolProvider;
import org.jspecify.annotations.Nullable;

/**
* Code to run Javac with NullAway enabled, designed to aid benchmarking. Construction of {@code
Expand All @@ -53,7 +53,7 @@ public class NullawayJavac {
//////////////////////
private List<JavaFileObject> compilationUnits;
private JavaCompiler compiler;
@Nullable private DiagnosticListener<JavaFileObject> diagnosticListener;
private @Nullable DiagnosticListener<JavaFileObject> diagnosticListener;
private StandardJavaFileManager fileManager;
private List<String> options;

Expand Down
2 changes: 1 addition & 1 deletion library-model/test-library-model-generator/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ dependencies {
compileOnly deps.apt.autoService
annotationProcessor deps.apt.autoService
compileOnly project(":nullaway")
implementation deps.build.jsr305Annotations
api deps.build.jspecify
}

jar.dependsOn ":library-model:library-model-generator-cli:assemble"
3 changes: 2 additions & 1 deletion nullaway/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ dependencies {
annotationProcessor deps.apt.autoValue
compileOnly deps.apt.autoServiceAnnot
annotationProcessor deps.apt.autoService
compileOnly deps.build.jsr305Annotations
// Using api following the guidance at https://jspecify.dev/docs/using#gradle
api deps.build.jspecify
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cpovirk this is encouraged, correct? This will bring in the JSpecify jar onto the annotation processor path for users, but they will have it there anyway I think if they are using a recent version of Error Prone. I think this could only cause a problem if for some reason they already have a dep on an old version of the JSpecify jar.

Copy link

Choose a reason for hiding this comment

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

I was wondering about Error Prone, too. All I've found there so far is a test dep, but it could have missed a full dep, especially since I didn't look at all at indirect deps. (So someday Error Prone will get it through Guava or Caffeine or whatever.)

I think that including it should be fine. I think you're right that there could in theory be trouble if some other annotation processor wanted the old package, but that seems very unlikely, and things would generally work out OK even in that case.

Copy link

Choose a reason for hiding this comment

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

Error Prone will have the dep in 2.30.0 (or 2.29.3 or whatever the next release after 2.29.2 is):

compileOnly deps.test.jetbrainsAnnotations
compileOnly deps.apt.javaxInject

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import com.uber.nullaway.handlers.Handler;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;
import org.jspecify.annotations.Nullable;

/**
* Provides APIs for querying whether code is annotated for nullness checking, and for related
Expand Down
5 changes: 2 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.sun.tools.javac.code.Symbol;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.Set;
import javax.annotation.Nullable;
import org.jspecify.annotations.Nullable;

/** Provides configuration parameters for the nullability checker. */
public interface Config {
Expand Down Expand Up @@ -238,8 +238,7 @@ public interface Config {
* return an @NonNull copy (likely through an unsafe downcast, but performing runtime checking
* and logging)
*/
@Nullable
String getCastToNonNullMethod();
@Nullable String getCastToNonNullMethod();

/**
* Gets an optional comment to add to auto-fix suppressions.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import com.sun.tools.javac.code.Symbol;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.Set;
import javax.annotation.Nullable;
import org.jspecify.annotations.Nullable;

/**
* Dummy Config class required for the {@link NullAway} empty constructor.
Expand Down Expand Up @@ -175,8 +175,7 @@ public Set<String> getOptionalClassPaths() {
}

@Override
@Nullable
public String getCastToNonNullMethod() {
public @Nullable String getCastToNonNullMethod() {
throw new IllegalStateException(ERROR_MESSAGE);
}

Expand Down
5 changes: 2 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@
import java.util.List;
import java.util.Set;
import java.util.stream.StreamSupport;
import javax.annotation.Nullable;
import javax.lang.model.element.Element;
import javax.lang.model.element.ElementKind;
import javax.tools.JavaFileObject;
import org.jspecify.annotations.Nullable;

/** A class to construct error message to be displayed after the analysis finds error. */
public class ErrorBuilder {
Expand Down Expand Up @@ -318,8 +318,7 @@ Description.Builder addSuppressWarningsFix(
* <p>TODO: actually use {@link
* com.google.errorprone.fixes.SuggestedFixes#addSuppressWarnings(VisitorState, String)} instead
*/
@Nullable
private Tree suppressibleNode(@Nullable TreePath path) {
private @Nullable Tree suppressibleNode(@Nullable TreePath path) {
if (path == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import javax.annotation.Nullable;
import org.jspecify.annotations.Nullable;

/**
* provides nullability configuration based on additional flags passed to ErrorProne via
Expand Down Expand Up @@ -189,12 +189,12 @@ final class ErrorProneCLIFlagsConfig implements Config {
private final Pattern unannotatedSubPackages;

/** Source code in these classes will not be analyzed for nullability issues */
@Nullable private final ImmutableSet<String> sourceClassesToExclude;
private final @Nullable ImmutableSet<String> sourceClassesToExclude;

/**
* these classes will be treated as unannotated (don't analyze *and* treat methods as unannotated)
*/
@Nullable private final ImmutableSet<String> unannotatedClasses;
private final @Nullable ImmutableSet<String> unannotatedClasses;

private final Pattern fieldAnnotPattern;
private final boolean isExhaustiveOverride;
Expand All @@ -214,7 +214,7 @@ final class ErrorProneCLIFlagsConfig implements Config {
private final ImmutableSet<String> initializerAnnotations;
private final ImmutableSet<String> externalInitAnnotations;
private final ImmutableSet<String> contractAnnotations;
@Nullable private final String castToNonNullMethod;
private final @Nullable String castToNonNullMethod;
private final String autofixSuppressionComment;
private final ImmutableSet<String> skippedLibraryModels;
private final ImmutableSet<String> extraFuturesClasses;
Expand Down Expand Up @@ -515,8 +515,7 @@ public boolean assertsEnabled() {
}

@Override
@Nullable
public String getCastToNonNullMethod() {
public @Nullable String getCastToNonNullMethod() {
return castToNonNullMethod;
}

Expand Down
5 changes: 2 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import javax.annotation.Nullable;
import javax.inject.Inject;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.Element;
Expand All @@ -122,6 +121,7 @@
import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode;
import org.checkerframework.nullaway.javacutil.ElementUtils;
import org.checkerframework.nullaway.javacutil.TreeUtils;
import org.jspecify.annotations.Nullable;

/**
* Checker for nullability errors. It assumes that any field, method parameter, or return type that
Expand Down Expand Up @@ -2154,8 +2154,7 @@ private Set<Element> getSafeInitMethods(
* @param state visitor state
* @return element of safe init function if stmt invokes that function; null otherwise
*/
@Nullable
private Element getInvokeOfSafeInitMethod(
private @Nullable Element getInvokeOfSafeInitMethod(
StatementTree stmt, Symbol.ClassSymbol enclosingClassSymbol, VisitorState state) {
Matcher<ExpressionTree> invokeMatcher =
(expressionTree, s) -> {
Expand Down
11 changes: 4 additions & 7 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.AnnotationValue;
import javax.lang.model.element.ExecutableElement;
import org.checkerframework.nullaway.javacutil.AnnotationUtils;
import org.jspecify.annotations.Nullable;

/** Helpful utility methods for nullability analysis. */
public class NullabilityUtil {
Expand Down Expand Up @@ -102,8 +102,7 @@ public static boolean lambdaParamIsImplicitlyTyped(VariableTree lambdaParameter)
* @return closest overridden ancestor method, or <code>null</code> if method does not override
* anything
*/
@Nullable
public static Symbol.MethodSymbol getClosestOverriddenMethod(
public static Symbol.@Nullable MethodSymbol getClosestOverriddenMethod(
Symbol.MethodSymbol method, Types types) {
// taken from Error Prone MethodOverrides check
Symbol.ClassSymbol owner = method.enclClass();
Expand Down Expand Up @@ -135,8 +134,7 @@ public static Symbol.MethodSymbol getClosestOverriddenMethod(
* @param others also stop and return in case of any of these tree kinds
* @return the closest enclosing method / lambda
*/
@Nullable
public static TreePath findEnclosingMethodOrLambdaOrInitializer(
public static @Nullable TreePath findEnclosingMethodOrLambdaOrInitializer(
TreePath path, ImmutableSet<Tree.Kind> others) {
TreePath curPath = path.getParentPath();
while (curPath != null) {
Expand Down Expand Up @@ -169,8 +167,7 @@ public static TreePath findEnclosingMethodOrLambdaOrInitializer(
* @param path the tree path
* @return the closest enclosing method / lambda
*/
@Nullable
public static TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) {
public static @Nullable TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) {
return findEnclosingMethodOrLambdaOrInitializer(path, ImmutableSet.of());
}

Expand Down
Loading