Homogeneous container remapping code fails for some containers

[nextsilicon-itay-bookstein]

Hi, @Naios !

Thanks for making continuable, it's a neat library!

I passed an absl::InlinedVector<T, N, A> to cti::when_all(), and to my surprise the continuables in it did not get connected to the hierarchy. Investigating further revealed that because of a substitution failure deep inside the container-remapping code, the code degraded from using the container logic to using the plain-value logic (for values "that aren't accepted by the mapper"). Specifically, it failed substitution for rebind_container(). If I understand correctly, it failed because the template type signature of absl::InlinedVector did not match the ones that rebind_container() tries to accept. I'm wondering whether it should somehow match against traits rather than template type parameter shape?

---

Commit Hash

Used continuable/4.2.0 and abseil/20210324.2 from conan, but I doubt it matters much.

Expected Behavior

Print "Connection timed out\n"

Actual Behavior

Print "hi\n" + trap on exceptional unhandled continuable (SIGILL).

If the code is changed to use std::vector instead, expected behavior is observed.

Steps to Reproduce

Example code:

#include <cstdio>
#include <absl/container/inlined_vector.h>
#include <continuable/continuable.hpp>

int main() {
  absl::InlinedVector<cti::continuable<>, 1> v;
  v.emplace_back(cti::make_exceptional_continuable<void>(std::errc::timed_out));

  cti::when_all(std::move(v))
    .then([]() { std::puts("hi"); })
    .fail([](std::error_condition e) { std::puts(e.message().c_str()); });
  return 0;
}

Your Environment

• OS: Debian 10
• Compiler and version: Clang 12
• Standard library (if non default): (libstdc++-8.3, the default)

[ibookstein]

I went and discussed this issue with a colleague, and these are the points I'd like to argue :)

1. There's no robust general container-agnostic way of rebinding containers, even when trying to look beyond https://en.cppreference.com/w/cpp/named_req/Container + C::allocator_type-if-present. The current implementation basically makes the following two assumptions, if I understand it correctly:

   1. The container is either of template-shape C<T> or C<T, A>.
   2. The 1-or-2 template parameters directly dictate C::value_type and C::allocator_type (i.e. they don't just 'happen to be' equal, but they are always directly set from the template type parameters).

   If you consider std::unordered_map as a counter-example (maybe you want to rebind std::unordered_map<int, cti::coninuable<int>> to std::unordered_map<int, int>; I know it's not within the current design to do that, I'm just using it to illustrate the point):

   1. It has more template parameters, which would also need container-specific rebind logic, like Hash and Eq which are possibly not defaulted)
   2. The template parameters are combined in a specific way to create value_type = std::pair<const K, V>.
   3. The allocator needs to be rebound not for NewType, but for the new value_type.

   Barring any container-provided rebinding machinery (a-la allocators), perhaps it's 'safer' to provide a per-type rebind logic and leave it as an extension point for users wanting to provide containers that don't fit the mold, while falling back to rebinding to e.g. an std::vector when none of these options pan out?

2. Having the SFINAE machinery match against a container and decide to rebind it, but then bail out to the propagate-plain-value case is quite dangerous (especially when it's for continuables returning void and you have no type-safety machinery in the .then() to save you). Perhaps it should cause a compiler error in these cases?

What do you think?

[Naios]

Hi @ibookstein ,
I'm glad that you like my library.

I fully agree with you reproduction as well as your second argumentation.

1. It probably would be better to provide a point for customizing the remappper e.g. through a trait.
2. You are right, routing objects though, which are not remapped by default, can lead to potentially dangerous results if the user does not take attention to the input. However, the API did never promise to remap any container that is passed as input. The documentation clearly describes that std::vector or similar is the ideal input:

Arbitrary arguments which are connected. Every type is allowed as arguments, continuables may be contained inside tuple like types (std::tuple) or in homogeneous containers such as std::vector. Non continuable arguments are preserved and passed to the final result as shown below:

Probably the best way to solve this is to provide a trait that performs the remapping. The trait could only be used if the potentially remapped type provides a begin() and end() method e.g. satisfies the concept of an iterable. Then we could guard the remapping with a hard static_assert in case no remapper is provided for the iterable.

Sadly I'm currently considering this of scope feature-wise. For most use-cases std::vector should be enough. I'm open for a PR that adds this feature but I won't add it by myself in the near future.