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

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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?

Thanks,
Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On Wed, Sep 12, 2018 at 10:07 AM Niall Douglas via Boost <
[hidden email]> wrote:

> What is boost-dev's opinion on this proposed change in semantics?
>

I'll weigh in even though we're not asking me, since I'm more boost-users
than boost-dev,
but from your email alone, I'd choose failure-to-compile anytime. My
understanding of UB
is that it exists to give leeway to compiler writers for performance
reasons, and what you
describe doesn't seem performance related at all, so why choose UB? --DD

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Dear Niall,

could you point to some explanation, as to why hard UB was
preferred over a compile time error in the first place?

Best

Mike



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
AMDG

On 09/12/2018 03:28 AM, Dominique Devienne via Boost wrote:

> On Wed, Sep 12, 2018 at 10:07 AM Niall Douglas via Boost <
> [hidden email]> wrote:
>
>> What is boost-dev's opinion on this proposed change in semantics?
>>
>
> I'll weigh in even though we're not asking me, since I'm more boost-users
> than boost-dev,
> but from your email alone, I'd choose failure-to-compile anytime. My
> understanding of UB
> is that it exists to give leeway to compiler writers for performance
> reasons, and what you
> describe doesn't seem performance related at all, so why choose UB? --DD
>

  What Niall's message fails to make clear is that
the actual choice is between UB and throwing an
exception.  The compiler error is only to require
the user to make an explicit choice between the
two instead of letting the library choose.

In Christ,
Steven Watanabe

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> could you point to some explanation, as to why hard UB was
> preferred over a compile time error in the first place?

It was a long time ago, and my memory may be faulty. But I think that
the general reasoning was that value-from-valueless is a logic error,
that means the program is incorrectly coded, that means hard UB. I
remember ancillary arguments about it being preferable that the UB
sanitiser fire on that logic error, that assert fires if NDEBUG is not
defined, and so on. So the hard UB only occurs in release builds, where
it invariably segfaults rather than being silent.

So, basically boost-dev, at that time, felt that *run time*
value-from-valueless on UDT error types ought to be treated as program
incorrectness.

As I mentioned, most of Outcome's users - and there are a quite a few of
them - seem very keen on this hard UB. However these early adopters are
not the typical Boost user base. And I do buy in to Andrzej's point that
the current default will likely be surprising to the average C++
programmer who does not read the documentation, and to the developer who
changes the error type without changing the customisation points to match.

Also, Outcome underwent two reviews here. They were in depth. At the
time, I don't remember anybody finding issue with UDT error types
defaulting to hard UB. But maybe my memory is wrong.

And of course, boost-dev can always change their mind if a consensus
presents here in favour of that. I just didn't want to change Outcome in
such a non-reviewed way without getting approval here first.

Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Wed, Sep 12, 2018 at 8:09 AM Steven Watanabe via Boost <
[hidden email]> wrote:

>   What Niall's message fails to make clear is that
> the actual choice is between UB and throwing an
> exception.  The compiler error is only to require
> the user to make an explicit choice between the
> two instead of letting the library choose.
>

It is still unclear to me how the user is making the choice. As far as I
see it, except when exceptions are disabled (which is not standard C++),
the point of using Outcome is to allow a function to return a "valueless"
outcome instead of throwng an exception, which can now be postponed until
the value is requested; so, if I want the exception, I can just say
foo().value(). This simplifies APIs which would otherwise have to provide
two variants for a function, one that throws and one that does not.

Can someone clarify?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
śr., 12 wrz 2018 o 23:19 Emil Dotchevski via Boost <[hidden email]>
napisał(a):

> On Wed, Sep 12, 2018 at 8:09 AM Steven Watanabe via Boost <
> [hidden email]> wrote:
>
> >   What Niall's message fails to make clear is that
> > the actual choice is between UB and throwing an
> > exception.  The compiler error is only to require
> > the user to make an explicit choice between the
> > two instead of letting the library choose.
> >
>
> It is still unclear to me how the user is making the choice. As far as I
> see it, except when exceptions are disabled (which is not standard C++),
> the point of using Outcome is to allow a function to return a "valueless"
> outcome instead of throwng an exception, which can now be postponed until
> the value is requested; so, if I want the exception, I can just say
> foo().value(). This simplifies APIs which would otherwise have to provide
> two variants for a function, one that throws and one that does not.
>
> Can someone clarify?
>

