eranpeer / FakeIt

C++ mocking made easy. A simple yet very expressive, headers only library for c++ mocking.
MIT License
1.22k stars 170 forks source link

ReturnAndSet compilation error when not all arguments are outputs #278

Closed fschuh closed 1 year ago

fschuh commented 2 years ago

ReturnAndSet seems to only compile if all arguments in a method are outputs. The only exception seems to be if the output is the first argument, but that only compiles if a placeholder isn't used.

Here are a few sample tests (with and without placeholders) that reproduce the issue:

    struct MyInterface {
        virtual int MyMethodOutputFirstArg(int&, int) = 0;
        virtual int MyMethodOutputSecondArg(int, int&) = 0;
        virtual int MyMethodOnlyOutputArgs(int&, int&) = 0;
    };

    TEST_CASE("ReturnAndSet + placeholder: output as first arg") // compilation error
    {
        Mock<MyInterface> mock;
        int value = 0;

        When(Method(mock, MyMethodOutputFirstArg)).ReturnAndSet(1, fakeit::placeholders::_1 <= 3);

        REQUIRE(mock.get().MyMethodOutputFirstArg(value, 10) == 1);
        REQUIRE(value == 3);
    }

    TEST_CASE("ReturnAndSet: output as first arg") // compiles OK
    {
        Mock<MyInterface> mock;
        int value = 0;

        When(Method(mock, MyMethodOutputFirstArg)).ReturnAndSet(1, 3);

        REQUIRE(mock.get().MyMethodOutputFirstArg(value, 10) == 1);
        REQUIRE(value == 3);
    }

    TEST_CASE("ReturnAndSet: output as second arg") // compilation error
    {
        Mock<MyInterface> mock;
        int value = 0;

        When(Method(mock, MyMethodOutputSecondArg)).ReturnAndSet(1, 3);

        REQUIRE(mock.get().MyMethodOutputSecondArg(10, value) == 1);
        REQUIRE(value == 3);
    }

    TEST_CASE("ReturnAndSet + placeholder: output as second arg") // compilation error
    {
        Mock<MyInterface> mock;
        int value = 0;

        When(Method(mock, MyMethodOutputSecondArg)).ReturnAndSet(1, fakeit::placeholders::_2 <= 3);

        REQUIRE(mock.get().MyMethodOutputSecondArg(10, value) == 1);
        REQUIRE(value == 3);
    }

    TEST_CASE("ReturnAndSet: only output args") // compiles OK
    {
        Mock<MyInterface> mock;
        int value = 0;
        int value2 = 0;

        When(Method(mock, MyMethodOnlyOutputArgs)).ReturnAndSet(1, 3, 5);

        REQUIRE(mock.get().MyMethodOnlyOutputArgs(value, value2) == 1);
        REQUIRE(value == 3);
        REQUIRE(value2 == 5);
    }

Using Visual Studio 2019 MSVC, and FakeIt 2.3.0.

FranckRJ commented 2 years ago

@hedayat, if you want to look at it. I may be able to look as well this weekend.

FranckRJ commented 1 year ago

I found the issue, in short if if constexpr isn't supported, FakeIt will generate an Assign function for each arguments of the mocked function (from left to right) until all the parameters from the ReturnAndSet/Set are consumed. The generated function either contain code for assigning the parameter from ReturnAndSet/Set to the parameter of the mocked function, or code for throwing an exception (because generation the assignment code isn't possible, like if we tried to assign an std::string to an int).

This decision was taken based on if parameter A was convertible to parameter B, but it was wrong because int is convertible to int, but not assignable (int is an rvalue, like 5 for example, and obviously 5 = 2 is not possible), so it generated code that wouldn't compile.

I changed the check from is_convertible to is_assignable and now it works, it generate the "assignment" code only if the two variables are compatible for assignment.

I'll merge the fix soon, and will release it in the 2.3.1, but be aware that your example "ReturnAndSet: output as second arg")" still won't compile because without placeholders you're trying to assign to the first arg which isn't an output arg.

FranckRJ commented 1 year ago

Fixed by #295 in 2.3.1.

hedayat commented 1 year ago

Thanks for the fix, and sorry that I wasn't able to spend time on it.

FranckRJ commented 1 year ago

No worries, it took me 5 months to change two lines, I understand.

hedayat commented 1 year ago

:D

fschuh commented 1 year ago

I found the issue, in short if if constexpr isn't supported, FakeIt will generate an Assign function for each arguments of the mocked function (from left to right) until all the parameters from the ReturnAndSet/Set are consumed. The generated function either contain code for assigning the parameter from ReturnAndSet/Set to the parameter of the mocked function, or code for throwing an exception (because generation the assignment code isn't possible, like if we tried to assign an std::string to an int).

This decision was taken based on if parameter A was convertible to parameter B, but it was wrong because int is convertible to int, but not assignable (int is an rvalue, like 5 for example, and obviously 5 = 2 is not possible), so it generated code that wouldn't compile.

I changed the check from is_convertible to is_assignable and now it works, it generate the "assignment" code only if the two variables are compatible for assignment.

I'll merge the fix soon, and will release it in the 2.3.1, but be aware that your example "ReturnAndSet: output as second arg")" still won't compile because without placeholders you're trying to assign to the first arg which isn't an output arg.

Great, thanks for the fix. I just tried it on the dev-2.3.1 branch and can confirm it works now.

Also my bad for the test you mentioned, "ReturnAndSet: output as second arg", it was incorrect and indeed shouldn't compile even with the fix.