Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

try_operation_return_as() customisation point is not ADL discovered #189

Closed
nikobarli opened this issue May 15, 2019 · 28 comments

Comments

@nikobarli
Copy link

commented May 15, 2019

Suppose I am using third-party library which exposes result<T,E>from my own library which exposes result<T,F>.

outcome::result<T,E> lib_func() {
  ...
}

outcome::result <T,F> my_func() {
  TRY(v, lib_func());
  ...
}

How can I provide a conversion from E to F when using TRY (I cannot add conversion operator into E, because it is a third-party component) ?

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

The following part from the tutorial describes the case that is similar to the problem you describe: https://ned14.github.io/outcome/tutorial/advanced/interop/app-go/

The library does not offer an "implicit solution" (that would be invisible in your code), so the applied solution for your case would look something like:

outcome::result <T,F> my_func() {
  TRY(v, adapt(lib_func()));
  ...
}

Which, to me, is equivalent to saying, "you have to do the conversion manually".

@nikobarli

This comment has been minimized.

Copy link
Author

commented May 15, 2019

Ah, I see.

I missed that Advanced Tutorial part. Now I can make my test program compiles and runs.

Which, to me, is equivalent to saying, "you have to do the conversion manually".

There must be a strong reason for that, which is beyond my comprehension at the moment. Will keep learning it though. Anyway, thanks a lot for the help !

@nikobarli nikobarli closed this May 15, 2019

@ned14

This comment has been minimized.

Copy link
Owner

commented May 15, 2019

There is a specific customisation point for the TRY operation: https://ned14.github.io/outcome/reference/converters/try_operation_return_as/

It should allow any ValueOrError concept matching input to be implicitly mapped into TRY, no manual conversion needed.

@nikobarli

This comment has been minimized.

Copy link
Author

commented May 15, 2019

I see. So I can also do something like:

template<typename T>
inline decltype(auto) try_operation_return_as(result<T, E> && v)
{
    // convert from E to F
    return F(convert_from(v.error()));
}
@ned14

This comment has been minimized.

Copy link
Owner

commented May 15, 2019

The function is ADL discovered, so you need to inject it into the same namespace as your source result type implementation.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

Wow. And will it work without an explicit conversion inside TRY?

@ned14

This comment has been minimized.

Copy link
Owner

commented May 15, 2019

Yes. I reworked it before the Boost release precisely due to feedback from your good self Andrzej! Sorry, I should have told you.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

I cannot get it to work on Boost 1.70: https://wandbox.org/permlink/H33vf6Yb3Nt36Ro1
Am I missing something?

@ned14 ned14 changed the title Providing error type conversion for third-party library try_operation_return_as() customisation point is not ADL discovered May 15, 2019

@ned14

This comment has been minimized.

Copy link
Owner

commented May 15, 2019

It looks like the customisation point is not ADL discovered, as documented. Here is a working example: https://wandbox.org/permlink/vr5hy4obBL11HWgR

I have reopened this as this is now a bug, though whether we make the function ADL discovered, or retain the outcome namespace injection is an excellent question for debate.

@ned14 ned14 reopened this May 15, 2019

@ned14 ned14 added the bug label May 15, 2019

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

My guess is, the function is not ADL discovered because the argument passed to the function (of type boost::outcome::result<int, Lib::Err> ) is not from namespace Lib but instead it is from namespace boost::outcome.

namespace Lib 
{
    template<typename T>
    inline decltype(auto) try_operation_return_as(out::result<T, Err> && v);
}

When I try too define the overload in namespace BOOST_OUTCOME_V2_NAMESPACE I gat an error about missing overload for make_exception_ptr: https://wandbox.org/permlink/iui8s5nyt2KwYuG0

I am not sure why make_exception_ptr would be needed here.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

Another thing that I am confused about here is that if this specialization is needed to allow the conversion from one error type E to another error type F, its declaration should mention both E and F. Otherwise, how would I be able to provide two conversions: one from E to F1 and the other from E to F2?

@ned14

This comment has been minimized.

Copy link
Owner

commented May 15, 2019

It is not ADL discovered because the TRY macros fully qualify the namespace lookup of try_operation_return_as. That was either intentional of me, or a mistake. I cannot remember.

