Don't report @Nullable type argument errors for unmarked classes #958

haewiful · 2024-05-19T03:19:58Z

Thank you for contributing to NullAway!

Please note that once you click "Create Pull Request" you will be asked to sign our Uber Contributor License Agreement via CLA assistant.

Before pressing the "Create Pull Request" button, please provide the following:

Changed the GenericsChecks.java and NullAway.java files so that they will consider classes that were annotated by the @NullUnmarked annotation.
It is "testUseOfUnannotatedCode()" in the GenericsTests.java file.

CLAassistant · 2024-05-19T03:20:03Z

All committers have signed the CLA.

msridhar

Looks good! See comment below

msridhar · 2024-05-25T00:24:22Z

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

+
+    Symbol calledClass = castToNonNull(ASTHelpers.getSymbol(tree));
+    boolean isNullUnmarked = codeAnnotationInfo.isSymbolUnannotated(calledClass, config, handler);
+


Let's move this code under the if (config.isJSpecifyMode()) check below.

Also, it seems to me this logic could be simpler. If isNullUnmarked is true, can't we just skip the call to checkInstantiationForParameterizedTypedTree entirely? I don't see how that method would ever report an error if isNullUnmarked is true. This way you could avoid changing GenericsChecks

codecov · 2024-05-25T00:25:26Z

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.94%. Comparing base (9860ab2) to head (06c01c1).

Files	Patch %	Lines
...away/src/main/java/com/uber/nullaway/NullAway.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #958      +/-   ##
============================================
- Coverage     85.94%   85.94%   -0.01%     
- Complexity     2045     2047       +2     
============================================
  Files            81       81              
  Lines          6761     6765       +4     
  Branches       1302     1305       +3     
============================================
+ Hits           5811     5814       +3     
  Misses          537      537              
- Partials        413      414       +1

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

msridhar

Overall this looks great! I have a few more minor comments

msridhar · 2024-06-13T00:12:57Z

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

