Why isn't std::function working with perfect forwarding?

Question

Note:

• I apologize in advance but I was unable to reduce/simplify the code any further (i.e., all my smaller tests couldn't reproduce this issue)
• Version 1 of the code compiles but does not execute properly using Visual Studio 2010 Premium
• Version 2 & 3 of the code compiles and executes successfully using Visual Studio 2010 Premium
• All versions of the code compiles and executes successfully using ideone.com (C++14)

Version 1 Output (Visual Studio 2010)

Signal1<A1>::raise(7)
Slot::operator()(7)
Slot1<A1>::operator()(7)
Handling new value: 3931764 // This value changes on each execution

Version 2 Output (Visual Studio 2010)

Signal1<A1>::raise(7)
Slot::operator()(7)
Slot1<A1>::operator()(7)
Handling new value: 7

Version 3 Output (Visual Studio 2010)

Signal1<A1>::raise(7)
Slot::operator()(7)
Slot1<A1>::operator()(7)
Handling new value: 7

Code

#include <functional>
#include <iostream>
#include <vector>

// Version 1: uses perfect forwarding with std::function<void (T)>
// Version 2: uses perfect forwarding with std::function<void (const T&)>
// Version 3: forgoes perfect forwarding with std::function<void (T)>

#define VER 1

class Slot
{
public:
    template<typename A1>
#if VER == 1 || VER == 2
    void operator()(A1&& a1) const;
#elif VER == 3
    void operator()(A1 a1) const;
#endif
};

template<typename A1>
class Slot1 : public Slot
{
public:
    template<typename T>
    Slot1(T* instance, void (T::*fn)(A1)) :
        mFn(std::bind(fn, instance, std::placeholders::_1))
    {
        // Do nothing
    }

    void operator()(A1 a1) const
    {
        std::cout << "Slot1<A1>::operator()(" << a1 << ")\n";
        mFn(a1);
    }

private:
#if VER == 1 || VER == 3
    std::function<void (A1)> mFn;
#elif VER == 2
    std::function<void (const A1&)> mFn;
#endif
};

template<typename A1>
#if VER == 1 || VER == 2
void Slot::operator()(A1&& a1) const
#elif VER == 3
void Slot::operator()(A1 a1) const
#endif
{
    std::cout << "Slot::operator()(" << a1 << ")\n";
#if VER == 1 || VER == 2
    static_cast<const Slot1<A1>&>(*this)(std::forward<A1>(a1));
#elif VER == 3
    static_cast<const Slot1<A1>&>(*this)(a1);
#endif
}

class Signal
{
public:
    void connect(Slot* slot)
    {
        mSlots.push_back(slot);
    }

    std::vector<Slot*> mSlots;
};

template<typename A1>
class Signal1 : public Signal
{
public:
    void raise(A1 a1)
    {
        std::cout << "Signal1<A1>::raise(" << a1 << ")\n";
        (*mSlots[0])(a1);
    }
};

class Model
{
public:
    void setValue(int value)
    {
        mValue = value;
        mValueChangedSignal.raise(value);
    }

    Signal1<int> mValueChangedSignal;

private:
    int mValue;
};

class View
{
public:
    void handleChange(int value)
    {
        std::cout << "Handling new value: " << value << "\n";
    }
};

int main()
{
    View view;

    Slot1<int> slot(&view, &View::handleChange);

    Signal1<int> signal;
    signal.connect(&slot);

    signal.raise(7);
    return 0;
}

---

Is this a Visual Studio bug or am I doing something wrong?

Answer 1

Version 1 and 2 do undefined behavior, as Slot::operator() of Slot1<int> is called with A1 equal to int& within Signal1<int>, which then static_casts itself to Slot1<int&>. It is not a Slot1<int&>, so that cast generates a bad reference, which you then use, and boom, whatever happens happens.

Please check your types and stop explicit casting based of parameter types, that is ridiculously unsafe and annoying to track down.

I am uninterested in untangling the mess of #ifdefs in your code to determine if a similar error occurs with version 3. Your design is fundamentally unsafe, as relatively innocuous argument changes within argument passing to Slot cause undefined behavior. You should not be implicitly taking deduced parameters to operator() and using it to cast the type of this to a derived type.

Treat type casting with respect.

Here is a detailed breakdown of the undefined behavior your code causes in case 1:

signal.raise(7);

calls

Signal1<int>::raise(int)

which calls

void raise(int a1)
{
    (*mSlots[0])(a1);
}

here a1 is an lvalue of type int. So this calls

Slot::operator()(int& a1) const because that is how forwarding references work -- T&& passed an int& deduces T as int&. The body then contains

static_cast<const Slot1<int&>&>(*this)(std::forward<int&>(a1));

which casts *this into a reference to Slot1<int&>, a class that is unrelated to the object in question. Undefined behavior results when you try to interact with it.

As I have said, fixing this is possible, but your fundamental problem is that the deduction of the type parameter in Slot::operator() is not a suitable way to determine what sub-type of Slot you want to cast *this into. The type of an expression passed to operator() is not, in general, obvious at point of call, nor at point of deduction. And if you do not get this type exactly right, the result is undefined behavior. Which can often "seem to work", until some completely unrelated change occurs and it breaks.

When casting to-derived from-base, you must must must be careful, explicit, and document what you are doing at every step.

If someone calls your Slot (which is actually a Slot1<int>) with an unsigned int, undefined behavior. A size_t, undefined behavior. A unsigned char, undefined behavior. A uint16_t, undefined behavior. Adds a long long to an unsigned int, calls you with result, undefined behavior. A type that implicitly converts to int, undefined behavior.

polymorphism without type safety is rarely a good idea.

On top of this, there is no good reason in your code to do this. Signal1<A1> could implement connect instead of Signal taking a Slot1<A1> -- you, after all, have the type right there -- and store an array of Slot1<A1> instead of an array of Slot. Given that the only type of Slot that would not result in undefined behavior later on is a Slot1<A1>, why store the wrong type at all?