From 9ee04c2fcee08db1ff715c1b6230ac54aadde44a Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 11 Aug 2023 12:22:34 -0700 Subject: [PATCH] Update to Error Prone 2.21.1 (#797) Mostly straightforward. Error Prone has a new [NullableOptional](https://errorprone.info/bugpattern/NullableOptional) check that warned on a couple instances in our code base. I changed these cases to use an `@Initializer` method which I think better captures what is going on. Will update the required CI checks before landing. --- .github/workflows/continuous-integration.yml | 10 +++++----- gradle/dependencies.gradle | 2 +- .../com/uber/nullaway/ASTHelpersBackports.java | 1 + .../handlers/ApacheThriftIsSetHandler.java | 10 +++++++--- .../com/uber/nullaway/handlers/GrpcHandler.java | 15 ++++++++------- 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index a7d278d86e..185735733e 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -21,16 +21,16 @@ jobs: epVersion: 2.4.0 - os: macos-latest java: 11 - epVersion: 2.20.0 + epVersion: 2.21.1 - os: ubuntu-latest java: 11 - epVersion: 2.20.0 + epVersion: 2.21.1 - os: windows-latest java: 11 - epVersion: 2.20.0 + epVersion: 2.21.1 - os: ubuntu-latest java: 17 - epVersion: 2.20.0 + epVersion: 2.21.1 fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -61,7 +61,7 @@ jobs: with: arguments: coveralls continue-on-error: true - if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.20.0' && github.repository == 'uber/NullAway' + if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.21.1' && github.repository == 'uber/NullAway' - name: Test publishToMavenLocal flow env: ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }} diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index a28c03123a..481be0dc74 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -19,7 +19,7 @@ import org.gradle.util.VersionNumber // The oldest version of Error Prone that we support running on def oldestErrorProneVersion = "2.4.0" // Latest released Error Prone version that we've tested with -def latestErrorProneVersion = "2.20.0" +def latestErrorProneVersion = "2.21.1" // Default to using latest tested Error Prone version def defaultErrorProneVersion = latestErrorProneVersion def errorProneVersionToCompileAgainst = defaultErrorProneVersion diff --git a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java index 32baa12c9b..06a91f99c1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java +++ b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java @@ -32,6 +32,7 @@ public static boolean isStatic(Symbol symbol) { * https://github.com/google/error-prone/blame/a1318e4b0da4347dff7508108835d77c470a7198/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java#L1148 * TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.20.0 */ + @SuppressWarnings("ASTHelpersSuggestions") public static List getEnclosedElements(Symbol symbol) { return symbol.getEnclosedElements(); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java index f477c801d5..802cedd447 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java @@ -23,7 +23,6 @@ import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements; -import com.google.common.base.Preconditions; import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; @@ -33,6 +32,7 @@ import com.sun.tools.javac.code.Types; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; +import com.uber.nullaway.annotations.Initializer; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.Objects; @@ -54,8 +54,13 @@ public class ApacheThriftIsSetHandler extends BaseNoOpHandler { private static final Supplier TBASE_TYPE_SUPPLIER = Suppliers.typeFromString(TBASE_NAME); - @Nullable private Optional tbaseType; + private Optional tbaseType; + /** + * This method is annotated {@code @Initializer} since it will be invoked when the first class is + * processed, before any other handler methods + */ + @Initializer @Override public void onMatchTopLevelClass( NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { @@ -173,7 +178,6 @@ private static String decapitalize(String str) { } private boolean thriftIsSetCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(tbaseType); // noinspection ConstantConditions return tbaseType.isPresent() && symbol.getSimpleName().toString().startsWith("isSet") diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java index 38e26c53f0..82b436fc46 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java @@ -24,7 +24,6 @@ import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements; import static com.uber.nullaway.NullabilityUtil.castToNonNull; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; @@ -37,6 +36,7 @@ import com.sun.tools.javac.code.Types; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; +import com.uber.nullaway.annotations.Initializer; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.ArrayList; @@ -61,9 +61,14 @@ public class GrpcHandler extends BaseNoOpHandler { private static final Supplier GRPC_METADATA_KEY_TYPE_SUPPLIER = Suppliers.typeFromString(GRPC_METADATA_KEY_TNAME); - @Nullable private Optional grpcMetadataType; - @Nullable private Optional grpcKeyType; + private Optional grpcMetadataType; + private Optional grpcKeyType; + /** + * This method is annotated {@code @Initializer} since it will be invoked when the first class is + * processed, before any other handler methods + */ + @Initializer @Override public void onMatchTopLevelClass( NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { @@ -135,8 +140,6 @@ private Symbol.MethodSymbol getGetterForMetadataSubtype( } private boolean grpcIsMetadataContainsKeyCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(grpcMetadataType); - Preconditions.checkNotNull(grpcKeyType); // noinspection ConstantConditions return grpcMetadataType.isPresent() && grpcKeyType.isPresent() @@ -149,8 +152,6 @@ private boolean grpcIsMetadataContainsKeyCall(Symbol.MethodSymbol symbol, Types } private boolean grpcIsMetadataGetCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(grpcMetadataType); - Preconditions.checkNotNull(grpcKeyType); // noinspection ConstantConditions return grpcMetadataType.isPresent() && grpcKeyType.isPresent()