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

Fix JSpecify support on JDK 21 #869

Merged
merged 27 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
dcb52e5
WIP
msridhar Aug 12, 2023
7cbdf62
Merge branch 'master' into bump-dataflow-to-3.37.0
msridhar Aug 26, 2023
527f749
WIP
msridhar Aug 26, 2023
8acfb89
WIP
msridhar Aug 26, 2023
1102fd7
WIP
msridhar Aug 26, 2023
62434cc
WIP
msridhar Aug 27, 2023
79dd0fa
Merge branch 'master' into wip-jdk-21-api-stuff
msridhar Nov 13, 2023
121e735
remove unused class
msridhar Nov 13, 2023
ce338a6
run the generics tests
msridhar Nov 13, 2023
72c5733
Merge branch 'master' into wip-jdk-21-api-stuff
msridhar Nov 15, 2023
89474f7
Merge branch 'master' into wip-jdk-21-api-stuff
msridhar Nov 21, 2023
ef844c8
adding logic for using addMetadata and dropMetadata through methodHandle
akulk022 Nov 25, 2023
79bc579
cleaning up unnecessary condition and adding a comment
akulk022 Nov 26, 2023
ee143b1
creating a common method for creating virtual method handle to re-use…
akulk022 Nov 26, 2023
bb502b2
Merge branch 'master' of https://github.com/akulk022/NullAway into jd…
akulk022 Nov 27, 2023
62500ed
method names and other minor updates, updating build.gradle to remove…
akulk022 Nov 28, 2023
417f888
Hacky way to collect test coverage for testJdk21 tasks
msridhar Nov 28, 2023
b0bf1ba
Adding Java docs
akulk022 Dec 1, 2023
fe1b8be
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Dec 1, 2023
4c6ef16
tweaks
msridhar Dec 1, 2023
708a494
tweak
msridhar Dec 1, 2023
132b6b3
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Dec 5, 2023
992e48c
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Dec 5, 2023
8c22d6c
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Dec 6, 2023
579b60c
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Dec 28, 2023
b7d94cf
Merge branch 'master' into jdk21_cloneMetaData_updates
msridhar Jan 3, 2024
ac250ed
add a TODO to remove method
msridhar Jan 10, 2024
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
36 changes: 20 additions & 16 deletions buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,6 @@ configurations.create('transitiveSourcesElements') {
}
}

// Share the coverage data to be aggregated for the whole product
configurations.create('coverageDataElements') {
visible = false
canBeResolved = false
canBeConsumed = true
extendsFrom(configurations.implementation)
attributes {
attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME))
attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION))
attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data'))
}
// This will cause the test task to run if the coverage data is requested by the aggregation task
outgoing.artifact(tasks.named("test").map { task ->
task.extensions.getByType(JacocoTaskExtension).destinationFile
})
}

test {
maxHeapSize = "1024m"
Expand Down Expand Up @@ -118,3 +102,23 @@ def testJdk21 = tasks.register("testJdk21", Test) {
tasks.named('check').configure {
dependsOn testJdk21
}


// Share the coverage data to be aggregated for the whole product
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is required so that test coverage when running tests on JDK 21 gets reported to CodeCov.

configurations.create('coverageDataElements') {
visible = false
canBeResolved = false
canBeConsumed = true
extendsFrom(configurations.implementation)
attributes {
attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME))
attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION))
attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data'))
}
// This will cause the test tasks to run if the coverage data is requested by the aggregation task
tasks.withType(Test).forEach {task ->
// tasks.named(task.name) is a hack to introduce the right task dependence in Gradle. We will fix this
// correctly when addressing https://github.com/uber/NullAway/issues/871
outgoing.artifact(tasks.named(task.name).map { t -> t.extensions.getByType(JacocoTaskExtension).destinationFile })
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole tasks.named(task.name) thing is a horrible hack to force the test task to run when the coverage report is requested. I'll see if I can find a better way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Meh, let's leave this hack for now and fix this properly in #871

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM!

}
}
8 changes: 0 additions & 8 deletions nullaway/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,6 @@ tasks.named('check').configure {
dependsOn(jdk8Test)
}