Another thing that I am confused about here is that if this specialization is needed to allow the conversion from one error type E to another error type F, its declaration should mention both E and F. Otherwise, how would I be able to provide two conversions: one from E to F1 and the other from E to F2?

TRY has no idea about destination, so it cannot tell you what return type is expected. Thus the present design.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

Ok, I managed to fix my example. I needed to use all_narrow policy: otherwise I am required to provide a conversion from error_type to exception_ptr. I also have to provide the overload for
try_operation_return_as() in boost namespace:

https://wandbox.org/permlink/PACUqWH054R8xBdO

Regarding the ADL thing, could you remind me why try_operation_return_as() customization point takes the entire result<T, E> rather than just E?

@ned14

This comment has been minimized.

Copy link
Owner

commented May 16, 2019

The try_operation_return_as() customisation point was intended for foreign result or expected types. It can be used for outcome results too, but there are almost always better ways.

The reason why try_operation_return_as() takes the entire of the input type is because it knows nothing about the foreign input type. So, in order to specialise the customisation point, it needs to accept the whole of the foreign type.

I am also minded to add a mechanism for interrogating the foreign type for its .has_value(), and then truly foreign types would gain TRY support.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Ok, this makes sense. In that case we can conclude that try_operation_return_as() is in fact discovered via ADL. It is just that ADL looks up the namespace of the entire argument rather than its parts. Given that the argument type is

outcome::result<T, MyLib::Err>

It is natural to expect that the namespace to put the overload in is outcome rather than MyLib.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

If I needed to specialize try_operation_return_as() for type pdimov::expected<T, MyLib::Err>, I would have to put it in namespace pdimov.

But there is a certain limitation with this solution. If I ever need to specialize try_operation_return_as() for std::expected<T, MyLib::Err>, I would need to add a declaration to namespace std and that would be undefined behavior.

A better alternative would be to either:

  1. Allow customization through specializing a class template.
  2. Introduce a second artificial argument to function try_operation_return_as() so that ADL looks up the namespace of the second argument also. You will always be passing outcome_try_tag as the second argument and therewith you will guarantee that ADL always looks in namespace outcome.

These alternatives are described in more detail in this post: https://akrzemi1.wordpress.com/2016/01/16/a-customizable-framework/

@ned14

This comment has been minimized.

Copy link
Owner

commented May 16, 2019

By ADL discovered, we mean "from the namespaces of the input types", which include any template types.

I haven't given it much thought yet on whether to remove the fully qualified outcome namespace lookup. On the one hand, it avoids people having to inject things into the outcome namespace. On the other hand, they probably still need to inject things somewhere, ADL is brittle and prone to collision, and of course it's also much slower on the build times.

I think that I need to decide by end of June, as that's the Boost 1.71 cutoff.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Indeed, you are right. I need to learn C++ better. The overload in namespace MyLib should work.

However, my other remark is still valid: if you need to overload for std::expected<int, std::error_code> you can only do it in std, and that is illegal.

@ned14

This comment has been minimized.

Copy link
Owner

commented May 16, 2019

std::expected already has a specialisation in try.hpp :)

But it's a good point about injecting non-specialisations into std.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

/opt/wandbox/boost-1.70.0/clang-head/include/boost/outcome/try.hpp:103:10: note: expanded from macro 'BOOST_OUTCOME_TRYV2'
  return BOOST_OUTCOME_V2_NAMESPACE::try_operation_return_as(static_cast<decltype(unique) &&>(unique))
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Function try_operation_return_as() is called namespace-qualified with BOOST_OUTCOME_V2_NAMESPACE. This disables ADL.

@nikobarli

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Thanks for all the discussions. I've learned many things just by reading this.

@ned14 ned14 added this to the Boost 1.71 cutoff milestone Jun 17, 2019

ned14 added a commit that referenced this issue Jun 20, 2019
Issue #189 refactored the `OUTCOME_TRY` implementation to use more cl…
…arified

customisation points capable of accepting very foreign inputs. Removed the
`std::experimental::expected<T, E>` specialisations, as those are no longer
necessary. Fixed the documentation for the customisation points which
previously claimed that they are ADL discovered, which they are not. Added
a recipe describing how to add in support for foreign input types.
ned14 added a commit that referenced this issue Jun 20, 2019
@ned14

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

