Skip to content

Commit 1a8c80c

Browse files
snarkmasterfacebook-github-bot
authored andcommitted
result<T>: value-or-exception_wrapper type & short-circuiting coro
Summary: ## Pitch for "working programmers" `result<T>` is a more ergonomic, safer `Try<T>` replacement for code that, for reasons of performance or reliability, chooses to explicitly propagate errors via `return`. Consider this "idiomatic `Try` code": ```cpp // Bug farm: what if this accidentally throws? Try<int> mayFail() { ... } Try<float> alsoFallible() { Try<int> tryN = mayFail(); float v = 5.0; if (tryN.hasValue()) { v += *tryN; } else if (tryN.hasError()) { return Try<float>(tryN.error()); } else { // This bug is easy to make, since `Try` default-constructs as empty! LOG(DFATAL) << "BUG: `mayFail()` returned an empty Try"; } return Try<float>(v); } ``` With `result`, both potential bugs are prevented, and the code is *just* business logic: - "What if `mayFail()` accidentally throws?" is a non-issue since `alsoFallible()` is a `result` coroutine, and thus automatically wraps any uncaught exceptions. - "Empty `Try`" bugs go away since `result` is almost-never-empty [*] ```cpp result<int> mayFail() { ... } result<float> alsoFallible() { co_return 5.0 + co_await mayFail(); } ``` [*] Until we have `std::expected` in C++23, `result<T>` is implemented via `folly::Expected<T, exception_wrapper>`, which is merely "almost-never-empty", rather than "never empty". That said, you will be hard-pressed to hit the empty state with well-formed types -- I tried and failed to construct one naturally in `result_test.cpp` (and settled for "synthetic" objects in an empty state). If you **do** manage hit the empty state, the `result` accessors are carefully crafted to degrade gracefully, so the end user really doesn't have to test for it. --- ## Cancellation -- library-author considerations Looking ahead to C++26 and [P2300](https://wg21.link/p2300), note that receivers have 3 completion states: value/error/stopped. In `folly::coro`, the latter 2 are fused -- cancelled tasks finish with exception `OperationCancelled`. On the other hand, [P2300](https://wg21.link/p2300) takes pains to separate them. That thinking derives from [P1677](https://wg21.link/p1677). The central "working programmer" problem that "cancellation is not an exception" tries to address is, in paraphrase: > "Child stopped" should NOT be handled by exception catch-alls, since a higher-level orchestrator decided the work is no longer needed. The right behavior is to free the resources from this tree of execution as quickly as possible. So, by letting "stopped" state bypass regular exception-handling, [P1677](https://wg21.link/p1677) / [P2300](https://wg21.link/p2300) aim to prevent the class of bugs where cancellation gets accidentally caught and "handled". Today's users of `folly` expect only 2 states: value & error (possibly `OperationCancelled`). Since `result` is a new API, this stack takes the opportunity to add some forward-compatibility. First, D72181807 and D72074521 take some steps to discourage the use of raw `OperationCancelled` by end-users -- `result` is one of the suggested solutions. Second, the top-level `result<T>` API is sort-of bi-state -- it exposes only `has_value()` as a primary test. The "error" and "stopped" state are deliberately hidden in a `non_value()` sub-structure. Yes, `result` also has `has_stopped()`, but that's just shorthand for `!has_value() && non_value().has_stopped()`. This two-level design works quite well to reconcile the goals of "good UX today" and "forward-compatibility with value/error/stopped semantics": - Getting values is easy & safe -- `co_await ...` / `co_await coro::co_ready(...)`. These recommended idioms auto-propagate unhandled errors & cancellation. - Testing specific errors is easy & safe -- `get_exception<Ex>(res)`. - Manually propagating unhandled errors & cancellation is also easy & safe -- `return std::move(res).non_value()`. On the other hand, `result` do **not** provide an easy catch-all -- it would be prone to accidentally catching the cancellation exception! Rather, `result` **deliberately** omits `has_error()` or `error()` -- you have to access `non_value()` to get at those, meaning that you're more likely to correctly test `has_stopped()`. From a user perspective, `result` is `variant<T, variant<stopped, error>>`. However, the implementation is just `Expected<T, exception_wrapper>` -- and it efficiently ingests exceptions from `folly::coro` and `Try`, which don't treat "stopped" separately. Here's how that works: - We use `make_legacy_...` and `get_legacy_...` methods to interact with old-school implementations like `folly::coro` & `Try`. These methods expect an `expection_wrapper` which **may** be an `OperationCancelled`. - In contrast, using normal **end-user** APIs, debug builds will **terminate** if you store `OperationCancelled` in, or retrieve an `error()` from a stopped-state `result` / `non_value_result`. The error says to use `has_stopped()` and `stopped_result` instead. Finally, `result` also prohibits `error()` from being an empty `exception_wrapper` -- that unconditionally terminates if you try to re-throw it! This creates some option value -- once we actively go after `OperationCancelled` deprecation, we can potentially co-opt this empty state as a cheap cancellation signal. Reviewed By: ispeters Differential Revision: D71522263 fbshipit-source-id: 8ae98c979c61e5cd9a26dfc26fbf86dd36471b3f
1 parent 2eed8f8 commit 1a8c80c

File tree

6 files changed

+1921
-2
lines changed

6 files changed

+1921
-2
lines changed

folly/Portability.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,15 @@ constexpr auto kCpplibVer = 0;
696696
#endif
697697
#endif // FOLLY_CFG_NO_COROUTINES
698698

699+
// It'd be possible to relax this, by refactoring `folly/result` code down to
700+
// C++17, and by only blocking the coroutine support for non-coro compiles.
701+
// However, `result<T>` is primarily targeted at newer codebases.
702+
#if FOLLY_CPLUSPLUS >= 202002L && FOLLY_HAS_COROUTINES
703+
#define FOLLY_HAS_RESULT 1
704+
#else
705+
#define FOLLY_HAS_RESULT 0
706+
#endif
707+
699708
// C++20 consteval
700709
#if FOLLY_CPLUSPLUS >= 202002L
701710
#define FOLLY_CONSTEVAL consteval

folly/lang/Exception.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -613,8 +613,9 @@ struct get_exception_tag_t {};
613613
/// This is most efficient when `Ex` matches the exact stored type, or when the
614614
/// type alias `Ex::folly_get_exception_hint_types` has a good hint.
615615
///
616-
/// NB: `Try<T>` currently omits `get_exception(get_exception_tag_t)`, because
617-
/// supporting it might encourage "empty state" bugs:
616+
/// NB: `result<T>` supports `get_exception<Ex>(res)`, but `Try<T>` currently
617+
/// omits `get_exception(get_exception_tag_t)`, because that might encourage
618+
/// "empty state" bugs:
618619
///
619620
/// if (auto* ex = get_exception<MyError>(tryData)) {
620621
/// // handle error

folly/result/gtest_helpers.h

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#pragma once
18+
19+
#include <folly/coro/GtestHelpers.h>
20+
#include <folly/result/result.h>
21+
22+
namespace folly {
23+
24+
#define RESULT_CO_UNWRAP_BODY(body) \
25+
{ \
26+
auto ret = body(); \
27+
if (!ret.has_value()) { \
28+
if (ret.non_value().has_error()) { \
29+
FAIL() << ret.non_value().error(); \
30+
} else { \
31+
FAIL() << "RESULT_CO_TEST got cancellation"; \
32+
} \
33+
} \
34+
}
35+
36+
/*
37+
Analog of GTest `TEST()` macro for writing `result` coroutine tests.
38+
39+
For assertions, use either standard `EXPECT_*` macros, or `CO_ASSERT_*` from
40+
`folly/coro/GtestHelpers.h`.
41+
*/
42+
#define RESULT_CO_TEST(test_case_name, test_name) \
43+
CO_TEST_( \
44+
test_case_name, \
45+
test_name, \
46+
::testing::Test, \
47+
::testing::internal::GetTestTypeId(), \
48+
result<>, \
49+
RESULT_CO_UNWRAP_BODY)
50+
51+
/*
52+
Analog of GTest `TEST_F()` macro for writing `result` coroutine tests.
53+
54+
For assertions, use either standard `EXPECT_*` macros, or `CO_ASSERT_*` from
55+
`folly/coro/GtestHelpers.h`.
56+
*/
57+
#define RESULT_CO_TEST_F(test_fixture, test_name) \
58+
CO_TEST_( \
59+
test_fixture, \
60+
test_name, \
61+
test_fixture, \
62+
::testing::internal::GetTypeId<test_fixture>(), \
63+
result<>, \
64+
RESULT_CO_UNWRAP_BODY)
65+
66+
} // namespace folly

folly/result/result.cpp

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#include <folly/result/result.h>
18+
19+
#include <glog/logging.h>
20+
#include <folly/Indestructible.h>
21+
22+
#if FOLLY_HAS_RESULT
23+
24+
namespace folly::detail {
25+
26+
const non_value_result& dfatal_get_empty_result_error() {
27+
static const folly::Indestructible<non_value_result> r{
28+
make_exception_wrapper<empty_result_error>()};
29+
LOG(DFATAL) << "`folly::result` had an empty underlying `folly::Expected`";
30+
return *r;
31+
}
32+
33+
const non_value_result& dfatal_get_bad_result_access_error() {
34+
static const folly::Indestructible<non_value_result> r{
35+
make_exception_wrapper<bad_result_access_error>()};
36+
LOG(DFATAL) << "Used `error()` accessor for `folly::result` in value state";
37+
return *r;
38+
}
39+
40+
void fatal_if_exception_wrapper_invalid(const exception_wrapper& ew) {
41+
if (!ew.has_exception_ptr()) {
42+
LOG(FATAL) << "`result` may not contain an empty `exception_wrapper`";
43+
}
44+
if (folly::get_exception<folly::OperationCancelled>(ew)) {
45+
LOG(FATAL)
46+
<< "Do not put `OperationCancelled` in `result`, or get it via "
47+
<< "`error()`. Instead, store `stopped_result{}`, and check "
48+
<< "`has_error()` (or `!has_stopped()`) before calling `error()`.";
49+
}
50+
}
51+
52+
} // namespace folly::detail
53+
54+
#endif // FOLLY_HAS_RESULT

0 commit comments

Comments
 (0)