Skip to content

[libc++] Avoid using **this in error messages for expected monadic operations #84840

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 1, 2024
Merged

[libc++] Avoid using **this in error messages for expected monadic operations #84840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions libcxx/include/__expected/expected.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,9 +918,9 @@ class expected : private __expected_base<_Tp, _Err> {
requires is_constructible_v<_Err, _Err&>
_LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) & {
using _Up = remove_cvref_t<invoke_result_t<_Func, _Tp&>>;
static_assert(__is_std_expected<_Up>::value, "The result of f(**this) must be a specialization of std::expected");
static_assert(__is_std_expected<_Up>::value, "The result of f(value()) must be a specialization of std::expected");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a bit misleading as value() can be ill-formed (because the error type is not copyable) while monadic operations being well-formed. See also LWG3938 and LWG3940.

The resolution of LWG3973 uses val (exposition-only member name). Perhaps we can use val or __val() (libc++-specific function name) in the error messages instead.

Copy link
Contributor

@yronglin yronglin Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. Since https://reviews.llvm.org/D154116 we have addressed LWG3973's std::expected part(It seems like we never use **this). This PR improves static_assert message. __val seems more closely to the LWG wording. Using __val() may requires the user to understand implementation details(e.g. __expected_base). @philnik777 Do you have any suggestions for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just say something like "The result type of the functor must be a specialization of std::expected"? Most people will probably just have a lambda for it anyways. @cjdb might also have thoughts on how to word this.

static_assert(is_same_v<typename _Up::error_type, _Err>,
"The result of f(**this) must have the same error_type as this expected");
"The result of f(value()) must have the same error_type as this expected");
if (has_value()) {
return std::invoke(std::forward<_Func>(__f), this->__val());
}
Expand All @@ -931,9 +931,9 @@ class expected : private __expected_base<_Tp, _Err> {
requires is_constructible_v<_Err, const _Err&>
_LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) const& {
using _Up = remove_cvref_t<invoke_result_t<_Func, const _Tp&>>;
static_assert(__is_std_expected<_Up>::value, "The result of f(**this) must be a specialization of std::expected");
static_assert(__is_std_expected<_Up>::value, "The result of f(value()) must be a specialization of std::expected");
static_assert(is_same_v<typename _Up::error_type, _Err>,
"The result of f(**this) must have the same error_type as this expected");
"The result of f(value()) must have the same error_type as this expected");
if (has_value()) {
return std::invoke(std::forward<_Func>(__f), this->__val());
}
Expand All @@ -945,9 +945,9 @@ class expected : private __expected_base<_Tp, _Err> {
_LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) && {
using _Up = remove_cvref_t<invoke_result_t<_Func, _Tp&&>>;
static_assert(
__is_std_expected<_Up>::value, "The result of f(std::move(**this)) must be a specialization of std::expected");
__is_std_expected<_Up>::value, "The result of f(std::move(value())) must be a specialization of std::expected");
static_assert(is_same_v<typename _Up::error_type, _Err>,
"The result of f(std::move(**this)) must have the same error_type as this expected");
"The result of f(std::move(value())) must have the same error_type as this expected");
if (has_value()) {
return std::invoke(std::forward<_Func>(__f), std::move(this->__val()));
}
Expand All @@ -959,9 +959,9 @@ class expected : private __expected_base<_Tp, _Err> {
_LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) const&& {
using _Up = remove_cvref_t<invoke_result_t<_Func, const _Tp&&>>;
static_assert(
__is_std_expected<_Up>::value, "The result of f(std::move(**this)) must be a specialization of std::expected");
__is_std_expected<_Up>::value, "The result of f(std::move(value())) must be a specialization of std::expected");
static_assert(is_same_v<typename _Up::error_type, _Err>,
"The result of f(std::move(**this)) must have the same error_type as this expected");
"The result of f(std::move(value())) must have the same error_type as this expected");
if (has_value()) {
return std::invoke(std::forward<_Func>(__f), std::move(this->__val()));
}
Expand Down
24 changes: 12 additions & 12 deletions libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,22 @@
// Test the mandates
// template<class F> constexpr auto and_then(F&& f) &;
// Mandates:
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(**this)>>
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(value())>>
// U is a specialization of std::expected and std::is_same_v<U:error_type, E> is true

