Adding support for nullable type upper bounds considering Library mod… #893

akulk022 · 2024-01-10T23:14:05Z

The below line previously reported an error considering the upperbound of the generic type parameter couldn't be Nullable, adding it into library models to allow it to be Nullable passes the test case.

Function<String,@Nullable String> removeA = a -> a.replace("a","");

…els for generic types

codecov · 2024-01-10T23:26:37Z

Codecov Report

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

Comparison is base (c1eb8ca) 86.95% compared to head (f6996d1) 87.07%.
Report is 5 commits behind head on master.

Files	Patch %	Lines
...away/src/main/java/com/uber/nullaway/NullAway.java	90.00%	0 Missing and 3 partials ⚠️
...ava/com/uber/nullaway/generics/GenericsChecks.java	85.71%	0 Missing and 3 partials ⚠️
...src/main/java/com/uber/nullaway/LibraryModels.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #893      +/-   ##
============================================
+ Coverage     86.95%   87.07%   +0.12%     
- Complexity     1953     1981      +28     
============================================
  Files            77       77              
  Lines          6315     6385      +70     
  Branches       1222     1239      +17     
============================================
+ Hits           5491     5560      +69     
+ Misses          420      419       -1     
- Partials        404      406       +2

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

msridhar

The approach mostly looks good! I just have one comment on the API structure.

msridhar · 2024-01-11T02:42:00Z

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

+        LibraryModelsHandler libHandler = new LibraryModelsHandler(config);
+        boolean hasNullableUpperBoundLibModel =
+            libHandler.nullableUpperBoundExistsInLibModel(baseType.tsym.toString(), i);


We don't want to create a new LibraryModelsHandler object here. To be consistent with the rest of the code, I think what we want here is to add a new method to the Handler interface like onOverrideTypeParameterUpperBound. We can implement that method in LibraryModelsHandler. Then we could pass in the handler object from the NullAway class and then call handler.onOverrideTypeParameterUpperBound here.

Thanks for the comment. I agree, I didn't want to add the method to the interface till I was sure this is the right approach. I'll add it now.

…and updating relevant implementations.

msridhar

Couple of quick comments, will look more closely later

msridhar · 2024-01-12T17:47:47Z

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

+   * @return map from the className to the position of the generic type argument that has a Nullable
+   *     upper bound.
+   */
+  ImmutableSetMultimap<String, Integer> nullableVariableTypeUpperBounds();


Suggested change

ImmutableSetMultimap<String, Integer> nullableVariableTypeUpperBounds();

ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds();

msridhar · 2024-01-12T17:49:08Z

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

@@ -97,9 +103,11 @@ public static void checkInstantiationForParameterizedTypedTree(
            upperBound.getAnnotationMirrors();
        boolean hasNullableAnnotation =
            Nullness.hasNullableAnnotation(annotationMirrors.stream(), config);
+        boolean hasNullableUpperBoundLibModel =


I don't think we need a new variable here. We could just do hasNullableAnnotation = Nullness.hasNullableAnnotation(annotationMirrors.stream(), config) || handler.onOverrideTypeParameterUpperBound(baseType.tsym.toString(), i)

msridhar

a few more comments

msridhar · 2024-01-14T23:36:59Z

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

+   * @return map from the className to the position of the generic type argument that has a Nullable
+   *     upper bound.
+   */
+  ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds();


So, thinking more about this change, one thing is that thus far, JSpecify mode changes have all been hidden under a flag and current NullAway users have not been impacted at all. This change would be different, since anyone who built their own LibraryModels implementation will need to implement this method. But, the LibraryModels interface is where this method belongs. I am leaning towards allowing this method here, but maybe we should also provide an abstract class like EmptyLibraryModels that users can subclass, where it would just return empty for everything. What do you think?

Also, maybe we should be more careful about version numbering, and make the next release 0.11.0 to indicate there is a "breaking" change for users who implemented the LibraryModels interface before. @yuxincs do you agree?

So should we also extend the new Empty library models abstract class for the classes currently implementing LibraryModels interface. for e.g the Example Library Models? So that the default implementations are kept in the EmptyLibraryModels.

Yeah, that's what I'm thinking. Our current implementations should extend the new empty library models class.

Thanks. Makes sense, I'll do that.

Ok, really sorry here, but I realized we are going about this the wrong way, and we really should be using the language feature introduced for just this case, default methods in interfaces. There is no need for introducing the EmptyLibraryModels class; we should just make typeVariablesWithNullableUpperBounds() a default method that returns the empty set and then all existing implementations will just work without any changes.

@akulk022 could you do the following:

Make typeVariablesWithNullableUpperBounds() a default method.

Also make LibraryModels.nullableFields() a default method that returns an empty set. We should have done this when introducing this method before but I forgot.

Get rid of EmptyLibraryModels, and undo the changes to ExampleLibraryModels and TestLibraryModels.

Sorry again for not remembering about default methods but they are definitely the best way to go here.

No problem, It's a nice language feature to deal with such a problem and I should have known it exists, I will certainly remember it now. I'll do the needful right away.

msridhar · 2024-01-14T23:37:58Z

nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java

@@ -305,4 +305,16 @@ public Integer castToNonNullArgumentPositionsForMethod(
    }
    return previousArgumentPosition;
  }
