Fix colliding mocking context ids (rebased)

[FranckRJ]

EDIT 2024-05-26:

The PR was reverted to the previous implementation with the hash, what follows do not apply anymore, but the implementation described below can be found in the branch alternative-stub-id-system.

OLD:

Follow up on what was started by #296.

The original issue was that two calls of Method() generated the same id when basing the generation of the id on __COUNTER__ (first call of this macro from two different TU will return the value 0). That id is used to identify which function is called later when the mock is used.

The original fix was to generate a hash based on the counter + file name of the TU, which work but is not ideal (even if it's almost impossible, two hash can collide, or __FILE__ can returns the same value for different TU depending on how they are built, etc).

After investigating the issue more, it appears that the ids don't need to be unique for every calls of Method(), they only need to be unique for every different calls of Method() (calls with different parameters).

So another solution was found, instead of having an integer id, the id is a pointer to an entity (a function in our case) that is dependent on the parameters of Method(). This guarantees us that we have different ids for different sets of parameters of Method().

I'm not 100% confident that two entities dependent on X in two different TU will have the same address, so maybe if there's two Method(mock, func) in two different TU they will generate two different ids, but it's not an issue as the only thing we need two guarantee is that two different calls to Method() (with different parameters) must generate two different ids. If the same call in two TU doesn't generate the same id it's ok (because the previous id stored in the mock will be overridden by the second one).

This PR should fix #294.

[coveralls]

coverage: 99.926%. remained the same
when pulling 9769ebb on otrempe-Fix-CollidingMockingContextIds
into c1d74ea on dev.

[otrempe]

Nice! Thanks for the extra work done on this issue!

We extensively use of the fix proprosed in #296. I will test this new solution on our code base as soon as possible.

[FranckRJ]

I will test this new solution on our code base as soon as possible.

That would be really helpful, thanks.

I noticed that you used the syntax When(Method(mock, func)).Return<std::string>("String"); for your tests in the original PR. I wanted to warn you that you're not supposed to explicitly provide the template parameter, it is deduced automatically from the mocked method return type, and since some changes have been made in FakeIt 2.5.0 to improve handling of move-only return types, it's not possible anymore to use this syntax. You should just write When(Method(mock, func)).Return("String");.

I wanted to know if there is a reason why you used this syntax, maybe a bug that you stumbled upon that was fixed by using it, or something else? Because if there's one I would like to fix it.

[otrempe]

That was indeed a bug we had to workaround.

If the stubbed function returns a reference, the stub would keep a reference to the value passed to AlwaysReturn, even if this value is an rvalue. In this case, you end up with a dangling reference when the function is invoked. Specifiying the template parameter forces AlwaysReturn to keep a copy of the value in all cases. To avoid this pitfall, we decided to always specify the template parameter.

I don't know if this is still relevant with the current release (2.4.0) though.

[FranckRJ]

That is somewhat fixed by #332. To summary, in the 2.5.0 passing an rvalue to Return() for functions returning a reference will trigger a static_assert that says that you should use ReturnCapture() instead. And ReturnCapture() will capture the passed object (either by copy or move) inside the mock to keep it alive as long as the mock is alive.

I guess it should fix your use-case, but it will require you to remove all explicit template parameter for the *Return() function, which may be annoying to do. So maybe I should reintroduce back this syntax (while keeping the new behavior) to not break too much existing code.

I'll think about it.

[otrempe]

Hi @FranckRJ, I pulled the "otrempe-Fix-CollidingMockingContextIds" branch to test the fix and I came across a potential bug.

The following code compiles fine using FakeIt 2.3.1.

class MyBaseInterface
{
public:
    virtual ~MyBaseInterface() = default;

    virtual double MyOverloadedMethod() const = 0;
    virtual double MyOverloadedMethod()       = 0;
};

class MyInterface : public MyBaseInterface
{
public:
    virtual ~MyInterface() = default;
};

void MyTest()
{
    fakeit::Mock< MyInterface > mock;

    fakeit::When( ConstOverloadedMethod( mock, MyOverloadedMethod, double() ) );
}

If the overloaded methods are in the child class, it works. But as soon as I have overloaded methods in a base class, it won't compile. I'm using Visual Studio 2022 version 17.3.5

[FranckRJ]

I think I found the issue. It's because &MyInterface::MyOverloadedMethod is of type double (MyBaseInterface::*)() but we use the type double (MyInterface::*)() to store the pointer to function which would require a conversion but that conversion doesn't seem to be allowed in template parameters (no idea why).

Anyway, I think I have a fix, I will push it this week.

[FranckRJ]

I pushed the fix @otrempe, when you have the time I would be interested to know if it fixes your issues or not.

[otrempe]

Thanks @FranckRJ . The fix works partially. If the overloaded methods are declared in different classes and the one coming from the base class is imported in the child class, I get an error.

class MyBaseInterface
{
public:
    virtual ~MyBaseInterface() = default;

    virtual double MyOverloadedMethod() const = 0;
};

class MyInterface : public MyBaseInterface
{
public:
    virtual ~MyInterface() = default;

    using MyBaseInterface::MyOverloadedMethod;
    virtual double MyOverloadedMethod() = 0;
};

void MyTest()
{
    fakeit::Mock< MyInterface > mock;

    fakeit::When( ConstOverloadedMethod( mock, MyOverloadedMethod, double() ) );
}

[FranckRJ]

It works on GCC / Clang for me, maybe it's a MSVC-specific issue, I'll look into it. Could you copy / paste your error ? I don't have Windows, I will try in a VM I guess but maybe the error can be enough for me to fix the issue.

[otrempe]

1>OverloadedMethodTest.cpp
1>OverloadedMethodTest.cpp(182,13): error C2672: 'fakeit::Prototype<double (void)>::getConst': no matching overloaded function found
1>fakeit.hpp(8984,39): message : could be 'R (__cdecl C::* fakeit::Prototype<double (void)>::getConst(R (__cdecl C::* )(void) const))(void) const'
1>        with
1>        [
1>            R=double
1>        ]
1>OverloadedMethodTest.cpp(182,13): message : 'R (__cdecl C::* fakeit::Prototype<double (void)>::getConst(R (__cdecl C::* )(void) const))(void) const': template parameter 'C' is ambiguous
1>        with
1>        [
1>            R=double
1>        ]
1>fakeit.hpp(8984): message : see declaration of 'fakeit::Prototype<double (void)>::getConst'
1>OverloadedMethodTest.cpp(182,13): message : could be 'MyBaseInterface'
1>OverloadedMethodTest.cpp(182,13): message : or       'MyInterface'
1>OverloadedMethodTest.cpp(182,13): message : 'R (__cdecl C::* fakeit::Prototype<double (void)>::getConst(R (__cdecl C::* )(void) const))(void) const': could not deduce template argument for 'R (__cdecl C::* )(void) const' from 'double (__cdecl MyInterface::* )(void) const'
1>        with
1>        [
1>            R=double
1>        ]
1>fakeit.hpp(8984): message : see declaration of 'fakeit::Prototype<double (void)>::getConst'
1>OverloadedMethodTest.cpp(182,13): message : 'R (__cdecl C::* fakeit::Prototype<double (void)>::getConst(R (__cdecl C::* )(void) const))(void) const': could not deduce template argument for 'R (__cdecl C::* )(void) const' from 'double (__cdecl MyInterface::* )(void) const'
1>        with
1>        [
1>            R=double
1>        ]
1>fakeit.hpp(8984): message : see declaration of 'fakeit::Prototype<double (void)>::getConst'
1>OverloadedMethodTest.cpp(182): error C2062: type 'unknown-type' unexpected
1>OverloadedMethodTest.cpp(182,13): error C2672: 'fakeit::Mock<MyInterface>::stub': no matching overloaded function found
1>fakeit.hpp(9144,67): message : could be 'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) const &&)'
1>fakeit.hpp(9136,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) &&)'
1>fakeit.hpp(9128,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) const &)'
1>fakeit.hpp(9120,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) &)'
1>fakeit.hpp(9112,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...))'
1>fakeit.hpp(9104,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) volatile const)'
1>fakeit.hpp(9096,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) volatile)'
1>fakeit.hpp(9088,67): message : or       'fakeit::MockingContext<WithCommonVoid<R,void>::type,arglist...> fakeit::Mock<MyInterface>::stub(R (__cdecl T::* )(arglist...) const)'
1>OverloadedMethodTest.cpp(182,13): error C3889: call to object of class type 'fakeit::WhenFunctor': no matching call operator found
1>fakeit.hpp(9300,39): message : could be 'fakeit::WhenFunctor::MethodProgress<R,arglist...> fakeit::WhenFunctor::operator ()(const fakeit::StubbingContext<R,arglist...> &)'

