Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider @NonNull in Map.computeIfAbsent #102

Open
cpovirk opened this issue Oct 10, 2024 · 0 comments
Open

Consider @NonNull in Map.computeIfAbsent #102

cpovirk opened this issue Oct 10, 2024 · 0 comments

Comments

@cpovirk
Copy link
Collaborator

cpovirk commented Oct 10, 2024

computeIfAbsent is annoying: The reason that almost everyone uses it is to insert a non-null value into the map if no value is yet present for that key. But very occasionally, people pass in a function that returns null, which is a perfectly valid thing to do according to the documentation and which is handled properly by implementations. But the natural result of that is that the return type of the method would @Nullable V, which is annoying for everyone else, requiring the kinds of suppressions and !!s that give people warnings blindness and give nullness checking a bad name.

Given that, a65745b (later commented upon in #32) annotated computeIfAbsent to require a function that returns plain V—and thus to have a return type of plain V[*]. This follows Kotlin's behavior (and following Kotlin's behavior may be important for some users for pragmatic reasons), and I had to migrate only one Kotlin caller (who was invoking the method on a Java Map subtype) to use compute instead (which required only a second function parameter and an associated if (existing != null) return existing).

I still feel reasonably good about that, if not completely good. We could talk again about whether to support nullable values.

(Yes, I think @PolyNull would be nice here, as in the Checker Framework, and one option would be to use @Nullable in the signature of computeIfAbsent but program special logic into checkers to give the method @PolyNull-like behavior, as they'll likely already want to do for methods like Optional.orElse.)

This issue is not about that. (Maybe I should preemptively open one now that is about that... :)) What this issue is about is whether to more aggressively use @NonNull. We could do so by making the V type in the function @NonNull and by making the return type @NonNull. This could theoretically maybe someday help someone who wants to use computeIfAbsent on a map with a nullable value type but still wants to get a non-null V out.

I'm actually filing this issue mostly to discuss a couple reasons not to do that:

  • As noted above, our current signature matches Kotlin's built-in treatment of the Map method.
  • An explicitly non-null type would close off escape hatches for getting a null through computeIfAbsent:
    • It might encourage tools to insert runtime null checks more aggressively.
    • It prevents you from casting your Map<Foo, Bar> to a Map<Foo, @Nullable Bar> and then being able to pass a null through without any problem.

(I also was mistakenly thinking that there was a separate opportunity to add @NonNull elsewhere in the signature. But I was thinking of cases like computeIfPresent, which receive a non-null "existing value" parameter, which obviously doesn't make sense for computeIfAbsent.)

[*] Observant readers may have noted that I made similar changes for compute and computeIfPresent and that merge was already in a similar boat. But I subsequently changed those methods to use @Nullable types after all (in #14 and #32).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant