JSpecify: handle Nullability for lambda expression parameters for Generic Types #852
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #852 +/- ##
============================================
+ Coverage 86.90% 86.91% +0.01%
- Complexity 1879 1881 +2
============================================
Files 74 74
Lines 6199 6206 +7
Branches 1202 1204 +2
============================================
+ Hits 5387 5394 +7
Misses 405 405
Partials 407 407
☔ View full report in Codecov by Sentry. |
akulk022
changed the title
| // This obtains the Types of the functional interface method with preserved annotations in case | ||
| // of generic types | ||
| List<Type> overridenMethodParamTypeList = | ||
| types.memberType(ASTHelpers.getType(code), fiMethodSymbol).getParameterTypes(); |
There was a problem hiding this comment.
I was expecting that somewhere we would have to check the config to see if we were in JSpecify mode. I think that coded this way, this will change NullAway's behavior outside of JSpecify mode, which could turn into a compatibility issue for existing code. Could we change this PR so that the changes only apply in JSpecify mode? Also maybe we should write a test to check that the NullAway behavior when JSpecify mode is off is unchanged.
There was a problem hiding this comment.
Done! Let me know if this works
…nto generic_types_lambda_abhijitk
There was a problem hiding this comment.
The code changes look good but we need more tests here. Can we write a test where the lambda takes two parameters, and only one of them is @Nullable according to the generic type arguments?
Also, this PR handles issues with lambda parameters but not return statements in lambdas. We should make clear in the PR description that handling returns from lambdas will come later.
akulk022
changed the title
akulk022
changed the title
…nto generic_types_lambda_abhijitk
msridhar
added
the
jspecify
…or Generic Types. (#854) ```java class Test { interface A<T1 extends @nullable Object> { T1 function(Object o); } static void testPositive() { // BUG: Diagnostic contains: returning @nullable expression from method with @nonnull return type A<String> p = x -> null; } static void testNegative() { // We were reporting a false positive here since the indirect Nullability through the generic was not handled correctly. A<@nullable String> p = x -> null; } } ``` For the negative scenario mentioned above where we do not expect an exception, we were getting a false positive. After computing the Nullability correctly when it is indirectly applied through the generic type we no longer get this false positive. All the test cases in NullAwayJSpecifyGenericTests.java have passed for these changes. Similar changes for handling lambda parameters were made under #852 --------- Co-authored-by: Manu Sridharan <[email protected]>
We were not getting the desired error for the positive test in the following test case:
Upon changing the way we compute the Nullability of the functional interface parameters, we are able to get the desired error.
Note: These changes only affect lambda parameters and do not deal with return types. That will be addressed in a future PR.
All the test cases in NullAwayJSpecifyGenericTests.java have passed for these changes.