Skip to content

Improve annotations on some java.util.concurrent collections. #9

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.

Sign up for GitHub

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

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

Improve annotations on some java.util.concurrent collections. #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e13d517
Improve annotations on some java.util.concurrent collections.
cpovirk Apr 28, 2022
4ae27b5
Merge branch 'master' into jucc
wmdietl May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/java.base/share/classes/java/util/concurrent/ArrayBlockingQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1481,15 +1481,15 @@ public boolean removeIf(Predicate<? super E> filter) {
/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> c.contains(e));
}

/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> !c.contains(e));
}
Expand Down
10 changes: 6 additions & 4 deletions src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.dataflow.qual.SideEffectFree;
import org.checkerframework.framework.qual.AnnotatedFor;

import java.lang.invoke.VarHandle;
import java.lang.reflect.Field;
Expand Down Expand Up @@ -97,6 +98,7 @@
* @author Doug Lea
* @param <E> the type of elements held in this list
*/
@AnnotatedFor({"nullness"})
public class CopyOnWriteArrayList<E>
implements List<E>, RandomAccess, Cloneable, java.io.Serializable {
private static final long serialVersionUID = 8673264195747942595L;
Expand Down Expand Up @@ -263,7 +265,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
Copy Markdown
Member

@wmdietl wmdietl May 3, 2022

Choose a reason for hiding this comment

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

Why is this change desirable (same for lastIndexOf below)? If the collection only accepts non-null elements, why allow checking with a null value? It won't throw an NPE, but it will always be false.
The indexOf variant that takes an Object needs to take an @Nullable Object parameter to allow both instantiations of the collection, but that doesn't apply here.

If you think they are indeed correct, having a @CFComment would be useful.

Copy link
Copy Markdown
Contributor Author

@cpovirk cpovirk May 3, 2022

Choose a reason for hiding this comment

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

Part of my goal here is consistency, too: contains already has a @Nullable parameter. So let's make them all @Nullable or all not @Nullable.

The broader question is still a good one. As in our retainAll discussion, I've been operating under the principle of "accept the most general type that can't lead to NPE," but of course we're well aware that the principles here are controversial :) (That's not necessarily even my personal favored principle.)

I probably should have sent the poll and peek changes (which probably actually affect users) separately from the more complicated changes. It would be nice to get those in. But for the changes we've been discussing, I don't think we're affected much by what we decide to do, so we can continue discussing or just drop them, as you prefer.

Copy link
Copy Markdown
Member

@wmdietl wmdietl May 4, 2022

Choose a reason for hiding this comment

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

contains takes an Object parameter, so that discussion should go along with the other Object vs. @Nullable Object methods.
For methods that take the type parameter, I don't see a benefit in allowing null. I agree that it wouldn't cause an NPE, but it would just allow calls that will return false. Is there a particular pattern where this would be much more convenient?

Copy link
Copy Markdown
Contributor Author

@cpovirk cpovirk May 4, 2022

Choose a reason for hiding this comment

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

Ah, thanks, I had forgotten about the distinction of Object vs. E. I am a bit surprised that they use E here, given the precedent of List.indexOf, but of course there are good arguments for not permitting calls that will always return false (compare https://errorprone.info/bugpattern/CollectionIncompatibleType). And if their goal is to use E to forbid such calls, then it's natural to extend that argument to keeping the type as E instead of @Nullable E.

I might still lean toward @Nullable E if we were writing The Official JSpecify Stubs: The theory would be that, if we didn't, tools could choose to insert runtime null checks for code that calls indexOf on a CopyOnWriteArrayList<@NonNull String>. But my recollection is that the only tool in common use that inserts such checks, Kotlin, does not insert checks in that particular case.

But for Checker Framework users (and for all non-hypothetical use cases), I'm not aware of examples in which @Nullable would improve anyone's life.

Object[] es = getArray();
return indexOfRange(e, es, index, es.length);
}
Expand Down Expand Up @@ -292,7 +294,7 @@ public int lastIndexOf(@Nullable Object o) {
* @throws IndexOutOfBoundsException if the specified index is greater
* than or equal to the current size of this list
*/
public int lastIndexOf(E e, int index) {
public int lastIndexOf(@Nullable E e, int index) {
Object[] es = getArray();
return lastIndexOfRange(e, es, 0, index + 1);
}
Expand Down Expand Up @@ -659,7 +661,7 @@ public boolean containsAll(Collection<?> c) {
* or if the specified collection is null
* @see #remove(Object)
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> c.contains(e));
}
Expand All @@ -680,7 +682,7 @@ public boolean removeAll(Collection<? extends @NonNull Object> c) {
* or if the specified collection is null
* @see #remove(Object)
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> !c.contains(e));
}
Expand Down
6 changes: 4 additions & 2 deletions src/java.base/share/classes/java/util/concurrent/CopyOnWriteArraySet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.dataflow.qual.SideEffectFree;
import org.checkerframework.framework.qual.AnnotatedFor;

import java.util.AbstractSet;
import java.util.Collection;
Expand Down Expand Up @@ -102,6 +103,7 @@
* @author Doug Lea
* @param <E> the type of elements held in this set
*/
@AnnotatedFor({"nullness"})
public class CopyOnWriteArraySet<E> extends AbstractSet<E>
implements java.io.Serializable {
private static final long serialVersionUID = 5457747651344034263L;
Expand Down Expand Up @@ -356,7 +358,7 @@ public boolean addAll(Collection<? extends E> c) {
* or if the specified collection is null
* @see #remove(Object)
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
return al.removeAll(c);
}

Expand All @@ -379,7 +381,7 @@ public boolean removeAll(Collection<? extends @NonNull Object> c) {
* or if the specified collection is null
* @see #remove(Object)
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
return al.retainAll(c);
}

Expand Down
10 changes: 5 additions & 5 deletions src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ public E getLast() {
}
}

public boolean removeFirstOccurrence(Object o) {
public boolean removeFirstOccurrence(@Nullable Object o) {
Copy link
Copy Markdown
Member

@wmdietl wmdietl May 3, 2022

Choose a reason for hiding this comment

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

Should this change (and the ones to removeLastOccurrence and remove below) go into collection-object-parameters-may-be-null.astub instead?
I see some entries there, e.g. for Deque.

Copy link
Copy Markdown
Contributor Author

@cpovirk cpovirk May 3, 2022

Choose a reason for hiding this comment

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

My impression has been that these files haven't quite reached a consistent state.

LinkedBlockingDeque already has @Nullable on its contains method. That method's docs don't mention null explicitly, but they do say...

this deque contains at least one element e such that o.equals(e)

...which at best implies that null is an acceptable input.

The methods I'm annotating in this PR are similar. So I think it would be defensible to remove the annotation on contains, rather than add the ones in this PR. (And as you say, we could update collection-object-parameters-may-be-null.astub to have the annotations for all the methods.)

I can do that, but I'll wait for confirmation just in case. But probably nothing I've said above will come as a surprise :)

Copy link
Copy Markdown
Member

@wmdietl wmdietl May 4, 2022

Choose a reason for hiding this comment

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

It unfortunately looks like there is some inconsistency here about which methods take @Nullable Object here or in collection-object-parameters-may-be-null.astub and I don't see a pattern.
As most of the changes in this PR are related to this, I think it would be great to merge the other changes and separately clean these methods.

if (o == null) return false;
final ReentrantLock lock = this.lock;
lock.lock();
Expand All @@ -598,7 +598,7 @@ public boolean removeFirstOccurrence(Object o) {
}
}

public boolean removeLastOccurrence(Object o) {
public boolean removeLastOccurrence(@Nullable Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
lock.lock();
Expand Down Expand Up @@ -790,7 +790,7 @@ public E pop() {
* @param o element to be removed from this deque, if present
* @return {@code true} if this deque changed as a result of the call
*/
public boolean remove(Object o) {
public boolean remove(@Nullable Object o) {
return removeFirstOccurrence(o);
}

Expand Down Expand Up @@ -1344,15 +1344,15 @@ public boolean removeIf(Predicate<? super E> filter) {
/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> c.contains(e));
}

/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
Comment thread
wmdietl marked this conversation as resolved.
Objects.requireNonNull(c);
return bulkRemove(e -> !c.contains(e));
}
Expand Down
4 changes: 2 additions & 2 deletions src/java.base/share/classes/java/util/concurrent/LinkedBlockingQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,15 +1031,15 @@ public boolean removeIf(Predicate<? super E> filter) {
/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> c.contains(e));
}

/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> !c.contains(e));
}
Expand Down
4 changes: 2 additions & 2 deletions src/java.base/share/classes/java/util/concurrent/PriorityBlockingQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,15 +1036,15 @@ public boolean removeIf(Predicate<? super E> filter) {
/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean removeAll(Collection<? extends @NonNull Object> c) {
public boolean removeAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> c.contains(e));
}

/**
* @throws NullPointerException {@inheritDoc}
*/
public boolean retainAll(Collection<? extends @NonNull Object> c) {
public boolean retainAll(Collection<?> c) {
Objects.requireNonNull(c);
return bulkRemove(e -> !c.contains(e));
}
Expand Down