Extend library models to mark fields as nullable #878
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #878 +/- ##
============================================
+ Coverage 87.03% 87.10% +0.06%
- Complexity 1926 1940 +14
============================================
Files 77 77
Lines 6231 6264 +33
Branches 1212 1218 +6
============================================
+ Hits 5423 5456 +33
Misses 405 405
Partials 403 403 ☔ View full report in Codecov by Sentry. |
| @@ -244,4 +252,58 @@ public String toString() { | |||
| + '}'; | |||
| } | |||
| } | |||
|
|
|||
| /** Representation of a field as a qualified class name + a field name */ | |||
| final class FieldRef { | |||
There was a problem hiding this comment.
Can we just use AutoValue for this class, since we already rely on it? Would get rid of a lot of boilerplate. See, e.g.:
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
Show resolved
Hide resolved
test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
Show resolved
Hide resolved
…ipour/NullAway into nimak/add-field-library-models
|
@msridhar Thanks for the review, this is ready for another round. I think it might be better to not land this even after approvals, and land it once I can locally test it with the updates on Annotator. |
nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
Show resolved
Hide resolved
| // check presence. This is a bit inefficient, but it's only used while indexing impact of | ||
| // making fields @Nullable on downstream dependencies. Also, the set of nullable fields is not | ||
| // expected to be large. We can optimize in future if needed. | ||
| return libraryModels.nullableFields().stream() | ||
| .anyMatch( | ||
| fieldRef -> | ||
| fieldRef.getFieldName().equals(fieldName) | ||
| && fieldRef.getEnclosingClassName().equals(enclosingClassName)); |
There was a problem hiding this comment.
One last question. Why not just do return libraryModels.nullableFields().contains(fieldRef(enclosingClassName,fieldName))? We don't care about performance too much, but that should be constant time assuming hashing, and also it seems clearer.
There was a problem hiding this comment.
Sure, make sense. 1728198
This reverts commit 3ee7072.
Followup on an update in NullAway recent change [#878](uber/NullAway#878) which enables NullAway to mark fields as `@Nullable`. This PR updates our `LibraryModelsLoader` to enable communication of Annotator and NullAway to mark fields `@Nullable` while analyzing downstream dependencies. This is required to prepare the code for the upcoming change which enables Annotator to be informed of impacts of making fields `@Nullable` on downstream dependencies.
This PR extends
LibraryModelsto add support for marking fields@Nullable. This feature is required to enable Annotator index impact of making fields@Nullableon downstream dependencies.Please note, since this feature is mostly going to be used while indexing impacts on downstream dependencies, this PR does not focus on the optimality of the implementation. Also the working set in iterations is not expected to be large. We can optimize the implementation in a follow up PR if needed.