Improve Map.merge annotations.

[cpovirk]

1. Add @Nullable to the method return type and to the "return-type" type
   argument of remappingFunction.
   
   We had been trying to get by without, requiring users to suppress where
   needed. But our philosophy has shifted more toward "If you want to be
   able to put null in or get null out, then it needs to be @Nullable."
   
   This change brings us closer to upstream, which uses @PolyNull.
2. Add @NonNull to value parameter and to the "input" types argument of
   remappingFunction.

[wmdietl]

Adding the @Nullable looks good. It would be interesting to learn how many new warnings you get from the @Nullable on the return type. That would be another case for adding @PolyNull.
I do have a question about the value parameter.

[wmdietl]

In the PR title, should on the method and function return type be on the parameter and return type or something else?

[cpovirk]

It would be interesting to learn how many new warnings you get from the @Nullable on the return type. That would be another case for adding @PolyNull.

Realistically, the answer is likely to be "zero," just because we don't use these stubs that widely yet.

Well, OK, it's not going to be "zero" because I'm going to have to update some implementations of these methods in com.google.common to match. Well, OK, those failures are technically going to come from the @Nullable inside the parameter type, not the one on the return type :-P

[cpovirk]

In the PR title, should on the method and function return type be on the parameter and return type or something else?

"function return type" was meant to refer to the final type argument of remappingFunction.

I'll see what I can do about that as I rework the description as a whole to cover the newly broadened scope from the @NonNull annotations discussed here and in eisop/jdk#41.

[wmdietl]

Thanks, looks good!

[cpovirk]

It would be interesting to learn how many new warnings you get from the @Nullable on the return type. That would be another case for adding @PolyNull.

If I make the change for our Checker Framework users, I do see errors. It's not many, simply because there merge is mildly rare and Checker Framework usage is still a minority in our depot. But I skimmed over almost every call that we have to merge in Checker-Framework-checked code (at least in files that have a suppression somewhere), and it's fair to say that essentially every merge call wants to return a non-nullable value :)

I still feel OK with this PR, at least for now :) I could imagine taking a different path by avoiding @Nullable in computeIfAbsent, which is more commonly used and which is "even more obviously" intended primarily for cases in which you want to insert a key. But I don't know really know what I think. I think the merge thing was prompted by some Kotlin work, so maybe we'll see if the Kotlin work touches on computeIfAbsent, too.

[cpovirk]

We might someday care that Kotlin avoids @Nullable on computeIfAbsent, even as it uses it on compute, computeIfPresent, and merge:

https://github.com/JetBrains/kotlin/blob/1391c983a29c52996ca85b747383edfbfb24997c/core/compiler.common.jvm/src/org/jetbrains/kotlin/load/java/typeEnhancement/predefinedEnhancementInfo.kt#L109-L110

[cpovirk]

...and a week later, we're seeing actual problems from the difference between our signatures and Kotlin's :)

[cpovirk]

(The Kotlin problems we're seeing appear to run deeper than a mismatch between our signatures and Kotlin's, but the mismatch does appear to be part of the picture. I'm hoping to report a Kotlin bug and then link to it from here.)

Backing up to computeIfAbsent, which I've mostly been ignoring here: I just encountered one user who does use it to sometimes compute a null value. (It's possible that putIfAbsent would be OK there, but the computation function is long. I can't tell at a glance whether it's slow, but it's in graphics code, so there may be good reason to avoid even small overhead.) I'm probably going to migrate that user to compute after #32 lands.

I still feel OK about annotating computeIfAbsent to forbid nullable results, but we now have an existence proof that that could break someone's compile. (I'm not sure it could realistically break anyone at runtime, which is the more important thing, but I haven't thought it through.)

[cpovirk]

(I'm not sure it could realistically break anyone at runtime, which is the more important thing, but I haven't thought it through.)

I think I can envision a way:

val result = concurrentMap.computeIfAbsent(key, ::unannotatedJavaFunctionThatMightReturnNull)

I don't think that's likely, but it's possible (I think... I haven't tested).

[cpovirk]

(I just learned of another case (this time in Java) in which someone is using computeIfAbsent with a function that might return null. At least, the person I talked to seemed to be suggesting that null was possible, and the function was calling a method with a nullable return type. I didn't ask explicitly, nor experiment with the code, but it seemed pretty clear that null was a possibility.)

[cpovirk]

...and a week later, we're seeing actual problems from the difference between our signatures and Kotlin's :)

To partially tie up this loose end: I've confirmed that the problem we were seeing will go away once we can get Kotlin to recognize our usages of @NonNull on Map.computeIfPresent and Map.merge (without which our signatures for those methods appear to differ from Kotlin's).

If I'd been thinking, I would have tested what would happen if I were to change our signature for Map.computeIfAbsent to differ from Kotlin's. However, I also got the impression that Kotlin's K2 compiler may have looser checks in this area in general (at least so far), so that may be mostly academic.