Skip to content

Commit f881fab

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Assorted improvements to nullness annotations.
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 `@ParametricNullness` 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: 427818689
1 parent f526477 commit f881fab

24 files changed

+162
-33
lines changed

android/guava/src/com/google/common/base/Equivalence.java

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,43 @@ public final <F> Equivalence<F> onResultOf(Function<? super F, ? extends @Nullab
153153
* @since 10.0
154154
*/
155155
public final <S extends @Nullable T> Wrapper<S> wrap(@ParametricNullness S reference) {
156-
return new Wrapper<S>(this, reference);
156+
/*
157+
* I'm pretty sure that this warning "makes sense" but doesn't indicate a real problem.
158+
*
159+
* Why it "makes sense": If we pass a `@Nullable Foo`, then we should also pass an
160+
* `Equivalence<? super @Nullable Foo>`. And there's no such thing because Equivalence doesn't
161+
* permit nullable type arguments.
162+
*
163+
* Why there's no real problem: Every Equivalence can handle null.
164+
*
165+
* We could work around this by giving Wrapper 2 type parameters. In the terms of this method,
166+
* that would be both the T parameter (from the class) and the S parameter (from this method).
167+
* However, such a change would be source-incompatible. (Plus, there's no reason for the S
168+
* parameter from the user's perspective, so it would be a wart.)
169+
*
170+
* We could probably also work around this by making Wrapper non-final and putting the
171+
* implementation into a subclass with those 2 type parameters. But we like `final`, if only to
172+
* deter users from using mocking frameworks to construct instances. (And could also complicate
173+
* serialization, which is discussed more in the next paragraph.)
174+
*
175+
* We could probably also work around this by having Wrapper accept an instance of a new
176+
* WrapperGuts class, which would then be the class that would declare the 2 type parameters.
177+
* But that would break deserialization of previously serialized Wrapper instances. And while we
178+
* specifically say not to rely on serialization across Guava versions, users sometimes do. So
179+
* we'd rather not break them without a good enough reason.
180+
*
181+
* (We could work around the serialization problem by writing custom serialization code. But
182+
* even that helps only the case of serializing with an old version and deserializing with a
183+
* new, not vice versa -- unless we introduce WrapperGuts and the logic to handle it today, wait
184+
* until "everyone" has picked up a version of Guava with that code, and *then* change to use
185+
* WrapperGuts.)
186+
*
187+
* Anyway, a suppression isn't really a big deal. But I have tried to do some due diligence on
188+
* avoiding it :)
189+
*/
190+
@SuppressWarnings("nullness")
191+
Wrapper<S> w = new Wrapper<>(this, reference);
192+
return w;
157193
}
158194

159195
/**

android/guava/src/com/google/common/base/FunctionalEquivalence.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.common.annotations.GwtCompatible;
2121
import java.io.Serializable;
2222
import javax.annotation.CheckForNull;
23+
import org.checkerframework.checker.nullness.qual.Nullable;
2324

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

3536
private static final long serialVersionUID = 0;
3637

37-
private final Function<? super F, ? extends T> function;
38+
private final Function<? super F, ? extends @Nullable T> function;
3839
private final Equivalence<T> resultEquivalence;
3940

4041
FunctionalEquivalence(
41-
Function<? super F, ? extends T> function, Equivalence<T> resultEquivalence) {
42+
Function<? super F, ? extends @Nullable T> function, Equivalence<T> resultEquivalence) {
4243
this.function = checkNotNull(function);
4344
this.resultEquivalence = checkNotNull(resultEquivalence);
4445
}

android/guava/src/com/google/common/collect/Collections2.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,8 @@ private static boolean isPermutation(List<?> first, List<?> second) {
695695
return true;
696696
}
697697

698-
private static <E> ObjectCountHashMap<E> counts(Collection<E> collection) {
698+
private static <E extends @Nullable Object> ObjectCountHashMap<E> counts(
699+
Collection<E> collection) {
699700
ObjectCountHashMap<E> map = new ObjectCountHashMap<>();
700701
for (E e : collection) {
701702
map.put(e, map.get(e) + 1);

android/guava/src/com/google/common/collect/ImmutableMap.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,9 @@ public ImmutableMap<K, V> buildKeepingLast() {
599599
}
600600

601601
static <V> void sortEntries(
602-
@Nullable Object[] alternatingKeysAndValues, int size, Comparator<V> valueComparator) {
602+
@Nullable Object[] alternatingKeysAndValues,
603+
int size,
604+
Comparator<? super V> valueComparator) {
603605
@SuppressWarnings({"rawtypes", "unchecked"})
604606
Entry<Object, V>[] entries = new Entry[size];
605607
for (int i = 0; i < size; i++) {

android/guava/src/com/google/common/collect/ImmutableMultiset.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.Iterator;
3232
import java.util.Set;
3333
import javax.annotation.CheckForNull;
34+
import org.checkerframework.checker.nullness.qual.Nullable;
3435

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

283284
@GwtIncompatible // not present in emulated superclass
284285
@Override
285-
int copyIntoArray(Object[] dst, int offset) {
286+
int copyIntoArray(@Nullable Object[] dst, int offset) {
286287
for (Multiset.Entry<E> entry : entrySet()) {
287288
Arrays.fill(dst, offset, offset + entry.getCount(), entry.getElement());
288289
offset += entry.getCount();

android/guava/src/com/google/common/collect/Maps.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,20 @@ SortedMapDifference<K, V> difference(
528528
onlyOnRight.putAll(right); // will whittle it down
529529
SortedMap<K, V> onBoth = Maps.newTreeMap(comparator);
530530
SortedMap<K, MapDifference.ValueDifference<V>> differences = Maps.newTreeMap(comparator);
531-
doDifference(left, right, Equivalence.equals(), onlyOnLeft, onlyOnRight, onBoth, differences);
531+
532+
/*
533+
* V is a possibly nullable type, but we decided to declare Equivalence with a type parameter
534+
* that is restricted to non-nullable types. Still, this code is safe: We made that decision
535+
* about Equivalence not because Equivalence is null-hostile but because *every* Equivalence can
536+
* handle null inputs -- and thus it would be meaningless for the type system to distinguish
537+
* between "an Equivalence for nullable Foo" and "an Equivalence for non-nullable Foo."
538+
*
539+
* (And the unchecked cast is safe because Equivalence is contravariant.)
540+
*/
541+
@SuppressWarnings({"nullness", "unchecked"})
542+
Equivalence<V> equalsEquivalence = (Equivalence<V>) Equivalence.equals();
543+
544+
doDifference(left, right, equalsEquivalence, onlyOnLeft, onlyOnRight, onBoth, differences);
532545
return new SortedMapDifferenceImpl<>(onlyOnLeft, onlyOnRight, onBoth, differences);
533546
}
534547

android/guava/src/com/google/common/collect/NullnessCasts.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ final class NullnessCasts {
5757
}
5858

5959
/** Returns {@code null} as any type, even one that does not include {@code null}. */
60-
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals"})
60+
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals", "ReturnMissingNullable"})
6161
// The warnings are legitimate. Each time we use this method, we document why.
6262
@ParametricNullness
6363
static <T extends @Nullable Object> T unsafeNull() {

android/guava/src/com/google/common/collect/RegularImmutableSortedSet.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ boolean isPartialView() {
155155
}
156156

157157
@Override
158-
int copyIntoArray(Object[] dst, int offset) {
158+
int copyIntoArray(@Nullable Object[] dst, int offset) {
159159
return elements.copyIntoArray(dst, offset);
160160
}
161161

android/guava/src/com/google/common/collect/TopKSelector.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,10 @@ private void trim() {
184184
}
185185
iterations++;
186186
if (iterations >= maxIterations) {
187+
@SuppressWarnings("nullness") // safe because we pass sort() a range that contains real Ts
188+
T[] castBuffer = (T[]) buffer;
187189
// We've already taken O(k log k), let's make sure we don't take longer than O(k log k).
188-
Arrays.sort(buffer, left, right + 1, comparator);
190+
Arrays.sort(castBuffer, left, right + 1, comparator);
189191
break;
190192
}
191193
}
@@ -263,7 +265,9 @@ public void offerAll(Iterator<? extends T> elements) {
263265
* this {@code TopKSelector}. This method returns in O(k log k) time.
264266
*/
265267
public List<T> topK() {
266-
Arrays.sort(buffer, 0, bufferSize, comparator);
268+
@SuppressWarnings("nullness") // safe because we pass sort() a range that contains real Ts
269+
T[] castBuffer = (T[]) buffer;
270+
Arrays.sort(castBuffer, 0, bufferSize, comparator);
267271
if (bufferSize > k) {
268272
Arrays.fill(buffer, k, buffer.length, null);
269273
bufferSize = k;

android/guava/src/com/google/common/util/concurrent/AbstractFuture.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1084,7 +1084,6 @@ protected void afterDone() {}
10841084
* method returns @Nullable, too. However, we're not sure if we want to make any changes to that
10851085
* class, since it's in a separate artifact that we planned to release only a single version of.
10861086
*/
1087-
@SuppressWarnings("nullness")
10881087
@CheckForNull
10891088
protected final Throwable tryInternalFastPathGetFailure() {
10901089
if (this instanceof Trusted) {

android/guava/src/com/google/common/util/concurrent/NullnessCasts.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,8 @@ final class NullnessCasts {
6565
* return to a caller, the code needs to a way to return {@code null} from a method that returns
6666
* "plain {@code T}." This API provides that.
6767
*/
68-
@SuppressWarnings("nullness")
68+
@SuppressWarnings({"nullness", "TypeParameterUnusedInFormals", "ReturnMissingNullable"})
69+
// The warnings are legitimate. Each time we use this method, we document why.
6970
@ParametricNullness
7071
static <T extends @Nullable Object> T uncheckedNull() {
7172
return null;

guava/src/com/google/common/base/Equivalence.java

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,43 @@ public final <F> Equivalence<F> onResultOf(Function<? super F, ? extends @Nullab
165165
* @since 10.0
166166
*/
167167
public final <S extends @Nullable T> Wrapper<S> wrap(@ParametricNullness S reference) {
168-
return new Wrapper<S>(this, reference);
168+
/*
169+
* I'm pretty sure that this warning "makes sense" but doesn't indicate a real problem.
170+
*
171+
* Why it "makes sense": If we pass a `@Nullable Foo`, then we should also pass an
172+
* `Equivalence<? super @Nullable Foo>`. And there's no such thing because Equivalence doesn't
173+
* permit nullable type arguments.
174+
*
175+
* Why there's no real problem: Every Equivalence can handle null.
176+
*
177+
* We could work around this by giving Wrapper 2 type parameters. In the terms of this method,
178+
* that would be both the T parameter (from the class) and the S parameter (from this method).
179+
* However, such a change would be source-incompatible. (Plus, there's no reason for the S
180+
* parameter from the user's perspective, so it would be a wart.)
181+
*
182+
* We could probably also work around this by making Wrapper non-final and putting the
183+
* implementation into a subclass with those 2 type parameters. But we like `final`, if only to
184+
* deter users from using mocking frameworks to construct instances. (And could also complicate
185+
* serialization, which is discussed more in the next paragraph.)
186+
*
187+
* We could probably also work around this by having Wrapper accept an instance of a new
188+
* WrapperGuts class, which would then be the class that would declare the 2 type parameters.
189+
* But that would break deserialization of previously serialized Wrapper instances. And while we
190+
* specifically say not to rely on serialization across Guava versions, users sometimes do. So
191+
* we'd rather not break them without a good enough reason.
192+
*
193+
* (We could work around the serialization problem by writing custom serialization code. But
194+
* even that helps only the case of serializing with an old version and deserializing with a
195+
* new, not vice versa -- unless we introduce WrapperGuts and the logic to handle it today, wait
196+
* until "everyone" has picked up a version of Guava with that code, and *then* change to use
197+
* WrapperGuts.)
198+
*
199+
* Anyway, a suppression isn't really a big deal. But I have tried to do some due diligence on
200+
* avoiding it :)
201+
*/
202+
@SuppressWarnings("nullness")
203+
Wrapper<S> w = new Wrapper<>(this, reference);
204+
return w;
169205
}
170206

171207
/**

guava/src/com/google/common/base/FunctionalEquivalence.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.common.annotations.GwtCompatible;
2121
import java.io.Serializable;
2222
import javax.annotation.CheckForNull;
23+
import org.checkerframework.checker.nullness.qual.Nullable;
2324

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

3536
private static final long serialVersionUID = 0;
3637

37-
private final Function<? super F, ? extends T> function;
38+
private final Function<? super F, ? extends @Nullable T> function;
3839
private final Equivalence<T> resultEquivalence;
3940

4041
FunctionalEquivalence(
41-
Function<? super F, ? extends T> function, Equivalence<T> resultEquivalence) {
42+
Function<? super F, ? extends @Nullable T> function, Equivalence<T> resultEquivalence) {
4243
this.function = checkNotNull(function);
4344
this.resultEquivalence = checkNotNull(resultEquivalence);
4445
}

guava/src/com/google/common/cache/LocalCache.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2189,7 +2189,7 @@ V waitForLoadingValue(ReferenceEntry<K, V> e, K key, ValueReference<K, V> valueR
21892189
}
21902190
}
21912191

2192-
V compute(K key, int hash, BiFunction<? super K, ? super V, ? extends V> function) {
2192+
V compute(K key, int hash, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
21932193
ReferenceEntry<K, V> e;
21942194
ValueReference<K, V> valueReference = null;
21952195
ComputingValueReference<K, V> computingValueReference = null;
@@ -3558,7 +3558,7 @@ public V apply(V newValue) {
35583558
}
35593559
}
35603560

3561-
public V compute(K key, BiFunction<? super K, ? super V, ? extends V> function) {
3561+
public V compute(K key, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
35623562
stopwatch.start();
35633563
V previousValue;
35643564
try {
@@ -4205,7 +4205,7 @@ public V putIfAbsent(K key, V value) {
42054205
}
42064206

42074207
@Override
4208-
public V compute(K key, BiFunction<? super K, ? super V, ? extends V> function) {
4208+
public V compute(K key, BiFunction<? super K, ? super @Nullable V, ? extends V> function) {
42094209
checkNotNull(key);
42104210
checkNotNull(function);
42114211
int hash = hash(key);

guava/src/com/google/common/collect/HashBiMap.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,7 @@ private final class KeySet extends Maps.KeySet<K, V> {
489489
public Iterator<K> iterator() {
490490
return new Itr<K>() {
491491
@Override
492+
@ParametricNullness
492493
K output(BiEntry<K, V> entry) {
493494
return entry.key;
494495
}
@@ -530,17 +531,20 @@ class MapEntry extends AbstractMapEntry<K, V> {
530531
}
531532

532533
@Override
534+
@ParametricNullness
533535
public K getKey() {
534536
return delegate.key;
535537
}
536538

537539
@Override
540+
@ParametricNullness
538541
public V getValue() {
539542
return delegate.value;
540543
}
541544

542545
@Override
543-
public V setValue(V value) {
546+
@ParametricNullness
547+
public V setValue(@ParametricNullness V value) {
544548
V oldValue = delegate.value;
545549
int valueHash = smearedHash(value);
546550
if (valueHash == delegate.valueHash && Objects.equal(value, oldValue)) {
@@ -675,6 +679,7 @@ public boolean remove(@CheckForNull Object o) {
675679
public Iterator<V> iterator() {
676680
return new Itr<V>() {
677681
@Override
682+
@ParametricNullness
678683
V output(BiEntry<K, V> entry) {
679684
return entry.value;
680685
}
@@ -703,17 +708,20 @@ class InverseEntry extends AbstractMapEntry<V, K> {
703708
}
704709

705710
@Override
711+
@ParametricNullness
706712
public V getKey() {
707713
return delegate.value;
708714
}
709715

710716
@Override
717+
@ParametricNullness
711718
public K getValue() {
712719
return delegate.key;
713720
}
714721

715722
@Override
716-
public K setValue(K key) {
723+
@ParametricNullness
724+
public K setValue(@ParametricNullness K key) {
717725
K oldKey = delegate.key;
718726
int keyHash = smearedHash(key);
719727
if (keyHash == delegate.keyHash && Objects.equal(key, oldKey)) {

guava/src/com/google/common/collect/ImmutableMultiset.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ public final boolean setCount(E element, int oldCount, int newCount) {
325325

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

0 commit comments

Comments
 (0)