-
Notifications
You must be signed in to change notification settings - Fork 299
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
Changes from 21 commits
dcb52e5
7cbdf62
527f749
8acfb89
1102fd7
62434cc
79dd0fa
121e735
ce338a6
72c5733
89474f7
ef844c8
79bc579
ee143b1
bb502b2
62500ed
417f888
b0bf1ba
fe1b8be
4c6ef16
708a494
132b6b3
992e48c
8c22d6c
579b60c
b7d94cf
ac250ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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 | ||
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 }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM! |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -74,10 +78,10 @@ | |
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 = | ||
|
@@ -91,4 +95,143 @@ | |
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); | ||
Check warning on line 155 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java Codecov / codecov/patchnullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L152-L155
|
||
} | ||
} | ||
|
||
/** | ||
* 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); | ||
Check warning on line 177 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java Codecov / codecov/patchnullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L174-L177
|
||
} | ||
} | ||
|
||
@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 188 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java Codecov / codecov/patchnullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L187-L188
|
||
} | ||
} | ||
|
||
/** | ||
* 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); | ||
Check warning on line 209 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java Codecov / codecov/patchnullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L208-L209
|
||
} | ||
} | ||
} | ||
|
||
/** 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. | ||
* | ||
* @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); | ||
Check warning on line 228 in nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java Codecov / codecov/patchnullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java#L228
|
||
} else { | ||
int dot = version.indexOf("."); | ||
if (dot != -1) { | ||
version = version.substring(0, dot); | ||
} | ||
} | ||
return Integer.parseInt(version); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We won't need this anymore once we drop support for JDK 8, right? Since we can use If so maybe worth adding a TODO here for a cleanup reminder since we plan to drop JDK 8 in the near future 😃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! Done in ac250ed |
||
} |
There was a problem hiding this comment.
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.