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.

Sign up for GitHub

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

Jump to bottom

Update to Error Prone 2.21.1 #797

Merged
merged 11 commits into from
Aug 11, 2023
Merged

Update to Error Prone 2.21.1 #797

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6cc20b4
WIP
msridhar Aug 4, 2023
b8edf31
turn off a check causing an Error Prone crash
msridhar Aug 4, 2023
d11e49e
simplify
msridhar Aug 4, 2023
b923cd4
try using variables for error prone versions
msridhar Aug 4, 2023
8d7fa4c
try again
msridhar Aug 4, 2023
fb935f6
try again
msridhar Aug 4, 2023
b02777c
try again
msridhar Aug 4, 2023
912e82a
one last try
msridhar Aug 4, 2023
f44ac1b
give up
msridhar Aug 4, 2023
282e986
Use version 2.21.1
msridhar Aug 4, 2023
6741ef3
Merge branch 'master' into ep-2.21.0
msridhar Aug 11, 2023
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
10 changes: 5 additions & 5 deletions .github/workflows/continuous-integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,16 @@ jobs:
epVersion: 2.4.0
- os: macos-latest
java: 11
epVersion: 2.20.0
epVersion: 2.21.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seeing this change, I wonder if there is a way to define this version as a constant once in this yml and reference it in all these jobs and the check below, like we do inside gradle :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is not possible. You can define variables in an env context but you can't use them when specifying properties in the matrix, see here.

- os: ubuntu-latest
java: 11
epVersion: 2.20.0
epVersion: 2.21.0
- os: windows-latest
java: 11
epVersion: 2.20.0
epVersion: 2.21.0
- os: ubuntu-latest
java: 17
epVersion: 2.20.0
epVersion: 2.21.0
fail-fast: false
runs-on: ${{ matrix.os }}
steps:
Expand Down Expand Up @@ -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.0' && github.repository == 'uber/NullAway'
- name: Test publishToMavenLocal flow
env:
ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }}
Expand Down
2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.0"
// Default to using latest tested Error Prone version
def defaultErrorProneVersion = latestErrorProneVersion
def errorProneVersionToCompileAgainst = defaultErrorProneVersion
Expand Down
1 change: 1 addition & 0 deletions nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Symbol> getEnclosedElements(Symbol symbol) {
return symbol.getEnclosedElements();
}
Expand Down
18 changes: 13 additions & 5 deletions nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -54,17 +54,26 @@ public class ApacheThriftIsSetHandler extends BaseNoOpHandler {

private static final Supplier<Type> TBASE_TYPE_SUPPLIER = Suppliers.typeFromString(TBASE_NAME);

@Nullable private Optional<Type> tbaseType;
private Optional<Type> tbaseType;

@Override
public void onMatchTopLevelClass(
NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) {
if (tbaseType == null) {
tbaseType =
Optional.ofNullable(TBASE_TYPE_SUPPLIER.get(state)).map(state.getTypes()::erasure);
init(state);
}
}

/**
* This method is annotated {@code @Initializer} since it will be invoked when the first class is
* processed by {@link #onMatchTopLevelClass(NullAway, ClassTree, VisitorState,
* Symbol.ClassSymbol)}
*/
@Initializer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, was there any issue marking onMatchTopLevelClass(...) itself as an initializer? Or is there a reason why that would be confusing? (I guess for all handlers, in general, we would expect that to be the first callback executed always?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, just annotating onMatchTopLevelClass(...) is better, and I switched to that. It's slightly strange since that method gets invoked multiple times (once for each class), but it does perform guaranteed initialization the first time it runs, so I think it's fine to annotate it as @Initializer.

private void init(VisitorState state) {
tbaseType = Optional.ofNullable(TBASE_TYPE_SUPPLIER.get(state)).map(state.getTypes()::erasure);
}

@Override
public NullnessHint onDataflowVisitMethodInvocation(
MethodInvocationNode node,
Expand Down Expand Up @@ -173,7 +182,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")
Expand Down
31 changes: 18 additions & 13 deletions nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -61,22 +61,31 @@ public class GrpcHandler extends BaseNoOpHandler {
private static final Supplier<Type> GRPC_METADATA_KEY_TYPE_SUPPLIER =
Suppliers.typeFromString(GRPC_METADATA_KEY_TNAME);

@Nullable private Optional<Type> grpcMetadataType;
@Nullable private Optional<Type> grpcKeyType;
private Optional<Type> grpcMetadataType;
private Optional<Type> grpcKeyType;

@Override
public void onMatchTopLevelClass(
NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) {
if (grpcMetadataType == null || grpcKeyType == null) {
grpcMetadataType =
Optional.ofNullable(GRPC_METADATA_TYPE_SUPPLIER.get(state))
.map(state.getTypes()::erasure);
grpcKeyType =
Optional.ofNullable(GRPC_METADATA_KEY_TYPE_SUPPLIER.get(state))
.map(state.getTypes()::erasure);
init(state);
}
}

/**
* This method is annotated {@code @Initializer} since it will be invoked when the first class is
* processed by {@link #onMatchTopLevelClass(NullAway, ClassTree, VisitorState,
* Symbol.ClassSymbol)}
*/
@Initializer
private void init(VisitorState state) {
grpcMetadataType =
Optional.ofNullable(GRPC_METADATA_TYPE_SUPPLIER.get(state)).map(state.getTypes()::erasure);
grpcKeyType =
Optional.ofNullable(GRPC_METADATA_KEY_TYPE_SUPPLIER.get(state))
.map(state.getTypes()::erasure);
}

@Override
public NullnessHint onDataflowVisitMethodInvocation(
MethodInvocationNode node,
Expand Down Expand Up @@ -135,8 +144,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()
Expand All @@ -149,8 +156,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()
Expand Down
2 changes: 2 additions & 0 deletions test-java-lib-lombok/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ tasks.withType(JavaCompile) {
if (!name.toLowerCase().contains("test")) {
options.errorprone {
check("NullAway", CheckSeverity.ERROR)
// Issue in Error Prone 2.21.0: https://github.com/google/error-prone/issues/4040
check("NotJavadoc", CheckSeverity.OFF)
option("NullAway:AnnotatedPackages", "com.uber")
option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated")
}
Expand Down