-
Notifications
You must be signed in to change notification settings - Fork 295
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
JSpecify: handle intersection type in one place #1015
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1015 +/- ##
============================================
+ Coverage 85.98% 86.00% +0.01%
- Complexity 2080 2086 +6
============================================
Files 83 83
Lines 6892 6901 +9
Branches 1328 1329 +1
============================================
+ Hits 5926 5935 +9
Misses 551 551
Partials 415 415 ☔ View full report in Codecov by Sentry. |
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, minimal tests nit, we might want the corresponding testNegative1
and testNegative2
cases, assuming I understand what intersection types do correctly.
" var x = (A<String> & Serializable) o;", | ||
" // BUG: Diagnostic contains: Cannot assign from type B to type A<String> & Serializable", | ||
" x = new B();", | ||
" }", |
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.
To be clear, the case where we have:
" var x = (A<@Nullable String> & Serializable) o;",
" x = new B();",
Should have no error, right? Should we encode that test case just to make sure we never regress 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.
you're right, and i think we will get a false positive on that test for now (since for some reason var
-declared variables don't get types with all the right annotations). i'll add tests and open an issue for outstanding tasks, but will fix in a subsequent PR
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.
Intersection types may arise, e.g., as part of string concatenation. We handling of an intersection type for one scenario; broader handling definitely requires more testing and thinking.
Fixes #1013