Fix unintended shallow copying when used with std::views::take (issue 261) #263

mikucionisaau · 2024-05-17T15:20:46Z

Enables view concept on generator so that lvalue references cannot be accepted by std::views:take, which in turn would lead to erroneous behavior described in issue 261.

include/coro/generator.hpp

test/test_generator.cpp

mikucionisaau · 2024-05-21T08:46:06Z

The following patch adds a compiler error message:

diff --git a/include/coro/generator.hpp b/include/coro/generator.hpp
index a15305a..020bf21 100644
--- a/include/coro/generator.hpp
+++ b/include/coro/generator.hpp
@@ -127,7 +127,7 @@ public:
 
     generator() noexcept : m_coroutine(nullptr) {}
 
-    generator(const generator&) = delete;
+    generator(const generator&) { static_assert(false, "copying is not supported"); }
     generator(generator&& other) noexcept : m_coroutine(other.m_coroutine) { other.m_coroutine = nullptr; }
 
     auto operator=(const generator&) = delete;

which looks like this:

FAILED: test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o 
/usr/bin/g++-14  -I/home/marius/Desktop/CPP/libcoro/test -I/home/marius/Desktop/CPP/libcoro/include -I/home/marius/Desktop/CPP/libcoro/cmake-build-debug-gcc14/include -g -std=gnu++20 -fdiagnostics-color=always -std=c++20 -fcoroutines -fconcepts -fexceptions -Wall -Wextra -pipe -MD -MT test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o -MF test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o.d -o test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o -c /home/marius/Desktop/CPP/libcoro/test/test_generator.cpp
In file included from /home/marius/Desktop/CPP/libcoro/include/coro/coro.hpp:33,
                 from /home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:3:
/home/marius/Desktop/CPP/libcoro/include/coro/generator.hpp: In instantiation of ‘coro::generator<T>::generator(const coro::generator<T>&) [with T = long unsigned int]’:
/usr/include/c++/14/ranges:1409:37:   required from ‘constexpr auto std::ranges::views::_All::operator()(_Range&&) const [with _Range = coro::generator<long unsigned int>&]’
 1409 |             return std::forward<_Range>(__r);
      |                                            ^
/usr/include/c++/14/ranges:1422:33:   required by substitution of ‘template<class _Range>  requires  viewable_range<_Range> using std::ranges::views::all_t = decltype (std::ranges::views::all(declval<_Range>())) [with _Range = coro::generator<long unsigned int>&]’
 1422 |       using all_t = decltype(all(std::declval<_Range>()));
      |                              ~~~^~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/14/ranges:2368:5:   required by substitution of ‘template<class _Range> std::ranges::take_view(_Range&&, range_difference_t<_Range>)-> take_view<views::all_t<_Range> > [with _Range = coro::generator<long unsigned int>&]’
 2368 |     take_view(_Range&&, range_difference_t<_Range>)
      |     ^~~~~~~~~