[otrempe]

I got that working on Compiler Explorer.

This indeed does not compile on MSVC, but works fine on gcc and clang

https://godbolt.org/z/7M6Ydh6n4

EDIT: You will have to add boost in the libraries options. I don't know why the shared link did not carry over this config.

[FranckRJ]

Thanks. It's weird that MSVC can't deduce the type of the class of the pointer to member, maybe it will work with a default value, I don't know, I will see.

[FranckRJ]

Another commit that should fix the remaining issues (I hope) but I'm not that confident in this new id system anymore, the compilers all seem to handle pointer-to-member in non-type template parameters in subtle different ways. Not sure about what to do with the PR.

[FranckRJ]

It doesn't work if you add one more level of inheritance, it's definitely not the right way to do it.

[otrempe]

Can you elaborate on how the proposed strategy in #296 is risky to the point that it cannot be integrated in an official release. More specifically how two hashes can collide and how FILE can return the same value for different TU?

[FranckRJ]

It's not too risky, it's just that it's perfectible so I tried to do something better but it turns out I wasn't able to.

Hash collision can theoretically happen, even if it's almost impossible I know, and even if it happens it will only cause issues in some very specific scenario (on the same mock for two different mocked methods).

Build systems can be very complex for big software, with a lot of distributed stuff happening, so we never know.

