Better @MonotonicNonNull support
#1149
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1149 +/- ##
============================================
+ Coverage 88.12% 88.15% +0.02%
- Complexity 2268 2281 +13
============================================
Files 87 87
Lines 7438 7455 +17
Branches 1484 1490 +6
============================================
+ Hits 6555 6572 +17
Misses 445 445
Partials 438 438 ☔ View full report in Codecov by Sentry. |
| @@ -480,7 +480,7 @@ public static boolean mayBeNullFieldFromType( | |||
| return !(symbol.getSimpleName().toString().equals("class") | |||
| || symbol.isEnum() | |||
| || codeAnnotationInfo.isSymbolUnannotated(symbol, config, null)) | |||
| && Nullness.hasNullableAnnotation(symbol, config); | |||
| && Nullness.hasNullableFieldAnnotation(symbol, config); | |||
There was a problem hiding this comment.
nit: At a first glance, it is not obvious that 'hasNullableFieldAnnotation' includes both @nullable and @MonotonicNonNull. It sounds like it only includes the @nullable annotation.
One possibility is to rename it to something like "hasNullabilityFieldAnnotation", so that one does not mistake it to only include @nullable annotation.
There was a problem hiding this comment.
Renamed to hasNullableOrMonotonicNonNullAnnotation in 1aa9747
| || !e.getModifiers().contains(Modifier.FINAL)) { | ||
| allAPNonRootElementsAreFinalFields = false; | ||
| break; | ||
| if (i != elements.size() - 1) { // "inner" elements of the access path, must be fields |
There was a problem hiding this comment.
Why must inner elements of the access path be fields?
We 'need them to be fields', but can't assume that they 'must be fields'. Why has the original "e.getKind().equals(ElementKind.FIELD)" check become unnecessary?
There was a problem hiding this comment.
You're right, this is sketchy. I was trying to simplify logic but may have over-simplified. Will dig in and give a better explanation.
There was a problem hiding this comment.
I restored the checks for fields and added tests showing why they are required in bbfd36c
| Nullness.isMonotonicNonNullAnnotation( | ||
| am.getAnnotationType().toString()))) { | ||
| allAPNonRootElementsAreFinalFields = false; | ||
| break; |
There was a problem hiding this comment.
nit: unnecessary break since this is the last iteration.
Fixes #1148
We add explicit support for any annotation named
@MonotonicNonNulland add our own version of the annotation to our annotations package. The main additional support is that we now reason that once assigned a non-null value,@MonotonicNullfields remain non-null when accessed from subsequent lambdas, even if the lambdas are invoked asynchronously.