Should we use optional<bool>? Alternatives?

[David Roger]

Is there any recommendation for or against optional<bool>, for example:

  absl::optional<bool> is_enabled;

where is_enabled == nullopt means that we don't know if it's enabled (e.g. because we're not initialized yet).

Problem: this may be error prone, as someone may be tempted to write code like this to check if the service is enabled:

  if (is_enabled) {

     // Do something requiring the service to be enabled.

  }

The code above is a bug, because the condition actually checks if |is_enabled| has a value, and not if the inner value of the optional is true.

Even in the absence of bugs, the code is harder to read, because at a glance, when seeing code like "if (is_enabled)" out of context, it easy to assume that |is_enabled| is a bool instead of an optional and misunderstand the code.

See also external discussions: example, example

Alternatives:

- Use an enum with three values (kFalse, kTrue, kUnknown). There are a few precedents in chrome code. Pro: simple. Con: verbose.

- Use a class like boost::tribool which has a safer and more explicit way to convert to bool (safe_bool()). Pro: nice behavior. Con: complex, need to learn an extra class.

Questions:

Is there already something like tribool in Chrome that we should use, or any plan to add it?

Any recommended option: keep using optional<bool>, or go with the enum?

I think my preference would be for the enum over optional<bool>, unless something like tribool already exists in chrome code.

[Alexei Svitkine]

This seems similar to:

https://reviews.llvm.org/D45601

Where clang now has a warning against a similar scenario that can happen with bool*. Perhaps something similar should be done for abs::optional? (Although not sure where - in our custom clang analyzer? since presumably clang core doesn't know anything about absl types.).

+Nico Weber who might have thoughts.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAL3rL5h_Wme%2Bx4%2B5Y7vQNr2TA854TL5JUwbe9BqMVaQxvAqX3Q%40mail.gmail.com.

[Daniel Cheng]

I believe that clang CL was abandoned due to hitting too many false positives.

This is probably a check that could make sense for clang-tidy rather than the clang plugin that Chrome builds with, due to the risk of false positives. I don't know if we have any pre-existing mechanism for extending clang-tidy with our own checks. +George Burgess 

I would strongly encourage using an enum for this. It's 100% clear, despite being a little verbose, and as a bonus, it even uses less storage (one byte for the enum vs two bytes for the optional<bool>). I suppose it's something we could consider adding to //base if it's that common.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SD9MkvR4_%3DD2H1yfzWwhFaht328OKkUAUVZZUVHbdTvKQ%40mail.gmail.com.

[dan...@chromium.org]

On Mon, May 31, 2021 at 6:18 PM Daniel Cheng <dch...@chromium.org> wrote:

I believe that clang CL was abandoned due to hitting too many false positives.

This is probably a check that could make sense for clang-tidy rather than the clang plugin that Chrome builds with, due to the risk of false positives. I don't know if we have any pre-existing mechanism for extending clang-tidy with our own checks. +George Burgess 

I would strongly encourage using an enum for this. It's 100% clear, despite being a little verbose, and as a bonus, it even uses less storage (one byte for the enum vs two bytes for the optional<bool>). I suppose it's something we could consider adding to //base if it's that common.

+1 I would use an enum over 2 levels of bools.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKryJtp9hPvjcztorruOKE-vDcKC74G4E1dkeuckf8xPrQ%40mail.gmail.com.

[Jeremy Roman]

+1 to using an enum. Also please avoid calling the enumerators kTrue and kFalse if at all possible. kEnabled/kDisabled is clearer IMHO and often even more clear terms are possible.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaRErgOGCQKhsnxM7P6YzhvwK-O5yP9ZNnoX34XFqMoVaA%40mail.gmail.com.

[Nohemi Fernandez]

Another alternative may be using OptionalOrNullptr, which is a simple wrapper around absl::optional that would resolve the confusion that may result with absl::optional<bool> and has some, albeit limited, precedence in the Chrome codebase. 

[Peter Boström]

To me, I don't see that OptionalOrNullptr helps. The existence of the optional<bool> will always have the operator bool foot-shooter, even if you use other ways of accessing the optional in original checked-in code. if(optional) or return optional; has to be caught in code review right now. This isn't a possible bug to accidentally introduce with a tri-state enum.

Imo when using an optional<bool> you have a pretty strong reason to always use optional.has_value() as you have to express what you actually mean, but that's right now only enforceable through code review, as would OptionalOrNullptr. "No optional<bool>" is easier to enforce or audit.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANMaTo%3DeB29EZ9NOiqpUW%3DEeGaLZDpXeWLo_VCvS1SaOfptBWA%40mail.gmail.com.

[Peter Boström]

FWIW I would be happy if optional<bool>::operator bool was a compiler warning/error in src/, but I have no idea if that's remotely feasible. That would also fix the foot shooter instead of requiring different types when what you want to express is an optional<bool>. You also wouldn't reinvent the API for kEnabled,kDisabled,kUndefined every time with different enum values in every location (unless there's a base::OptionalBool).

[Reilly Grant]

In general I like the existence of  optional<T>::operator bool but maybe we can make an exception for optional<bool> that doesn't allow it to nudge this gun away from developers' feet.

Reilly Grant | Software Engineer | rei...@chromium.org | Google Chrome

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGFX3sEQQ1kuTvW7j%2BF6mgq-_WnP_Ph1rYDotk%2BakUWYRcMVUQ%40mail.gmail.com.

[dan...@chromium.org]

On Tue, Jun 1, 2021 at 2:25 PM Reilly Grant <rei...@chromium.org> wrote:

In general I like the existence of  optional<T>::operator bool but maybe we can make an exception for optional<bool> that doesn't allow it to nudge this gun away from developers' feet.

Removing it just for optional<bool> will be a bit of a headache in templated code. The template works until you pass in the wrong type, now someone has to go fix the template.

I'd rather we drop the operator bool from absl entirely if we want to do that, but it would preclude us from ever migrating to std::optional (if our hardening checks don't already in practice end up doing so).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEmk%3DMa46F7oeHBopX7PvVT0Ue3gCauiUnyH82fGNX7em4PCtA%40mail.gmail.com.

[dan...@chromium.org]

On Tue, Jun 1, 2021 at 2:45 PM <dan...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 2:25 PM Reilly Grant <rei...@chromium.org> wrote:

In general I like the existence of  optional<T>::operator bool but maybe we can make an exception for optional<bool> that doesn't allow it to nudge this gun away from developers' feet.

Removing it just for optional<bool> will be a bit of a headache in templated code. The template works until you pass in the wrong type, now someone has to go fix the template.

I'd rather we drop the operator bool from absl entirely if we want to do that, but it would preclude us from ever migrating to std::optional (if our hardening checks don't already in practice end up doing so).

(to be clear, I don't consider that to have to be a deal breaker myself)

[Colin Blundell]

Could we just make it a Chromium C++ style guide directive to use an enum instead of optional<bool> and have a presubmit check that disallows optional<bool> from being used in the codebase?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaSiV8j_%3DQFwY6QKh_O-56wRfqV4RwhjzxeVbKorgvMw7Q%40mail.gmail.com.

[Peter Kasting]

I don't think this proposed rule "pulls its weight", so to speak. I also wonder what is different about Chromium here that means we should have such a rule when the Google store guide does not 

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NG9XnKLEROK2j_dduMTfEhUXd7OpuWrd8Q-idOn5yC5KA%40mail.gmail.com.

[Sylvain Defresne]

base::Value uses absl::optional<T> for the new API to get items for key/path in a dictionary (except for string where it returns a pointer to the internal stored string).

This means that base::Value::FindBoolKey() returns a base::optional<bool>. For the user, this is nice, since the code to base::Value new dictionary API is the same for all types. Using an enum here would make the bool type different (also, not sure that we could use anything besides kTrue, kFalse, kUnset for base::Value since there is not enough context for the API to know what the bool mean).

For less generic code, I agree that an enum is probably a better option.

-- Sylvain

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NG9XnKLEROK2j_dduMTfEhUXd7OpuWrd8Q-idOn5yC5KA%40mail.gmail.com.

[Colin Blundell]

On Wed, Jun 2, 2021 at 11:08 AM Peter Kasting <pkas...@chromium.org> wrote:

I don't think this proposed rule "pulls its weight", so to speak. I also wonder what is different about Chromium here that means we should have such a rule when the Google store guide does not

I would definitely defer to your judgment. I thought specifically of the suggestion as an alternative to that of generically removing optional<T>::operator bool. The latter to me seems like a net loss.

[Colin Blundell]

On Wed, Jun 2, 2021 at 11:12 AM Sylvain Defresne <sdef...@chromium.org> wrote:

base::Value uses absl::optional<T> for the new API to get items for key/path in a dictionary (except for string where it returns a pointer to the internal stored string).

This means that base::Value::FindBoolKey() returns a base::optional<bool>. For the user, this is nice, since the code to base::Value new dictionary API is the same for all types. Using an enum here would make the bool type different (also, not sure that we could use anything besides kTrue, kFalse, kUnset for base::Value since there is not enough context for the API to know what the bool mean).

That's good to know, thanks!

[David Roger]

Thank you all for the feedback, this is a very interesting discussion.

I've heard some other good points off-thread (thanks +Nohemi Fernandez):

- "is_enabled" is probably a bad name for an optional, and it would be better to use "maybe_is_enabled" or "try_is_enabled".

Similarly a function returning an optional should probably be called "MaybeIsEnabled()" or "TryIsEnabled()".

Having consistent naming conventions for optionals could probably help here reducing confusion.

- google style and internal google code do not ban optional<bool>

Also note there are some related recommendations for google internal code:

  go/totw/141#optional-values-and-scoped-assignments (internal link).

  They acknowledge it is a dangerous pattern, and propose mitigations (always use has_value() rather than automatic conversion, limit the scope of the variable as much as possible)

[Xiaohan Wang (王消寒)]

Just hit this in one of my own CLs. Given this is a common (dangerous) pattern, I'd suggest we have a dedicated class/struct wrapper for this, e.g. 

```

class OptionalBool {

  bool is_true() { return data_.value(); }

  bool is_set() { return data_.has_value(); }\

  // no bool() operator!

 private:

  absl::optional<bool> data_;

};

```

Thoughts and comments?

Xiaohan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAL3rL5hSqYXqSjdr1OCS9Xm7jqqbaQ1k1j0dxhgjgMc13QCmTQ%40mail.gmail.com.

[Daniel Cheng]

I can imagine situations where we'd still have an absl::optional<bool>. Instead of doing this, I think it'd be nice to add a clang-tidy check instead (I think this could even be upstream?) that would warn when not using the more explicit has_value().

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF1j9YME3FcxNZO2U6fuBWbc-Po6NJ1Xq5eNSZQV1ijrox1NgA%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

I'd like to have a common solution in Chromium instead of having every developer defining their own solutions. It could be a wrapper class or enum or something else. Personally I like a wrapper class more since we can have a very clean interface...

On Tue, Mar 1, 2022 at 10:59 AM Thomas Sepez <tse...@google.com> wrote:

Alternatively one might avoid the bool in the first place with something more descriptive like

enum class Foo { kNotEnabled = 0, kEnabled };

optional<Foo> is_enabled;

esp. if we are passing this value as an argument somewhere, Foo::kNotEnabled avoids writing /*is_enabled=*/false, and such.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKpZ2KCsWq0a%2BUzk5JbibvD%3DUutvPpopNP9QL5o2CCo%2BSA%40mail.gmail.com.

[Daniel Cheng]

There's a cost to this too though--more special-case patterns that every developer will need to remember. That's why I'd prefer we just stick with optional<bool> and just add warnings when someone uses the explicit bool conversion or * instead of the clearer has_value() and value().

(bool* has a similar issue too...)

Daniel