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 17 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
34 changes: 18 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,21 @@ 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 ->
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 @@ -694,7 +694,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 @@ -1423,7 +1425,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 @@ -1576,7 +1578,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 @@ -1707,8 +1711,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,9 @@
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 = tmBuilder.create(nullableAnnotationCompound);
Type currentTypeArgType = curTypeArg.accept(this, null);
Type newTypeArgType = currentTypeArgType.cloneWithMetadata(typeMetadata);
Type newTypeArgType = tmBuilder.cloneTypeWithMetadata(currentTypeArgType, typeMetadata);
newTypeArgs.add(newTypeArgType);
}
Type.ClassType finalType =
Expand All @@ -91,4 +94,99 @@
protected Type defaultAction(Tree node, Void unused) {
return castToNonNull(ASTHelpers.getType(node));
}

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);
}

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));
}

@Override
public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) {
return typeToBeCloned.cloneWithMetadata(metadata);
}
}

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);

Check warning on line 133 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L130-L133

Added lines #L130 - L133 were not covered by tests
}
}

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);

Check warning on line 146 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L143-L146

Added lines #L143 - L146 were not covered by tests
}
}

@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);

Check warning on line 157 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L156-L157

Added lines #L156 - L157 were not covered by tests
}
}

@Override
public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) {
try {
// In Jdk21 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
msridhar marked this conversation as resolved.
Show resolved Hide resolved
Type clonedTypeWithoutMetadata =
(Type) dropMetadataHandle.invoke(typeToBeCloned, metadata.getClass());
return (Type) addMetadataHandle.invoke(clonedTypeWithoutMetadata, metadata);
} catch (Throwable e) {
throw new RuntimeException(e);

Check warning on line 170 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L169-L170

Added lines #L169 - L170 were not covered by tests
}
}
}

private static final TypeMetadataBuilder tmBuilder =
getVersion() >= 21
? new JDK21TypeMetadataBuilder()
: new JDK17AndEarlierTypeMetadataBuilder();

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);

Check warning on line 183 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L183

Added line #L183 was not covered by tests
} else {
int dot = version.indexOf(".");
if (dot != -1) {
version = version.substring(0, dot);
}
}
return Integer.parseInt(version);
}
}