Skip to content
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

Finish annotating some classes for nullness. #71

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cpovirk
Copy link

@cpovirk cpovirk commented Aug 3, 2020

Parts of this are relevant to
typetools/checker-framework#3197

Other parts are a followup to
typetools/checker-framework#3063

Merge with typetools/checker-framework#3549

@@ -509,7 +512,8 @@ public int remainingCapacity() {
* @param o element to be removed from this queue, if present
* @return {@code true} if this queue changed as a result of the call
*/
public boolean remove(@Nullable Object o) {
@CFComment("probably accepts null in practice, but docs at best imply this")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like typetools/checker-framework#3063 didn't touch this file. (That is, this annotation was there before that PR.) I'm not sure if that means that you hadn't looked at it at the time or that you found some reason to justify omitting @Nullable here.

Likewise for the other queues/deques in this PR.

@@ -262,7 +264,7 @@ public int indexOf(@Nullable Object o) {
* {@code -1} if the element is not found.
* @throws IndexOutOfBoundsException if the specified index is negative
*/
public int indexOf(E e, int index) {
public int indexOf(@Nullable E e, int index) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nullable here is pretty weird. It looks well justified to me by the contract (and implementation). However, I'm not sure how often callers would want to pass null here. I'm not sure if you'd want to leave @Nullable off for that reason.

@@ -1478,15 +1483,15 @@ public boolean removeIf(Predicate<? super E> filter) {
/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Copy link
Author

@cpovirk cpovirk Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This takes us back to typetools/checker-framework#3197

removeAll and retainAll officially throw only if both:

  • the argument throws on null queries
  • the receiver contains nulls

In this case, the latter condition can't hold. So any call is safe.

Below is a different case: That collection can contain nulls, so the only provably safe argument is another collection that can contain nulls.

Ao-senXiong pushed a commit to Ao-senXiong/jdkopprop that referenced this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants