Skip to content

Commit

Permalink
Avoid calling checkNotNull on nullable values except during actual …
Browse files Browse the repository at this point in the history
…precondition checks.

It's not that we're not going to make such calls illegal, I promise :) I mean, we certainly aren't going to _in general_, but I am tempted for `com.google.common`, as discussed on cl/372346107 :) (It would have caught the problem of cl/612591080!) I'm testing what would happen if we did do it for `com.google.common` in case it shakes out any more bugs.

It does reveal that I didn't complete the cleanup of cl/612591080. And it reveals a few places where we'd normally use `requireNonNull`, since the checks aren't "preconditions" in the sense of "the caller did something wrong" (from cl/15376243 and cl/526930990). I've made those changes. (I would have made some more changes if I had tried to address more of `com.google.common`. But I stuck to the "main" packages, and I didn't even fix enough errors to see full results.)

Honestly, the more interesting thing that this exercise revealed was that there are more cases in which I'm especially sympathetic to calling `checkNotNull` on nullable values:
- `DummyProxy` is making an `InvocationHandler` perform automatic precondition tests based on annotations on the interface it's implementing.
- `EqualsTester` and Truth have permissive signatures because they're test utilities, as documented in cl/578260904 and discussed during the Truth CLs.

And the yet more interesting thing that it revealed is that we may want to use `@NonNull` here in the future, similar to what we've discussed in #6824.

RELNOTES=n/a
PiperOrigin-RevId: 614074533
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Mar 9, 2024
1 parent 0b1c477 commit 52a62b2
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
Expand Down Expand Up @@ -211,7 +212,7 @@ final <T> T newFreshProxy(final Class<T> interfaceType) {
return pickInstance(rawType.getEnumConstants(), null);
}
if (type.isArray()) {
TypeToken<?> componentType = checkNotNull(type.getComponentType());
TypeToken<?> componentType = requireNonNull(type.getComponentType());
Object array = Array.newInstance(componentType.getRawType(), 1);
Array.set(array, 0, generate(componentType));
return array;
Expand Down
14 changes: 6 additions & 8 deletions android/guava/src/com/google/common/collect/TableCollectors.java
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,12 @@ void merge(V value, BinaryOperator<V> mergeFunction) {
}
}

private static <
R extends @Nullable Object, C extends @Nullable Object, V extends @Nullable Object>
void mergeTables(
Table<R, C, V> table,
@ParametricNullness R row,
@ParametricNullness C column,
@ParametricNullness V value,
BinaryOperator<V> mergeFunction) {
private static <R extends @Nullable Object, C extends @Nullable Object, V> void mergeTables(
Table<R, C, V> table,
@ParametricNullness R row,
@ParametricNullness C column,
V value,
BinaryOperator<V> mergeFunction) {
checkNotNull(value);
V oldValue = table.get(row, column);
if (oldValue == null) {
Expand Down
5 changes: 3 additions & 2 deletions android/guava/src/com/google/common/collect/TreeMultimap.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.common.collect;

import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
Expand Down Expand Up @@ -217,8 +218,8 @@ private void writeObject(ObjectOutputStream stream) throws IOException {
@SuppressWarnings("unchecked") // reading data stored by writeObject
private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
stream.defaultReadObject();
keyComparator = checkNotNull((Comparator<? super K>) stream.readObject());
valueComparator = checkNotNull((Comparator<? super V>) stream.readObject());
keyComparator = requireNonNull((Comparator<? super K>) stream.readObject());
valueComparator = requireNonNull((Comparator<? super V>) stream.readObject());
setMap(new TreeMap<K, Collection<V>>(keyComparator));
Serialization.populateMultimap(this, stream);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
Expand Down Expand Up @@ -215,7 +216,7 @@ final <T> T newFreshProxy(final Class<T> interfaceType) {
return pickInstance(rawType.getEnumConstants(), null);
}
if (type.isArray()) {
TypeToken<?> componentType = checkNotNull(type.getComponentType());
TypeToken<?> componentType = requireNonNull(type.getComponentType());
Object array = Array.newInstance(componentType.getRawType(), 1);
Array.set(array, 0, generate(componentType));
return array;
Expand Down
14 changes: 6 additions & 8 deletions guava/src/com/google/common/collect/TableCollectors.java
Original file line number Diff line number Diff line change
Expand Up @@ -195,14 +195,12 @@ void merge(V value, BinaryOperator<V> mergeFunction) {
}
}

private static <
R extends @Nullable Object, C extends @Nullable Object, V extends @Nullable Object>
void mergeTables(
Table<R, C, V> table,
@ParametricNullness R row,
@ParametricNullness C column,
@ParametricNullness V value,
BinaryOperator<V> mergeFunction) {
private static <R extends @Nullable Object, C extends @Nullable Object, V> void mergeTables(
Table<R, C, V> table,
@ParametricNullness R row,
@ParametricNullness C column,
V value,
BinaryOperator<V> mergeFunction) {
checkNotNull(value);
V oldValue = table.get(row, column);
if (oldValue == null) {
Expand Down
5 changes: 3 additions & 2 deletions guava/src/com/google/common/collect/TreeMultimap.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.common.collect;

import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
Expand Down Expand Up @@ -217,8 +218,8 @@ private void writeObject(ObjectOutputStream stream) throws IOException {
@SuppressWarnings("unchecked") // reading data stored by writeObject
private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
stream.defaultReadObject();
keyComparator = checkNotNull((Comparator<? super K>) stream.readObject());
valueComparator = checkNotNull((Comparator<? super V>) stream.readObject());
keyComparator = requireNonNull((Comparator<? super K>) stream.readObject());
valueComparator = requireNonNull((Comparator<? super V>) stream.readObject());
setMap(new TreeMap<K, Collection<V>>(keyComparator));
Serialization.populateMultimap(this, stream);
}
Expand Down

0 comments on commit 52a62b2

Please sign in to comment.