@@ -738,10 +738,16 @@ public Description matchParameterizedType(ParameterizedTypeTree tree, VisitorSta
    if (!withinAnnotatedCode(state)) {
      return Description.NO_MATCH;
    }
+


remove this extra newline

msridhar · 2024-06-13T00:13:38Z

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

    if (config.isJSpecifyMode()) {
-      GenericsChecks.checkInstantiationForParameterizedTypedTree(
-          tree, state, this, config, handler);
+      Symbol calledClass = ASTHelpers.getSymbol(tree);


I think baseClass is a better name for this variable. "called" implies something like a method call.

msridhar · 2024-06-13T00:13:48Z

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

}
+

also remove this newline

msridhar · 2024-06-13T00:14:43Z

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  @NullUnmarked",
+            "  static class nullUnmarkedClass<S> {",


In Java, class names should start with a capital letter: https://google.github.io/styleguide/javaguide.html#s5.2.2-class-names

msridhar · 2024-06-13T00:15:04Z

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

+            "      static <T> void m1(T t) {}",
+            "    }",
+            "  @NullMarked",
+            "  static class markedClass {",


Again class name should start with capital letter

msridhar · 2024-06-13T00:15:42Z

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

+            "  @NullMarked",
+            "  static class markedClass {",
+            "    static void testInstantiation() {",
+            "      new nullUnmarkedClass<@Nullable String>();",


Let's add a comment here (within quotes) indicating that despite the type variable not having a @Nullable upper bound, we don't expect an error since the type is @NullUnmarked

msridhar · 2024-06-13T22:53:36Z

@haewiful CI is failing:

https://github.com/uber/NullAway/actions/runs/9507798037/job/26207945224?pr=958#step:6:506

You can run ./gradlew :nullaway:buildWithNullAway to reproduce. It's failing when we try to run NullAway on itself.

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [org.tukaani:xz](https://tukaani.org/xz/java.html) ([source](https://github.com/tukaani-project/xz-java)) | compile | minor | `1.9` -> `1.10` | | [org.eclipse:yasson](https://projects.eclipse.org/projects/ee4j.yasson) ([source](https://github.com/eclipse-ee4j/yasson)) | compile | patch | `3.0.3` -> `3.0.4` | | [org.eclipse.parsson:parsson](https://github.com/eclipse-ee4j/parsson) | compile | patch | `1.1.6` -> `1.1.7` | | [com.uber.nullaway:nullaway](https://github.com/uber/NullAway) | | patch | `0.11.0` -> `0.11.1` | | [io.hosuaby:inject-resources-junit-jupiter](https://github.com/hosuaby/inject-resources) | test | patch | `0.3.3` -> `0.3.5` | | [org.codehaus.mojo:exec-maven-plugin](https://www.mojohaus.org/exec-maven-plugin) ([source](https://github.com/mojohaus/exec-maven-plugin)) | build | minor | `3.3.0` -> `3.4.0` | --- ### Release Notes <details> <summary>tukaani-project/xz-java</summary> ### [`v1.10`](https://github.com/tukaani-project/xz-java/releases/tag/v1.10): XZ for Java 1.10 [Compare Source](tukaani-project/xz-java@v1.9...v1.10) </details> <details> <summary>eclipse-ee4j/yasson</summary> ### [`v3.0.4`](eclipse-ee4j/yasson@3.0.3...3.0.4) [Compare Source](eclipse-ee4j/yasson@3.0.3...3.0.4) </details> <details> <summary>eclipse-ee4j/parsson</summary> ### [`v1.1.7`](eclipse-ee4j/parsson@1.1.6...1.1.7) [Compare Source](eclipse-ee4j/parsson@1.1.6...1.1.7) </details> <details> <summary>uber/NullAway</summary> ### [`v0.11.1`](https://github.com/uber/NullAway/blob/HEAD/CHANGELOG.md#Version-0111) [Compare Source](uber/NullAway@v0.11.0...v0.11.1) - Fix issue 1008 ([#1009](uber/NullAway#1009)) - JSpecify: read upper bound annotations from bytecode and add tests ([#1004](uber/NullAway#1004)) - Fix crash with suggested suppressions in JSpecify mode ([#1001](uber/NullAway#1001)) - Update to JSpecify 1.0 and use JSpecify annotations in NullAway code ([#1000](uber/NullAway#1000)) - Expose [@EnsuresNonNull](https://github.com/EnsuresNonNull) and [@RequiresNonNull](https://github.com/RequiresNonNull) in annotations package ([#999](uber/NullAway#999)) - Don't report initializer warnings on [@NullUnmarked](https://github.com/NullUnmarked) constructors / methods ([#997](uber/NullAway#997)) - Strip annotations from MethodSymbol strings ([#993](uber/NullAway#993)) - JSpecify: fix crashes where declared parameter / return types were raw ([#989](uber/NullAway#989)) - JSpecify: Handle [@nullable](https://github.com/nullable) elements for enhanced-for-loops on arrays ([#986](uber/NullAway#986)) - Features/944 tidy stream nullability propagator ([#985](uber/NullAway#985)) - Tests for loops over arrays ([#982](uber/NullAway#982)) - Bug fixes for array subtyping at returns / parameter passing ([#980](uber/NullAway#980)) - JSpecify: Handle [@nonnull](https://github.com/nonnull) elements in [@nullable](https://github.com/nullable) content arrays ([#963](uber/NullAway#963)) - Don't report [@nullable](https://github.com/nullable) type argument errors for unmarked classes ([#958](uber/NullAway#958)) - External Library Models: Adding support for Nullable upper bounds of Generic Type parameters ([#949](uber/NullAway#949)) - Refactoring / code cleanups: - Test on JDK 22 ([#992](uber/NullAway#992)) - Add test case for [@nullable](https://github.com/nullable) Void with override in JSpecify mode ([#990](uber/NullAway#990)) - Enable UnnecessaryFinal and PreferredInterfaceType EP checks ([#991](uber/NullAway#991)) - Add missing [@test](https://github.com/test) annotation ([#988](uber/NullAway#988)) - Fix typo in variable name ([#987](uber/NullAway#987)) - Remove AbstractConfig class ([#974](uber/NullAway#974)) - Fix Javadoc for MethodRef ([#973](uber/NullAway#973)) - Refactored data clumps with the help of LLMs (research project) ([#960](uber/NullAway#960)) - Build / CI tooling maintenance: - Various cleanups enabled by bumping minimum Java and Error Prone versions ([#962](uber/NullAway#962)) - Disable publishing of snapshot builds from CI ([#967](uber/NullAway#967)) - Update Gradle action usage in CI workflow ([#969](uber/NullAway#969)) - Update Gradle config to always compile Java code using JDK 17 ([#971](uber/NullAway#971)) - Update JavaParser to 3.26.0 ([#970](uber/NullAway#970)) - Reenable JMH benchmarking in a safer manner ([#975](uber/NullAway#975)) - Updated JMH Benchmark Comment Action ([#976](uber/NullAway#976)) - Update to Gradle 8.8 ([#981](uber/NullAway#981)) - Update to Error Prone 2.28.0 ([#984](uber/NullAway#984)) - Update to Gradle 8.9 ([#998](uber/NullAway#998)) - Update to WALA 1.6.6 ([#1003](uber/NullAway#1003)) </details> <details> <summary>hosuaby/inject-resources</summary> ### [`v0.3.5`](hosuaby/inject-resources@v0.3.4...v0.3.5) [Compare Source](hosuaby/inject-resources@v0.3.4...v0.3.5) ### [`v0.3.4`](hosuaby/inject-resources@v0.3.3...v0.3.4) [Compare Source](hosuaby/inject-resources@v0.3.3...v0.3.4) </details> <details> <summary>mojohaus/exec-maven-plugin</summary> ### [`v3.4.0`](https://github.com/mojohaus/exec-maven-plugin/releases/tag/3.4.0) [Compare Source](mojohaus/exec-maven-plugin@3.3.0...3.4.0)  #### 🚀 New features and improvements - Allow `<includePluginDependencies>` to be specified for the exec:exec goal ([#432](mojohaus/exec-maven-plugin#432)) [@sebthom](https://github.com/sebthom) #### 🐛 Bug Fixes - Do not get UPPERCASE env vars ([#427](mojohaus/exec-maven-plugin#427)) [@wheezil](https://github.com/wheezil) #### 📦 Dependency updates - Bump org.codehaus.mojo:mojo-parent from 82 to 84 ([#434](mojohaus/exec-maven-plugin#434)) [@dependabot](https://github.com/dependabot) - Bump org.codehaus.plexus:plexus-xml from 3.0.0 to 3.0.1 ([#431](mojohaus/exec-maven-plugin#431)) [@dependabot](https://github.com/dependabot) #### 👻 Maintenance - Remove Log4j 1.2.x from ITs ([#437](mojohaus/exec-maven-plugin#437)) [@slawekjaranowski](https://github.com/slawekjaranowski) #### 🔧 Build - Use Maven 3.9.7 and 4.0.0-beta-3 ([#433](mojohaus/exec-maven-plugin#433)) [@slawekjaranowski](https://github.com/slawekjaranowski) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

haewiful added 2 commits May 21, 2024 22:59

add NullUnmarked checks

403f8f3

formatting for CI jobs

bec4bff

haewiful force-pushed the yk branch from 99f245f to bec4bff Compare May 22, 2024 06:01

msridhar added 2 commits May 24, 2024 17:13

Merge branch 'master' into yk

8ef8df9

add castToNonNull

765e1ec

msridhar reviewed May 25, 2024

View reviewed changes

msridhar mentioned this pull request Jun 6, 2024

External Library Models: Adding support for Nullable upper bounds of Generic Type parameters #949

Merged

haewiful added 2 commits June 10, 2024 01:32

code order change

a9bf295

formating

e623404

msridhar reviewed Jun 13, 2024

View reviewed changes

haewiful and others added 2 commits June 12, 2024 17:35

cleanup and changing names

14cf177

Merge branch 'master' into yk

2b6ad6d

haewiful and others added 3 commits June 14, 2024 16:32

debuged :nullaway:buildWithNullAway

fce9c3f

simplify

055b250

tweaks

06c01c1

msridhar approved these changes Jun 15, 2024

View reviewed changes

msridhar changed the title ~~add NullUnmarked class checks~~ Don't report @Nullable type argument errors for unmarked classes Jun 15, 2024

msridhar enabled auto-merge (squash) June 15, 2024 01:46

msridhar merged commit 2f62d78 into uber:master Jun 15, 2024
10 of 11 checks passed

msridhar mentioned this pull request Jun 21, 2024

Properly handle @NullUnmarked generic types #872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't report @Nullable type argument errors for unmarked classes #958

Don't report @Nullable type argument errors for unmarked classes #958

haewiful commented May 19, 2024 •

edited

Loading

CLAassistant commented May 19, 2024 •

edited

Loading

msridhar left a comment

msridhar May 25, 2024

codecov bot commented May 25, 2024 •

edited

Loading

msridhar left a comment

msridhar Jun 13, 2024

msridhar Jun 13, 2024

msridhar Jun 13, 2024

msridhar Jun 13, 2024

msridhar Jun 13, 2024

msridhar Jun 13, 2024

msridhar commented Jun 13, 2024


		Symbol calledClass = castToNonNull(ASTHelpers.getSymbol(tree));
		boolean isNullUnmarked = codeAnnotationInfo.isSymbolUnannotated(calledClass, config, handler);

Don't report @Nullable type argument errors for unmarked classes #958

Don't report @Nullable type argument errors for unmarked classes #958

Conversation

haewiful commented May 19, 2024 • edited Loading

CLAassistant commented May 19, 2024 • edited Loading

msridhar left a comment

Choose a reason for hiding this comment

msridhar May 25, 2024

Choose a reason for hiding this comment

codecov bot commented May 25, 2024 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar Jun 13, 2024

Choose a reason for hiding this comment

msridhar commented Jun 13, 2024

haewiful commented May 19, 2024 •

edited

Loading

CLAassistant commented May 19, 2024 •

edited

Loading

codecov bot commented May 25, 2024 •

edited

Loading