// template<class F> constexpr auto and_then(F&& f) const &;
// Mandates:
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(**this)>>
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(value())>>
// U is a specialization of std::expected and std::is_same_v<U:error_type, E> is true

// template<class F> constexpr auto and_then(F&& f) &&;
// Mandates:
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(**this)>>
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(value())>>
// U is a specialization of std::expected and std::is_same_v<U:error_type, E> is true

// template<class F> constexpr auto and_then(F&& f) const &&;
// Mandates:
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(**this)>>
// Let U be std::remove_cvref_t<std::invoke_result<F, decltype(value())>>
// U is a specialization of std::expected and std::is_same_v<U:error_type, E> is true

#include <expected>
Expand All @@ -52,7 +52,7 @@ void test() {
{
std::expected<int, int> f1(1);
f1.and_then(lval_return_not_std_expected); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<int (&)(int &)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(**this) must be a specialization of std::expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(value()) must be a specialization of std::expected}}
// expected-error-re@*:* {{{{.*}}cannot be used prior to '::' because it has no members}}
// expected-error-re@*:* {{no matching constructor for initialization of{{.*}}}}
}
Expand All @@ -61,7 +61,7 @@ void test() {
{
std::expected<int, int> f1(1);
f1.and_then(lval_error_type_not_same_as_int); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<std::expected<int, NotSameAsInt> (&)(int &)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(**this) must have the same error_type as this expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(value()) must have the same error_type as this expected}}
}
}

Expand All @@ -71,7 +71,7 @@ void test() {
{
const std::expected<int, int> f1(1);
f1.and_then(clval_return_not_std_expected); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<int (&)(const int &)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(**this) must be a specialization of std::expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(value()) must be a specialization of std::expected}}
// expected-error-re@*:* {{{{.*}}cannot be used prior to '::' because it has no members}}
// expected-error-re@*:* {{no matching constructor for initialization of{{.*}}}}
}
Expand All @@ -80,7 +80,7 @@ void test() {
{
const std::expected<int, int> f1(1);
f1.and_then(clval_error_type_not_same_as_int); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<std::expected<int, NotSameAsInt> (&)(const int &)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(**this) must have the same error_type as this expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(value()) must have the same error_type as this expected}}

}
}
Expand All @@ -91,7 +91,7 @@ void test() {
{
std::expected<int, int> f1(1);
std::move(f1).and_then(rval_return_not_std_expected); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<int (&)(int &&)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(**this)) must be a specialization of std::expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(value())) must be a specialization of std::expected}}
// expected-error-re@*:* {{{{.*}}cannot be used prior to '::' because it has no members}}
// expected-error-re@*:* {{no matching constructor for initialization of{{.*}}}}
}
Expand All @@ -100,7 +100,7 @@ void test() {
{
std::expected<int, int> f1(1);
std::move(f1).and_then(rval_error_type_not_same_as_int); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<std::expected<int, NotSameAsInt> (&)(int &&)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(**this)) must have the same error_type as this expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(value())) must have the same error_type as this expected}}
}
}

Expand All @@ -110,7 +110,7 @@ void test() {
{
const std::expected<int, int> f1(1);
std::move(f1).and_then(crval_return_not_std_expected); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<int (&)(const int &&)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(**this)) must be a specialization of std::expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(value())) must be a specialization of std::expected}}
// expected-error-re@*:* {{{{.*}}cannot be used prior to '::' because it has no members}}
// expected-error-re@*:* {{no matching constructor for initialization of{{.*}}}}
}
Expand All @@ -119,7 +119,7 @@ void test() {
{
const std::expected<int, int> f1(1);
std::move(f1).and_then(crval_error_type_not_same_as_int); // expected-note{{in instantiation of function template specialization 'std::expected<int, int>::and_then<std::expected<int, NotSameAsInt> (&)(const int &&)>' requested here}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(**this)) must have the same error_type as this expected}}
// expected-error-re@*:* {{static assertion failed {{.*}}The result of f(std::move(value())) must have the same error_type as this expected}}
}
}
}
Expand Down
Loading