Update signature of Map.computeIfAbsent

[msridhar]

mappingFunction is allowed to return null to avoid updating the mapping, and in this case, computeIfAbsent returns null. Also updated overrides in @NullMarked classes.

I added the @PolyNull comment on Map.computeIfAbsent since I think we'd use it here if it were available; didn't add the comment on all the overrides.

Discovered in ben-manes/caffeine#2004 (comment)

[cpovirk]

That is all true, yet this has been the only(?) case in which I've been holding out hope that we can declare the signature as more restrictive than it really "should" be, just because a @Nullable return type is so inconvenient for 99% of callers. There is some discussion in #102, but beware that the actual title of that issue is about going even further in the opposite direction (as probably no one actually wants), even though the text goes on to mostly discuss the thing proposed here that people reasonably would want :) I'd be interested in your read on it, whether here or there. I do kind of feel like we'll need to go with the "honest" type eventually, though it would be nice to sort out the Kotlin story first, and I haven't found time :(

[msridhar]

Ah...bummer. We could change the type of mappingFunction to be Function<? super K, ? extends @Nullable V>, but (inconsistently) leave the return type as V (without a @Nullable). But the current version is safer.

We have an open issue (uber/NullAway#1616) on adding some kind of ad hoc @PolyNull support to NullAway for library models, for orElseGet. We might have to do the same thing here, or just ask users to suppress the warning. I'll have to think about how best to recommend a narrow suppression, though, rather than the entire surrounding method.

[msridhar]

@cpovirk I am curious, it seems that Optional.orElseGet is a pretty analogous situation to this one. Do you see more cases where the Supplier passed to orElseGet actually returns null?

[cpovirk]

I agree that there is at most a difference in degree between the two, not a difference in kind.

I do somewhat expect that we're going to need to eventually relent on Map.computeIfAbsent, though ideally after I get to the bottom of the recent Kotlin changes we saw.

Anyway, to actually answer your question:

• I see a surprising amount even of optional.orElseGet(() -> null), which of course is an unnecessarily long version of optional.orElse(null).
• I see a good amount of optional.orElseGet(otherOptional.orElse(null)), which is more reasonable, though it could still be slightly simplified to optional.or(otherOptional).orElse(null).
• I don't have an extremely convenient way to look for null returns other than those, though I could probably get a sample without too much trouble. But my uninformed guess is that we'd see a fair bit more.

null is much rarer on the computeIfAbsent side, though it does come up:

• I finally did what I should have done years ago and looked at our Checker Framework users for those who return nullable values from a computeIfAbsent function. I see a small number that might amount to around 1% of computeIfAbsent in such code, though I am estimating a bit here.
• This may well be in line with what we saw when we set up our Kotlin builds to disallow null returns from computeIfAbsent.

Additionally, there are some slight practical differences between the two methods:

• alternatives:
  • computeIfAbsent has a fairly straightforward alternative of compute, which we have left with @Nullable types. (The @Nullable return is probably necessary for compute users in general, but it also crossed my mind that it's necessary to provide an alternative for would-be users of computeIfAbsent.)
  • Optional.orElseGet (I think?) doesn't quite have so close an alternative. That said, it would be fair to say that it doesn't need a direct alternative the way that computeIfAbsent needs one for performance and concurrency support: You can always just test/unwrap the Optional.
• runtime enforcement:
  • If we were to omit @Nullable, then Optional.orElseGet would have a return type that is straight-up non-null. This opens the possibility of runtime enforcement, which I've seen a couple open-source projects play around with and which I've experimented a little with internally (leading to Annotate ResourceBundle.parent as @Nullable. #157 and Add a few missing @Nullable annotations. #158, coincidentally :)).
  • Map.computeIfAbsent returns V, which doesn't definitively exclude null. Of course, it does when V is itself a non-null type, but it's difficult for runtime instrumentation to know that.
  • But if we look at computeIfAbsent on a value whose runtime type is ConcurrentMap, then we do know that it's non-null, and the danger returns.
  • This incidentally also means that there's probably another workaround, which is to cast your map to Map<K, @Nullable V> (i.e., add @Nullable and remove ConcurrentMap if necessary) before calling computeIfAbsent. I'm not sure whether I've tried that.
• complexity in general?
  • I have wondered whether even static analysis benefits from the simplicity of orElseGet relative to computeIfAbsent. By "simplicity," I mean something related to the "straight-up non-null" point under my "runtime enforcement" bullet: I see two reasonable ways to define @PolyNull, and those two amount to the same thing for orElseGet but to different things for computeIfAbsent. (And "the computeIfAbsent approach" is wrong for other APIs for which it would be nice to be able to use something roughly along the lines of @PolyNull.) Sorry, I am not expressing any of this well. I will cut my losses here and try to do better at some future point.

[msridhar]

Super helpful, thanks @cpovirk! FWIW the computeIfAbsent case came up in the context of Caffeine, which perhaps uses the null return functionality more than most projects. FYI @ben-manes