Skip to content

Commit

Permalink
Assorted improvements to nullness annotations.
Browse files Browse the repository at this point in the history
I don't think any of the Guava changes are user-visible.

Details:
- Many of these fix bugs that will be noticed by the forthcoming version of our hacky internal nullness checker.
- Some of these fix bugs that even the forthcoming version won't notice but that I happened to see.
- Some changes add `@Nullable` to return types of methods that always throw an exception. There's no strict need for this, but we've mostly done it otherwise, so I figured I'd be consistent (and quiet `ReturnMissingNullable`, at least until I quiet it for all such methods with google/error-prone#2910).
- The `NullnessCasts` change is to discourage `ReturnMissingNullable` from adding a `@Nullable` annotation where we don't want it. (But we'll probably never run `ReturnMissingNullable` in the "aggressive" mode over this code, anyway, so there's not likely to be a need for the suppression.)
- The `@ParametricNulless` changes evidently aren't necessary for anyone right now, but they could theoretically be necessary for j2objc users until j2objc further enhances its support for nullness annotations.
- The `AbstractFuture` change removes a suppression that would be necessary under the Checker Framework (which would consider the supermethod's return type to be non-nullable) but isn't necessary for us (because we consider the supermethod's return type to have unspecified nullness).
RELNOTES=n/a
PiperOrigin-RevId: 421129392
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Feb 10, 2022
1 parent f526477 commit 3e71023
Show file tree
Hide file tree
Showing 24 changed files with 162 additions and 33 deletions.
38 changes: 37 additions & 1 deletion android/guava/src/com/google/common/base/Equivalence.java
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,43 @@ public final <F> Equivalence<F> onResultOf(Function<? super F, ? extends @Nullab
* @since 10.0
*/
public final <S extends @Nullable T> Wrapper<S> wrap(@ParametricNullness S reference) {
return new Wrapper<S>(this, reference);
/*
* I'm pretty sure that this warning "makes sense" but doesn't indicate a real problem.
*
* Why it "makes sense": If we pass a `@Nullable Foo`, then we should also pass an
* `Equivalence<? super @Nullable Foo>`. And there's no such thing because Equivalence doesn't
* permit nullable type arguments.
*
* Why there's no real problem: Every Equivalence can handle null.
*
* We could work around this by giving Wrapper 2 type parameters. In the terms of this method,
* that would be both the T parameter (from the class) and the S parameter (from this method).
* However, such a change would be source-incompatible. (Plus, there's no reason for the S
* parameter from the user's perspective, so it would be a wart.)
*
* We could probably also work around this by making Wrapper non-final and putting the
* implementation into a subclass with those 2 type parameters. But we like `final`, if only to
* deter users from using mocking frameworks to construct instances. (And could also complicate
* serialization, which is discussed more in the next paragraph.)
*
* We could probably also work around this by having Wrapper accept an instance of a new
* WrapperGuts class, which would then be the class that would declare the 2 type parameters.
* But that would break deserialization of previously serialized Wrapper instances. And while we
* specifically say not to rely on serialization across Guava versions, users sometimes do. So
* we'd rather not break them without a good enough reason.
*
* (We could work around the serialization problem by writing custom serialization code. But
* even that helps only the case of serializing with an old version and deserializing with a
* new, not vice versa -- unless we introduce WrapperGuts and the logic to handle it today, wait
* until "everyone" has picked up a version of Guava with that code, and *then* change to use
* WrapperGuts.)
*
* Anyway, a suppression isn't really a big deal. But I have tried to do some due diligence on
* avoiding it :)
*/
@SuppressWarnings("nullness")
Wrapper<S> w = new Wrapper<>(this, reference);
return w;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.annotations.GwtCompatible;
import java.io.Serializable;
import javax.annotation.CheckForNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Equivalence applied on functional result.
Expand All @@ -34,11 +35,11 @@ final class FunctionalEquivalence<F, T> extends Equivalence<F> implements Serial

private static final long serialVersionUID = 0;

private final Function<? super F, ? extends T> function;
private final Function<? super F, ? extends @Nullable T> function;
private final Equivalence<T> resultEquivalence;

FunctionalEquivalence(
Function<? super F, ? extends T> function, Equivalence<T> resultEquivalence) {
Function<? super F, ? extends @Nullable T> function, Equivalence<T> resultEquivalence) {
this.function = checkNotNull(function);
this.resultEquivalence = checkNotNull(resultEquivalence);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,8 @@ private static boolean isPermutation(List<?> first, List<?> second) {
return true;
}

private static <E> ObjectCountHashMap<E> counts(Collection<E> collection) {
private static <E extends @Nullable Object> ObjectCountHashMap<E> counts(
Collection<E> collection) {
ObjectCountHashMap<E> map = new ObjectCountHashMap<>();
for (E e : collection) {
map.put(e, map.get(e) + 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,9 @@ public ImmutableMap<K, V> buildKeepingLast() {
}

static <V> void sortEntries(
@Nullable Object[] alternatingKeysAndValues, int size, Comparator<V> valueComparator) {
@Nullable Object[] alternatingKeysAndValues,
int size,
Comparator<? super V> valueComparator) {
@SuppressWarnings({"rawtypes", "unchecked"})
Entry<Object, V>[] entries = new Entry[size];
for (int i = 0; i < size; i++) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Iterator;
import java.util.Set;
import javax.annotation.CheckForNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* A {@link Multiset} whose contents will never change, with many other important properties
Expand Down Expand Up @@ -282,7 +283,7 @@ public final boolean setCount(E element, int oldCount, int newCount) {

@GwtIncompatible // not present in emulated superclass
@Override
int copyIntoArray(Object[] dst, int offset) {
int copyIntoArray(@Nullable Object[] dst, int offset) {
for (Multiset.Entry<E> entry : entrySet()) {
Arrays.fill(dst, offset, offset + entry.getCount(), entry.getElement());
offset += entry.getCount();
Expand Down
15 changes: 14 additions & 1 deletion android/guava/src/com/google/common/collect/Maps.java
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,20 @@ SortedMapDifference<K, V> difference(
onlyOnRight.putAll(right); // will whittle it down
SortedMap<K, V> onBoth = Maps.newTreeMap(comparator);
SortedMap<K, MapDifference.ValueDifference<V>> differences = Maps.newTreeMap(comparator);
doDifference(left, right, Equivalence.equals(), onlyOnLeft, onlyOnRight, onBoth, differences);

/*
* V is a possibly nullable type, but we decided to declare Equivalence with a type parameter
* that is restricted to non-nullable types. Still, this code is safe: We made that decision
* about Equivalence not because Equivalence is null-hostile but because *every* Equivalence can
* handle null inputs -- and thus it would be meaningless for the type system to distinguish
* between "an Equivalence for nullable Foo" and "an Equivalence for non-nullable Foo."
*
* (And the unchecked cast is safe because Equivalence is contravariant.)
*/
@SuppressWarnings({"nullness", "unchecked"})
Equivalence<V> equalsEquivalence = (Equivalence<V>) Equivalence.equals();

doDifference(left, right, equalsEquivalence, onlyOnLeft, onlyOnRight, onBoth, differences);
return new SortedMapDifferenceImpl<>(onlyOnLeft, onlyOnRight, onBoth, differences);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ final class NullnessCasts {
}

/** Returns {@code null} as any type, even one that does not include {@code null}. */
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals"})
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals", "ReturnMissingNullable"})
// The warnings are legitimate. Each time we use this method, we document why.
@ParametricNullness
static <T extends @Nullable Object> T unsafeNull() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ boolean isPartialView() {
}

@Override
int copyIntoArray(Object[] dst, int offset) {
int copyIntoArray(@Nullable Object[] dst, int offset) {
return elements.copyIntoArray(dst, offset);
}

Expand Down
8 changes: 6 additions & 2 deletions android/guava/src/com/google/common/collect/TopKSelector.java
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,10 @@ private void trim() {
}
iterations++;
if (iterations >= maxIterations) {
@SuppressWarnings("nullness") // safe because we pass sort() a range that contains real Ts
T[] castBuffer = (T[]) buffer;
// We've already taken O(k log k), let's make sure we don't take longer than O(k log k).
Arrays.sort(buffer, left, right + 1, comparator);
Arrays.sort(castBuffer, left, right + 1, comparator);
break;
}
}
Expand Down Expand Up @@ -263,7 +265,9 @@ public void offerAll(Iterator<? extends T> elements) {
* this {@code TopKSelector}. This method returns in O(k log k) time.
*/
public List<T> topK() {
Arrays.sort(buffer, 0, bufferSize, comparator);
@SuppressWarnings("nullness") // safe because we pass sort() a range that contains real Ts
T[] castBuffer = (T[]) buffer;
Arrays.sort(castBuffer, 0, bufferSize, comparator);
if (bufferSize > k) {
Arrays.fill(buffer, k, buffer.length, null);
bufferSize = k;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,6 @@ protected void afterDone() {}
* method returns @Nullable, too. However, we're not sure if we want to make any changes to that
* class, since it's in a separate artifact that we planned to release only a single version of.
*/
@SuppressWarnings("nullness")
@CheckForNull
protected final Throwable tryInternalFastPathGetFailure() {
if (this instanceof Trusted) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ final class NullnessCasts {
* return to a caller, the code needs to a way to return {@code null} from a method that returns
* "plain {@code T}." This API provides that.
*/
@SuppressWarnings("nullness")
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals", "ReturnMissingNullable"})
// The warnings are legitimate. Each time we use this method, we document why.
@ParametricNullness
static <T extends @Nullable Object> T uncheckedNull() {
return null;
Expand Down
38 changes: 37 additions & 1 deletion guava/src/com/google/common/base/Equivalence.java
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,43 @@ public final <F> Equivalence<F> onResultOf(Function<? super F, ? extends @Nullab
* @since 10.0
*/
public final <S extends @Nullable T> Wrapper<S> wrap(@ParametricNullness S reference) {
return new Wrapper<S>(this, reference);
/*
* I'm pretty sure that this warning "makes sense" but doesn't indicate a real problem.
*
* Why it "makes sense": If we pass a `@Nullable Foo`, then we should also pass an
* `Equivalence<? super @Nullable Foo>`. And there's no such thing because Equivalence doesn't
* permit nullable type arguments.
*
* Why there's no real problem: Every Equivalence can handle null.
*
* We could work around this by giving Wrapper 2 type parameters. In the terms of this method,
* that would be both the T parameter (from the class) and the S parameter (from this method).
* However, such a change would be source-incompatible. (Plus, there's no reason for the S
* parameter from the user's perspective, so it would be a wart.)
*
* We could probably also work around this by making Wrapper non-final and putting the
* implementation into a subclass with those 2 type parameters. But we like `final`, if only to
* deter users from using mocking frameworks to construct instances. (And could also complicate
* serialization, which is discussed more in the next paragraph.)
*
* We could probably also work around this by having Wrapper accept an instance of a new
* WrapperGuts class, which would then be the class that would declare the 2 type parameters.
* But that would break deserialization of previously serialized Wrapper instances. And while we
* specifically say not to rely on serialization across Guava versions, users sometimes do. So
* we'd rather not break them without a good enough reason.
*
* (We could work around the serialization problem by writing custom serialization code. But
* even that helps only the case of serializing with an old version and deserializing with a
* new, not vice versa -- unless we introduce WrapperGuts and the logic to handle it today, wait
* until "everyone" has picked up a version of Guava with that code, and *then* change to use
* WrapperGuts.)
*
* Anyway, a suppression isn't really a big deal. But I have tried to do some due diligence on
* avoiding it :)
*/
@SuppressWarnings("nullness")
Wrapper<S> w = new Wrapper<>(this, reference);
return w;
}

/**
Expand Down
5 changes: 3 additions & 2 deletions guava/src/com/google/common/base/FunctionalEquivalence.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.annotations.GwtCompatible;
import java.io.Serializable;
import javax.annotation.CheckForNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* Equivalence applied on functional result.
Expand All @@ -34,11 +35,11 @@ final class FunctionalEquivalence<F, T> extends Equivalence<F> implements Serial

private static final long serialVersionUID = 0;

private final Function<? super F, ? extends T> function;
private final Function<? super F, ? extends @Nullable T> function;
private final Equivalence<T> resultEquivalence;

FunctionalEquivalence(
Function<? super F, ? extends T> function, Equivalence<T> resultEquivalence) {
Function<? super F, ? extends @Nullable T> function, Equivalence<T> resultEquivalence) {
this.function = checkNotNull(function);
this.resultEquivalence = checkNotNull(resultEquivalence);
}
Expand Down
6 changes: 3 additions & 3 deletions guava/src/com/google/common/cache/LocalCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -2189,7 +2189,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
}
}

V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> function) {
V compute(K key, int hash, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
ReferenceEntry<K, V> e;
ValueReference<K, V> valueReference = null;
ComputingValueReference<K, V> computingValueReference = null;
Expand Down Expand Up @@ -3558,7 +3558,7 @@ public V apply(V newValue) {
}
}

public V compute(K key, BiFunction<? super K, ? super V, ? extends V> function) {
public V compute(K key, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
stopwatch.start();
V previousValue;
try {
Expand Down Expand Up @@ -4205,7 +4205,7 @@ public V putIfAbsent(K key, V value) {
}

@Override
public V compute(K key, BiFunction<? super K, ? super V, ? extends V> function) {
public V compute(K key, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
checkNotNull(key);
checkNotNull(function);
int hash = hash(key);
Expand Down
12 changes: 10 additions & 2 deletions guava/src/com/google/common/collect/HashBiMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,7 @@ private final class KeySet extends Maps.KeySet<K, V> {
public Iterator<K> iterator() {
return new Itr<K>() {
@Override
@ParametricNullness
K output(BiEntry<K, V> entry) {
return entry.key;
}
Expand Down Expand Up @@ -530,17 +531,20 @@ class MapEntry extends AbstractMapEntry<K, V> {
}

@Override
@ParametricNullness
public K getKey() {
return delegate.key;
}

@Override
@ParametricNullness
public V getValue() {
return delegate.value;
}

@Override
public V setValue(V value) {
@ParametricNullness
public V setValue(@ParametricNullness V value) {
V oldValue = delegate.value;
int valueHash = smearedHash(value);
if (valueHash == delegate.valueHash && Objects.equal(value, oldValue)) {
Expand Down Expand Up @@ -675,6 +679,7 @@ public boolean remove(@CheckForNull Object o) {
public Iterator<V> iterator() {
return new Itr<V>() {
@Override
@ParametricNullness
V output(BiEntry<K, V> entry) {
return entry.value;
}
Expand Down Expand Up @@ -703,17 +708,20 @@ class InverseEntry extends AbstractMapEntry<V, K> {
}

@Override
@ParametricNullness
public V getKey() {
return delegate.value;
}

@Override
@ParametricNullness
public K getValue() {
return delegate.key;
}

@Override
public K setValue(K key) {
@ParametricNullness
public K setValue(@ParametricNullness K key) {
K oldKey = delegate.key;
int keyHash = smearedHash(key);
if (keyHash == delegate.keyHash && Objects.equal(key, oldKey)) {
Expand Down
2 changes: 1 addition & 1 deletion guava/src/com/google/common/collect/ImmutableMultiset.java
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ public final boolean setCount(E element, int oldCount, int newCount) {

@GwtIncompatible // not present in emulated superclass
@Override
int copyIntoArray(Object[] dst, int offset) {
int copyIntoArray(@Nullable Object[] dst, int offset) {
for (Multiset.Entry<E> entry : entrySet()) {
Arrays.fill(dst, offset, offset + entry.getCount(), entry.getElement());
offset += entry.getCount();
Expand Down

0 comments on commit 3e71023

Please sign in to comment.