-
Notifications
You must be signed in to change notification settings - Fork 288
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
Adding support for nullable type upper bounds considering Library mod… #893
Changes from 5 commits
f71df65
e27b474
529a26b
f10c491
b122a66
50578d7
273a959
88a4799
7849690
ce9609c
b56733a
81f8b7d
488f005
2634b16
2730d2a
ec115af
f72959f
3e0fb0b
ee15c59
f7462aa
2ee3522
8699e85
ae62aaa
c3eb477
c3e17cf
107d40c
e80e35f
0cfd0e2
dc1af63
e4d5813
3bf421d
334af66
20f3fe1
2453b08
c70ec1d
84247a2
ac94e7a
6b92af4
27918ff
026970c
4e829d7
b94597a
091ac38
115d683
373917e
986d060
00a317a
35eac21
71cecb4
18a0c6d
1f28ff5
c911842
9aea517
ba88839
75436eb
865873e
91a12f4
db16c7a
836ff44
6a9d7c1
cea3614
7bf7497
37c7cb0
26c82dd
cd0e659
1bf78df
268022a
5e5df6a
6c24cbd
32f84ac
5f6d9a8
dcc1507
4bba915
d24e9c5
2ba7932
2ea0b77
1c1316d
924396c
24d5ed0
0de4d12
32aef72
2acffb3
e1e23d1
fb23172
bc0edd1
2dd0ca6
badfd16
6fc4337
c97e8dc
05fe856
37a2008
57a2116
e2beddc
f6996d1
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 |
---|---|---|
|
@@ -305,4 +305,16 @@ public Integer castToNonNullArgumentPositionsForMethod( | |
} | ||
return previousArgumentPosition; | ||
} | ||
|
||
@Override | ||
public boolean onOverrideTypeParameterUpperBound(String className, int arg) { | ||
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. Document in Javadoc that this returns true if any handler returns true |
||
boolean result = false; | ||
for (Handler h : handlers) { | ||
result = h.onOverrideTypeParameterUpperBound(className, arg); | ||
if (result) { | ||
break; | ||
} | ||
} | ||
return result; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -402,6 +402,18 @@ Integer castToNonNullArgumentPositionsForMethod( | |||||
List<? extends ExpressionTree> actualParams, | ||||||
@Nullable Integer previousArgumentPosition); | ||||||
|
||||||
/** | ||||||
* 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 arg 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. | ||||||
*/ | ||||||
akulk022 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
boolean onOverrideTypeParameterUpperBound(String className, int arg); | ||||||
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.
Suggested change
|
||||||
|
||||||
/** | ||||||
* A three value enum for handlers implementing onDataflowVisitMethodInvocation to communicate | ||||||
* their knowledge of the method return nullability to the rest of NullAway. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,6 +343,12 @@ private void setUnconditionalArgumentNullness( | |
} | ||
} | ||
|
||
@Override | ||
public boolean onOverrideTypeParameterUpperBound(String className, int arg) { | ||
ImmutableSet<Integer> res = libraryModels.typeVariablesWithNullableUpperBounds().get(className); | ||
return res.contains(arg); | ||
} | ||
|
||
/** | ||
* Get all the stream specifications loaded from any of our library models. | ||
* | ||
|
@@ -826,7 +832,11 @@ private static class DefaultLibraryModels implements LibraryModels { | |
"getDrawable(android.content.Context,int)")) | ||
.add(methodRef("android.support.design.widget.TextInputLayout", "getEditText()")) | ||
.build(); | ||
|
||
private static final ImmutableSetMultimap<String, Integer> NULLABLE_VARIABLE_TYPE_UPPER_BOUNDS = | ||
new ImmutableSetMultimap.Builder<String, Integer>() | ||
.put("java.util.function.Function", 0) | ||
.put("java.util.function.Function", 1) | ||
.build(); | ||
private static final ImmutableSetMultimap<MethodRef, Integer> CAST_TO_NONNULL_METHODS = | ||
new ImmutableSetMultimap.Builder<MethodRef, Integer>().build(); | ||
|
||
|
@@ -870,6 +880,11 @@ public ImmutableSet<MethodRef> nonNullReturns() { | |
return NONNULL_RETURNS; | ||
} | ||
|
||
@Override | ||
public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() { | ||
return NULLABLE_VARIABLE_TYPE_UPPER_BOUNDS; | ||
} | ||
|
||
@Override | ||
public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() { | ||
return CAST_TO_NONNULL_METHODS; | ||
|
@@ -902,6 +917,8 @@ private static class CombinedLibraryModels implements LibraryModels { | |
|
||
private final ImmutableSet<MethodRef> nonNullReturns; | ||
|
||
private final ImmutableSetMultimap<String, Integer> nullableVariableTypeUpperBounds; | ||
|
||
private final ImmutableSet<FieldRef> nullableFields; | ||
|
||
private final ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods; | ||
|
@@ -916,6 +933,8 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) { | |
new ImmutableSetMultimap.Builder<>(); | ||
ImmutableSetMultimap.Builder<MethodRef, Integer> nonNullParametersBuilder = | ||
new ImmutableSetMultimap.Builder<>(); | ||
ImmutableSetMultimap.Builder<String, Integer> nullableVariableTypeUpperBoundsBuilder = | ||
new ImmutableSetMultimap.Builder<>(); | ||
ImmutableSetMultimap.Builder<MethodRef, Integer> nullImpliesTrueParametersBuilder = | ||
new ImmutableSetMultimap.Builder<>(); | ||
ImmutableSetMultimap.Builder<MethodRef, Integer> nullImpliesFalseParametersBuilder = | ||
|
@@ -988,6 +1007,10 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) { | |
} | ||
castToNonNullMethodsBuilder.put(entry); | ||
} | ||
for (Map.Entry<String, Integer> entry : | ||
libraryModels.typeVariablesWithNullableUpperBounds().entries()) { | ||
nullableVariableTypeUpperBoundsBuilder.put(entry); | ||
} | ||
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. Use |
||
for (StreamTypeRecord streamTypeRecord : libraryModels.customStreamNullabilitySpecs()) { | ||
customStreamNullabilitySpecsBuilder.add(streamTypeRecord); | ||
} | ||
|
@@ -1006,6 +1029,7 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) { | |
castToNonNullMethods = castToNonNullMethodsBuilder.build(); | ||
customStreamNullabilitySpecs = customStreamNullabilitySpecsBuilder.build(); | ||
nullableFields = nullableFieldsBuilder.build(); | ||
nullableVariableTypeUpperBounds = nullableVariableTypeUpperBoundsBuilder.build(); | ||
} | ||
|
||
private boolean shouldSkipModel(MethodRef key) { | ||
|
@@ -1052,6 +1076,11 @@ public ImmutableSet<MethodRef> nonNullReturns() { | |
return nonNullReturns; | ||
} | ||
|
||
@Override | ||
public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() { | ||
return nullableVariableTypeUpperBounds; | ||
} | ||
|
||
@Override | ||
public ImmutableSet<FieldRef> nullableFields() { | ||
return nullableFields; | ||
|
@@ -1068,16 +1097,13 @@ public ImmutableList<StreamTypeRecord> customStreamNullabilitySpecs() { | |
} | ||
} | ||
|
||
/** | ||
* A view of library models optimized to make lookup of {@link | ||
* com.sun.tools.javac.code.Symbol.MethodSymbol}s fast | ||
*/ | ||
/** A view of library models optimized to make lookup of {@link Symbol.MethodSymbol}s fast */ | ||
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. | ||
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. Why this change? 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. I am not sure why this went in there, It was probably the IDE that did something, I'll revert it. |
||
* | ||
* @param <T> the type of the associated state. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1615,6 +1615,22 @@ public void testForNullTypeRhsTypeForArrayType() { | |
.doTest(); | ||
} | ||
|
||
@Test | ||
public void testForUtilFunctionLibModel() { | ||
makeHelper() | ||
.addSourceLines( | ||
"Test.java", | ||
"package com.uber;", | ||
"import org.jspecify.annotations.Nullable;", | ||
"import java.util.function.Function;", | ||
"class Test {", | ||
" static void testNegative() {", | ||
" Function<String,@Nullable String> removeA = a -> a.replace(\"a\",\"\");", | ||
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. Let's write more tests to show the type argument is being handled correctly. E.g., check that passing in 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. This comment is still not addressed |
||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
private CompilationTestHelper makeHelper() { | ||
return makeTestHelperWithArgs( | ||
Arrays.asList( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,11 @@ public ImmutableSet<MethodRef> nonNullReturns() { | |
return ImmutableSet.of(); | ||
} | ||
|
||
@Override | ||
public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() { | ||
return ImmutableSetMultimap.of(); | ||
} | ||
|
||
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. Code like in this class could be the 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. @akulk022 we can now delete this method right? |
||
@Override | ||
public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() { | ||
return ImmutableSetMultimap.of(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,11 @@ public ImmutableSet<MethodRef> nonNullReturns() { | |
return ImmutableSet.of(); | ||
} | ||
|
||
@Override | ||
public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() { | ||
return ImmutableSetMultimap.of(); | ||
} | ||
|
||
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. With the |
||
@Override | ||
public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() { | ||
return ImmutableSetMultimap.<MethodRef, Integer>builder() | ||
|
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.
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, theLibraryModels
interface is where this method belongs. I am leaning towards allowing this method here, but maybe we should also provide an abstract class likeEmptyLibraryModels
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?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.
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.
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.
Yeah, that's what I'm thinking. Our current implementations should extend the new empty library models class.
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.
Thanks. Makes sense, I'll do that.
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.
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 maketypeVariablesWithNullableUpperBounds()
adefault
method that returns the empty set and then all existing implementations will just work without any changes.@akulk022 could you do the following:
typeVariablesWithNullableUpperBounds()
adefault
method.LibraryModels.nullableFields()
adefault
method that returns an empty set. We should have done this when introducing this method before but I forgot.EmptyLibraryModels
, and undo the changes toExampleLibraryModels
andTestLibraryModels
.Sorry again for not remembering about
default
methods but they are definitely the best way to go here.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.
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.