From aa988ef2b055b3ee56bfd00f7d9bed87d0ea1b9c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 2 Apr 2025 14:23:03 +0200 Subject: [PATCH] Run NullAway in JSpecify mode (#1608) --- .../BugPatternTestExtractor.java | 6 ++- .../errorprone/bugpatterns/DirectReturn.java | 6 ++- .../bugpatterns/JUnitValueSource.java | 10 +++- .../errorprone/bugpatterns/NonEmptyMono.java | 5 +- .../refasterrules/ReactorRules.java | 46 ++++++++++++++----- .../refasterrules/RxJava2AdapterRules.java | 6 ++- .../bugpatterns/MethodReferenceUsage.java | 6 ++- .../utils/AnnotationAttributeMatcher.java | 14 ++++-- pom.xml | 1 + 9 files changed, 76 insertions(+), 24 deletions(-) diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java index 467a664f..c5dafdcf 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java @@ -2,6 +2,7 @@ package tech.picnic.errorprone.documentation; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static java.util.Objects.requireNonNull; import static java.util.function.Predicate.not; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; @@ -167,7 +168,10 @@ public final class BugPatternTestExtractor implements Extractor inputCode = getSourceCode(inputTree); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java index 089e38bc..7b7a0931 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -11,6 +11,7 @@ import static com.google.errorprone.matchers.Matchers.not; import static com.google.errorprone.matchers.Matchers.returnStatement; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; +import static java.util.Objects.requireNonNull; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -79,7 +80,10 @@ public final class DirectReturn extends BugChecker implements BlockTreeMatcher { return Description.NO_MATCH; } - Symbol variableSymbol = ASTHelpers.getSymbol(((ReturnTree) finalStatement).getExpression()); + Symbol variableSymbol = + requireNonNull( + ASTHelpers.getSymbol(((ReturnTree) finalStatement).getExpression()), + "Missing symbol for returned variable"); StatementTree precedingStatement = statements.get(statements.size() - 2); return tryMatchAssignment(variableSymbol, precedingStatement) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java index 5a873263..cfea5b2e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -17,6 +17,7 @@ import static com.google.errorprone.matchers.Matchers.isSameType; import static com.google.errorprone.matchers.Matchers.methodHasParameters; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; +import static java.util.Objects.requireNonNull; import static java.util.function.Predicate.not; import static java.util.stream.Collectors.joining; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; @@ -129,7 +130,10 @@ public final class JUnitValueSource extends BugChecker implements MethodTreeMatc return Description.NO_MATCH; } - Type parameterType = ASTHelpers.getType(Iterables.getOnlyElement(tree.getParameters())); + Type parameterType = + requireNonNull( + ASTHelpers.getType(Iterables.getOnlyElement(tree.getParameters())), + "Missing type for method parameter"); return findMethodSourceAnnotation(tree, state) .flatMap( @@ -173,7 +177,9 @@ public final class JUnitValueSource extends BugChecker implements MethodTreeMatc private static Optional findMatchingSibling( MethodTree tree, Predicate predicate, VisitorState state) { - return state.findEnclosing(ClassTree.class).getMembers().stream() + return requireNonNull(state.findEnclosing(ClassTree.class), "No class enclosing method") + .getMembers() + .stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) .filter(not(tree::equals)) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java index 5ec869c8..53b1caf0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonEmptyMono.java @@ -5,6 +5,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static java.util.Objects.requireNonNull; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -86,7 +87,9 @@ public final class NonEmptyMono extends BugChecker implements MethodInvocationTr return Description.NO_MATCH; } - ExpressionTree receiver = ASTHelpers.getReceiver(tree); + ExpressionTree receiver = + requireNonNull( + ASTHelpers.getReceiver(tree), "Instance method invocation must have receiver"); if (!NON_EMPTY_MONO.matches(receiver, state)) { return Description.NO_MATCH; } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 12c4f39d..955d3753 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -515,8 +515,14 @@ final class ReactorRules { mono.switchIfEmpty(Mono.empty()), mono.flux().next(), mono.flux().singleOrEmpty()); } + // XXX: Consider filing a SonarCloud issue for the S2637 false positive. @BeforeTemplate - Mono<@Nullable Void> before2(Mono<@Nullable Void> mono) { + @SuppressWarnings({ + "java:S2637" /* False positive: result is never `null`. */, + "java:S4968" /* Result may be `Mono`. */, + "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict" + }) + Mono before2(Mono<@Nullable Void> mono) { return Refaster.anyOf(mono.ignoreElement(), mono.then()); } @@ -945,7 +951,8 @@ final class ReactorRules { /** Prefer direct invocation of {@link Mono#then()}} over more contrived alternatives. */ static final class MonoThen { @BeforeTemplate - Mono<@Nullable Void> before(Mono mono) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before(Mono mono) { return Refaster.anyOf( mono.ignoreElement().then(), mono.flux().then(), @@ -954,7 +961,8 @@ final class ReactorRules { } @AfterTemplate - Mono<@Nullable Void> after(Mono mono) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono after(Mono mono) { return mono.then(); } } @@ -962,17 +970,25 @@ final class ReactorRules { /** Avoid vacuous invocations of {@link Flux#ignoreElements()}. */ static final class FluxThen { @BeforeTemplate - Mono<@Nullable Void> before(Flux flux) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before(Flux flux) { return flux.ignoreElements().then(); } + // XXX: Consider filing a SonarCloud issue for the S2637 false positive. @BeforeTemplate - Mono<@Nullable Void> before2(Flux<@Nullable Void> flux) { + @SuppressWarnings({ + "java:S2637" /* False positive: result is never `null`. */, + "java:S4968" /* Result may be `Mono`. */, + "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict" + }) + Mono before2(Flux<@Nullable Void> flux) { return flux.ignoreElements(); } @AfterTemplate - Mono<@Nullable Void> after(Flux flux) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono after(Flux flux) { return flux.then(); } } @@ -980,12 +996,14 @@ final class ReactorRules { /** Avoid vacuous invocations of {@link Mono#ignoreElement()}. */ static final class MonoThenEmpty { @BeforeTemplate - Mono<@Nullable Void> before(Mono mono, Publisher<@Nullable Void> publisher) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before(Mono mono, Publisher<@Nullable Void> publisher) { return mono.ignoreElement().thenEmpty(publisher); } @AfterTemplate - Mono<@Nullable Void> after(Mono mono, Publisher<@Nullable Void> publisher) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono after(Mono mono, Publisher<@Nullable Void> publisher) { return mono.thenEmpty(publisher); } } @@ -993,12 +1011,14 @@ final class ReactorRules { /** Avoid vacuous invocations of {@link Flux#ignoreElements()}. */ static final class FluxThenEmpty { @BeforeTemplate - Mono<@Nullable Void> before(Flux flux, Publisher<@Nullable Void> publisher) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before(Flux flux, Publisher<@Nullable Void> publisher) { return flux.ignoreElements().thenEmpty(publisher); } @AfterTemplate - Mono<@Nullable Void> after(Flux flux, Publisher<@Nullable Void> publisher) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono after(Flux flux, Publisher<@Nullable Void> publisher) { return flux.thenEmpty(publisher); } } @@ -1054,7 +1074,8 @@ final class ReactorRules { } @BeforeTemplate - Mono<@Nullable Void> before2(Mono mono1, Mono<@Nullable Void> mono2) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before2(Mono mono1, Mono<@Nullable Void> mono2) { return mono1.thenEmpty(mono2); } @@ -1072,7 +1093,8 @@ final class ReactorRules { } @BeforeTemplate - Mono<@Nullable Void> before2(Flux flux, Mono<@Nullable Void> mono) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before2(Flux flux, Mono<@Nullable Void> mono) { return flux.thenEmpty(mono); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java index c2d2724d..22e01825 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RxJava2AdapterRules.java @@ -23,14 +23,16 @@ final class RxJava2AdapterRules { /** Use the fluent API style when using {@link RxJava2Adapter#completableToMono}. */ static final class CompletableToMono { @BeforeTemplate - Mono<@Nullable Void> before(Completable completable) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono before(Completable completable) { return Refaster.anyOf( RxJava2Adapter.completableToMono(completable), completable.to(RxJava2Adapter::completableToMono)); } @AfterTemplate - Mono<@Nullable Void> after(Completable completable) { + @SuppressWarnings("java:S4968" /* Result may be `Mono`. */) + Mono after(Completable completable) { return completable.as(RxJava2Adapter::completableToMono); } } diff --git a/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/MethodReferenceUsage.java b/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/MethodReferenceUsage.java index 1bea7ef9..b108c78c 100644 --- a/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/MethodReferenceUsage.java +++ b/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/MethodReferenceUsage.java @@ -29,6 +29,7 @@ import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.PackageSymbol; import com.sun.tools.javac.code.Type; import java.util.List; import java.util.Optional; @@ -126,7 +127,7 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr return Optional.empty(); } - Symbol sym = ASTHelpers.getSymbol(methodSelect); + Symbol sym = ASTHelpers.getSymbol(subTree); return ASTHelpers.isStatic(sym) ? constructFix(lambdaExpr, sym.owner, methodSelect) : constructFix(lambdaExpr, "this", methodSelect); @@ -200,7 +201,8 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr Name sName = target.getSimpleName(); Optional fix = constructFix(lambdaExpr, sName, methodName); - if (!"java.lang".equals(ASTHelpers.enclosingPackage(target).toString())) { + PackageSymbol pkg = ASTHelpers.enclosingPackage(target); + if (pkg != null && !"java.lang".equals(pkg.toString())) { Name fqName = target.getQualifiedName(); if (!sName.equals(fqName)) { return fix.map(b -> b.addImport(fqName.toString())); diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/AnnotationAttributeMatcher.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/AnnotationAttributeMatcher.java index 4a95fe38..11e905e7 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/AnnotationAttributeMatcher.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/AnnotationAttributeMatcher.java @@ -1,5 +1,7 @@ package tech.picnic.errorprone.utils; +import static java.util.Objects.requireNonNull; + import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableSet; @@ -115,9 +117,15 @@ public final class AnnotationAttributeMatcher implements Serializable { } private static String extractAttributeName(ExpressionTree expr) { - return (expr instanceof AssignmentTree assignment) - ? ASTHelpers.getSymbol(assignment.getVariable()).getSimpleName().toString() - : "value"; + if (!(expr instanceof AssignmentTree assignment)) { + return "value"; + } + + return requireNonNull( + ASTHelpers.getSymbol(assignment.getVariable()), + "Missing symbol for annotation attribute") + .getSimpleName() + .toString(); } // XXX: The caller of this method can be implemented more efficiently in case of a "wholeTypes" diff --git a/pom.xml b/pom.xml index 6e3aa345..e64f6791 100644 --- a/pom.xml +++ b/pom.xml @@ -1940,6 +1940,7 @@ -XepOpt:NullAway:AnnotatedPackages=tech.picnic -XepOpt:NullAway:AssertsEnabled=true -XepOpt:NullAway:CheckOptionalEmptiness=true + -XepOpt:NullAway:JSpecifyMode=true -XepOpt:Nullness:Conservative=false -XepOpt:StatementSwitchToExpressionSwitch:EnableDirectConversion=true