A gotcha with Optional

This post is about one gotcha in Boost.Optional library. When starting to use it, you might get the impression that when you try to put optional<T> where T is expected, you will get a compile-time error. In most of the cases it is exactly so, but sometimes you may get really surprised.

Let’s consider an example similar to the one described in my other post.

double Flight_plan::weight()
{
  if (Impl* impl = get_impl()) {
    return impl->compute_weight();
  }
  else {
    DO_WHAT; // ???
  }
}

Once in a while, it is possible to get a null pointer, and for some reason you choose not to signal it via an exception. One idea to reflect this situation is to use Boost.Optional:

boost::optional<double> Flight_plan::weight()
{
  if (Impl* impl = get_impl()) {
    return impl->compute_weight();
  }
  else {
    return boost::none;
  }
}

Sooner or later someone will use this function like this:

bool is_too_heavy(Flight_plan const& p)
{
  return p.weight() > p.aircraft().max_weight();
}

You might be thinking that at this point the compiler will detect a type mismatch and signal an error. This is what I was thinking when writing my other post, and I was wrong.

Look at this line:

return impl->compute_weight();

Function Impl::compute_weight obviously returns type double, but the type expected by the return statement is optional<double>. Nonetheless, the line compiles because optional<T> defines a converting constructor from T. This is very convenient in the cases like ours here, but it also implies some consequences.

Another thing that is not obvious and surprises some people is that optional<T> is LessThanComparable (whenever T is LessThanComparable). In this comparison, the value of boost::none is considered as a unique value less than any other value of T. This is a sort of lexicographical order.

These two features combined together render a “mixed” comparison a valid operation. Expression:

p.weight() > p.aircraft().max_weight();

Is interpreted as:

p.weight() > optional<double>{p.aircraft().max_weight()};

In this case the optional on the right-hand side is never “empty”, while the one on the left-hand side sometimes is, and in this case the comparison returns false, which means “aircraft is not too heavy”!

Advertisements
This entry was posted in programming and tagged , , , . Bookmark the permalink.

