[boost] [outcome] Review of Outcome

Subject: [boost] [outcome] Review of Outcome
From: Peter Dimov (lists_at_[hidden])
Date: 2017-05-28 13:57:32

What follows is my review of the Outcome library.

I. DESIGN
---------

The general idea of an error handling framework is sound, extremely 
appealing, and I found it well explained in the documentation. I like the 
concept very much and agree with many of the design decisions taken 
throughout. Specifically, constraining the error type to be the fixed 
std::error_code (or equivalent) and providing a wide accessor interface.

I don't think that option<T> should have been included at all. It duplicates 
optional<T> and has no good reason to exist other than to showcase that the 
underlying policy-based implementation can produce it.

I don't consider expected<T, E> a particularly needed part of the library; 
the focus should be on result<T> and outcome<T>, the two classes that 
represent the error handling philosophy on which the library is built.

As already stated, I consider the interface of result<T> and outcome<T> a 
bit too cluttered. Not everyone shares my design preferences for minimal but 
complete interfaces, so there's legitimate argument to be had over whether, 
for instance, operator*, operator-> or value_or ought to be included, but 
the needless duplication between get() and value() is, well, needless.

For an initial submission, it's better to err on the side of too little, 
rather than too much, because it's easy to add later, hard to remove.

The empty state has been a source of controversy, and with reason. My 
initial stance was that it simply needs to go. This however presents a 
legitimate design question as to what the behavior of the default 
constructor ought to be. After seeing the suggestions of leaving the object 
uninitialized there, I now see the empty state as a significantly lesser 
evil. Having an empty state without the default constructor putting the 
object into it however is pointless.

I do not agree with the direction things have taken of providing a multitude 
of result classes instead of a single one. Components that are intended to 
be used in interfaces should provide one, carefully designed option, which 
encourages what the designer deems best practices. The idea here, assuming 
that we shoot for standard acceptance at some point, is to provide 
std::result<T> and have APIs return it, not provide a bag of options and let 
everyone pick its own thing. (They already do that today and we're not 
better for it.)

I do not agree with making value() unchecked by default. As I explained, one 
expression of the error handling philosophy behind the library is the 
following snippet:

  auto r = function().value();

where function() is noexcept and returns result<T>, whereas the place where 
the above line appears is a higher level component that signals errors not 
via error_code, but via exceptions. This one-liner provides the transition 
in an elegant way; losing it obscures the vision behind the library.

Instead of using std::error_code, the library provides its own 
error_code_extended, complete with an interesting and very useful ring 
buffer logging mechanism that stores additional information along with the 
standard code/category values. Specifically, it returns error_code_extended 
from result<T>::error().

I would separate things a bit further here, attaching the additional 
information not to a separate error_code_extended class, but to result<> 
itself. That is, result<>::error() would return std::error_code, and I'd 
provide a separate accessor, result<>::error_info(), say, that would return 
the ring buffer entry associated with this result<>. This for me provides a 
cleaner separation between the basic result<> functionality, and the 
extended one, although this is a matter of taste.

I would also support chaining result<>s by storing the index of the parent 
ring buffer entry in the current ring buffer entry. This is a 
straightforward and very useful extension.

Instead of providing a macro BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT, I 
would provide

  result<T>::set_error_from_exception(std::exception const& e);

and

  outcome<T>::set_error_from_exception(std::exception const& e, 
std::exception_ptr const & p);

that would do more or less what the macro does, except the fallback in the 
second case would be to store p instead of (EINVAL, e.what()).

I'd also use make_error_result and make_value_result instead of 
make_errored_result and make_valued_result, as the former seem more 
grammatically correct.

II. IMPLEMENTATION
------------------

The implementation, from a cursory look, appears of high quality, which is 
not surprising given that it was in use for two years. The preprocessed 
header is not easy to review, but the non-preprocessed form is available and 
amenable to study.

Simple examples using outcome<T> compile and work well, although under 
VS2017 Clang/C2 windows.h is included, which is unfortunate but fixable.

The library does not provide a test/Jamfile, which means that as submitted 
it will not integrate into Boost's testing infrastructure. Given that a 
Jamfile that performs the equivalent of withmsvc.bat is trivial to write:

  import testing ;

  project : requirements <include>../include/boost/outcome/v1.0 ;

  run unittests.cpp ;

the omission is inexplicable. (Further integration into the Boost test 
matrix would ideally entail splitting unittests.cpp into separate .cpp tests 
so that success/failure can be tracked independently.)

With the above Jamfile, trying to test the library with Cygwin gcc fails 
with:

D:\tmp2\boost-master\libs\outcome\test>b2 toolset=gcc-cxx14
[...]

gcc.compile.c++ 
..\..\..\bin.v2\libs\outcome\test\unittests.test\gcc-cxx14-6.3.0\debug\unittests.o
In file included from ../include/boost/outcome/outcome.hpp:26:0,
                 from unittests.cpp:34:
../include/boost/outcome/outcome_v1.0.hpp:1446:22: fatal error: execinfo.h: 
No such file or directory
#include <execinfo.h>
                      ^
compilation terminated.

I realize that Cygwin is not a supported platform, but this is what I have 
here and this is what I test with, so I figured I'll give it a try.

Trying to test with VS 2017 fails with:

D:\tmp2\boost-master\libs\outcome\test>b2 toolset=msvc-14.1
[...]
compile-c-c++ 
..\..\..\bin.v2\libs\outcome\test\unittests.test\msvc-14.1\debug\threading-multi\unittests.obj
unittests.cpp
unittests.cpp(780): error C3245: 
'boost::outcome::_1_0_std_std_9274c0d4::basic_monad<boost::outcome::_1_0_std_std_9274c0d4::policy::monad_policy<udt>>::is_constructible': 
use of a variable template requires template argument list
d:\tmp2\boost-master\libs\outcome\include\boost\outcome\outcome_v1.0.hpp(4271):
note: see declaration of 
'boost::outcome::_1_0_std_std_9274c0d4::basic_monad<boost::outcome::_1_0_std_std_9274c0d4::policy::monad_policy<udt>>::is_constructible'
unittests.cpp(780): fatal error C1903: unable to recover from previous 
error(s); stopping compilation

I get the same failure with /std:c++latest.

Testing with the msvc-14.0 toolset works.

The library as submitted contains both a preprocessed, fully self-contained, 
single header named outcome_v1_0.hpp, which is used by default, and a 
traditional non-preprocessed form that can be chosen with a macro. For the 
non-preprocessed form to be functional, a git submodule is incorporated by 
reference:

[submodule "include/boost/outcome/boost-lite"]
        path = include/boost/outcome/boost-lite
        url = https://github.com/ned14/boost-lite.git
        branch = master
        fetchRecurseSubmodules = true
        ignore = untracked

which in turn incorporates two more submodules:

[submodule "include/gsl-lite"]
        path = include/gsl-lite
        url = https://github.com/martinmoene/gsl-lite.git
        branch = master
        fetchRecurseSubmodules = true
        ignore = untracked
[submodule "pcpp"]
        path = pcpp
        url = https://github.com/ned14/pcpp.git
        branch = master
        fetchRecurseSubmodules = true
        ignore = untracked

The first of these is not by the library author, and the second is a derived 
work.

It is not possible for a Boost release, say, boost_1_65_0.zip, to 
incorporate git submodules by reference, so there remains a question of 
what, exactly, is being submitted for inclusion. My attempts to clarify this 
with the author have proven unsuccessful.

I consider this a critical defect of the submission. It is simply not 
possible for a reviewer to give an opinion whether the submission ought to 
be included into Boost releases without it being known what, specifically, 
is being proposed to be included.

There is no problem with the library containing implementation details under 
its directory, but we need to know what these details are.

What the library actually needs as dependencies from boost-lite is not much 
and it's perfectly possible to extract the necessary parts and prepare a 
self-contained submission. This should have been done prior to the review.

The files in the library contain the following:

```
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License in the accompanying file
Licence.txt or at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

Distributed under the Boost Software License, Version 1.0.
    (See accompanying file Licence.txt or copy at
          http://www.boost.org/LICENSE_1_0.txt)
```

This intends to dual license the library, but as written, it doesn't. What 
it does is apply the terms of BOTH licenses to distribution and use. This is 
also a critical defect. If the library is included in a Boost release, it 
should make it perfectly clear that it's licensed under the Boost license, 
and not under the intersection of Boost and Apache 2.0.

There are several ways to do it, one is to clarify the above to specifically 
say "ASL2 or BSL, at your option" in the appropriate legalese, another is to 
license the Boost-included Outcome under BSL and the standalone Outcome 
under whatever. My recommendation would be to just license the lot under BSL 
and be done with it. Dual licensing creates unnecessary legal complications 
and may, from a legal point of view, be more burdensome than just using the 
BSL. Contributors need to be aware that they are licensing their patches 
under both licenses, it might not be very clear whether they have been 
aware, and so on.

At times, the library duplicates certain functionality present elsewhere in 
Boost. I do not consider this a defect. It's a legitimate design decision to 
avoid intra-Boost dependencies, although the author needs to be aware of the 
downsides - using Boost components is basically taking advantage of free 
work instead of having to maintain everything oneself.

III. DOCUMENTATION
------------------

I found the documentation perfectly adequate and was able to get a very good 
outline of how the classes were designed, how they were intended to be used, 
and what problem they intend to solve. It focuses a bit too much on 
expected<T, E>, which is not in itself a bad thing but is, for me, not 
exactly what the library is about. Fixing E to std::error_code is a 
fundamental (and correct) design decision from which these classes derive 
much of their utility.

The Doxygen-generated reference will not win any awards, but it does the 
job. It however contains too many implementation details that would ideally 
either not be there at all, or tucked away in their own separate section. 
The monadic content is also a bit too high for my taste, and I would prefer 
if a user of result<> and outcome<> does not encounter the word monad at 
all.

IV. VERDICT
-----------

Should Outcome be accepted?

In short, eventually. This, at present, is a NO.

Ideally, what I would like to see is the library resubmitted in a 
conventional form, consisting of exactly the files that are intended to go 
into the libs/outcome directory in boost_1_65_0.zip, licensed under the BSL, 
only containing the files necessary for the non-preprocessed version of the 
code to compile and work. In addition, the test/ subdirectory would contain 
a Jamfile that is ready to integrate into the Boost test matrix.