You have just described something that one could call "value or throw"
idiom: `foo().value()`. It will only work under certain policies:

template <typename T>
using resultA = result<T, std::error_code,
error_code_throw_as_system_error<T, EC, void>>;
// the above will throw on value from valueless

template <typename T>
using resultB = result<T, std::error_code, all_narrow>;
// the above will be UB on value from valueless

template <typename T>
using resultC = result<T, std::error_code>;
// the above will throw on value from valueless
// (third argument defaults to default_policy<T, std::error_code, void>
which throws for std::error_code)

template <typename T>
using resultC = result<T, MyErrorCode>;
// the above will be *UB* on value from valueless, because default_policy
is UB for unknown (to the framework) types.

The fourth case is described as it works today. The proposal in this thread
is to change the fourth case so that default_policy fails to compile when
used for unknown types.

Does the above answer your question?

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
Andrzej Krzemienski wrote:

> You have just described something that one could call "value or throw"
> idiom: `foo().value()`. It will only work under certain policies: [...]

The basic point here is that "value or throw" or "value, period" are used in
different ways. One is

    auto r = function().value();

and the other is

    auto r = function();

    if( r )
    {
        // use the value of `r`
    }

Since the two forms are distinct, it doesn't really make sense to express
both using the same name because then you can't tell at a glance whether the
code is correct or not.

If I remember correctly, one suggestion during the first review was for
"value, period" semantics to be expressed as `*r`, thereby making both
options available for the user at the same time. I think that Niall was
against that as being too short, so he at some point provided "value,
period" as r.assume_value().

I'm not sure whether the present `outcome` has assume_value though; the
reference lists assume_error and assume_exception, but not assume_value.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
śr., 12 wrz 2018 o 10:07 Niall Douglas via Boost <[hidden email]>
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;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
czw., 13 wrz 2018 o 00:18 Peter Dimov via Boost <[hidden email]>
napisał(a):

> Andrzej Krzemienski wrote:
>
> > You have just described something that one could call "value or throw"
> > idiom: `foo().value()`. It will only work under certain policies: [...]
>
> The basic point here is that "value or throw" or "value, period" are used
> in
> different ways. One is
>
>     auto r = function().value();
>
> and the other is
>
>     auto r = function();
>
>     if( r )
>     {
>         // use the value of `r`
>     }
>
> Since the two forms are distinct, it doesn't really make sense to express
> both using the same name because then you can't tell at a glance whether
> the
> code is correct or not.
>

Yes, they are two distinct cases. And yes, they should probably be
expressed in two different ways.


> If I remember correctly, one suggestion during the first review was for
> "value, period" semantics to be expressed as `*r`, thereby making both
> options available for the user at the same time. I think that Niall was
> against that as being too short, so he at some point provided "value,
> period" as r.assume_value().
>
> I'm not sure whether the present `outcome` has assume_value though; the
> reference lists assume_error and assume_exception, but not assume_value.
>

In the present Outcome you have both `value()` and `assume_value()`. In the
default policy, with default `EC` the former throws and the latter is UB,
but this is in general subject to policy, and if you are nasty, you can
actually configure them the other way around, I think.

Regards,
&rzej;

>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On Wed, Sep 12, 2018 at 3:25 PM Andrzej Krzemienski via Boost <
[hidden email]> wrote:

