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.

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

Finish annotating some classes for nullness. #71

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

Finish annotating some classes for nullness. #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7f2607
Finish annotating some classes for nullness.
cpovirk Aug 3, 2020
0d42c9a
Add missing imports.
cpovirk Aug 3, 2020
760559b
Merge ../jdk-branch-master into alreadyannotated
mernst Aug 12, 2020
b68ae06
Merge ../jdk-branch-master into alreadyannotated
mernst Sep 8, 2020
2f6f0e0
Merge ../jdk-branch-master into alreadyannotated
mernst May 29, 2021
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
2 changes: 1 addition & 1 deletion src/java.base/share/classes/java/io/InputStreamReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
* @since 1.1
*/

@AnnotatedFor({"index", "mustcall"})
@AnnotatedFor({"index", "mustcall", "nullness"})
public class InputStreamReader extends Reader {

private final StreamDecoder sd;
Expand Down
10 changes: 5 additions & 5 deletions src/java.base/share/classes/java/util/Scanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@
*
* @since 1.5
*/
@AnnotatedFor({"index", "interning", "lock", "mustcall", "signedness"})
@AnnotatedFor({"index", "interning", "lock", "mustcall", "nullness", "signedness"})
public final @UsesObjectEquals class Scanner implements Iterator<String>, Closeable {

// Internal buffer used to hold input
Expand Down Expand Up @@ -1687,7 +1687,7 @@ public String nextLine(@GuardSatisfied Scanner this) {
* @return the text that matched the specified pattern
* @throws IllegalStateException if this scanner is closed
*/
public String findInLine(String pattern) {
public @Nullable String findInLine(String pattern) {
return findInLine(patternCache.forName(pattern));
}

Expand All @@ -1709,7 +1709,7 @@ public String findInLine(String pattern) {
* @return the text that matched the specified pattern
* @throws IllegalStateException if this scanner is closed
*/
public String findInLine(Pattern pattern) {
public @Nullable String findInLine(Pattern pattern) {
ensureOpen();
if (pattern == null)
throw new NullPointerException();
Expand Down Expand Up @@ -1756,7 +1756,7 @@ public String findInLine(Pattern pattern) {
* @throws IllegalStateException if this scanner is closed
* @throws IllegalArgumentException if horizon is negative
*/
public String findWithinHorizon(String pattern, @NonNegative int horizon) {
public @Nullable String findWithinHorizon(String pattern, @NonNegative int horizon) {
return findWithinHorizon(patternCache.forName(pattern), horizon);
}

Expand Down Expand Up @@ -1791,7 +1791,7 @@ public String findWithinHorizon(String pattern, @NonNegative int horizon) {
* @throws IllegalStateException if this scanner is closed
* @throws IllegalArgumentException if horizon is negative
*/
public String findWithinHorizon(Pattern pattern, @NonNegative int horizon) {
public @Nullable String findWithinHorizon(Pattern pattern, @NonNegative int horizon) {
ensureOpen();
if (pattern == null)
throw new NullPointerException();
Expand Down
2 changes: 1 addition & 1 deletion src/java.base/share/classes/java/util/WeakHashMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
* @see java.lang.ref.WeakReference
*/
@CFComment({"lock: permits null keys and values"})
@AnnotatedFor({"lock", "index"})
@AnnotatedFor({"lock", "index", "nullness"})
public class WeakHashMap<K,V>
extends AbstractMap<K,V>
implements Map<K,V> {
Expand Down
15 changes: 10 additions & 5 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 @@ -39,6 +39,8 @@
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.framework.qual.CFComment;

import java.lang.ref.WeakReference;
import java.util.AbstractQueue;
Expand Down Expand Up @@ -88,7 +90,8 @@
* @author Doug Lea
* @param <E> the type of elements held in this queue
*/
public class ArrayBlockingQueue<E> extends AbstractQueue<E>
@AnnotatedFor({"nullness"})
public class ArrayBlockingQueue<E extends Object> extends AbstractQueue<E>
implements BlockingQueue<E>, java.io.Serializable {

/*
Expand Down Expand Up @@ -510,7 +513,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.

public boolean remove(Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
lock.lock();
Expand Down Expand Up @@ -542,7 +546,8 @@ public boolean remove(@Nullable Object o) {
* @param o object to be checked for containment in this queue
* @return {@code true} if this queue contains the specified element
*/
public boolean contains(@Nullable Object o) {
@CFComment("probably accepts null in practice, but docs at best imply this")
public boolean contains(Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
lock.lock();
Expand Down Expand Up @@ -1479,15 +1484,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 •
edited
Loading

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.

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
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.

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<@Nullable ?> 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<@Nullable ?> 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<@Nullable ?> 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<@Nullable ?> c) {
return al.retainAll(c);
}

Expand Down
33 changes: 20 additions & 13 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 @@ -39,6 +39,8 @@
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.dataflow.qual.Pure;
import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.framework.qual.CFComment;

import java.util.AbstractQueue;
import java.util.Collection;
Expand Down Expand Up @@ -80,7 +82,8 @@
* @author Doug Lea
* @param <E> the type of elements held in this deque
*/
public class LinkedBlockingDeque<E>
@AnnotatedFor({"nullness"})
public class LinkedBlockingDeque<E extends Object>
extends AbstractQueue<E>
implements BlockingDeque<E>, java.io.Serializable {

Expand Down Expand Up @@ -459,7 +462,7 @@ public E removeLast() {
return x;
}

public E pollFirst() {
public @Nullable E pollFirst() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
Expand All @@ -469,7 +472,7 @@ public E pollFirst() {
}
}

public E pollLast() {
public @Nullable E pollLast() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
Expand Down Expand Up @@ -505,7 +508,7 @@ public E takeLast() throws InterruptedException {
}
}

public E pollFirst(long timeout, TimeUnit unit)
public @Nullable E pollFirst(long timeout, TimeUnit unit)
throws InterruptedException {
long nanos = unit.toNanos(timeout);
final ReentrantLock lock = this.lock;
Expand All @@ -523,7 +526,7 @@ public E pollFirst(long timeout, TimeUnit unit)
}
}

public E pollLast(long timeout, TimeUnit unit)
public @Nullable E pollLast(long timeout, TimeUnit unit)
throws InterruptedException {
long nanos = unit.toNanos(timeout);
final ReentrantLock lock = this.lock;
Expand Down Expand Up @@ -559,7 +562,7 @@ public E getLast() {
return x;
}

public E peekFirst() {
public @Nullable E peekFirst() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
Expand All @@ -569,7 +572,7 @@ public E peekFirst() {
}
}

public E peekLast() {
public @Nullable E peekLast() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
Expand All @@ -579,6 +582,7 @@ public E peekLast() {
}
}

@CFComment("probably accepts null in practice, but docs at best imply this")
public boolean removeFirstOccurrence(Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
Expand All @@ -596,6 +600,7 @@ public boolean removeFirstOccurrence(Object o) {
}
}

@CFComment("probably accepts null in practice, but docs at best imply this")
public boolean removeLastOccurrence(Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
Expand Down Expand Up @@ -668,15 +673,15 @@ public E remove() {
return removeFirst();
}

public E poll() {
public @Nullable E poll() {
return pollFirst();
}

public E take() throws InterruptedException {
return takeFirst();
}

public E poll(long timeout, TimeUnit unit) throws InterruptedException {
public @Nullable E poll(long timeout, TimeUnit unit) throws InterruptedException {
return pollFirst(timeout, unit);
}

Expand All @@ -694,7 +699,7 @@ public E element() {
return getFirst();
}

public E peek() {
public @Nullable E peek() {
return peekFirst();
}

Expand Down Expand Up @@ -788,6 +793,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
*/
@CFComment("probably accepts null in practice, but docs at best imply this")
public boolean remove(Object o) {
return removeFirstOccurrence(o);
}
Expand Down Expand Up @@ -816,7 +822,8 @@ public int size() {
* @param o object to be checked for containment in this deque
* @return {@code true} if this deque contains the specified element
*/
public boolean contains(@Nullable Object o) {
@CFComment("probably accepts null in practice, but docs at best imply this")
public boolean contains(Object o) {
if (o == null) return false;
final ReentrantLock lock = this.lock;
lock.lock();
Expand Down Expand Up @@ -1342,15 +1349,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
Loading