Fixes OCMockObjects when init* methods are stubbed under ARC.

[dmaclach]

Previously OCMockObjects were being leaked when init* methods were being stubbed. Init method invocations are required to return an object to be considered part of the init method family (https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families).

We verify that it is an init method by looking for the init prefix and verifying that it returns an id.

[dmaclach]

Updated code to be much tighter about checking ARC rules. Now verifies exact matches or next letter is lower case for names, and verifies that the method returns an object type.

[imhuntingwabbits]

It would be worth considering as an enhancement to this a callback clients can register to opt "difficult" to detect methods in to this paradigm. create* is a convention, but in theory anything returning instancetype marked NS_RETURNS_RETAINED could need this support.

Having a block that could be registered at "principle class time" to detect these funky selectors would be elegant / minimally invasive.

[dmaclach]

I've updated this pull to be more complete after spending a couple of days tracking down other init related issues in our code base.

Previously OCMockObjects were being handled incorrectly when init* methods were being stubbed leading to leaks and/or crashes.

Init method invocations are required to return an object to be considered part of the init method family (https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families).

Init methods are also required to consume self and return a retained object (https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-init).

In the case of a recorder we need to be sure that we return either the mockobject or the recorder depending on how the invocation reaches the recorder (i.e. via macro or via stub invocation).

In the case of a partial mock object we need to be sure that we return either the mock, or the value coming back from the realObject (usually realObject, but not necessarily) depending on how/where the invocation is invoked.

[twitterkb]

for discussion here, i've applied this locally, and it did help with some "regressions" that were occurring with only #392 applied . reference the above "mentions" for more info.

[twitterkb]

following up … interested in knowing what else needs to happen to make a decision about either moving forward with this, or backing and reverting #392 …

[dmaclach]

I'm assuming that Erik is busy with other things at the moment. AFAIK he's the only person who can unblock this.

[erikdoe]

@twitterkb Dave was right, I have been super busy over the past few weeks, but I needed a fair bit of time to understand this PR.

Now I had some time and looked into this PR in detail. In the process of understanding it I made some refactorings, e.g. moved the logic to determine the method family from functions into the category on NSInvocation. This is on a local branch so far, but it means that future changes to this PR itself would be tricky for me to integrate. So, I'd rather discuss and then push my local branch for further review.

I have also refactored the tests, to make them more readable, but I have stashed these changes for now. Reason is that even though I'm 99% sure that the refactorings of the tests didn't change anything, I do want to make absolutely sure that refactorings of the code pass the tests originally part of this PR.

The next step for me would be a slightly larger refactoring, to get rid of initTarget. To be honest, the name itself and logic really confused me. In the end initTarget is either the recorder or the mock. Technically, it can be nil but in that case the recorder is used. This sounds more like a condition (a bool or maybe an enum) an not like a reference that could hold all sorts of values.

I'm also not happy with the setInitTarget: sprinkled through the recorder subclasses. And, further, it's confusing that in the macro case, the recorder will call setInitTarget: but the newly set target will not be used, because the recorder already took the previous value earlier.

To go ahead with the refactoring I have in mind I would need clarifications regarding the questions I have left in the code. Thank you again for your help (and patience).

[erikdoe]

So... the more I look at this the less I like the mixing of styles. It seems to be the result of a superficial conversion, and it was never meant to be supported.

Good news is that the mixing easy to detect. When the mock receives forwardingTargetForSelector: and determines it is inside a macro, then it can simply check whether the method is actually implemented by the recorder, via class_getInstanceMethod. If that's the case, a mix has been detected.

If we disallow mixing, then the whole initTarget logic can be removed. The recorder can simply check, when an init method family invocation occurs, whether it's inside a macro. In that case it sets the mock as the return value. In all other cases, it sets itself.

Now, though, the bad news is if we throw an exception when mixing is detected, we trigger the issue addressed by #410. So, for now I would go ahead and do this but temporarily add mixing of styles as a reason for the "Did not record an invocation..." exception, because that will be thrown when an exception is thrown at the place where the mixing is detected. Once #410 is merged we can clean this up.

Hope this works for you, even though I realise you'll have 205 instances where the mixing will throw an exception.....

[dmaclach]

I'm ok with that. A lot of the ignoringNonObjectArgs I can probably regex.

[dmaclach]

OK, so one place this is causing problems is that previously one could do:

OCMVerify([[mock ignoringNonObjectArgs] bar:0]);

and given the way that verifiers currently work I'm not sure if we can support

OCMVerify([mock bar:0]).ignoringNonObjectArgs();

Is this also not a supported use case?

[dmaclach]

Also,

I'm ok with that. A lot of the ignoringNonObjectArgs I can probably regex.

Also, I take this back... I screwed up on my regex. I have 1000s of cases of mixed syntax. Lots of [OCMStub(.*) andReturn:]. Can we please please please leave the mixed syntax? Sorry.

[erikdoe]

You are right. Verification would be a problem. I never found a way to tack something onto OCMVerify(). The other macros can simply accept further optional information from chained macros. Verify, however, needs to do something, and I couldn't think of any way for a macro in a chain to determine that it is the last macro. If I had, I would have done quantifiers differently (and earlier).

So, this one definitely "saved" you. It means that mixing the styles must work. I have an idea, which is not as elegant as the non-mixing version. Give me a day or two.

[erikdoe]

Given that the refactorings I made in the end didn't change the functionality, I have merged this straight into main now. At 1cab63de9045cdfdf521568d60e6198d0dc06d30 you can see how the refactored code passes the test originally in this PR. After that I added in the refactored tests.

For some reason Github doesn't see that the commits in this PR made it into main. So, I'm closing this PR manually. Please double check that things are working.

[twitterkb]

thanks for all the work on this. i'll pull this and run our tests through it.