> czw., 13 wrz 2018 o 00:18 Peter Dimov via Boost <[hidden email]>
> napisał(a):
>
> > Andrzej Krzemienski wrote:
> >
> > > You have just described something that one could call "value or throw"
> > > idiom: `foo().value()`. It will only work under certain policies: [...]
> >
> > The basic point here is that "value or throw" or "value, period" are used
> > in
> > different ways. One is
> >
> >     auto r = function().value();
> >
> > and the other is
> >
> >     auto r = function();
> >
> >     if( r )
> >     {
> >         // use the value of `r`
> >     }
> >
> > Since the two forms are distinct, it doesn't really make sense to express
> > both using the same name because then you can't tell at a glance whether
> > the
> > code is correct or not.
> >
>
> Yes, they are two distinct cases. And yes, they should probably be
> expressed in two different ways.
>

My understading was that initally Outcome attempted, in part, to be an easy
solution to this problem (building interfaces which return value-or-throw
results). It appears that this functionality is now deeply burried in
policies and other complexity (assuming it still exists, which is still
unclear to me). I assume this is driven by pressure from users who don't
want to use exceptions at all.

If someone is building an API that simply needs to return value-or-throw
results, would Outcome be the recommended provider of this functionality?
Is there even a single example of such a library that uses Outcome?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 13/09/2018 09:48, Andrzej Krzemienski wrote:
> template <typename T>
> using resultC = result<T, MyErrorCode>;
> // the above will be *UB* on value from valueless, because default_policy
> is UB for unknown (to the framework) types.
>
> The fourth case is described as it works today. The proposal in this thread
> is to change the fourth case so that default_policy fails to compile when
> used for unknown types.

That seems sensible to me.

In my view it is always superior to choose fail-to-compile over UB
whenever possible, at least for obviously silly code.  And "quietly"
switching from throw semantics to UB is probably not a good idea, as you
pointed out.


*Having said that*, if I'm understanding correctly, this sounds like it
would mean that you won't be able to use a custom error_type with
Outcome unless you either:

   1. Declare some "this is how you throw an exception for this
error_type" method (presumably a template specialisation?)

   2. Only ever use it with some non-default policy (that either never
throws or has user-implemented code to throw).

