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

requireNonNull(..., Supplier<@Nullable String> messageSupplier? #104

Open
cpovirk opened this issue Nov 6, 2024 · 3 comments
Open

requireNonNull(..., Supplier<@Nullable String> messageSupplier? #104

cpovirk opened this issue Nov 6, 2024 · 3 comments

Comments

@cpovirk
Copy link
Collaborator

cpovirk commented Nov 6, 2024

Currently, we require a supplier that returns a non-null string:

public static <T> T requireNonNull( @Nullable T obj, Supplier<String> messageSupplier) {

However, the resulting string is merely passed as an exception message, so null would be fine as a result.

The somewhat unfortunate thing is that the Java base type forces us to choose between Supplier<String> and Supplier<@Nullable String>, at least in the absence of @PolyNull. (What we'd prefer is a base type of Supplier<? extends String>, which we could then annotate as Supplier<? extends @Nullable String>.)

It's also a little sad that "Supplier<@Nullable String>" just looks more complicated in general. But then probably few people actually look at the stubs directly.

We could try Supplier<@Nullable String> and see if there's any fallout from callers who pass an explicit Supplier<@Nullable String>. My guess is that callers are using lambdas over anonymous classes, so they won't generally specify explicit types. But we might see cases in which people have declared their own APIs with Supplier<String>, in which case this change would break them, requiring them to change their own APIs or pass supplier::get as a way of translating (and setting off static analysis like https://errorprone.info/bugpattern/UnnecessaryMethodReference... :). I'll see what the Google results look like. If I see a lot of errors, I'm not sure if my conclusion will be "not worth the trouble" or "better fix this now before it gets harder"....

cpovirk added a commit that referenced this issue Nov 6, 2024
The docs at best indirectly imply that it can be `null`: They have a
`@throws NullPointerException` clause that mentions only the case in
which `obj` is `null`, without saying anything similar about
`messageSupplier`.

But of course I'm actually going mostly off the implementation....

(Note that this PR is separate from
#104, which describes *another*
change that we could make to this parameter.)
@cpovirk
Copy link
Collaborator Author

cpovirk commented Nov 7, 2024

The change works fine in the Google depot.

@wmdietl
Copy link
Collaborator

wmdietl commented Dec 18, 2024

Do you see any code that is incorrect when using Supplier<String>?
It seems odd to go through the trouble of providing a supplier, just for it to return null. I see how it is more flexible, but are we encouraging better code with that flexibility?
I agree that using PolyNull would be nicest.

cpovirk added a commit that referenced this issue Dec 18, 2024
The docs at best indirectly imply that it can be `null`: They have a
`@throws NullPointerException` clause that mentions only the case in
which `obj` is `null`, without saying anything similar about
`messageSupplier`.

But of course I'm actually going mostly off the implementation....

(Note that this PR is separate from
#104, which describes *another*
change that we could make to this parameter.)

Co-authored-by: Werner Dietl <[email protected]>
@cpovirk
Copy link
Collaborator Author

cpovirk commented Dec 18, 2024

I'm not aware of any trouble from the Supplier<String> type. And actually, it turns out that the method has so few callers in our depot (ignoring third-party code) that I can look at them all. And they're all definitely fine—mostly things like String.format and string concatenation, as you'd expect.

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

2 participants