-
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 4 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 |
---|---|---|
|
@@ -29,6 +29,7 @@ | |
import com.uber.nullaway.ErrorMessage; | ||
import com.uber.nullaway.NullAway; | ||
import com.uber.nullaway.Nullness; | ||
import com.uber.nullaway.handlers.Handler; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
@@ -57,9 +58,14 @@ private GenericsChecks() {} | |
* @param state visitor state | ||
* @param analysis the analysis object | ||
* @param config the analysis config | ||
* @param handler the handler instance | ||
*/ | ||
public static void checkInstantiationForParameterizedTypedTree( | ||
ParameterizedTypeTree tree, VisitorState state, NullAway analysis, Config config) { | ||
ParameterizedTypeTree tree, | ||
VisitorState state, | ||
NullAway analysis, | ||
Config config, | ||
Handler handler) { | ||
if (!config.isJSpecifyMode()) { | ||
return; | ||
} | ||
|
@@ -97,9 +103,11 @@ public static void checkInstantiationForParameterizedTypedTree( | |
upperBound.getAnnotationMirrors(); | ||
boolean hasNullableAnnotation = | ||
Nullness.hasNullableAnnotation(annotationMirrors.stream(), config); | ||
boolean hasNullableUpperBoundLibModel = | ||
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 don't think we need a new variable here. We could just do 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. Done |
||
handler.onOverrideTypeParameterUpperBound(baseType.tsym.toString(), i); | ||
// if base type argument does not have @Nullable annotation then the instantiation is | ||
// invalid | ||
if (!hasNullableAnnotation) { | ||
if (!hasNullableAnnotation && !hasNullableUpperBoundLibModel) { | ||
reportInvalidInstantiationError( | ||
nullableTypeArguments.get(i), baseType, typeVariable, state, analysis); | ||
} | ||
|
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.nullableVariableTypeUpperBounds().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> nullableVariableTypeUpperBounds() { | ||
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.nullableVariableTypeUpperBounds().entries()) { | ||
nullableVariableTypeUpperBoundsBuilder.put(entry); | ||
} | ||
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> nullableVariableTypeUpperBounds() { | ||
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( | ||
|
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.