Fix JSpecify support on JDK 21 #869

akulk022 · 2023-11-25T22:16:10Z

Our JSpecify mode would not run on JDK 21 due some some internal javac API changes. This PR adapts to those changes using code specific to JDK 21.

Fixes #827

codecov · 2023-11-25T22:19:35Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (257e4bb) 87.20% compared to head (b7d94cf) 86.94%.

❗ Current head b7d94cf differs from pull request most recent head ac250ed. Consider uploading reports for the commit ac250ed to get more accurate results

Files	Patch %	Lines
...laway/generics/PreservedAnnotationTreeVisitor.java	66.66%	13 Missing and 2 partials ⚠️
...away/src/main/java/com/uber/nullaway/NullAway.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #869      +/-   ##
============================================
- Coverage     87.20%   86.94%   -0.26%     
- Complexity     1947     1952       +5     
============================================
  Files            77       77              
  Lines          6268     6314      +46     
  Branches       1216     1222       +6     
============================================
+ Hits           5466     5490      +24     
- Misses          403      420      +17     
- Partials        399      404       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… for both add and drop metadata

msridhar

Looks great! A few requested changes / comments

nullaway/build.gradle

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

…k21_cloneMetaData_updates

… unnecessary code

nullaway/src/main/java/com/uber/nullaway/NullAway.java

msridhar · 2023-11-28T23:07:25Z

buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle

+    }
+    // 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 })


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.

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

msridhar · 2023-12-01T05:01:33Z

No, if tests are failing then you can just leave the changes.

…

On Nov 30, 2023 at 20:32:49, Abhijit Kulkarni ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nullaway/src/main/java/com/uber/nullaway/NullAway.java <#869 (comment)>: > @@ -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()) { I tried removing the changes and saw some tests failing, I haven't looked into it more deeply yet, Should I do that? — Reply to this email directly, view it on GitHub <#869 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABPEUOULW66HFFOKE2R5U3YHFMXDAVCNFSM6AAAAAA72L4ZR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJYHE4DKMJYGQ> . You are receiving this because you commented.Message ID: ***@***.***>

msridhar · 2023-12-01T18:35:51Z

buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle

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


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

yuxincs

Overall LGTM! Just have a minor comment suggestions since we plan to drop JDK 8 in the near future.

yuxincs · 2024-01-10T03:31:26Z

buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle

+    }
+    // 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 })


yuxincs · 2024-01-10T03:31:49Z

nullaway/build.gradle

-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"
-    }
-}
-


yuxincs · 2024-01-10T03:43:37Z

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

+  /**
+   * Utility method to get the current JDK version, that works on Java 8 and above.
+   *
+   * @return the current JDK version
+   */
+  private static int getVersion() {
+    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);
+  }


We won't need this anymore once we drop support for JDK 8, right? Since we can use Runtime.version() starting from JDK 9 https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Runtime.html#version()

If so maybe worth adding a TODO here for a cleanup reminder since we plan to drop JDK 8 in the near future 😃

Good point! Done in ac250ed

msridhar and others added 12 commits August 12, 2023 15:57

WIP

dcb52e5

Merge branch 'master' into bump-dataflow-to-3.37.0

7cbdf62

WIP

527f749

WIP

8acfb89

WIP

1102fd7

WIP

62434cc

Merge branch 'master' into wip-jdk-21-api-stuff

79dd0fa

remove unused class

121e735

run the generics tests

ce338a6

Merge branch 'master' into wip-jdk-21-api-stuff

72c5733

Merge branch 'master' into wip-jdk-21-api-stuff

89474f7

adding logic for using addMetadata and dropMetadata through methodHandle

ef844c8

akulk022 added 2 commits November 26, 2023 00:16

cleaning up unnecessary condition and adding a comment

79bc579

creating a common method for creating virtual method handle to re-use…

ee143b1

… for both add and drop metadata

msridhar requested changes Nov 26, 2023

View reviewed changes

akulk022 and others added 3 commits November 27, 2023 15:55

Merge branch 'master' of https://github.com/akulk022/NullAway into jd…

bb502b2

…k21_cloneMetaData_updates

method names and other minor updates, updating build.gradle to remove…

62500ed

… unnecessary code

Hacky way to collect test coverage for testJdk21 tasks

417f888

msridhar reviewed Nov 28, 2023

View reviewed changes

nullaway/src/main/java/com/uber/nullaway/NullAway.java Show resolved Hide resolved

msridhar reviewed Nov 28, 2023

View reviewed changes

akulk022 and others added 4 commits November 30, 2023 21:55

Adding Java docs

b0bf1ba

Merge branch 'master' into jdk21_cloneMetaData_updates

fe1b8be

tweaks

4c6ef16

tweak

708a494

msridhar approved these changes Dec 1, 2023

View reviewed changes

msridhar marked this pull request as ready for review December 1, 2023 16:58

msridhar requested a review from yuxincs December 1, 2023 16:59

msridhar changed the title ~~JSpecify: changes for using addMetadata and dropMetadata for jdk21 using MethodHandle~~ Fix JSpecify support on JDK 21 Dec 1, 2023

msridhar reviewed Dec 1, 2023

View reviewed changes

msridhar added 5 commits December 5, 2023 13:17

Merge branch 'master' into jdk21_cloneMetaData_updates

132b6b3

Merge branch 'master' into jdk21_cloneMetaData_updates

992e48c

Merge branch 'master' into jdk21_cloneMetaData_updates

8c22d6c

Merge branch 'master' into jdk21_cloneMetaData_updates

579b60c

Merge branch 'master' into jdk21_cloneMetaData_updates

b7d94cf

yuxincs approved these changes Jan 10, 2024

View reviewed changes

add a TODO to remove method

ac250ed

msridhar enabled auto-merge (squash) January 10, 2024 16:16

msridhar merged commit 97771dc into uber:master Jan 10, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JSpecify support on JDK 21 #869

Fix JSpecify support on JDK 21 #869

akulk022 commented Nov 25, 2023 •

edited by msridhar

Loading

codecov bot commented Nov 25, 2023 •

edited

Loading

msridhar left a comment

msridhar Nov 28, 2023

msridhar Nov 29, 2023

yuxincs Jan 10, 2024

msridhar commented Dec 1, 2023 via email

msridhar Dec 1, 2023

yuxincs left a comment

yuxincs Jan 10, 2024

yuxincs Jan 10, 2024

yuxincs Jan 10, 2024

msridhar Jan 10, 2024

Fix JSpecify support on JDK 21 #869

Fix JSpecify support on JDK 21 #869

Conversation

akulk022 commented Nov 25, 2023 • edited by msridhar Loading

codecov bot commented Nov 25, 2023 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Dec 1, 2023 via email

Choose a reason for hiding this comment

yuxincs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akulk022 commented Nov 25, 2023 •

edited by msridhar

Loading

codecov bot commented Nov 25, 2023 •

edited

Loading