From 55f607b4af3b0f3e40bc0d9ca45ca030c5742641 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 8 Mar 2024 16:18:25 -0800 Subject: [PATCH] Avoid calling `checkNotNull` on nullable values except during actual 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 https://github.com/google/guava/issues/6824. RELNOTES=n/a PiperOrigin-RevId: 614074533 --- .../google/common/testing/FreshValueGenerator.java | 3 ++- .../com/google/common/collect/TableCollectors.java | 14 ++++++-------- .../com/google/common/collect/TreeMultimap.java | 5 +++-- .../google/common/testing/FreshValueGenerator.java | 3 ++- .../com/google/common/collect/TableCollectors.java | 14 ++++++-------- .../com/google/common/collect/TreeMultimap.java | 5 +++-- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/android/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java b/android/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java index 4cc25e1289fb..d1a7819f1977 100644 --- a/android/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java +++ b/android/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java @@ -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; @@ -211,7 +212,7 @@ final T newFreshProxy(final Class 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; diff --git a/android/guava/src/com/google/common/collect/TableCollectors.java b/android/guava/src/com/google/common/collect/TableCollectors.java index 0508c4802fc9..f53569627e5e 100644 --- a/android/guava/src/com/google/common/collect/TableCollectors.java +++ b/android/guava/src/com/google/common/collect/TableCollectors.java @@ -199,14 +199,12 @@ void merge(V value, BinaryOperator mergeFunction) { } } - private static < - R extends @Nullable Object, C extends @Nullable Object, V extends @Nullable Object> - void mergeTables( - Table table, - @ParametricNullness R row, - @ParametricNullness C column, - @ParametricNullness V value, - BinaryOperator mergeFunction) { + private static void mergeTables( + Table table, + @ParametricNullness R row, + @ParametricNullness C column, + V value, + BinaryOperator mergeFunction) { checkNotNull(value); V oldValue = table.get(row, column); if (oldValue == null) { diff --git a/android/guava/src/com/google/common/collect/TreeMultimap.java b/android/guava/src/com/google/common/collect/TreeMultimap.java index 8e2afe3594eb..289ea54f67b2 100644 --- a/android/guava/src/com/google/common/collect/TreeMultimap.java +++ b/android/guava/src/com/google/common/collect/TreeMultimap.java @@ -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; @@ -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) stream.readObject()); - valueComparator = checkNotNull((Comparator) stream.readObject()); + keyComparator = requireNonNull((Comparator) stream.readObject()); + valueComparator = requireNonNull((Comparator) stream.readObject()); setMap(new TreeMap>(keyComparator)); Serialization.populateMultimap(this, stream); } diff --git a/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java b/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java index 65e50df176e6..567b7a76aac7 100644 --- a/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java +++ b/guava-testlib/src/com/google/common/testing/FreshValueGenerator.java @@ -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; @@ -215,7 +216,7 @@ final T newFreshProxy(final Class 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; diff --git a/guava/src/com/google/common/collect/TableCollectors.java b/guava/src/com/google/common/collect/TableCollectors.java index 1164e822de53..ca67a693aec4 100644 --- a/guava/src/com/google/common/collect/TableCollectors.java +++ b/guava/src/com/google/common/collect/TableCollectors.java @@ -195,14 +195,12 @@ void merge(V value, BinaryOperator mergeFunction) { } } - private static < - R extends @Nullable Object, C extends @Nullable Object, V extends @Nullable Object> - void mergeTables( - Table table, - @ParametricNullness R row, - @ParametricNullness C column, - @ParametricNullness V value, - BinaryOperator mergeFunction) { + private static void mergeTables( + Table table, + @ParametricNullness R row, + @ParametricNullness C column, + V value, + BinaryOperator mergeFunction) { checkNotNull(value); V oldValue = table.get(row, column); if (oldValue == null) { diff --git a/guava/src/com/google/common/collect/TreeMultimap.java b/guava/src/com/google/common/collect/TreeMultimap.java index 8e2afe3594eb..289ea54f67b2 100644 --- a/guava/src/com/google/common/collect/TreeMultimap.java +++ b/guava/src/com/google/common/collect/TreeMultimap.java @@ -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; @@ -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) stream.readObject()); - valueComparator = checkNotNull((Comparator) stream.readObject()); + keyComparator = requireNonNull((Comparator) stream.readObject()); + valueComparator = requireNonNull((Comparator) stream.readObject()); setMap(new TreeMap>(keyComparator)); Serialization.populateMultimap(this, stream); }