Fixed!

@ned14 ned14 closed this Jun 21, 2019

@ned14

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

FYI guys there is a new recipe dedicated to this exact topic: https://ned14.github.io/outcome/recipes/foreign-try/

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Thanks.
Let me report certain concerns.

The docs say, I have to add specializations of the "try helper functions", but the example shows function overloads rather than specializations. This has some consequences. If they are overloads, This customization does not work in certain places. For instance, if I change a demo example a bit, so that the functions old_code and new_code are templates independent of any outcome or expected library, the example fails to compile:
https://wandbox.org/permlink/YDmdbgwC962E8w35

I moved the definitions of these template functions above the customizations of "try helper functions", but there is nothing unusual about it: now that they are templates independent of any error-handling type, enforcing the order is not justified: templated_new_code and templated_old_code could have been included through another header and we do not want the order of unrelated headers to affect the result of the compilation.

There would be no compilation error if either:

  1. Outcome was using template specializations for customizations (rather than function overloading, or
  2. Outcome was doing an uqualified call inside macro OUTCOME_TRY in order to enable ADL.

However, you cannot do partial function template specializations, so for #1 you would have to do class template specializations.

@ned14

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

now that they are templates independent of any error-handling type, enforcing the order is not justified: templated_new_code and templated_old_code could have been included through another header and we do not want the order of unrelated headers to affect the result of the compilation.

This isn't Outcome doing this though. This is two phase lookup. Which is in the standard precisely so that binding happens early according to the overload set available at the point of template parse, rather than later according to the overload set available at the point of template instantiation.

And that's a good thing. It reduces build times significantly. Which is why I deliberately chose early binding, incidentally, because code does lots and lots of OUTCOME_TRY, and causing an overload set lookup and resolution every single time you use it would horribly impact build times.

If you look at all the ADL customisation points in Outcome, they are either (i) per basic_result<> specialisation or (ii) attached to specific NoValuePolicy implementations, which are also per basic_result<> specialisation. The idea is that every time you vary the template argument set of basic_result<>, you pay a large price in build time impact, but equally that probably only happens at most less than a dozen times per translation unit. And, moreover, because it's shared across all TUs and subject to ODR, Modules should take away much of the impact one day.

But contrast that to a source file, filled with OUTCOME_TRY based logic, where hundreds of TRY operations are totally possible, and still many more again if you place that logic inside a templated function. You would be doing ADL discovery and overload set resolution for every TRY use. Do you see why I chose the tradeoff I did?

Outcome was using template specializations for customizations (rather than function overloading

The problem with template specialisations is that they are very specific. Probably, for TRY operations, you want to preserve the ordinary conversion operations and matching which C++ overload resolution provides. Don't get me wrong, function overload resolution is very expensive compared to template specialisation lookup, but given the early bind which forces a once-off cost, I reckoned it a better tradeoff in terms of usability.

I should stress that I am not a compiler writer, and my understanding of build time scalability might be very misplaced. So everything I just said might be wrong. But I think that I am right?

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

So, we could summarize this as follows.
There are four options to consider:

  1. Early binding (overloads in outcome::v2 namespace).
  2. Early binding + function template specializations.
  3. Early binding + class template specializations.
  4. Late binding ADL.

With the current bugfix, the prose in the docs recommend #2, whereas the examples use #1.

I never payed much attention to what is faster and by what factor, so I will accept your input: #4 would be too slow. So would be #2 and #3.

Additionally, for #2 you cannot provide partial specializations.

#1 on the other hand has the potential to surprise the users when OUTCOME_TRY is used inside templates: they will provide overloads of outcome::v2::try_operation_return_as, but will not work.

You choose #1.

If the above summary is correct, I would recommend changing the docs a bit, so that:

  • the prose says that we are adding function overloads rather than specializations.
  • add a rationale for this trade off (which I, for one, consider unusual).
  • add a warning saying that OUTCOME_TRY makes a qualified call to outcome::v2::try_operation_return_as, which will result in skipping the user-provided overloads in certain use cases.

Would you be ok with this recommendation?

ned14 added a commit that referenced this issue Jun 21, 2019
@ned14

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

Ok, try these docs improvements based on your feedback.

@akrzemi1

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

It's good. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.