Avoid calling checkNotNull on nullable values except during actual precondition checks.
#7090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![copybara-service[bot]](https://avatars.githubusercontent.com/in/44061?s=60&v=4){kind=small}
…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 #6824. RELNOTES=n/a PiperOrigin-RevId: 614074533
copybara-service
bot
force-pushed
the
test_612937549
branch
from
6b5956a to
52a62b2
Compare
copybara-service
bot
closed this pull request by merging all changes
into
master
in
52a62b2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Avoid calling
checkNotNullon 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 forcom.google.commonin 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 ofcom.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
checkNotNullon nullable values:DummyProxyis making anInvocationHandlerperform automatic precondition tests based on annotations on the interface it's implementing.EqualsTesterand 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
@NonNullhere in the future, similar to what we've discussed in #6824.RELNOTES=n/a