ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
704 stars 62 forks source link

constexpr basic_result? #255

Closed burnpanck closed 2 years ago

burnpanck commented 3 years ago

The following code fails with the current master:

constexpr outcome::experimental::status_result<int> test() {
    return outcome::success(42);
}

with an error along the lines of

<source>: In function 'constexpr outcome_v2_35644f5c::experimental::status_result<int> test()':
<source>:6:53: error: invalid return type 'outcome_v2_35644f5c::experimental::status_result<int>' {aka 'outcome_v2_35644f5c::basic_result<int, system_error2::errored_status_code<system_error2::erased<long int> >, outcome_v2_35644f5c::experimental::policy::status_code_throw<int, system_error2::errored_status_code<system_error2::erased<long int> >, void> >'} of 'constexpr' function 'constexpr outcome_v2_35644f5c::experimental::status_result<int> test()'
    6 | constexpr outcome::experimental::status_result<int> test() {
      |                                                     ^~~~
In file included from <source>:2:
outcome-experimental.hpp:4145:25: note: 'outcome_v2_35644f5c::basic_result<int, system_error2::errored_status_code<...>' is not literal because:
 4145 | class OUTCOME_NODISCARD basic_result : public detail::basic_result_final<R, S, NoValuePolicy>
      |                         ^~~~~~~~~~~~
outcome-experimental.hpp:4145:25: note:   'outcome_v2_35644f5c::basic_result<int, system_error2::errored_status_code<...>' has a non-trivial destructor
Compiler returned: 1

(check godbolt).

I believe that did work before - tough I'm not 100 % sure. Any thoughts?

burnpanck commented 3 years ago

It appears that this did still work with 2.1.5, but not with 2.2.0.

ned14 commented 3 years ago

I have to say, I would be surprised if this worked in 2.1.5, or at least if it was supposed to. In 2.1.5, the default error type is system_code, which is a move only type with a non-trivial destructor. Therefore, any basic_result with a non-trivially destructible error type would itself be non-trivially destructible, and thus not possible to be a literal type.

I suppose what could have happened here is that you were compiling with C++ 20, whereby if a non-trivial destructor is constexpr, now that works in constexpr?

burnpanck commented 3 years ago

Indeed, the original code that I discovered this is C++20, and this is what I care about. However, I have to admit that I don't fully understand the rules that make up a literal type, and how that interacts with returning from constexpr functions.

In my code-base, the error type is outcome::errored_status_code over a custom domain that is derived from outcome::status_code_domain, whose value type is a custom enum. If I recall correctly, the system_code implicitly used in my MVE is an erased int-sized status - does that make system_code non-trivially destructible, while mine is?

In any case, with 2.1.5 I am able to return status_result<std::size_t, errored_status_code<my_domain>> from constexpr functions, but with 2.2.1, I get that above error. Is there a simple error code in outcome which is a literal type and thus would allow for a quick test?

ned14 commented 3 years ago

It was pure luck that system_code happened to work in constexpr if in C++ 20 in your use case. I certainly never intended it to do so, I was writing for C++ 11 (for status code) and C++ 14 (for Outcome) and I was always thinking that the non-trivial destructor made system_code unavailable to any constexpr code. This is the case for all erased status codes, because they may be carrying an erased move only type. So they have to have move only semantics, and therefore non-trivial destructors.

(The pertinent rule for literal types in this situation is that before C++ 20, the type had to have a trivial destructor, but from C++ 20 onwards, the destructor merely needs to be constexpr. Because Outcome v2.1 just happened to default destructors, by happy accident your use case worked)

In any case, with 2.1.5 I am able to return status_result<std::size_t, errored_status_code> from constexpr functions, but with 2.2.1, I get that above error. Is there a simple error code in outcome which is a literal type and thus would allow for a quick test?

Most if not all of the non-erased status codes are literal types, if the payload is a literal type, so will be its status code.

I am minded however to try to fix this for you because status code is before LEWG for standardisation, and LEWG I expect would prefer to see all of status code available to constexpr. I honestly don't know if it's possible however, but I intend to try.

burnpanck commented 3 years ago

Thanks for looking into this!

I think it makes perfectly sense for non-trivial status types not to be available for constexpr use. My own experience with constexpr and type-erasure is that they don't go well together: The reinterpret_cast you'll likely need somewhere is a hard no. Maybe the new std::bit_cast might help here, but my compiler doesn't support it yet, so I have little experience.

burnpanck commented 3 years ago

Here is a more accurate minimal failing example (godbolt):

#include <exception>
#include <outcome-experimental.hpp>

namespace outcome = OUTCOME_V2_NAMESPACE;

enum class Status {
    success,
    failure
};

class _status_domain : public outcome::experimental::status_code_domain {
  // We permit status code to call our protected functions
  template <class DomainType>
  friend class system_error2::status_code;
  using _base = outcome::experimental::status_code_domain;

 public:
  // Our value type is Status
  using value_type = Status;

  using _base::string_ref;

  // Always use https://www.random.org/cgi-bin/randbyte?nbytes=8&format=h to
  // create a unique 64 bit value for every unique domain you create!
  _status_domain() noexcept : _base(0x8f9f78114be043e4) {}

  // Default all the copy, move and destruct. This makes the type 100% constexpr
  // in every way which in turns allows the compiler to assume it will not be
  // instantiated at runtime.
  _status_domain(const _status_domain &) = default;
  _status_domain(_status_domain &&) = default;
  _status_domain &operator=(const _status_domain &) = default;
  _status_domain &operator=(_status_domain &&) = default;
  ~_status_domain() = default;

  // Fetch a constexpr instance of this domain
  static inline constexpr const _status_domain &get();

  // Return the name of this domain
  virtual _base::string_ref name() const noexcept override final {
    return _base::string_ref("_status_domain");
  }
};

using errored_status_code = outcome::experimental::errored_status_code<_status_domain>;

constexpr outcome::experimental::status_result<void, errored_status_code> foo(int) {
    return outcome::success();
}

If I understood correctly, the errored_status_code in this example should be a literal type (Status is), though the derived status_result<void, errored_status_code> is not.

ned14 commented 3 years ago

Sorry to not have gotten onto this yet. Awaiting an unoccupied morning before work slot to turn up.

burnpanck commented 3 years ago

No worries, this is open-source, I wasn't expecting priority support. If it were an urgent need for me, I ought to try to provide a PR myself. Its not urgent though, so I just wanted to make sure that I understand if this is even possible and intended to be supported, or if I better start refactoring my code.

ned14 commented 3 years ago

Try the commit above and let me know how you get on.

ned14 commented 2 years ago

Any news on the above?

burnpanck commented 2 years ago

Hey @ned14, sorry for the delay. Unfortunately, on my internal code-base, that commit with GCC 10.2 still complains with _invalid return type outcome::basic_result<...,system_error2::errored_status_code<my_status_domain>,...> of 'constexpr' function_, where my_status_domain has a simple enum class value_type. I'll try to run my minimal failing example locally later today.

burnpanck commented 2 years ago

Indeed, the above minimal example does compile now. I'll have to dig deeper then to see where the problem in my code is...

ned14 commented 2 years ago

It could be as little as a single missing constexpr anywhere in my code or yours. I find using latest versions of GCC, clang and MSVC can help generate a useful diagnostic. Good luck in finding it!

ned14 commented 2 years ago

I'm going to go ahead and mark this closed. Thanks for the BR!