Is that correct?  (Or would #1 always be required regardless of usage?)

If so, while I'm not sure whether that's enough to change my mind, it
does make me a little hesitant about adding another hurdle.


On the *other* other hand, it wouldn't be a silly idea to have a generic
"throw this error_type" method.  system_error itself was somewhat trying
to be that generic method, but with the caveat of only supporting plain
enum error_types.

Perhaps boost::throw_error, which is expected to internally construct an
appropriate exception based on its argument type and then call
boost::throw_exception?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
Mere moments ago, quoth I:
> Perhaps boost::throw_error, which is expected to internally construct an
> appropriate exception based on its argument type and then call
> boost::throw_exception?

To clarify, I was wondering if we should:

  1. Implement an error-to-exception conversion trait as a generic
template which does not compile by default.
  2. Provide a specialisation for boost::system::error_code which
returns boost::system::system_error.
  3. (Where supported by the STL) Provide a specialisation for
std::error_code which returns std::system_error.
  4. Allow users to provide additional specialisations for other error
types.
  5. Provide generic boost::throw_error which calls
boost::throw_exception on the result of the conversion trait.
  6. Make Outcome's default policy call boost::throw_error instead of
hard UB.

The people who want hard UB can still use a policy that requests it.

Everyone else will get a compile error until they either implement the
conversion trait, use an error_type which is already implemented, or
switch to a UB policy.

Bonus: the conversion trait provides an easy way to ask metaprogramming
questions such as "what is the exception type for this error code" or
"is this a throwable error code type".

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
Gavin Lambert wrote:

> To clarify, I was wondering if we should:
>
>   1. Implement an error-to-exception conversion trait as a generic
> template which does not compile by default.
>   2. Provide a specialisation for boost::system::error_code which returns
> boost::system::system_error.
>   3. (Where supported by the STL) Provide a specialisation for
> std::error_code which returns std::system_error.
>   4. Allow users to provide additional specialisations for other error
> types.
>   5. Provide generic boost::throw_error which calls boost::throw_exception
> on the result of the conversion trait.
>   6. Make Outcome's default policy call boost::throw_error instead of hard
> UB.

Looking at my various expected/result experiments, what I've done there is:

- in expected<>, I call `throw_on_unexpected(e)` unqualified, and if that
returns, I throw bad_expected_access<E>(e):

https://github.com/pdimov/variant2/blob/81023c3569b3edcd9c5d631862905744e20cd9c5/include/boost/variant2/expected.hpp#L141
https://github.com/pdimov/variant2/blob/81023c3569b3edcd9c5d631862905744e20cd9c5/include/boost/variant2/expected.hpp#L93

- in (one) result<>, I call error_code_to_exception(e) unqualified, then
throw the result:

https://github.com/pdimov/result/blob/e7f0b076e267111098e49d8e7f7c15b5dbb7cad8/include/boost/result/result.hpp#L521
https://github.com/pdimov/result/blob/e7f0b076e267111098e49d8e7f7c15b5dbb7cad8/include/boost/result/result.hpp#L90

However, I vaguely remember that I convinced myself at one point that the
latter (calling a function to convert) is inferior to calling a function
that throws. For one thing, you can't convert an exception_ptr to an
exception (*); for another, sometimes you want to throw different exceptions
based on the runtime value of the error code (bad_alloc on ENOMEM for
instance.)

So I would probably go with calling `throw_exception_from_error_code(e)`
unqualified nowadays. (And follow that call with __builtin_unreachable(),
allowing diehard fans of undefined behavior to obtain it by returning.)

(*) This is needed, for instance, when you build an outcome<> on top of a
result<> by

struct my_error_code
{
    std::error_code code_;
    std::exception_ptr exc_;
};

template<class T> using outcome = result<T, my_error_code>;

Now you want something like this:

void throw_exception_on_error_code( my_error_code const & e )
{
    if( e.exc_ )
        std::rethrow_exception( e.exc_ );
    else
        throw std::system_error( e.code_ );
}


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On 13/09/2018 12:54, Peter Dimov wrote:
> However, I vaguely remember that I convinced myself at one point that
> the latter (calling a function to convert) is inferior to calling a
> function that throws. For one thing, you can't convert an exception_ptr
> to an exception (*); for another, sometimes you want to throw different
> exceptions based on the runtime value of the error code (bad_alloc on
> ENOMEM for instance.)

True.  I did start writing the email that way, but along the way thought
that it might be a good idea to require use of boost::throw_exception
via a return value (and thus getting a return-type trait for "free").

But you're right that there are some cases that don't work as well that
way, since value returns aren't polymorphic.

> (*) This is needed, for instance, when you build an outcome<> on top of
> a result<> by
>
> struct my_error_code
> {
>     std::error_code code_;
>     std::exception_ptr exc_;
> };
>
> template<class T> using outcome = result<T, my_error_code>;

Arguably that sort of construction ought to "unpack" the error into the
three-type outcome<T, my_error_code_without_exception, exception_ptr>.
Other than interop with pre-Outcome code I would think that such cases
should be fairly rare.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
czw., 13 wrz 2018 o 01:52 Gavin Lambert via Boost <[hidden email]>
napisał(a):

> On 13/09/2018 09:48, Andrzej Krzemienski wrote:
> > template <typename T>
> > using resultC = result<T, MyErrorCode>;
> > // the above will be *UB* on value from valueless, because default_policy
> > is UB for unknown (to the framework) types.
> >
> > The fourth case is described as it works today. The proposal in this
> thread
> > is to change the fourth case so that default_policy fails to compile when
> > used for unknown types.
>
> That seems sensible to me.
>
> In my view it is always superior to choose fail-to-compile over UB
> whenever possible, at least for obviously silly code.  And "quietly"
> switching from throw semantics to UB is probably not a good idea, as you
> pointed out.
>
>
> *Having said that*, if I'm understanding correctly, this sounds like it
> would mean that you won't be able to use a custom error_type with
> Outcome unless you either:
>
>    1. Declare some "this is how you throw an exception for this
> error_type" method (presumably a template specialisation?)
>
>    2. Only ever use it with some non-default policy (that either never
> throws or has user-implemented code to throw).
>
> Is that correct?  (Or would #1 always be required regardless of usage?)
>
> If so, while I'm not sure whether that's enough to change my mind, it
> does make me a little hesitant about adding another hurdle.
>
>
> On the *other* other hand, it wouldn't be a silly idea to have a generic
> "throw this error_type" method.  system_error itself was somewhat trying
> to be that generic method, but with the caveat of only supporting plain
> enum error_types.
>
> Perhaps boost::throw_error, which is expected to internally construct an
> appropriate exception based on its argument type and then call
> boost::throw_exception?
>

This is actually what Outcome is currently doing (modulo the unfortunate UB
case in the default policy). For the sake of keeping this discussion
focused, I did not describe the the full behavior of the default policy,
which is:

+ Throw system error when `EC` is:
  * error_code
  * an enum "plugged" into the error_code framework
  * a type for which ADL can find function make_error_code() that converts
it to error_code
+ if `EC` is exception_ptr, rethrow the contained exception
- If `EC` is void call std::terminate
- otherwise UB

The last two (marked with `-`) are the ones that I would propose to change.

And Outcome is also using an ADL-based customization point
outcome_throw_as_system_error_with_payload(), which gets an `EC` and throws
the desired exception (quite similar to what Peter suggests).

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
My 2 cents: The unexpected is always bad, so value() should NEVER be UB.

The arguments already came:

>>      auto r = function().value();
>>
>> and the other is
>>
>>      auto r = function();
>>
>>      if( r )
>>      {
>>          // use the value of `r`
>>      }
Together with
> In the present Outcome you have both `value()` and `assume_value()`. In the
> default policy, with default `EC` the former throws and the latter is UB,
> but this is in general subject to policy, and if you are nasty, you can
> actually configure them the other way around, I think.
So you have `value()` for the 1st case which throws on valueless and
`assume_value()` which UBs on valueless.

Given that, the policy should only be there to decide WHAT exception to
throw and fail to compile for user-defined error types. The policy might
also NOT define any exception throwing in which case `value()` should be
unusable (compile error on use). This would make sense if you don't
allow any exceptions in your code and are always using the 2nd use case
instead. Note that the actual "throw" has to be in library code so no
one can accidentally create a policy that does not throw.

In any case: Even keeping the current design there should NOT be an
automatic switch to UB. But as detailed above overloading the
_semantics_ of `value()` is surprising and therefore bad.

Regard, Alex



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
czw., 13 wrz 2018 o 01:26 Emil Dotchevski via Boost <[hidden email]>
napisał(a):

> On Wed, Sep 12, 2018 at 3:25 PM Andrzej Krzemienski via Boost <
> [hidden email]> wrote:
>
> > czw., 13 wrz 2018 o 00:18 Peter Dimov via Boost <[hidden email]>
> > napisał(a):
> >
> > > Andrzej Krzemienski wrote:
> > >
> > > > You have just described something that one could call "value or
> throw"
> > > > idiom: `foo().value()`. It will only work under certain policies:
> [...]
> > >
> > > The basic point here is that "value or throw" or "value, period" are
> used
> > > in
> > > different ways. One is
> > >
> > >     auto r = function().value();
> > >
> > > and the other is
> > >
> > >     auto r = function();
> > >
> > >     if( r )
> > >     {
> > >         // use the value of `r`
> > >     }
> > >
> > > Since the two forms are distinct, it doesn't really make sense to
> express
> > > both using the same name because then you can't tell at a glance
> whether
> > > the
> > > code is correct or not.
> > >
> >
> > Yes, they are two distinct cases. And yes, they should probably be
> > expressed in two different ways.
> >
>
> My understading was that initally Outcome attempted, in part, to be an easy
> solution to this problem (building interfaces which return value-or-throw
> results).


I am not sure if we mean the same thing. Do you mean the "dual" interfaces
like in filesystem, when nearly each function has two overloads? If so yes:
this was one of the goals of Outcome, it still is, and I think this case is
handled successfully. This is of course a personal opinion.


> It appears that this functionality is now deeply burried in
> policies and other complexity (assuming it still exists, which is still
> unclear to me).


I would not agree with this characterization. It is assumed that the most
natural choice for `EC` would be `error_code`. The type the user would use
is `result<T>` (second and third parameter defaulted), and the
`fun().value()` idiom that you described works out of the box.

If the user wants her custom `EC` (other than an enum plugged into
error_code framework), under my proposed change, she will get a compiler
error with instructions how to make her type work. She has to make a choice
what exception to throw:
* provide er own exception
* decide to go with the default exception type that comes with Outcome

The UB policies that were mentioned in this thread, would have to be
requested explicitly by the user.


> I assume this is driven by pressure from users who don't
> want to use exceptions at all.
>

In part, probably so. If I work in a no-exceptions environment, and want to
use `result<>` with my custom error type, and I do not want to think "which
type of exception to throw". UB polict or call-abort policy seems fine.



> If someone is building an API that simply needs to return value-or-throw
> results, would Outcome be the recommended provider of this functionality?
>

That would be my personal preference. As to general recommendations, I do
not feel sufficiently qualified to answer this question. Outcome requires
C++14 features, so it would be a no non-recommendation for libraries that
need to support older standards.


> Is there even a single example of such a library that uses Outcome?
>

This would be best answered by Niall.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> 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.

That is my recollection as well.

> 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.

It wasn't a compromise. Original Outcome v1 was entirely wide contract,
indeed that was part of why it was rejected. People felt it should not
be wide contract by default. So, as per peer review feedback, v2 became
narrow contract by default, with carve outs for specific trait matched
categories of E type, namely `error_code` and `exception_ptr`.

Boost was then happy (ish) with v2 and its narrow-by-default contract,
and it was accepted. That's why I felt it was important that if we're
going to change this, it needs to be brought here first. This decision
was part of why v1 was rejected, and v2 was accepted. It's a non-trivial
change to the outermost presented convenience API, namely `result<T, E>`.

> 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.

I should also add that any serious user of Outcome doesn't use the
default `result<T, E>`. They typedef `basic_result<T, E, Policy>` into a
namespace-local custom `result<T, E>`. The tutorial says to do this, and
the userbase is clearly doing this.

So default `result<T, E>` is really only ever used by those playing with
the library, or for trivial or toy applications. It's the "plaything"
`result<T, E>`.

That's why I'm personally minded to agree with Andrzej's proposal. The
"training wheels toy type" probably ought to hold their hand, not blow
their hand off.

Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> *Having said that*, if I'm understanding correctly, this sounds like it
> would mean that you won't be able to use a custom error_type with
> Outcome unless you either:
>
>   1. Declare some "this is how you throw an exception for this
> error_type" method (presumably a template specialisation?)
>
>   2. Only ever use it with some non-default policy (that either never
> throws or has user-implemented code to throw).
>
> Is that correct?  (Or would #1 always be required regardless of usage?)

Front line `result<T, E>` would become unusable with UDT E types yes. If
people want UDT E types, they would need to use `result<T, E, Policy>`,
`unchecked<T, E>`, or `checked<T, E>`.

So what's being proposed here is that `result<T, E>` becomes strictly
for E types which are convertible to error code or exception ptr only.
Everything else fails to compile with a suggestion "please go use
checked<T, E> or unchecked<T, E> for UDT E types".

> On the *other* other hand, it wouldn't be a silly idea to have a generic
> "throw this error_type" method.  system_error itself was somewhat trying
> to be that generic method, but with the caveat of only supporting plain
> enum error_types.
>
> Perhaps boost::throw_error, which is expected to internally construct
> an appropriate exception based on its argument type and then call
> boost::throw_exception?

Me personally I almost exclusively use Outcome in P1028 SG14
`status_code` mode now. Far superior codegen. SG14 `status_code` permits
arbitrary error coding types, and can throw arbitrary exceptions from
those. Outcome in SG14 `status_code` mode delegates the exception
throwing to `status_code`.

I'm fairly sure a major games studio has rolled out SG14 `status_code`
into its code. They seem very pleased with it. It does indeed produce
very lovely looking assembler.

Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
12