Add ReturnCapture for methods returning a ref to capture a value inside the mock then return a ref to it.

[FranckRJ]

Follow up on was was started by #280 (and #310).

What this PR contains:

• A (Always)ReturnCapture for methods returning a reference that will either copy or move the parameter inside the lambda and return a reference to it when the mocked function is called.
• Disable (Always)Return taking an rvalue reference as parameter for methods returning a reference. (Always)Return doesn't support mocking methods returning rvalues reference anyway, and most often this overload would be called with a temporary which would result in returning a dangling reference.

What changed compared to the previous PR:

• (Always)ReturnCopy was renamed to (Always)ReturnCapture because it also support moving values instead of only copying it.
• The automatic (Always)ReturnCopy when passing an rvalue reference to (Always)Return() for methods returning a reference was replaced by a compilation error. The intent is to not introduce too much magic inside the different functions, and make their behavior a bit more explicit ((Always)Return() always only forward references for methods returning references, and if you want to capture then the only way to do it is to call (Always)ReturnCapture). The error message should be clear enough to not cause too much trouble to the user:

[...]
/home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp: In instantiation of ‘fakeit::MethodStubbingProgress<R, arglist ...>& fakeit::helper::BasicReturnImpl<R, true, arglist ...>::Return(typename std::remove_cv<typename std::remove_reference<_Tp>::type>::type&&) [with U = const std::__cxx11::basic_string<char>&; R = const std::__cxx11::basic_string<char>&; arglist = {}; typename std::remove_cv<typename std::remove_reference<_Tp>::type>::type = std::__cxx11::basic_string<char>; typename std::remove_reference<_Tp>::type = const std::__cxx11::basic_string<char>]’:
/home/franckrj/Projects/FakeIt/tests/referece_types_tests.cpp:298:53:   required from here
/home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp:76:41: error: static assertion failed: Return() cannot take an rvalue references for functions returning a reference because it would make it dangling, use ReturnCapture() instead.
   76 |                 static_assert(sizeof(U) != sizeof(U), "Return() cannot take an rvalue references for functions returning a reference because it would make it dangling, use ReturnCapture() instead.");
      |                               ~~~~~~~~~~^~~~~~~~~~~~
[...]

• The value stored inside the mock has the same type as the values passed by parameter to (Always)ReturnCapture, this will prevent slicing issues, but forces us to pass a type that can be bound to the return value of the method:

struct Parent
{
    virtual int getVal() { return -1; }
};

struct Child : Parent
{
    int val = 0;
    int getVal() override { return val; }
};

struct Interface
{
    virtual Child& getChild() = 0;
    virtual Parent& getParent() = 0;
};

void testCopy()
{
    Mock<Interface> mock;

    When(Method(mock, getChild)).ReturnCopy(5); // You can pass only "5" if you want, the "Child" object will be constructed from it.
    When(Method(mock, getParent)).ReturnCopy(Child{5});

    mock.get().getChild().getVal() // Will return 5.
    mock.get().getParent().getVal() // Will return -1 because of the object slicing.
}


void testCapture()
{
    Mock<Interface> mock;

    // When(Method(mock, getChild)).ReturnCapture(5); // ERROR: The parameter must be boundable to Child&.
    When(Method(mock, getChild)).ReturnCapture(Child{5}); // You're forced to write it this way.
    When(Method(mock, getParent)).ReturnCapture(Child{5}); // Valid.

    mock.get().getChild().getVal() // Will return 5.
    mock.get().getParent().getVal() // Will return 5 because we store the original type.
}

The last point make the syntax a bit worse than (Always)ReturnCopy, but it prevents a bug so I guess it's ok, and the error message is somewhat nice if the wrong type is passed so it's not that bad:

[...]
/home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp: In instantiation of ‘fakeit::MethodStubbingProgress<R, arglist ...>& fakeit::helper::BasicReturnImpl<R, true, arglist ...>::ReturnCapture(T&&) [with T = const char (&)[15]; R = ReferenceTypesTests::AbstractType&; arglist = {}]’:
/home/franckrj/Projects/FakeIt/tests/referece_types_tests.cpp:302:61:   required from here
/home/franckrj/Projects/FakeIt/include/./fakeit/StubbingProgress.hpp:93:107: error: static assertion failed: The type captured by ReturnCapture() (named T) must be compatible with the return type of the function (named R), i.e. T t{...}; R& r = t; must compile without creating temporaries.
   93 |                                 typename std::remove_cv<typename std::remove_reference<T>::type>::type&>::value,
      |                                                                                                           ^~~~~
[...]

The things I'm not so sure about:

• (Always)ReturnCapture is only implemented for methods returning a reference, as it doesn't really make any sense for methods returning a value as (Always)Return already captures. But maybe it's better to make it available for all methods to make the API more consistent, I don't know.
• These changes (and the ones of the previous PRs about returning move-only types) only concern classic returns, not (Always)ReturnAndSet or Method(mock, func) = obj, maybe they should be expanded to impact them as well.
• Maybe (Always)Return should work for methods returning rvalue references. But (Always)ReturnCapture should already work with these methods so maybe it's enough (but it's not tested I guess so tests should be added to ensure it works).
• Is (Always)ReturnCapture a good name ?

@malcolmdavey Does it seem like the feature still covers your needs ?

[coveralls]

coverage: 99.926%. remained the same
when pulling c579f5a on iress-OSS-7-copy-return-for-return-by-ref-methods
into 1ed5f80 on dev.

[malcolmdavey]

TL;DR - I'm happy enough for the PR to be merged as is.

Good pickup with the slicing issue.

"But maybe it's better to make it available for all methods to make the API more consistent, I don't know."

My own thinking, that when there is only one possible implementation, then it might make sense.
The only potential gotcha.complication is if editing the reference value after getting it from an AlwaysReturn.
But if you are Always returning a reference then you may expect it to be a ref to the same object, so it makes sense

From memory I added a test which indicates this, but haven't checked the updated PR.

However not to fussed for the current PR. At least there is an option to work around it. And we could change our minds later as it wouldn't be a breaking change.

ReturnCapture - I guess it's okay as a name. Don't want to hold it up based on that.

These changes (and the ones of the previous PRs about returning move-only types) only concern classic returns,

I guess they are lower priority.

For me I sort of wanted to fix these hopefully straight forward Return improvements to handle all sensible and common C++11 scenarios. (it's simple and easy to pass in temporaries to a (Always)Return).

After that I was hoping there would be an agreed way forward to handle the dangling reference problem with argument passing for mocked methods and then validating afterwards ... I think this is the biggest problem with the library as it is, and was also meant to be a selling point. I think the assumption that args are either values, or long lived is usually not the case for things like strings or container classes. If it's a breaking change then we may need a way to opt into it.

[FranckRJ]

Thanks for the feedback. I think you're right, the PR looks good enough as is and future improvements can always be added later in another PR, as it seems they won't break anything.

If you're talking about #274, yeah it's a tricky issue, I've never looked too deep into it. But I guess that an opt-in way of copying reference arguments (instead of the reference itself) could be a short-term workaround for most usages (it will only work for copyable types that don't have too disruptive side effects on their copy constructor, which is probably true for the majority of types but not for all).