fu2::function_view created from empty fu2::function is not itself empty

eigenwhat commented 5 years ago

@Naios

Instantiating an fu2::function_view from an empty fu2::function does not produce an empty fu2::function_view. I didn't look into it too deeply but my initial impression is that it's due to the constructor for arbitrary callables function(T&& callable) getting called instead of function(function<RightConfig, property_t> const& right) as I would expect, so the erasure_t is constructed to wrap the fu2::function as a whole instead of unpacking it.

Commit Hash

e3695b4b4fa3c672e25c6462d7900f8d2417a417

Steps to Reproduce

// empty_function_view_test.cpp
#include <catch2/catch.hpp>
#include <function2/function2.hpp>

#include <optional>
#include <string>

TEST_CASE("Function2 function -> function_view", "[function2]")
{
    fu2::function<std::optional<size_t>(const std::string&)> f = [](const std::string &str) -> std::optional<size_t> {
        if (str.size() > 0) { return str.size(); }
        else { return std::nullopt; }
    };
    REQUIRE_FALSE(f.empty()); // pass
    fu2::function_view<std::optional<size_t>(const std::string&)> fv = f;
    REQUIRE_FALSE(fv.empty()); // pass

    fu2::function<std::optional<size_t>(const std::string&)> emptyf{};
    REQUIRE(emptyf.empty()); // pass
    fu2::function_view<std::optional<size_t>(const std::string&)> emptyfv = emptyf;
    REQUIRE(emptyfv.empty()); // fail
}

Your Environment

OS: Mac OS Mojave, Gentoo Linux
Compiler and version: appleclang-10.0.1, gcc-8.3

Naios commented 5 years ago

Thanks for your bug report. From my point of view this should be the right behaviour since one couldn't distinguish anymore between whether a function_view was empty constructed or through a function which is itself empty. function_view is designed to work without knowing anything about the function, hence it works the the same way when being passed a fu2::function or std::function (or similar implementations). Changing it in a way you suggest would heavily violate that principle.

What do you think about it?

eigenwhat commented 5 years ago

I think it's reasonable to expect that the members of the module (function, unique_function, and function_view) would have smarter interop with each other, that is to say, to be aware of each others' existence as callable object wrappers and to handle this case as I had expected. When I initialize a function with a function (std or fu2), if the first one is empty, the second one is as well. This is reasonable. It works just like any other smart pointer or container. For example, if I had a function (let's say originally implemented with std::function and recently migrated) like this:

bool evaluate(expr_args args, function<bool(expr_args &&) const> const &predicate)
{
    if (predicate) {
        return predicate(std::move(args));
    else {
        return false;
    }
}

everything still works like it did before. If I pass in a lambda, it works. If I pass in nullptr, it works. If I pass in a function initialized with a lambda, it works. If I pass in a function initialized with nullptr, it works. If I now decide to replace that with function_view (much like how one would replace std::string const & with std::string_view), now my code breaks because function_view is not empty and this will instead crash. I need to either revert to taking in a function (which is sub-optimal when passing in non-function values), or make sure every call site checks that the thing they're passing in is populated. To work this way makes it more inconsistent. To me this behavior is counter-intuitive. To me it's like converting one shared_ptr equal to nullptr to a shared_ptr of a related type and having it result in a shared_ptr with the address of the original shared_ptr in it (since shared_ptr is convertible to its raw pointer equivalent).

I could understand if this were going between two types from different libraries, but they're both designed to work with each other, so I'd expect that they would each be aware that they're wrappers. I might even go as far as to say that it should also work with std::function in the same way.

eigenwhat commented 5 years ago

To add, a similar issue occurs between compatible (but different) instances of function, e.g.

TEST_CASE("empty fu2::function -> fu2::function", "[function2]")
{
    fu2::function<std::optional<size_t>(const std::string&)> emptyf{};
    REQUIRE(emptyf.empty()); // pass
    fu2::function<std::optional<size_t>(std::string)> emptyf2 = emptyf;
    REQUIRE(emptyf2.empty()); // fail
}

This is incongruent with std::function, where the null state is transferred:

TEST_CASE("empty std::function -> std::function", "[function2]")
{
    std::function<std::optional<size_t>(const std::string&)> emptystdf{};
    REQUIRE_FALSE(emptystdf); // pass
    std::function<std::optional<size_t>(std::string)> emptystdf2 = emptystdf;
    REQUIRE_FALSE(emptystdf2); // pass
}

Naios commented 5 years ago

Thank you for giving me a better insight about your issue. I now agree with you and plan to implement the suggested improvements in a way that doesn't use a specialization for fu2::function or std::function but will rely on a convertibility to bool such that similar function like classes are taken into account as well. The rule will be that if the callable object is also convertible to bool (implements operator bool()) and returns false then the function or function_view will be empty as well.

This will bring additional benefits and breaking changes:

The empty state will be propagated correctly between all function like type erasures (and also from null plain function pointers).
If a null plain function pointer is stored into function or function_view and invoked, an exception will be thrown (or abort() is called according to your configuration set).
If a callable which implements operator bool() and returns false is copied into function then no copy will be performed saving an eventual heap allocation.

@axalon900 What do you think about this suggested solution?

Naios / function2