tasks.named('testJdk21', Test).configure {
filter {
// JSpecify Generics tests do not yet pass on JDK 21
// See https://github.com/uber/NullAway/issues/827
excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests"
}
}

Comment on lines -127 to -134
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! 👍

// Create a task to build NullAway with NullAway checking enabled
tasks.register('buildWithNullAway', JavaCompile) {
onlyIf {
Expand Down
18 changes: 12 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
doUnboxingCheck(state, tree.getExpression());
}
// generics check
if (lhsType != null && lhsType.getTypeArguments().length() > 0) {
if (lhsType != null && lhsType.getTypeArguments().length() > 0 && config.isJSpecifyMode()) {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
}

Expand Down Expand Up @@ -695,7 +695,9 @@ public Description matchParameterizedType(ParameterizedTypeTree tree, VisitorSta
if (!withinAnnotatedCode(state)) {
return Description.NO_MATCH;
}
GenericsChecks.checkInstantiationForParameterizedTypedTree(tree, state, this, config);
if (config.isJSpecifyMode()) {
GenericsChecks.checkInstantiationForParameterizedTypedTree(tree, state, this, config);
}
return Description.NO_MATCH;
}

Expand Down Expand Up @@ -1424,7 +1426,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
return Description.NO_MATCH;
}
VarSymbol symbol = ASTHelpers.getSymbol(tree);
if (tree.getInitializer() != null) {
if (tree.getInitializer() != null && config.isJSpecifyMode()) {
GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
}

Expand Down Expand Up @@ -1577,7 +1579,9 @@ public Description matchUnary(UnaryTree tree, VisitorState state) {
public Description matchConditionalExpression(
ConditionalExpressionTree tree, VisitorState state) {
if (withinAnnotatedCode(state)) {
GenericsChecks.checkTypeParameterNullnessForConditionalExpression(tree, this, state);
if (config.isJSpecifyMode()) {
GenericsChecks.checkTypeParameterNullnessForConditionalExpression(tree, this, state);
}
doUnboxingCheck(state, tree.getCondition());
}
return Description.NO_MATCH;
Expand Down Expand Up @@ -1708,8 +1712,10 @@ private Description handleInvocation(
: Nullness.NONNULL);
}
}
GenericsChecks.compareGenericTypeParameterNullabilityForCall(
formalParams, actualParams, methodSymbol.isVarArgs(), this, state);
if (config.isJSpecifyMode()) {
GenericsChecks.compareGenericTypeParameterNullabilityForCall(
formalParams, actualParams, methodSymbol.isVarArgs(), this, state);
}
}

// Allow handlers to override the list of non-null argument positions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.TypeMetadata;
import com.sun.tools.javac.util.ListBuffer;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -74,10 +78,10 @@ public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) {
new Attribute.TypeCompound(
nullableType, com.sun.tools.javac.util.List.nil(), null)))
: com.sun.tools.javac.util.List.nil();
TypeMetadata typeMetadata =
new TypeMetadata(new TypeMetadata.Annotations(nullableAnnotationCompound));
TypeMetadata typeMetadata = TYPE_METADATA_BUILDER.create(nullableAnnotationCompound);
Type currentTypeArgType = curTypeArg.accept(this, null);
Type newTypeArgType = currentTypeArgType.cloneWithMetadata(typeMetadata);
Type newTypeArgType =
TYPE_METADATA_BUILDER.cloneTypeWithMetadata(currentTypeArgType, typeMetadata);
newTypeArgs.add(newTypeArgType);
}
Type.ClassType finalType =
Expand All @@ -91,4 +95,145 @@ public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) {
protected Type defaultAction(Tree node, Void unused) {
return castToNonNull(ASTHelpers.getType(node));
}

/**
* Abstracts over the different APIs for building {@link TypeMetadata} objects in different JDK
* versions.
*/
private interface TypeMetadataBuilder {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs);

Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metaData);
}

/**
* Provides implementations for methods under TypeMetadataBuilder compatible with JDK 17 and
* earlier versions.
*/
private static class JDK17AndEarlierTypeMetadataBuilder implements TypeMetadataBuilder {
msridhar marked this conversation as resolved.
Show resolved Hide resolved

@Override
public TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs) {
return new TypeMetadata(new TypeMetadata.Annotations(attrs));
}

