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

`When(Method(mock, foo)).Throw("myException")` no longer works #254

Closed poeschlr closed 1 year ago

poeschlr commented 2 years ago

It seems .Throw("myException") does no longer work in conjunction with REQUIRE_THROWS_WITH(bar(), "myException");.

If i use .AlwaysThrow("myException) instead, then this seems to still work.

Edit: the strange thing is that .Do([](){throw "myException";}) works. And if my limited c++ knowledge is correct then .Throw just returns a .Do.

We use the single header for catch. The one that does not seem to work is the current one on master (at time of writing: Generated: 2021-05-12 13:47:04.979584). The one that still worked was Generated: 2019-06-01 12:14:44.281637


One change i can identify between these two versions is that there now is a new class FakeObjectImpl. The applyTuple members now take an additional template type for the FunctionType and the template for Times has been removed. (plus some formatting changes and some changes related to i assume compiler warnings)

FranckRJ commented 2 years ago

The issue is that since the lambda isn't used anymore when the method call is finished, it is deleted, thus deleting the array containing the string "myException". And because only a pointer to the array is thrown (not the full array itself) you get a pointer to an area in memory that is deleted, so the string matching of catch is now undefined behavior.

I'm not sure what the fix is, or why it worked before (i would guess just luck with undefined behavior ?), the lambda should be deleted once it is used, and the throw throwing a pointer to char instead of the array is expected behavior from C++ as well, not a bug in the library.

Unless you have another idea, I guess you should use std::string instead of char arrays, because it will be properly copied when thrown. If you find it annoying to type "std::string" all the time you can also use the s prefix : https://en.cppreference.com/w/cpp/string/basic_string/operator%22%22s

FranckRJ commented 2 years ago

Or maybe the fix should be to decay the parameter of Throw, so it would take a char pointer instead of a char array. The pointer would then point to the string literal (instead of to a copy of it) which will be always valid during the entire execution of the software.

malcolmdavey commented 2 years ago

I was actually trying out a change, for a different purpose, which allowed the lambda's life time to live as long as the mock, which might also fix this problem.

FranckRJ commented 2 years ago

I think the best thing to do should be to throw a std::string if the capture of the throw lambda is an array of chars. Everything else seems too complicated (somehow take a char* instead of a char[] for string literals, so the lambda capture a copy of the char* and not a copy of the char[]) or looks like bugs (keeping one-time lambda alive even after we already used them).

Throwing a char[] is always a bug currently, so even if converting it to a std::string may cause some bugs (if catching code specifically require char*, then the exception won't be caught), i guess it is ok because this new bug will only replace the old one, and most of the time it will probably fix some bugs.

FranckRJ commented 2 years ago

Should be fixed with the same fix as #258, when lambda life will be extended to match the mock lifetime.

FranckRJ commented 1 year ago

Should be fixed by #301, in FakeIt 2.3.1.