Unintended function_ref constructor selection

[Life4gal]

function_ref has two constructors for functor (excluding the overloading of the one whose argument type is a lambda that can be converted to a function pointer):

type_safe/include/type_safe/reference.hpp

Lines 592 to 621 in a80a3ed

	/// \effects Creates a reference to the specified functor.
	/// It will store a pointer to the function object,
	/// so it must live as long as the reference.
	/// \notes This constructor does not participate in overload resolution,
	/// unless the functor is compatible with the specified signature.
	/// \param 1
	/// \exclude
	template <typename Functor, typename = detail::enable_function_tag<detail::matching_functor_tag,
	Functor, Return, Args...>>
	explicit function_ref(Functor& f) : cb_(&invoke_functor<Functor>)
	{
	::new (storage_.get()) void*(&f);
	}
	/// Converting copy constructor.
	/// \effects Creates a reference to the same function referred by `other`.
	/// \notes This constructor does not participate in overload resolution,
	/// unless the signature of `other` is compatible with the specified signature.
	/// \notes This constructor may create a bigger conversion chain.
	/// For example, if `other` has signature `void(const char*)` it can refer to a function taking
	/// `std::string`. If this signature than accepts a type `T` implicitly convertible to `const
	/// char*`, calling this will call the function taking `std::string`, converting `T ->
	/// std::string`, even though such a conversion would be ill-formed otherwise. \param 1 \exclude
	template <typename Return2, typename... Args2>
	explicit function_ref(
	const function_ref<Return2(Args2...)>& other,
	typename std::enable_if<detail::compatible_return_type<Return2, Return>::value, int>::type
	= 0)
	: storage_(other.storage_), cb_(other.cb_)
	{}

It looks like this part of the test case is for testing the second constructor, but in fact, it's not!

type_safe/test/reference.cpp

Lines 308 to 318 in a80a3ed

	SECTION("ref conversion")
	{
	function_ref<int(int, int)> a([](int a, int b) { return a + b; });
	REQUIRE(a(1, 3) == 4);
	function_ref<int(int, short)> b(a);
	REQUIRE(b(1, 3) == 4);
	function_ref<void(int, int)> c(a);
	c(1, 3);
	}

As you can see, it uses the first overload!

This makes sense, because the first overload can obviously accept such a construct. I'd even go so far as to say that the second construct is obviously wrong (it's just that it's never called, so it doesn't generate an error for now).

Obviously:

type_safe/include/type_safe/reference.hpp

Lines 676 to 677 in a80a3ed

	using storage = detail::aligned_union<void*, Return (*)(Args...)>;
	using callback = Return (*)(const void*, Args...);

Relies on the template parameters Return and Args, so it is obviously not possible to simply convert a variable of a different type to the desired type.

type_safe/include/type_safe/reference.hpp

Line 620 in a80a3ed

: storage_(other.storage_), cb_(other.cb_)

Even so, it is clear that calling the first overload is not as expected.

If a function is passed a function_ref by copy construction, then there is an intermediate state that holds the function_ref.

class State
{
    // ...
private:
    // ....
    function_ref<int(int, int)> function_;
    // ....
};

auto foo(int a, double b, std::string c, function_ref<int(int, int)> function) -> void
{
    // ...
    State state{a, b, c, function};
    // ....
}

auto bar(function_ref<int(int, int)> function) -> void
{
    // ...
    foo(42, 3.14, "hello world", function);
    // ...
}

auto main() -> int
{
    int i = 42;
    auto lambda = [&i](int a, int b) -> int { return i + a + b; };
    
    bar(lambda);
}

The result may not be as expected, and may even lead to segmentation fault! Because the function_ in State references another function_ref that may be destructed.

So the first overloading candidate should be restricted so that the above case calls a copy construct of function_ref.

After that I'll start a PR where I'll restrict the first overload, remove the second one, and add a new overload like the second one to allow for automatic conversion of function_refs where the return type and parameter type is compatible with that function_ref, but this will cause the constructed function_ref to refer directly to the incoming function_ref.