Guidances WRT java.util.Optional

[Andrew Grieve]

I think it'd be a good idea to add some style guide text about the use of Optional (bug). Please share any thoughts / opinions on the following (not a suggestion for text to add, but first trying to align on what the stance should be):

1. Optional methods that accept lambdas should be banned.

E.g.: myOptional.ifPresent(x -> x.foo());

Rationale: The lambda makes this 10x bigger than an "if" in binary size, and it also requires an object allocation.

2. Optional Boxed types should be banned

E.g. Optional<Integer>

* Better to use @Nullable Integer. 

Rationale: The use of "Integer" outside of a collection already implies "this can be null".

Rationale: Boxing a boxed type makes for two layers of unboxing.

Rationale: @IntDef works with Integer, but not with Optional<Integer> (I think)

3. Optional should be banned for reference types

E.g.: Always use @Nullable

Rationale: No risk of using banned optional methods if we don't use optional (this would make the guidance very simple)

Rationale: Cuts down on wrapping / unwrapping verbosity

Rationale: Compared to std::optional, NPEs are not security bugs, and std::optional does not require a heap allocation.

4. Allow Optional only for return types

Rationale: There seems to be some vocal proponents of this, although I don't really see why it's different form parameters / fields.

For some concrete examples, here are some actual conversion CLs wrt Optional:

• Optional.ifPresent (shows large size overhead of lambdas)
• Use Optional<Long> for Tab timestamp (shows size increase even not using lambdas, and an example of where a INVALID_TIMESTAMP sentinel already exists)
• Use of Optional in AutoFill code (mostly show verbosity of wrapping / unwrapping, and small size difference)

[Patrick Noland]

>2. Optional Boxed types should be banned

This seems fine.

> 1. Optional methods that accept lambdas should be banned.

> 3. Optional should be banned for reference types

Either one of these would make optionals close to pointless for me; if we agree on these restrictions I think we'd be better off just blanket banning them instead of making their scope pointlessly small. This isn't necessarily a bad thing depending on your feelings about them. 

I like that Optional is explicit; @Nullable is easily ignored, omitted, or inverted by accident. You have to interact with an Optional in a way that makes it clear the value may not be present. I also like the ergonomics of being able to chain computations without explicit if(null) checking. I could certainly live without these things; the question with the eternal ergonomics vs. performance debate is always cost-benefit.

The linked CL showing the overhead shows a +292 byte, +1 dex method increase from 5 new calls to ifPresent.  Is that a big increase in absolute terms? Is the use of optional causing substantial binary bloat? How expensive is that extra object allocation in practice? How much does/could R8 help us optimize this sort of stuff away?

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1VTvJMe5_nVZsvRJbnU_S3Qqa-cD9uP24MmzPURpyyaKw%40mail.gmail.com.

[Tommy Nyquist]

Hi, Andrew!

Thanks for bringing this up! I agree that (1) is unnecessary and I think I'd be OK with possibly banning it, though I see the appeal.

For the other ones though, I do have a recent example where I have intentionally decided to use Optional instead of Nullable or special values for integer values or enums.

I prefer using Optional if you have a service that is defined in C++, but it has a shim layer in Java, if the main service in C++ uses std::optional. I strongly prefer having the same interface in C++ instead of having to use things like @Nullable, or special magic `int` values like -1 or magic enum values.

A recent example is MessagingBackendService (KeyedService API, Java bridge, C++ bridge).
Over the JNI boundary, which is internal to the bridge implementations, we do use special values, but the public API matches the C++ counterpart it is intending to mirror.

I know we could decide that if you use std::optional in a C++ service and want to mirror it to Java, std::optional should be written as @Nullable. It's just that we are not consistent across our codebase of using @Nullable, and using Optional to me feels like the API is telling me that something is really Optional, whereas @Nullable feels more like "this class can technically deal with null values".

--

Tommy

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CACoNVwyi9c-5sMMa6iJZEDjSDDBSfLbEMMmDc9wQ3%2B%2B6Xfu1Pg%40mail.gmail.com.

[Henrique Nakashima]

I agree #1 should be banned based on binary size.

The nice thing about Optional over @Nullable is the compilation error when you're connecting two pieces of the code, one that knows it's providing a possibly non-existent value (Optional), the other that assumes it exists (regular type). I would be 100% in favor of banning Optional if passing a @Nullable to an argument that's not-@Nullable gave me the same compilation errors (and we'd need some way to show a @Nullable is actually non-null, so it can be passed). Is that possible? Would we need @NonNull across the whole code base (which would make it hard to justify)? We already should consider that params and fields without @Nullable are effectively @NonNull. I found this email thread about enforcing @NonNull.

