Skip to content

Commit

Permalink
Improve nullness of types in some APIs related to Map merging, and …
Browse files Browse the repository at this point in the history
…fix `Collectors.toMap` null-handling.

- Restrict `Collections.toMap` value-type arguments to non-nullable types.
  - ...in J2KT, following what [we'd found in JSpecify research](jspecify/jdk@15eda89)
- Fix `Collections.toMap` to remove the key in question when `mergeFunction` returns `null`.
  - ...in J2KT
  - ...in J2CL
- Use `@NonNull` / `& Any` in a few places in `Map.merge` and `Map.computeIfPresent`.
  - ...in J2KT
  - ...in Guava `Map` implementations, even though we don't yet include `@NonNull` annotations in the JDK APIs that we build Guava against. (See post-submit discussion on cl/559605577. But I've taken the shortcut of not waiting for the JDK APIs.)
- Use `@Nullable` (to match the existing Kotlin `?` types) in the return types of `Map.computeIfPresent` and `Map.compute`.
  - ...in J2KT
- Test a bunch of this.

Note that the test for `mergeFunction` has to work around an overly restricted `toMap` signature that J2KT inherited from JSpecify. As discussed in a code comment there, this is fundamentally the same issue as we have in Guava with `ImmutableMap.toImmutableMap`, which is discussed as part of #6824.

RELNOTES=n/a
PiperOrigin-RevId: 580659517
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Feb 29, 2024
1 parent fe62266 commit 351de58
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 22 deletions.
32 changes: 18 additions & 14 deletions guava/src/com/google/common/collect/Maps.java
Original file line number Diff line number Diff line change
Expand Up @@ -1735,15 +1735,15 @@ public V computeIfAbsent(
throw new UnsupportedOperationException();
}

/*
* TODO(cpovirk): Uncomment the @NonNull annotations below once our JDK stubs and J2KT
* emulations include them.
*/
@Override
@CheckForNull
/*
* Our checker arguably should produce a nullness error here until we see @NonNull in JDK APIs.
* But it doesn't, which may be a sign that we still permit parameter contravariance in some
* cases?
*/
public V computeIfPresent(
K key,
BiFunction<? super K, ? super /*@NonNull*/ V, ? extends @Nullable V> remappingFunction) {
K key, BiFunction<? super K, ? super @NonNull V, ? extends @Nullable V> remappingFunction) {
throw new UnsupportedOperationException();
}

Expand All @@ -1757,11 +1757,11 @@ public V compute(

@Override
@CheckForNull
@SuppressWarnings("nullness") // TODO(b/262880368): Remove once we see @NonNull in JDK APIs
public V merge(
K key,
/*@NonNull*/ V value,
BiFunction<? super /*@NonNull*/ V, ? super /*@NonNull*/ V, ? extends @Nullable V>
function) {
@NonNull V value,
BiFunction<? super @NonNull V, ? super @NonNull V, ? extends @Nullable V> function) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -3659,9 +3659,13 @@ public V computeIfAbsent(
*/
@Override
@CheckForNull
/*
* Our checker arguably should produce a nullness error here until we see @NonNull in JDK APIs.
* But it doesn't, which may be a sign that we still permit parameter contravariance in some
* cases?
*/
public V computeIfPresent(
K key,
BiFunction<? super K, ? super /*@NonNull*/ V, ? extends @Nullable V> remappingFunction) {
K key, BiFunction<? super K, ? super @NonNull V, ? extends @Nullable V> remappingFunction) {
throw new UnsupportedOperationException();
}

Expand All @@ -3675,11 +3679,11 @@ public V compute(

@Override
@CheckForNull
@SuppressWarnings("nullness") // TODO(b/262880368): Remove once we see @NonNull in JDK APIs
public V merge(
K key,
/*@NonNull*/ V value,
BiFunction<? super /*@NonNull*/ V, ? super /*@NonNull*/ V, ? extends @Nullable V>
function) {
@NonNull V value,
BiFunction<? super @NonNull V, ? super @NonNull V, ? extends @Nullable V> function) {
throw new UnsupportedOperationException();
}

Expand Down
14 changes: 6 additions & 8 deletions guava/src/com/google/common/collect/Synchronized.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import java.util.function.UnaryOperator;
import java.util.stream.Stream;
import javax.annotation.CheckForNull;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
Expand Down Expand Up @@ -1187,15 +1188,11 @@ public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction
}
}

/*
* TODO(cpovirk): Uncomment the @NonNull annotations below once our JDK stubs and J2KT
* emulations include them.
*/
@Override
@CheckForNull
@SuppressWarnings("nullness") // TODO(b/262880368): Remove once we see @NonNull in JDK APIs
public V computeIfPresent(
K key,
BiFunction<? super K, ? super /*@NonNull*/ V, ? extends @Nullable V> remappingFunction) {
K key, BiFunction<? super K, ? super @NonNull V, ? extends @Nullable V> remappingFunction) {
synchronized (mutex) {
return delegate().computeIfPresent(key, remappingFunction);
}
Expand All @@ -1213,10 +1210,11 @@ public V compute(

@Override
@CheckForNull
@SuppressWarnings("nullness") // TODO(b/262880368): Remove once we see @NonNull in JDK APIs
public V merge(
K key,
/*@NonNull*/ V value,
BiFunction<? super /*@NonNull*/ V, ? super /*@NonNull*/ V, ? extends @Nullable V>
@NonNull V value,
BiFunction<? super @NonNull V, ? super @NonNull V, ? extends @Nullable V>
remappingFunction) {
synchronized (mutex) {
return delegate().merge(key, value, remappingFunction);
Expand Down

0 comments on commit 351de58

Please sign in to comment.