22 Responses to A gotcha with Optional

  1. Shea Levy says:

    Seems like a use case for N4029?

  2. rhalbersma says:

    Why not return optional<bool> when comparing two optional<T>’s instead of enforcing an arbitrary convention that none is less than all elements?

    [I have modified your question a bit, I hope I preserved the intent.]

  3. rhalbersma says:

    Well, a) that is a lame argument because it is an artefact that operator bool() checks the engaged state and b) it is also false because true is not less than true, so irreflexivity is still maintained. Having said that, the real problem of having an optional bool returned by comparisons seems to me that they cannot implicitly convert to bool if they are not engaged.

    • it is an artefact that operator bool() checks the engaged state

      — According to Wikipedia (http://en.wikipedia.org/wiki/Artifact) “artifact” means “undesired alteration in data, introduced by a technique and/or technology”, unless you meant something else? Note that the contextual conversion to type bool is an intended, documented and advertized feature of Optional’s interface and it also proves very useful in practice. This is quite the opposite of being an “artifact”. My guess is that you are familiar with some other type in some other language of the similar functionality, and you expect that Boost.Optional should behave the same way because this is so in the other language. Is my guess correct?

      that is a lame argument

      — Because of the reasoning above, I strongly disagree with your opinion.

      it is also false because true is not less than true, so irreflexivity is still maintained

      — I am not sure what you are trying to say here. I agree that true is not less than true, but I do not see how this relates to our discussion. A generic algorithm relying on the concept LessThanComparable will be implemented like this:

      // THIS CODE IS A MISTAKE
      template <typename T>
      T min(T a, t b) // forget the references
      {
        if (b < a) {
          return b;
        }
        else {
          assert (a < b); // Irreflexivity safety check
          return a;
        }
      }
      

      If you tried to instantiate it with:

      min(optional<int>(1), optional<int>(1));

      Assuming you have operator< defined like this:

      template <typename T>
      optional<bool> operator<(optional<T>, optional<T>);

      You get a bug (possibly a crash). No optional<bool> will be compared here.

      the real problem of having an optional bool returned by comparisons seems to me that they cannot implicitly convert to bool if they are not engaged.

      — Having optional<T> implicitly convert to T would be a disaster. This would cause so many bugs, if the programmers were not forced to convert manually (and think for a moment if this is what they want, and what they want if Optional contains no value).

      • rhalbersma says:

        Sorry, I realize my post must have come across snarkier than intended. Your points about implicit conversions are correct and well taken.

        My main point was that you use the term “irreflexive” in a somewhat confusing way. AFAIK, it simply means that comp(x,x) == false for any x, so also for unengaged optionals.

        I don’t quite get your assert(), shouldn’t that have a “a less or equal than b” inside? In any case, for your min(T, T) example should (IMO!) return a T, and so there should be a separate min(optional[T], optional[T]) overload returning an optional[T]. That means that there would be 3 cases inside this “lifted” min() overload, with the 3rd case returning “none” if either of the two arguments is not engaged.

        Does that make sense to you? It was for that line of reasoning that the Strict Weak Ordering breakdown does not seem obvious to me.

        • First, you are right that something is wrong with my example. My apologies for confusing you. It should have been:

          template <typename T>
          T min(T a, t b) // forget the references
          {
            if (b < a) {
              assert (!(a < b)); // Antisymmetry safety-check
              return b;
            }
            else {
              return a;
            }
          }
          

          Now, this min would fire an assert when instantiated with:

          min(optional<int>(1), optional<int>(1));
          

          Second, my understanding of what you are saying is that everything that takes an Optional as an input must return something that is Optional itself. And this is propagated throughout the whole program. Did I get it?

  4. Krzysiek says:

    “Second, my understanding of what you are saying is that everything that takes an Optional as an input must return something that is Optional itself.”

    This would look like a Maybe in Haskell, and would mean that optionals are as infectious as const.

  5. ethouris says:

    Well… I would personally prefer that if a boost::none value is being less-than-compared in this particular case it should throw an exception. I really don’t understand the idea of “optionals” that simply behave as if this were just a value like any other, just with some special properties. Using such an optional in a comparison in the code that was working so far with “double” introduces an new execution path that wasn’t predicted by the original author!

    You say that the original author should have been aware of that he’s comparing to optional (and could have checked that before comparing)? First, not if the so-far code was a template. Second, even if not in a template, this still introduces another bug-opportunity.

    Maybe exception isn’t the best solution for everyone, but this could’ve been at least given an option, for example, in Optional’s constructor, or as a second template parameter.

    • viboes says:

      Forget for a moment that the class is named optional. Imagine that we used this instead (nothing+T) where (nothing+T) means the union of nothing and T.
      The following ordering seems natural to me:

      nothing is less than something

      I have some trouble understanding your refactoring case. Could you give a concrete example where the code was working and replacing by optional it doesn’t works?

      • ethouris says:

        The code in this article is the example you’re asking for.

        Moreover, in this particular case you have a comparison of potential:

        (T) > (nothing+T)

        in which case it results in

        (T) > (nothing)

        which is more-less like comparing T to something that T has never been aware of, nor usually an author of the code that was using this instruction.

        The “works/doesn’t work” isn’t a criterion for evaluating correctness in the software engineering.

        • viboes says:

          The problem is not on the ordering but on the implicit conversion from T to optional. If the conversion was explicit, the user would need to require the comparison using


          p.weight() > optional{p.aircraft().max_weight()};

          and here we see what the user is requesting. It is no more an accident.
          Note that I have not use as the parameters are deduced in C++17.

          Implicit conversion are very often surprising. Having an explicit conversion we have that an optional can be constructed from nullopt | optional(t). This is quite close to Haskel Nothing | Just T.

          BTW, std::variant has the same issue with variant.
          Having explicit constructors for variant is not very friendly because we can not deduce the other parameters. The following will not work


          p.weight() > variant{p.aircraft().max_weight()};

          But we can add an explicit alternative class that is implicitly convertible to any variant having this type. T is explicitly convertible to alternative, that is implicitly convertible to variant


          p.weight() > alternative{p.aircraft().max_weight()};

          Wondering if it is not too late to have explicit constructor for C++17 to avoid this unfortunate cases.

        • Wondering if it is not too late to have explicit constructor for C++17 to avoid this unfortunate cases.

          — In my current mental model, that favors explicitness, the implicit conversion in optional is wrong, it compromises type safety. But I find it unlikely that this could be changed. There exist other factors that drive the current state:

          1. Optional has been used in this form (with converting constructor) in Boost for almost 15 years. We know it works fine for people. The explicit constructor is technically sound, but it was not tested in practice.
          2. Optional has other features that amount to its total success, apart from type safety: you can change the function signature from T to optional<T> and function calls still work.
        • viboes says:

          Replying to Andrezj October 2, 2016 at 6:34 pm

          Thanks for your quick replay.

          I don’t buy the argument for a transparent change from T to optional in a function signature. This is exactly the source of the error when comparing optional values. optional is not a T and doesn’t behave like a T. The implicit conversion as you show in this blog is a source of bugs.

          I understand that it is too late for C++17 and that we need more experience with the explicit conversion. What do you think of having a safer optional with an explicit conversion, why not in Boost or in the TS, so that we can experiment?

          I was wondering also if the std::expected proposal shouldn’t have an explicit conversion so that we can experiment.

        • What do you think of having a safer optional with an explicit conversion, why not in Boost or in the TS, so that we can experiment?

          — I am slowly trying to propose to Boost an alternative to Optional, that serves a slightly different purpose: markable. It has taken different trade-offs between type-safety and flexibility, which includes zero implicit conversions.

          Another optional in a TS: do you mean shipping the current one as is and at the same time testing (and later proposing) a second one?

          There might be a way of testing it in Boost: having a slightly different interface around the same implementation as the current Optional is using. I will need to think about it a bit more.

  6. Darrell Wright says:

    I ended up implementing an Optional in the past. Using a unique_ptr to store the value. The comparison operators would throw (a custom exception but could be null derefence) if one of the values did not contain a value. For me, it did not make sense to compare a value with a non-value for order. However, == and != do make sense.

    I would much rather crash if I didn’t account for empty value’s than go forward and mask the errors.

    On the other hand, the Expected classes are nice in that they will carry the exception/error to the get method and throw then.

  7. Christopher Beck says:

    Can someone explain the motivation for defining `operator <` at all for boost::optional? It seems that this is really the source of the gotcha and I can't understand how it is natural that optional should support this operation. I can understand why you would want to define `operator ==` and that seems safe, but with `operator <` it seems there is no way to extend it to make none comparable with values without bringing back the bad old "magic value" type behavior as Andrzej exhibited.

    From a mathematical point of view it seems that you are then forced to make some messy and arbitrary decisions which usually one avoids, as these are a sign of design-smell.

    Also, even if we think of optional as a container, it doesn't usually make sense that containers can be compared either. Usually the programmer should choose a comparator explicitly to avoid confusion.

    I realize this is an old post, hopefully you'll forgive me.

    • The goal is to provide what Elements of Programming calls the default total order for optional<T>. This is a fairly common view that operator< is better used for the default total order than for checking if a number exceeded a given threshold. This is a similar reasoning that led to operator= do anything else than equality comparison.

      With this decision operator< works out of the box with STL tree-based containers, as explained in the docs.

      Note that std::vector<T> is also LessThanComparable with the semantics of lexicographical compare: the same one that is used in optional<T>. The only reason for std::vector<T> not exposing the same problem is that you cannot convert a T to a std::vector<T>.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s