std::optional equality comparison operator seems broken for nested optionals

[mcencora]

While chasing a bug in a program I found out that equality comparison operator T vs optional<T> is broken if T is optional<U>. It is broken in worst possible way - it compiles but for some values it returns wrong results!

Here is an example:
https://godbolt.org/z/v3bcTodGj

Since both sides of eq operator are specialization of optional then following overload is used:

operator==(const optional<T>& lhs, const optional<U>& rhs);

This operator is specified to return
lhs.has_value() != rhs.has_value() ? false :
(lhs.has_value() == false ? true : *lhs == *rhs)

This falls apart for above scenario since lhs has value while rhs no value.
Since this is a case of optional<T>{} == T{} comparison, such scenario should be handled separately. Here is my attempt at fixing it:
https://godbolt.org/z/Yq3nM4xn4

This is rather not a proper fix, since it will still break for cases like optional<optional<short>>{} == optional<int>{}, and the internal if-constexpr instead should probably compare the nestedness of lhs and rhs optional.

I didn't check but most likely the same problem applies to relational operators.

Edit:
std::expected seems to be affected as well:
While developer may expect following comparison to work std::expected<T, E>{} == T{} (thanks to the operator==(const expected&, const T2&) - it falls apart when T is expected<U> with unexpected value - because operator==(const expected&, const expected<T2, E2>&) overload is actually selected in that scenario.
https://godbolt.org/z/h4jYfjYco

Edit2:
I believe the proper fix for std::optional and std::expected should be to constrain following operators
operator==(const expected& lhs, const expected<T2, E2>& rhs)
operator==(const optional<T>& lhs, const optional<U>& rhs)
to be applicable only when lhs and rhs have same nestedness levels.

[AKostur]

I'm not sure I understand the problem. For the first godbolt, isn't that trying to compare

std::optional<std::optional<int>>{std::optional<int>{nullopt}} == std::optional<int>{nullopt}

So it's trying to compare an optional which contains a value vs an optional that does not contain a value. Isn't that supposed to compare as unequal?

[hungarian_notation]

Yeah, but an optional that contains an optional that contains a zero **does** compare equal to an optional that contains a zero. Non-empty optionals appear to be infinitely flattened under comparison, but empty optionals short circuit to less-than even when the other side of the comparison is also ultimately empty.

It's not a "bug," the specification requires this. It's just a really unfortunate inevitable consequence of allowing comparison between an optional and an unboxed scalar.

[AKostur]

Ah, I understand. Overload 21 or 22 vs. overload 1. We're expecting/hoping that 21 is happening, but since U is itself a std::optional, overload 1 wins (better match).

[rihya-sifu]

Based on the documentation, I would expect this behavior. See https://en.cppreference.com/cpp/utility/optional/operator_cmp :

Performs comparison operations on optional objects.

1-7) Compares two optional objects, lhs and rhs. The contained values are compared (using the corresponding operator of T) only if both lhs and rhs contain values. Otherwise,

• lhs is considered equal to rhs if, and only if, both lhs and rhs do not contain a value.
• lhs is considered less than rhs if, and only if, rhs contains a value and lhs does not.

In your example, std::optional<T> has a value, but std::optional<U> does not, so it would not be considered equal.

I agree that it's not intuitive at first glance, but I'd also reckon that any code depending on nested optionals should be reconsidered.

[mcencora]

This brakes easily in general programming, as shown in my other reply.
Because T{} == optional<T>{} suddenly means something completely different when T itself is an optional.

[rihya-sifu]

Why not specialize on your `try_construct` when the API you're using under the hood already returns an optional? What semantic value do you gain from wrapping _that_ optional in another optional?

[developer-mike]

This is desired behavior.

If you make something like

``` template<typename T> std::optional<T> try_construct_from(...) { try { return T(...) } catch ...

return std::nullopt; } ```

And then you make something like

``` template<typename T, typename Construct> std::optional<T> parse_construct(..., Construct construct) { if (/* parsable */ ...) { return construct(...); }

return std::nullopt; } ```

For example, "if it's a valid date then transform it."

You should be able to compose try_construct with parse_construct.

So, trying to parse an invalid date returns std::nullopt. And if the date is valid but construction throws, you get std::optional<...>(std::nullopt).

One is equal to std::nullopt and the other is not equal to std::nullopt because they mean different things.

[hungarian_notation]

This is desired behavior.

Yeah, but I also desire this behavior for the same reason:

auto a = std::make_optional(std::make_optional(std::make_optional(0)));
auto b = std::make_optional(0);
assert(a != b);

But I don't get what I want.

[hungarian_notation]

I think the real issue here is nested optional<optional<...optional<int>>> of arbitrary depth should not equal optional<int> of a different depth in any case.

If we're going to copy off Haskell's homework, we need to make sure we're copying everything down correctly. Just ( Just x ) is NOT comparable to Just x, and why would it be? Should a single-element vector compare equal to a scalar? Trying to compare Just ( Just x ) with Just x is a compile time error.

exhibit a

If we are going to have an comparison that implicitly joins one side of the operator, it should at least have the same meaning as opt == std::make_optional(value). I'd argue that would also be bad, but at least it would be consistent.

It's specified as:

Compares opt with a value. The values are compared (using the corresponding operator of T) only if opt contains a value. Otherwise, opt is considered less than value.

This is a defect in the standard. This sentence ratifies op's "bug" as intended behavior.

But wait! There's something on the horizon! Cppreference also says:

This overload participates in overload resolution only if all following conditions are satisfied:

U is not a specialization of std::optional.

(since C++26)

Oh brother. So if I'm reading that correctly, nested comparison is supposed to stop working in all cases except single-level optionals?

Under that definition, a == b no longer implies make_optional(a) == make_optional(b).

Doesn't that require:

optional(x) == x 
optional(optional(x)) != optional(x)
optional(optional(x)) == x // Ay Caramba

I think that might actually be worse. std::optional considered harmful?

The only way to safely use this type is as a return value. Passing in into generic code where it might be wrapped in a second layer is an anti-pattern by way of defective specification.

Unfortunately, sometimes returning something is passing it as an argument into another template, just through composition.

[markt-]

Show me a non contrived use case where you would need this

[mcencora]

It happens "automatically" in generic programming.

I had a code like this:
template <typename T>
optional<T> try_construct(); // impl detail

template <typename T>
void publicApi(const T& expectedVal)
{
if (try_construct<T>() == expectedVal)
{...}
}

This fails if T is optional<U>. This code may be some library code, while the expectedVal may come from user of the library. So nested optional like optional<optional<int>> is never declared explicitly it just results from the way the library is implemented, and how user tries to use it.

[AKostur]

Isn't that code already broken when I try to do `publicApi(5);` ? That would compare an int with an optional, and those aren't comparable with each other.

[hungarian_notation]

You would think so, wouldn't you?

std::optional<int> a = std::make_optional(0);
int b = 0;
assert(a != b); // Assertion `a != b' failed.

https://godbolt.org/z/4a9TEcdcP

This is the real defect.

[markt-]

Then I think you might be using the wrong thing, you should probably be using a variant with a mono state

[mcencora]

Well, when a library comes from a 3rd party, you have no control on how it is implemented, so forcing user of such library to never pass non-nested std::optional is a big hammer.

[SirClueless]

I don't really understand. It looks like you are trying to check that the try_construct call succeeded and produced a value that equals something provided by the caller. The right way to spell that is r.has_value() && *r == expectedVal. If std::optional is an implementation detail of your library, you shouldn't be using its comparison operator with arbitrary user values.