Re: [boost] [outcome] Change semantics on UB from peer review agreed semantics?

Subject: Re: [boost] [outcome] Change semantics on UB from peer review agreed semantics?
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2018-09-12 22:19:25

śr., 12 wrz 2018 o 10:07 Niall Douglas via Boost <boost_at_[hidden]>
napisał(a):

> Dear Boost,
>
> At https://github.com/ned14/outcome/issues/150, Andrzej has raised his
> desire to change Outcome's semantics on UB from what the two peer
> reviews here agreed to previously. As this would be a major change to
> interface different to what the consensus here agreed to, I am asking
> boost-dev to weigh in with its opinion.
>
> Status:
> -------
> Outcome has been finished, apart from docs, since April
> (
> http://boost.2283326.n4.nabble.com/outcome-v2-1-progress-report-td4702900.html
> )
> for the Boost and standalone variants, and since July for the
> experimental P1028 edition. The documentation has not been touched since
> the February review, but will be tackled when my current work contract
> ends this November, and I regain free time.
>
> Preamble:
> ---------
> Outcome has the notion of programmable "default actions" which occur
> when the user does something like call `.value()` on a `result<T, E>`
> which has an error. Let's call this "value-on-valueless".
>
> Outcome, as was approved here, has the following selector on `result<T,
> E>` in the Boost edition:
>
> ```
> template <class T, class EC, class E>
> using default_policy = std::conditional_t<
>   std::is_void<EC>::value && std::is_void<E>::value,
>   terminate,
>
>   std::conditional_t<
>
>   trait::has_error_code_v<EC>, error_code_throw_as_system_error<T, EC,
> E>,
>   std::conditional_t<
>
>   trait::has_exception_ptr_v<EC> || trait::has_exception_ptr_v<E>,
> exception_ptr_rethrow<T, EC, E>,
>   all_narrow
>
> >>>;
>
> template <class R, class S = boost::system::error_code, class
> NoValuePolicy = policy::default_policy<R, S, void>>
> using boost_result = basic_result<R, S, NoValuePolicy>;
>
> template <class R, class S = boost::system::error_code, class
> NoValuePolicy = policy::default_policy<R, S, void>>
> using result = boost_result<R, S, NoValuePolicy>;
> ```
>
> So, if result's `E` is:
>
> - `void`, value-on-valueless calls `std::terminate()`.
> - contains an error code, value-on-valueless throws `system_error()`.
> - otherwise is hard undefined behaviour i.e. we tell the compiler that
> value-on-valueless will never happen, and the compiler doesn't even
> compile code for it to happen.
>
> The documentation, as was approved here, made it very clear that hard UB
> is the default for custom defined `E` types. See
> https://ned14.github.io/outcome/tutorial/default-actions/happens1/. A
> nice segfault usually emerges.
>
> For the standalone edition, judging from user feedback and bug reports,
> this hard UB behaviour is the most common intentional configuration for
> Outcome users. I have not received any complaints about this choice, if
> anything a majority has been choosing Outcome precisely *because* of
> this choice.
>
>
> Andrzej's request
> -----------------
> Andrzej points out in https://github.com/ned14/outcome/issues/150 that
> it's too easy to get surprised by a small source change which causes the
> error code trait to no longer match the `E` type, thus flipping the code
> base from throwing `system_error` into hard UB unexpectedly and without
> warning. He wishes to change the default to failure to compile i.e. if
> result's `E` is:
>
> - `void`, value-on-valueless calls `std::terminate()`.
> - contains an error code, value-on-valueless throws `system_error()`.
> - otherwise a failure to compile with useful diagnostic.
>
> Users can, of course, manually specify a `NoValuePolicy` of `all_narrow`
> if they wish hard UB on value-on-valueless. It would be a non-trivial
> change from what peer review (twice) agreed, as the default would have
> substantially changed.
>
> What is boost-dev's opinion on this proposed change in semantics?
>

Let me add some perspective and clarification to what Niall said.

There is first the important issue to be clear about. Should "value on
valueless" be considered a programmer error, or a correct action? My
*impression* of the first review was that most of the people were of
opinion that this is a programmer bug and the controversy was about whether
it should cause a language-level UB or be nonetheless be reported via an
exception. But this may be just my impression.

Under this impression, it was my understanding that in order to propose a
compromise solution for the impasse Niall added the policies that determine
if "value on valueless" is an exception or an UB. But since the throwing
policies now guarantee to throw, no-one can tell to the programmers "you
are using value on valueless incorrectly", because for throwing policies
the result is well defined.

After some experience with using the Outcome library it became clear to me
that throwing on value from valueless is actually a very useful feature,
which enables the switch from Outcome-like error handling to normal
exception handling. In a more abstract level this means that that function
converting a string to an int does not make a call whether obtaining string
"A" is an error or not: it pushes this decision on the caller. If the
caller wants to consider this an error, she types `convert(s).value()`. and
the error is signaled via an exception.

The only thing that is disrupting this view is that the default policy in
`result<T, EC>` is maybe-throwing maybe-UB depending on the type of EC.

I suppose the motivation behind the semantics of the default
value-on-valueless policy was that for the semi-experienced users, who know
that they can also control the `EC` type we do not want to expose the
knowledge the third parameter (the policy) yet, so we choose the default
action. But as it is easy to figure out what type of exception to throw for
`std::error_code` (it is std::system_error), you cannot guess it for
`MyErrorType`, so the default in that case was not to throw but cause the
UB. But I think failure to compile is even better: we are clear to the user
that no good default can be chosen.

I am actually proposing something slightly different to what Niall
described. if `EC` is `void` I would also go for fail-to-compile.

Regards,
&rzej;