-
Notifications
You must be signed in to change notification settings - Fork 299
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
Fix bug with computing direct type use annotations on parameters #864
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #864 +/- ##
=========================================
Coverage 87.02% 87.02%
- Complexity 1921 1922 +1
=========================================
Files 77 77
Lines 6219 6219
Branches 1209 1210 +1
=========================================
Hits 5412 5412
Misses 403 403
Partials 404 404 ☔ View full report in Codecov by Sentry. |
@@ -102,7 +102,7 @@ def test = [ | |||
"org.junit.jupiter:junit-jupiter-api:5.0.2", | |||
"org.apiguardian:apiguardian-api:1.0.0" | |||
], | |||
jetbrainsAnnotations : "org.jetbrains:annotations:13.0", | |||
jetbrainsAnnotations : "org.jetbrains:annotations:24.1.0", |
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.
Needed so that @NotNull
is a type use annotation
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.
LGTM! One quick question above, but this is ready to land either way.
"class TypeArgumentAnnotation {", | ||
" List<@Nullable String> fSafe = new ArrayList<>();", | ||
" @Nullable List<String> fUnsafe = new ArrayList<>();", | ||
" void useParamSafe(List<@Nullable String> list) {", | ||
" list.hashCode();", | ||
" }", | ||
" void unsafeCall() {", | ||
" // BUG: Diagnostic contains: passing @Nullable parameter", | ||
" useParamSafe(null);", |
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.
Before this change, this was a false negative? (Either way, great to have the test, but just curious)
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.
Yes that's right, this was a false negative before
NullAway was still treating annotations on generic type arguments as being on the top-level type of a parameter itself, which could lead to false positives and potentially also missed bugs.