/usr/include/c++/14/ranges:912:44:   recursively required by substitution of ‘template<class _Range>  requires  __adaptor_invocable<_Adaptor, _Range, const _Arg&> constexpr auto std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>::operator()(_Range&&) const [with _Range = std::ranges::views::_Take]’
  912 |       = requires { std::declval<_Adaptor>()(declval<_Args>()...); };
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/14/ranges:912:44:   required by substitution of ‘template<class _Self, class _Range>  requires (__is_range_adaptor_closure<_Self>) && (__adaptor_invocable<_Self, _Range>) constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, _Self&&) [with _Self = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Range = coro::generator<long unsigned int>&]’
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:64:49:   required from here
   64 |         for (auto&& n : nat | std::views::take(3)) {
      |                                                 ^
/home/marius/Desktop/CPP/libcoro/include/coro/generator.hpp:130:49: error: static assertion failed: copying is not supported
  130 |     generator(const generator&) { static_assert(false, "copying is not supported"); }
      |                                                 ^~~~~
/home/marius/Desktop/CPP/libcoro/include/coro/generator.hpp:130:49: note: ‘false’ evaluates to false

Which is still ugly, but I think it's better than without:

FAILED: test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o 
/usr/bin/g++-14  -I/home/marius/Desktop/CPP/libcoro/test -I/home/marius/Desktop/CPP/libcoro/include -I/home/marius/Desktop/CPP/libcoro/cmake-build-debug-gcc14/include -g -std=gnu++20 -fdiagnostics-color=always -std=c++20 -fcoroutines -fconcepts -fexceptions -Wall -Wextra -pipe -MD -MT test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o -MF test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o.d -o test/CMakeFiles/libcoro_test.dir/test_generator.cpp.o -c /home/marius/Desktop/CPP/libcoro/test/test_generator.cpp
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp: In function ‘void CATCH2_INTERNAL_TEST_4()’:
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:64:29: error: no match for ‘operator|’ (operand types are ‘coro::generator<long unsigned int>’ and ‘std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>’)
   64 |         for (auto&& n : nat | std::views::take(3)) {
      |                         ~~~ ^ ~~~~~~~~~~~~~~~~~~~
      |                         |                     |
      |                         |                     std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>
      |                         coro::generator<long unsigned int>
In file included from /home/marius/Desktop/CPP/libcoro/include/coro/concepts/range_of.hpp:4,
                 from /home/marius/Desktop/CPP/libcoro/include/coro/coro.hpp:7,
                 from /home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:3:
/usr/include/c++/14/ranges:952:5: note: candidate: ‘template<class _Self, class _Range>  requires (__is_range_adaptor_closure<_Self>) && (__adaptor_invocable<_Self, _Range>) constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, _Self&&)’
  952 |     operator|(_Range&& __r, _Self&& __self)
      |     ^~~~~~~~
/usr/include/c++/14/ranges:952:5: note:   template argument deduction/substitution failed:
/usr/include/c++/14/ranges:952:5: note: constraints not satisfied
/usr/include/c++/14/ranges: In substitution of ‘template<class _Self, class _Range>  requires (__is_range_adaptor_closure<_Self>) && (__adaptor_invocable<_Self, _Range>) constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, _Self&&) [with _Self = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Range = coro::generator<long unsigned int>&]’:
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:64:49:   required from here
/usr/include/c++/14/ranges:952:5: note:    64 |         for (auto&& n : nat | std::views::take(3)) {
/usr/include/c++/14/ranges:952:5: note:       |                                                 ^
/usr/include/c++/14/ranges:911:13:   required for the satisfaction of ‘__adaptor_invocable<_Self, _Range>’ [with _Self = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Range = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:912:9:   in requirements  [with _Adaptor = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Args = {coro::generator<long unsigned int>&}]
/usr/include/c++/14/ranges:912:44: note: the required expression ‘declval<_Adaptor>()((declval<_Args>)()...)’ is invalid
  912 |       = requires { std::declval<_Adaptor>()(declval<_Args>()...); };
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
cc1plus: note: set ‘-fconcepts-diagnostics-depth=’ to at least 2 for more detail
/usr/include/c++/14/ranges:961:5: note: candidate: ‘template<class _Lhs, class _Rhs>  requires (__is_range_adaptor_closure<_Lhs>) && (__is_range_adaptor_closure<_Rhs>) constexpr auto std::ranges::views::__adaptor::operator|(_Lhs&&, _Rhs&&)’
  961 |     operator|(_Lhs&& __lhs, _Rhs&& __rhs)
      |     ^~~~~~~~
/usr/include/c++/14/ranges:961:5: note:   template argument deduction/substitution failed:
/usr/include/c++/14/ranges:961:5: note: constraints not satisfied
/usr/include/c++/14/ranges: In substitution of ‘template<class _Lhs, class _Rhs>  requires (__is_range_adaptor_closure<_Lhs>) && (__is_range_adaptor_closure<_Rhs>) constexpr auto std::ranges::views::__adaptor::operator|(_Lhs&&, _Rhs&&) [with _Lhs = coro::generator<long unsigned int>&; _Rhs = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>]’:
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:64:49:   required from here
/usr/include/c++/14/ranges:961:5: note:    64 |         for (auto&& n : nat | std::views::take(3)) {
/usr/include/c++/14/ranges:961:5: note:       |                                                 ^
/usr/include/c++/14/ranges:942:13:   required for the satisfaction of ‘__is_range_adaptor_closure<_Lhs>’ [with _Lhs = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:943:9:   in requirements with ‘_Tp __t’ [with _Tp = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:943:70: note: the required expression ‘std::ranges::views::__adaptor::__is_range_adaptor_closure_fn(__t, __t)’ is invalid
  943 |       = requires (_Tp __t) { __adaptor::__is_range_adaptor_closure_fn(__t, __t); };
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:68:29: error: no match for ‘operator|’ (operand types are ‘coro::generator<long unsigned int>’ and ‘std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>’)
   68 |         for (auto&& n : nat | std::views::take(3)) {
      |                         ~~~ ^ ~~~~~~~~~~~~~~~~~~~
      |                         |                     |
      |                         |                     std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>
      |                         coro::generator<long unsigned int>
/usr/include/c++/14/ranges:952:5: note: candidate: ‘template<class _Self, class _Range>  requires (__is_range_adaptor_closure<_Self>) && (__adaptor_invocable<_Self, _Range>) constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, _Self&&)’
  952 |     operator|(_Range&& __r, _Self&& __self)
      |     ^~~~~~~~
/usr/include/c++/14/ranges:952:5: note:   template argument deduction/substitution failed:
/usr/include/c++/14/ranges:952:5: note: constraints not satisfied
/usr/include/c++/14/ranges: In substitution of ‘template<class _Self, class _Range>  requires (__is_range_adaptor_closure<_Self>) && (__adaptor_invocable<_Self, _Range>) constexpr auto std::ranges::views::__adaptor::operator|(_Range&&, _Self&&) [with _Self = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Range = coro::generator<long unsigned int>&]’:
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:68:49:   required from here
/usr/include/c++/14/ranges:952:5: note:    68 |         for (auto&& n : nat | std::views::take(3)) {
/usr/include/c++/14/ranges:952:5: note:       |                                                 ^
/usr/include/c++/14/ranges:911:13:   required for the satisfaction of ‘__adaptor_invocable<_Self, _Range>’ [with _Self = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Range = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:912:9:   in requirements  [with _Adaptor = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>; _Args = {coro::generator<long unsigned int>&}]
/usr/include/c++/14/ranges:912:44: note: the required expression ‘declval<_Adaptor>()((declval<_Args>)()...)’ is invalid
  912 |       = requires { std::declval<_Adaptor>()(declval<_Args>()...); };
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/14/ranges:961:5: note: candidate: ‘template<class _Lhs, class _Rhs>  requires (__is_range_adaptor_closure<_Lhs>) && (__is_range_adaptor_closure<_Rhs>) constexpr auto std::ranges::views::__adaptor::operator|(_Lhs&&, _Rhs&&)’
  961 |     operator|(_Lhs&& __lhs, _Rhs&& __rhs)
      |     ^~~~~~~~
/usr/include/c++/14/ranges:961:5: note:   template argument deduction/substitution failed:
/usr/include/c++/14/ranges:961:5: note: constraints not satisfied
/usr/include/c++/14/ranges: In substitution of ‘template<class _Lhs, class _Rhs>  requires (__is_range_adaptor_closure<_Lhs>) && (__is_range_adaptor_closure<_Rhs>) constexpr auto std::ranges::views::__adaptor::operator|(_Lhs&&, _Rhs&&) [with _Lhs = coro::generator<long unsigned int>&; _Rhs = std::ranges::views::__adaptor::_Partial<std::ranges::views::_Take, int>]’:
/home/marius/Desktop/CPP/libcoro/test/test_generator.cpp:68:49:   required from here
/usr/include/c++/14/ranges:961:5: note:    68 |         for (auto&& n : nat | std::views::take(3)) {
/usr/include/c++/14/ranges:961:5: note:       |                                                 ^
/usr/include/c++/14/ranges:942:13:   required for the satisfaction of ‘__is_range_adaptor_closure<_Lhs>’ [with _Lhs = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:943:9:   in requirements with ‘_Tp __t’ [with _Tp = coro::generator<long unsigned int>&]
/usr/include/c++/14/ranges:943:70: note: the required expression ‘std::ranges::views::__adaptor::__is_range_adaptor_closure_fn(__t, __t)’ is invalid
  943 |       = requires (_Tp __t) { __adaptor::__is_range_adaptor_closure_fn(__t, __t); };
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

jbaldwin · 2024-05-23T23:29:28Z

Really sorry how slow I've been to respond, life is a little wild right now. Thank you for opening the PR, we'll get this fixed.

jbaldwin · 2024-05-23T23:35:26Z

Thanks for adding tests already, it looks like the macos build in generally might be broken right now ☹️

test/test_generator.cpp

jbaldwin · 2024-05-24T02:38:25Z

I've merged a fix for the macos failures if you want to rebase and re-run the CI

Enables `view` concept on `generator` so that lvalue references cannot be accepts by `std::views:take`, which in turn would lead to erronious behavior.

…enerator

mikucionisaau · 2024-05-24T05:55:35Z

@jbaldwin I have rebased to the new main as requested.
What do you think of static_assert in copy constructor?
It's a bit unorthodox, but I think it gives a better explanation for users, rather than exposing the complexity of the standard library without a definite answer.
If you like it, I'll add that patch on top.

jbaldwin · 2024-05-24T14:21:26Z

Yeah lets go ahead and add it in please.

…ove compiler error messages

mikucionisaau · 2024-05-29T09:35:22Z

@jbaldwin I think I am done with this :-)

jbaldwin · 2024-05-29T12:04:22Z

Looks like it's uncovered some failing tests now if you could take a look at them.

This reverts commit 12f738c.

mikucionisaau · 2024-05-30T09:40:51Z

I see, the older GCC (<13) compiler treats static_assert more aggressively (it fails immediately when it sees it), so that last patch was not a good idea. Sorry.
I reverted that static_assert patch, it should work now.

mikucionisaau commented May 21, 2024

View reviewed changes

include/coro/generator.hpp Outdated Show resolved Hide resolved

mikucionisaau commented May 21, 2024

View reviewed changes

test/test_generator.cpp Outdated Show resolved Hide resolved

jbaldwin assigned jbaldwin and mikucionisaau and unassigned jbaldwin May 23, 2024

jbaldwin previously approved these changes May 23, 2024

View reviewed changes

test/test_generator.cpp Show resolved Hide resolved

mikucionisaau added 4 commits May 24, 2024 07:46

Fix for issue 261

420eb5b

Enables `view` concept on `generator` so that lvalue references cannot be accepts by `std::views:take`, which in turn would lead to erronious behavior.

Added a test case to check std::ranges:view concept, just like std::g…

5091639

…enerator

undo clang-format

00aeae2

undo clang-format

eb52dfb

mikucionisaau force-pushed the disable-pass-by-lvalue branch from afd80a7 to eb52dfb Compare May 24, 2024 05:47

Replaced generator deleted copy operations with static_assert to impr…

12f738c

…ove compiler error messages

mikucionisaau dismissed jbaldwin’s stale review via 12f738c May 29, 2024 09:28

mikucionisaau changed the title ~~Fix for issue 261~~ Fix unintended shallow copying when used with std::views::take (issue 261) May 29, 2024

jbaldwin previously approved these changes May 29, 2024

View reviewed changes

Revert static_asserts as older GCC (<13) treat them overly sensitively

72dd519

This reverts commit 12f738c.

mikucionisaau dismissed jbaldwin’s stale review via 72dd519 May 30, 2024 09:39

jbaldwin approved these changes May 30, 2024

View reviewed changes

jbaldwin merged commit d4a5515 into jbaldwin:main May 31, 2024
41 checks passed

mikucionisaau deleted the disable-pass-by-lvalue branch May 31, 2024 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unintended shallow copying when used with std::views::take (issue 261) #263

Fix unintended shallow copying when used with std::views::take (issue 261) #263

mikucionisaau commented May 17, 2024 •

edited

Loading

mikucionisaau commented May 21, 2024

jbaldwin commented May 23, 2024

jbaldwin commented May 23, 2024

jbaldwin commented May 24, 2024

mikucionisaau commented May 24, 2024 •

edited

Loading

jbaldwin commented May 24, 2024

mikucionisaau commented May 29, 2024

jbaldwin commented May 29, 2024

mikucionisaau commented May 30, 2024

Fix unintended shallow copying when used with std::views::take (issue 261) #263

Fix unintended shallow copying when used with std::views::take (issue 261) #263

Conversation

mikucionisaau commented May 17, 2024 • edited Loading

mikucionisaau commented May 21, 2024

jbaldwin commented May 23, 2024

jbaldwin commented May 23, 2024

jbaldwin commented May 24, 2024

mikucionisaau commented May 24, 2024 • edited Loading

jbaldwin commented May 24, 2024

mikucionisaau commented May 29, 2024

jbaldwin commented May 29, 2024

mikucionisaau commented May 30, 2024

mikucionisaau commented May 17, 2024 •

edited

Loading

mikucionisaau commented May 24, 2024 •

edited

Loading