-
Notifications
You must be signed in to change notification settings - Fork 294
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
Generics checks for method overriding #755
Conversation
88e5f09
to
362ee2f
Compare
Thanks for the great review @lazaroclapp! I've pushed a few fixes and cleanups, and I think this is ready for another pass. In particular, while testing the |
/benchmark |
Main Branch:
With This 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.
Ok, finally did a pass on this! There are a few nits that occurred to me, but nothing blocking :)
nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
Outdated
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
Show resolved
Hide resolved
Thanks for the detailed review @lazaroclapp! Addressed all remaining comments and will land this one! |
…s classes (#808) This completes a task lingering from #755, which did not handle overriding in anonymous classes. This case is slightly tricky as javac does not preserve annotations on type arguments in the types it computes for anonymous classes. To fix, we pass a `Symbol` where we were passing a `Type` in a couple places, as then we can call `Symbol.isAnonymous()` to check for an anonymous type. In such a case, we try to get the type from the enclosing `NewClassTree` of the overriding method. Note that this PR still does not handle anonymous classes that use the diamond operator; we add a test to show the gaps.
This PR implements JSpecify-related generics checks for method overriding, including:
See the new tests and docs for further examples.
Note: This PR does not handle overriding in explicitly-typed anonymous inner classes. Those will be handled in a follow-up PR, to keep the PR size manageable.