+
+  @Override
+  public boolean onOverrideTypeParameterUpperBound(String className, int arg) {


Document in Javadoc that this returns true if any handler returns true

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

msridhar · 2024-01-14T23:40:08Z

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

+   * @return boolean value of regardless of whether the upper bound for that argument has a Nullable
+   *     upper bound.
+   */
+  boolean onOverrideTypeParameterUpperBound(String className, int arg);


Suggested change

boolean onOverrideTypeParameterUpperBound(String className, int arg);

boolean onOverrideTypeParameterUpperBound(String className, int index);

msridhar · 2024-01-14T23:42:00Z

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

+        for (Map.Entry<String, Integer> entry :
+            libraryModels.typeVariablesWithNullableUpperBounds().entries()) {
+          nullableVariableTypeUpperBoundsBuilder.put(entry);
+        }


Use putAll instead of writing a loop

msridhar · 2024-01-14T23:42:12Z

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

  private static class OptimizedLibraryModels {

    /**
     * Mapping from {@link MethodRef} to some state, where lookups first check for a matching method
     * name as an optimization. The {@link Name} data structure is used to avoid unnecessary String
-     * conversions when looking up {@link com.sun.tools.javac.code.Symbol.MethodSymbol}s.
+     * conversions when looking up {@link Symbol.MethodSymbol}s.


Why this change?

I am not sure why this went in there, It was probably the IDE that did something, I'll revert it.

msridhar · 2024-01-14T23:43:24Z

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

+            "import java.util.function.Function;",
+            "class Test {",
+            "  static void testNegative() {",
+            "   Function<String,@Nullable String> removeA = a -> a.replace(\"a\",\"\");",


Let's write more tests to show the type argument is being handled correctly. E.g., check that passing in @Nullable when the first type argument is not @Nullable yields an error, etc.

This comment is still not addressed

msridhar · 2024-01-14T23:44:10Z

sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java

+  @Override
+  public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() {
+    return ImmutableSetMultimap.of();
+  }
+


Code like in this class could be the EmptyLibraryModels implementation maybe

@akulk022 we can now delete this method right?

Co-authored-by: Manu Sridharan <[email protected]>

…7' into library_models_Function_abhijitk7 # Conflicts: # nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

…terface" This reverts commit 81f8b7d. "Reverting EmptyLibraryModels changes"

…teHandler.java

…ds and nullableFields

msridhar

A few more comments

msridhar · 2024-01-16T22:28:25Z

sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java

+  @Override
+  public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() {
+    return ImmutableSetMultimap.of();
+  }
+


@akulk022 we can now delete this method right?

msridhar · 2024-01-16T22:28:38Z

test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java

+  @Override
+  public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() {
+    return ImmutableSetMultimap.of();
+  }
+


With the default method I think we can delete this

msridhar · 2024-01-16T22:30:10Z

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

+  /**
+   * Called to get the library models which have Nullable upper bound for their Generic Type
+   * Arguments.
+   *
+   * @param className A String containing the name of the class for which we want to check Nullable
+   *     upper bound.
+   * @param index Indicates the generic type argument for which we want to check Nullable upper
+   *     bound.
+   * @return boolean value of regardless of whether the upper bound for that argument has a Nullable
+   *     upper bound.
+   */
+  boolean onOverrideTypeParameterUpperBound(String className, int index);
+


Didn't I suggest some changes in these docs before? Did those changes get lost?

I added that in a javadoc for the implementation of the method inside CompositeHandler since it seemed specific to the implementation inside CompositeHandler, would it be more appropriate to mention it here instead?

msridhar · 2024-01-16T22:30:47Z

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

+        new ImmutableSetMultimap.Builder<String, Integer>()
+            .put("java.util.function.Function", 0)
+            .put("java.util.function.Function", 1)
+            .put("java.util.Map", 1)


I don't recall wanting to model java.util.Map in this PR. Why did we add this change?

Sorry, I had added it for a positive test case where only one of the indices has the upper bound and we still fail for the other. But in hindsight it is not the correct way to add a model for it. I'll remove it.

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

…sHandler.java Co-authored-by: Manu Sridharan <[email protected]>

…and TestLibraryModels

msridhar

Mostly looks good, just one comment remaining to be addressed

msridhar · 2024-01-17T19:27:47Z

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

+            "import java.util.function.Function;",
+            "class Test {",
+            "  static void testNegative() {",
+            "   Function<String,@Nullable String> removeA = a -> a.replace(\"a\",\"\");",


This comment is still not addressed

…els for generic types

…and updating relevant implementations.

…terface" This reverts commit 81f8b7d. "Reverting EmptyLibraryModels changes"

…ds and nullableFields

…and TestLibraryModels

…7' into library_models_Function_abhijitk7 # Conflicts: # nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java # nullaway/src/main/java/com/uber/nullaway/LibraryModels.java # nullaway/src/main/java/com/uber/nullaway/NullAway.java # nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java # nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java # nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java # nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

…7' into library_models_Function_abhijitk7 # Conflicts: # nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

This reverts commit 00a317a.

This reverts commit 986d060.

This reverts commit 115d683.

)" This reverts commit 091ac38

We should skip checking for errors here instead of crashing (cherry picked from commit 091ac38)

msridhar

Unfortunately something has gone wrong with this PR. It has removed changes that are present in the master branch and those removals are not showing up in the GitHub UI for some reason. Compare this branch's README.md Android section:

https://github.com/akulk022/NullAway/tree/library_models_Function_abhijitk7?tab=readme-ov-file#android

With the master branch:

https://github.com/uber/NullAway?tab=readme-ov-file#android

I think the best thing to do is to re-create this PR somehow, to make sure we're not losing master branch changes unintentionally.

msridhar · 2024-01-31T20:46:15Z

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

@@ -145,10 +147,11 @@ private static boolean isClassFieldOfPrimitiveType(Symbol symbol) {
   *
   * @param symbol symbol for entity
   * @param config NullAway config
+   * @param handler NullAway Handler


Suggested change

* @param handler NullAway Handler

* @param handler optional NullAway Handler

msridhar · 2024-01-31T20:46:35Z

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

@@ -176,12 +179,15 @@ public boolean isSymbolUnannotated(Symbol symbol, Config config) {
   * Check whether a class should be treated as nullness-annotated.
   *
   * @param classSymbol The symbol for the class to be checked
+   * @param config NullAway config
+   * @param handler NullAway handler


Suggested change

* @param handler NullAway handler

* @param handler optional NullAway handler

msridhar · 2024-01-31T20:47:00Z

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

@@ -190,12 +196,15 @@ public boolean isClassNullAnnotated(Symbol.ClassSymbol classSymbol, Config confi
   * <p>This method is recursive, using the cache on the way up and populating it on the way down.
   *
   * @param classSymbol The class to query, possibly an inner class
+   * @param config NullAway config
+   * @param handler NullAway handler


Suggested change

* @param handler NullAway handler

* @param handler optional NullAway handler

…els (#903) The below line previously reported an error considering the upperbound of the generic type parameter couldn't be Nullable, adding it into library models to allow it to be Nullable passes the test case. ```java Function<String,@nullable String> removeA = a -> a.replace("a",""); ``` We now have library model methods for treating a type as `@NullMarked` and also for making the upper bound of type variables `@Nullable`. We use these new methods to model `java.util.function.Function` as `@NullMarked`, in JSpecify mode only. We fix a related issue with unwanted errors when passing a lambda / method reference to `@NullUnmarked` code; this was necessary to get NullAway itself to still build with NullAway checking enabled. (Due to issues with the branch this PR was recreated from the original #893) --------- Co-authored-by: Manu Sridharan <[email protected]>

akulk022 added 2 commits January 10, 2024 15:10

Adding support for nullable type upper bounds considering Library mod…

f71df65

…els for generic types

Adding implementation for nullableVariableTypeUpperBounds method

e27b474

msridhar reviewed Jan 11, 2024

View reviewed changes

akulk022 added 2 commits January 11, 2024 14:41

Adding method onOverrideTypeParameterUpperBound to Handler Interface …

529a26b

…and updating relevant implementations.

Updating java docs

f10c491

akulk022 marked this pull request as ready for review January 12, 2024 01:12

akulk022 requested a review from msridhar January 12, 2024 01:13

msridhar reviewed Jan 12, 2024

View reviewed changes

updating variable name and removing redundant variable

b122a66

msridhar reviewed Jan 14, 2024

View reviewed changes

akulk022 and others added 10 commits January 14, 2024 21:44

Update nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

50578d7

Co-authored-by: Manu Sridharan <[email protected]>

updating var name and documentation

273a959

Merge remote-tracking branch 'origin/library_models_Function_abhijitk…

88a4799

…7' into library_models_Function_abhijitk7 # Conflicts: # nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

Merge branch 'master' into library_models_Function_abhijitk7

7849690

using putAll instead of loop

ce9609c

adding positive test case

b56733a

Adding EmptyLibraryModels abstract class for LibraryModels interface

81f8b7d

Revert "Adding EmptyLibraryModels abstract class for LibraryModels in…

488f005

…terface" This reverts commit 81f8b7d. "Reverting EmptyLibraryModels changes"

Adding documentation for onOverrideTypeParameterUpperBound in Composi…

2634b16

…teHandler.java

Adding default implementations for typeVariablesWithNullableUpperBoun…

2730d2a

…ds and nullableFields

msridhar reviewed Jan 16, 2024

View reviewed changes

akulk022 and others added 4 commits January 16, 2024 14:36

Update nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModel…

ec115af

…sHandler.java Co-authored-by: Manu Sridharan <[email protected]>

Removing redundant default implementations from ExampleLibraryModels …

f72959f

…and TestLibraryModels

Removing positive test case and incorrect java.util.Map library model

3e0fb0b

documentation improvements

ee15c59

msridhar reviewed Jan 17, 2024

View reviewed changes

akulk022 added 3 commits January 22, 2024 14:36

Adding support for nullable type upper bounds considering Library mod…

f7462aa

…els for generic types

Adding implementation for nullableVariableTypeUpperBounds method

2ee3522

Adding method onOverrideTypeParameterUpperBound to Handler Interface …

8699e85

…and updating relevant implementations.

akulk022 and others added 25 commits January 29, 2024 15:37

updating var name and documentation

32f84ac

using putAll instead of loop

5f6d9a8

adding positive test case

dcc1507

Adding EmptyLibraryModels abstract class for LibraryModels interface

4bba915

Revert "Adding EmptyLibraryModels abstract class for LibraryModels in…

d24e9c5

…terface" This reverts commit 81f8b7d. "Reverting EmptyLibraryModels changes"

Adding default implementations for typeVariablesWithNullableUpperBoun…

2ba7932

…ds and nullableFields

Removing redundant default implementations from ExampleLibraryModels …

2ea0b77

…and TestLibraryModels

Removing positive test case and incorrect java.util.Map library model

1c1316d

documentation improvements

924396c

updating isAnnotatedTopLevelClass

24d5ed0

bug fix, still more to do

0de4d12

more fixes, still needs cleanup

32aef72

docs

2acffb3

handler updates

e1e23d1

handler updates

fb23172

fixes

2dd0ca6

updating docs

badfd16

fixes

6fc4337

Merge remote-tracking branch 'origin/library_models_Function_abhijitk…

c97e8dc

…7' into library_models_Function_abhijitk7 # Conflicts: # nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

Revert "Prepare next development version."

05fe856

This reverts commit 00a317a.

Revert "Prepare for release 0.10.22."

37a2008

This reverts commit 986d060.

Revert "Update instructions for Android and our sample app (uber#900)"

57a2116

This reverts commit 115d683.

Revert "Fix crash with raw types in overrides in JSpecify mode (uber#899

e2beddc

)" This reverts commit 091ac38

Fix crash with raw types in overrides in JSpecify mode (uber#899)

f6996d1

We should skip checking for errors here instead of crashing (cherry picked from commit 091ac38)

msridhar requested changes Jan 31, 2024

View reviewed changes

akulk022 marked this pull request as draft February 1, 2024 00:49

akulk022 closed this Feb 1, 2024

akulk022 mentioned this pull request Feb 1, 2024

Adding support for nullable type upper bounds considering Library models #903

Merged

	ImmutableSetMultimap<String, Integer> nullableVariableTypeUpperBounds();
	ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds();

	boolean onOverrideTypeParameterUpperBound(String className, int arg);
	boolean onOverrideTypeParameterUpperBound(String className, int index);

	* @param handler NullAway Handler
	* @param handler optional NullAway Handler

Adding support for nullable type upper bounds considering Library mod… #893

Adding support for nullable type upper bounds considering Library mod… #893

Conversation

akulk022 commented Jan 10, 2024

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akulk022 Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

akulk022 Jan 16, 2024 • edited Loading

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

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

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

akulk022 Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

codecov bot commented Jan 10, 2024 •

edited

Loading

akulk022 Jan 11, 2024 •

edited

Loading

akulk022 Jan 16, 2024 •

edited

Loading

akulk022 Jan 16, 2024 •

edited

Loading