/**
* Clones the given type with the specified Metadata for getting the right nullability
* annotations.
*
* @param typeToBeCloned The Type we want to clone with the required Nullability Metadata
* @param metadata The required Nullability metadata which is lost from the type
* @return Type after it has been cloned by applying the required Nullability metadata
*/
@Override
public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) {
return typeToBeCloned.cloneWithMetadata(metadata);
}
}

/**
* Provides implementations for methods under TypeMetadataBuilder compatible with the updates made
* to the library methods for Jdk 21. The implementation calls the logic specific to JDK 21
* indirectly using MethodHandles since we still need the code to compile on earlier versions.
*/
private static class JDK21TypeMetadataBuilder implements TypeMetadataBuilder {
msridhar marked this conversation as resolved.
Show resolved Hide resolved

private static final MethodHandle typeMetadataHandle = createHandle();
private static final MethodHandle addMetadataHandle =
createVirtualMethodHandle(Type.class, TypeMetadata.class, Type.class, "addMetadata");
private static final MethodHandle dropMetadataHandle =
createVirtualMethodHandle(Type.class, Class.class, Type.class, "dropMetadata");

private static MethodHandle createHandle() {
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodType mt = MethodType.methodType(void.class, com.sun.tools.javac.util.ListBuffer.class);
try {
return lookup.findConstructor(TypeMetadata.Annotations.class, mt);
} catch (NoSuchMethodException e) {
throw new RuntimeException(e);
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}

/**
* Used to get a MethodHandle for a virtual method from the specified class
*
* @param retTypeClass Class to indicate the return type of the desired method
* @param paramTypeClass Class to indicate the parameter type of the desired method
* @param refClass Class within which the desired method is contained
* @param methodName Name of the desired method
* @return The appropriate MethodHandle for the virtual method
*/
private static MethodHandle createVirtualMethodHandle(
Class<?> retTypeClass, Class<?> paramTypeClass, Class<?> refClass, String methodName) {
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodType mt = MethodType.methodType(retTypeClass, paramTypeClass);
try {
return lookup.findVirtual(refClass, methodName, mt);
} catch (NoSuchMethodException e) {
throw new RuntimeException(e);
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}

@Override
public TypeMetadata create(com.sun.tools.javac.util.List<Attribute.TypeCompound> attrs) {
ListBuffer<Attribute.TypeCompound> b = new ListBuffer<>();
b.appendList(attrs);
try {
return (TypeMetadata) typeMetadataHandle.invoke(b);
} catch (Throwable e) {
throw new RuntimeException(e);
}
}

/**
* Calls dropMetadata and addMetadata using MethodHandles for JDK 21, which removed the previous
* cloneWithMetadata method.
*
* @param typeToBeCloned The Type we want to clone with the required Nullability metadata
* @param metadata The required Nullability metadata
* @return Cloned Type with the necessary Nullability metadata
*/
@Override
public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) {
try {
// In JDK 21 addMetadata works if there is no metadata associated with the type, so we
// create a copy without the existing metadata first and then add it
Type clonedTypeWithoutMetadata =
(Type) dropMetadataHandle.invoke(typeToBeCloned, metadata.getClass());
return (Type) addMetadataHandle.invoke(clonedTypeWithoutMetadata, metadata);
} catch (Throwable e) {
throw new RuntimeException(e);
}
}
}

/** The TypeMetadataBuilder to be used for the current JDK version. */
private static final TypeMetadataBuilder TYPE_METADATA_BUILDER =
getVersion() >= 21
? new JDK21TypeMetadataBuilder()
: new JDK17AndEarlierTypeMetadataBuilder();

/**
* Utility method to get the current JDK version, that works on Java 8 and above.
*
* <p>TODO remove this method once we drop support for Java 8
*
* @return the current JDK version
*/
private static int getVersion() {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
String version = System.getProperty("java.version");
if (version.startsWith("1.")) {
version = version.substring(2, 3);
} else {
int dot = version.indexOf(".");
if (dot != -1) {
version = version.substring(0, dot);
}
}
return Integer.parseInt(version);
}
}