I know that those are hypothetical drawbacks, and it's not like the library was flawless, there's definitely issues that are a lot more likely to happen in it than that, but it's the reason why I wanted to search another solution initially.

[FranckRJ]

So yeah, unless I have an epiphany I'll revert my commits and merge your work soon. And thanks for all your help.

[FranckRJ]

I realize that instead of updating that PR I should have closed it and opened a new one, it would have been simpler. Too late now I guess.

[FranckRJ]

I added a compilation benchmark of FakeIt and for 10 files, each containing 10 interfaces, each containing 10 functions, each mocked 10 times, the time it takes to compile them went from ~17.3 seconds to ~18.2 on my computer with this PR. The compile time increased by ~5%, I guess it's not that much, especially since this value will be lower for real projects when tests won't only have FakeIt-related code to compile.

[FranckRJ]

I tried to benchmark my solution (the one that is now in the alternative-stub-id-system branch). Compilation time went from ~17.3 seconds on dev to ~7.5 seconds. That's a massive improvement. It comes from the fact that instead of having an unique ID per stub, the ID is only unique per mocked methods, so if you create 10 stubs for the same method, instead of instantiating the stubs-related templates 10 times, they are only instantiated 1 time.

If instead I have one file containing 10 interfaces, each containing 10 methods, each stubbed one time, then it goes from ~2.5 seconds to ~2.25 seconds. There's still an improvement (no idea why) but only 10% this time.

Like stated in my previous message, in real code bases the impact of these changes (your solution or mine) won't be as big as what I measured, because the code of my benchmark only contains the include of fakeit and (a lot of) stubs, and in a real code base there will be a lot more stuff (and probably fewer stubs).

For the FakeIt tests for example, they went from compiling in ~8.2 seconds on dev to compiling in ~7.6 seconds, a 7.5% improvement.

I believe I can make my solution work on GCC and MSVC by having a different implementation for both of them (deducing the real type of the method on GCC, casting it on MSVC, as the cast of the method doesn't work in GCC and the deducing doesn't work in MSVC), but I'm wondering if it's worth it.

I guess I'll move to other things I have to do before releasing FakeIt 2.5.0 (adding back .Return<type>(obj) to not break too much code) and I'll go back to this with a fresher mind.