> I know we could decide that if you use std::optional in a C++ service and want to mirror it to Java, std::optional should be written as @Nullable. It's just that we are not consistent across our codebase of using @Nullable, and using Optional to me feels like the API is telling me that something is really Optional, whereas @Nullable feels more like "this class can technically deal with null values".

That's a good point since what matters is everybody's collective perception, but my personal perception was that @Nullable means that the argument is optional, and the method can fulfill its purpose with a null argument, and not just that the method won't crash given a null argument.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CAA6XDjO%2Bwaph2M3Vmoktttp8jPDNumokoNLoxCCKioVSp6acXA%40mail.gmail.com.

[Andrew Grieve]

That C++ uses std::optional doesn't seem like it's necessarily the best move to use Java's Optional. std::optional generally requires using 2 registers in place of one, but Java's Optional requires two heap-allocated objects instead of a single register. While individual uses wouldn't matter performance-wise, if it becomes common-practice, it could add-up. Having @IntDef not work with Optional is another point against. I still see there's an upside in that the APIs look the same... But I do wonder if in this case relying on the UNDEFINED enum value would be the best cross-language solution. Or maybe just use "int" when searching, and define a sentinel constant beside the enum to be "UNDEFINED".

On Fri, Nov 22, 2024 at 1:03 PM Tommy Nyquist <nyq...@chromium.org> wrote:

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CAA6XDjO%2Bwaph2M3Vmoktttp8jPDNumokoNLoxCCKioVSp6acXA%40mail.gmail.com.

[Andrew Grieve]

On Fri, Nov 22, 2024 at 1:47 PM Henrique Nakashima <hnaka...@google.com> wrote:

I agree #1 should be banned based on binary size.

The nice thing about Optional over @Nullable is the compilation error when you're connecting two pieces of the code, one that knows it's providing a possibly non-existent value (Optional), the other that assumes it exists (regular type). I would be 100% in favor of banning Optional if passing a @Nullable to an argument that's not-@Nullable gave me the same compilation errors (and we'd need some way to show a @Nullable is actually non-null, so it can be passed). Is that possible? Would we need @NonNull across the whole code base (which would make it hard to justify)? We already should consider that params and fields without @Nullable are effectively @NonNull. I found this email thread about enforcing @NonNull.

> I know we could decide that if you use std::optional in a C++ service and want to mirror it to Java, std::optional should be written as @Nullable. It's just that we are not consistent across our codebase of using @Nullable, and using Optional to me feels like the API is telling me that something is really Optional, whereas @Nullable feels more like "this class can technically deal with null values".

That's a good point since what matters is everybody's collective perception, but my personal perception was that @Nullable means that the argument is optional, and the method can fulfill its purpose with a null argument, and not just that the method won't crash given a null argument.

Thanks for digging up that thread. In case anyone doesn't have access, the email was effectively just a link to this bug: https://crbug.com/40657579, which is about adding compiler checks for nullablity.

AFAIK, no one actually ever spent any time looking into it, and it looks like the NullAway project is still actively supported, so perhaps it'd be wise to see how feasible it is before making any decisions on Optional usage. I'll sign up to at least prototype it.

[Ted Choc]

Just weighing in w/ my 2c:

1.) I do find the lambda API usage of Optionals to be really clunky and exceptionally verbose as compared to a null check.

2.) I personally think Optionals can be useful in public APIs (although I think the nullability of capital letter Integer / primitive types is sufficient in many cases), but generally are overkill for internal-only use.

Happy to go with the flow here as I don't have a hugely strong opinion one way or another.

- Ted

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1Xhy7HmdkwoKjuBHyqTpZTMcFuhyN9Wurje3pE_BS0vkg%40mail.gmail.com.

[Peter Conn]

I think generally it makes sense to standardise on either nullability or Optional, and given the binary size implications of the latter this seems like the right path to me.

 Having NullAway in our codebase would be great.

[Andrew Grieve]

I've made a good amount of progress on answering whether or not we can adopt NullAway as a @Nullable checker. I've been tracking progress in this doc, and have annotated & enabled enforcement on base_java. 

My team and I plan to spend next week trying to annotate other java targets before we make a final go/no-go on adopting this. I'll update the thread again afterwards and we can maybe decide on Optional in light of knowing whether or not a null checker is doable.