Skip to content

Commit 0ee6a61

Browse files
committed
Improve andDo: type safety and simplify usage.
The current `andDo` blocks have some major problems: a) They are difficult to use from ARC. When getting values out of NSInvocations, one has to be careful to use `__unsafe_unretained` on arguments, or risk over retaining/releasing and corrupting the heap. When returning values one has to be careful to make sure that they are handled correctly (often using `__autoreleasing`) or once again risk memory issues. b) They don't deal with refactors well. Since extracting argument inside of the blocks depends on extracting by index, it is easy to change a method signature and forget to deal with the index. In many cases it may "just pass" even though the arguments no longer line up correctly. In other cases it leads to crashes that are difficult to debug. c) The signature is often overly complicated for simple blocks. In many cases all we want is a block that just returns a value, or doesn't need to take arguments or return a value. This changes modifies andDo blocks so that the current `andDo:^void(NSInvocation *invocation) { ... }` block is now considered deprecated and gives you options to the type of block you want to pass in. The first is the simple block `^{ ... }` that takes no arguments and has an optional return value. The runtime code verifies that the return value (if supplied) is compatible with what the invocation expects. The second option is the full block which is `^(returnType)(id localSelf, Arg1Type arg1, ...)` where `returnType` and the argument list match the return type and arguments to the invocation. These values are checked at runtime to make sure that they match what the invocation expects. To aid in the transition from the deprecated block type to the new block types, the runtime will `NSLog` a warning about the deprecated block type, and will attempt to suggest a good block declaration to replace it with. They would look something like this: ``` Warning: Replace `^(NSInvocation *invocation) { ... }` with `^id(NSSet<ObjectType*> *localSelf, NSNumber *count, NSSet<ObjectType*> *set) { return ... }` ``` This changes code that previously would have looked like this: ``` OCMStub([mockURL setResourceValues:attributes error:[OCMArg anyObjectRef]]) .andDo(^(NSInvocation *invocation) { __unsafe_unretained NSError **errorOut = nil; [invocation getArgument:&errorOut atIndex:3]; *errorOut = error; BOOL returnValue = NO; [invocation setReturnValue:&returnValue]; }); ``` to ``` OCMStub([mockURL setResourceValues:attributes error:[OCMArg anyObjectRef]]) .andDo(^BOOL(NSURL *localSelf, NSDictionary<NSURLResourceKey, id> *keyedValues, NSError **error) { *error = [NSError errorWithDomain:@"Error" code:0 userInfo:nil]; return NO; }); ``` and adds a large amount of runtime checking to verify safety. I can break this up into some smaller CLs if we agree that this is a reasonable direction to head in. I have tested the changes here extensively on all of our code at Google.
1 parent 6358799 commit 0ee6a61

15 files changed

+949
-49
lines changed

Source/OCMock.xcodeproj/project.pbxproj

-6
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
objects = {
88

99
/* Begin PBXBuildFile section */
10-
031E50581BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */; };
11-
031E50591BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */; };
1210
0322DA65191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */; };
1311
0322DA66191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */; };
1412
0322DA6919118B4600CACAF1 /* OCMVerifier.h in Headers */ = {isa = PBXBuildFile; fileRef = 0322DA6719118B4600CACAF1 /* OCMVerifier.h */; settings = {ATTRIBUTES = (Public, ); }; };
@@ -477,7 +475,6 @@
477475
030EF0B814632FD000B04273 /* OCMock.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = OCMock.h; sourceTree = "<group>"; };
478476
030EF0DC14632FF700B04273 /* libOCMock.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libOCMock.a; sourceTree = BUILT_PRODUCTS_DIR; };
479477
030EF0E114632FF700B04273 /* OCMockLib-Prefix.pch */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "OCMockLib-Prefix.pch"; sourceTree = "<group>"; };
480-
031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMBoxedReturnValueProviderTests.m; sourceTree = "<group>"; };
481478
0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMockObjectVerifyAfterRunTests.m; sourceTree = "<group>"; };
482479
0322DA6719118B4600CACAF1 /* OCMVerifier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OCMVerifier.h; sourceTree = "<group>"; };
483480
0322DA6819118B4600CACAF1 /* OCMVerifier.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMVerifier.m; sourceTree = "<group>"; };
@@ -765,7 +762,6 @@
765762
8BF73E52246CA75E00B9A52C /* OCMNoEscapeBlockTests.m */,
766763
03B316271463350E0052CD09 /* OCMStubRecorderTests.m */,
767764
037ECD5318FAD84100AF0E4C /* OCMInvocationMatcherTests.m */,
768-
031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */,
769765
03B316211463350E0052CD09 /* OCMConstraintTests.m */,
770766
8B11D4B62448E2E900247BE2 /* OCMCPlusPlus98Tests.mm */,
771767
8B11D4B92448E53600247BE2 /* OCMCPlusPlus11Tests.mm */,
@@ -1544,7 +1540,6 @@
15441540
8BF73E53246CA75E00B9A52C /* OCMNoEscapeBlockTests.m in Sources */,
15451541
8B11D4B72448E2E900247BE2 /* OCMCPlusPlus98Tests.mm in Sources */,
15461542
2FA28AB33F01A7D980F2C705 /* OCMockObjectDynamicPropertyMockingTests.m in Sources */,
1547-
031E50581BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */,
15481543
);
15491544
runOnlyForDeploymentPostprocessing = 0;
15501545
};
@@ -1637,7 +1632,6 @@
16371632
buildActionMask = 2147483647;
16381633
files = (
16391634
3C76716C1BB3EBC500FDC9F4 /* TestClassWithCustomReferenceCounting.m in Sources */,
1640-
031E50591BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */,
16411635
8B3786A924E5BD6400FD1B5B /* OCMFunctionsTests.m in Sources */,
16421636
03C9CA1E18F05A84006DF94D /* OCMArgTests.m in Sources */,
16431637
036865651D3571A8005E6BEE /* OCMQuantifierTests.m in Sources */,

Source/OCMock/NSMethodSignature+OCMAdditions.h

+4
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,8 @@
2626
- (NSString *)fullTypeString;
2727
- (const char *)fullObjCTypes;
2828

29+
// True if the return type of the method is "compatible" with the valueType and value.
30+
// Compatible is defined the same as `OCMIsObjCTypeCompatibleWithValueType`.
31+
- (BOOL)isMethodReturnTypeCompatibleWithValueType:(const char *)valueType value:(const void *)value valueSize:(size_t)valueSize;
32+
2933
@end

Source/OCMock/NSMethodSignature+OCMAdditions.m

+5
Original file line numberDiff line numberDiff line change
@@ -178,4 +178,9 @@ - (const char *)fullObjCTypes
178178
return [[self fullTypeString] UTF8String];
179179
}
180180

181+
- (BOOL)isMethodReturnTypeCompatibleWithValueType:(const char *)valueType value:(const void *)value valueSize:(size_t)valueSize
182+
{
183+
return OCMIsObjCTypeCompatibleWithValueType([self methodReturnType], valueType, value, valueSize);
184+
}
185+
181186
@end

Source/OCMock/OCMBlockCaller.h

+16-2
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,24 @@
1818

1919
@interface OCMBlockCaller : NSObject
2020
{
21-
void (^block)(NSInvocation *);
21+
id block;
2222
}
2323

24-
- (id)initWithCallBlock:(void (^)(NSInvocation *))theBlock;
24+
/*
25+
* Call blocks can have one of four types:
26+
* a) A simple block ^{ NSLog(@"hi"); }.
27+
* b) The new style ^(id localSelf, type0 arg0, type1 arg1) { ... }
28+
* where types and args match the arguments passed to the selector we are
29+
* stubbing.
30+
* c) The deprecated style ^(NSInvocation *anInvocation) { ... }. This case
31+
* cannot have a return value. If a return value is desired it must be set
32+
* on `anInvocation`.
33+
* d) nil
34+
*
35+
* If it is (a) or (b) and there is a return value it must match the return type
36+
* of the selector. If there is no return value then the method will return 0.
37+
*/
38+
- (id)initWithCallBlock:(id)theBlock;
2539

2640
- (void)handleInvocation:(NSInvocation *)anInvocation;
2741

Source/OCMock/OCMBlockCaller.m

+76-4
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
*/
1616

1717
#import "OCMBlockCaller.h"
18-
18+
#import "NSMethodSignature+OCMAdditions.h"
19+
#import "OCMFunctionsPrivate.h"
20+
#import "NSInvocation+OCMAdditions.h"
1921

2022
@implementation OCMBlockCaller
2123

22-
- (id)initWithCallBlock:(void (^)(NSInvocation *))theBlock
24+
-(id)initWithCallBlock:(id)theBlock
2325
{
2426
if((self = [super init]))
2527
{
@@ -37,9 +39,79 @@ - (void)dealloc
3739

3840
- (void)handleInvocation:(NSInvocation *)anInvocation
3941
{
40-
if(block != nil)
42+
if(!block)
43+
{
44+
return;
45+
}
46+
NSMethodSignature *blockMethodSignature = [NSMethodSignature signatureForBlock:block];
47+
NSUInteger blockNumberOfArguments = [blockMethodSignature numberOfArguments];
48+
if(blockNumberOfArguments == 2 && strcmp([blockMethodSignature getArgumentTypeAtIndex:1], "@\"NSInvocation\"") == 0)
49+
{
50+
// This is the deprecated ^(NSInvocation *) {} case.
51+
if([blockMethodSignature methodReturnLength] != 0)
52+
{
53+
[NSException raise:NSInvalidArgumentException format:@"NSInvocation style `andDo:` block for `-%@` cannot have return value.", NSStringFromSelector([anInvocation selector])];
54+
}
55+
56+
void (^theBlock)(NSInvocation *) = block;
57+
theBlock(anInvocation);
58+
NSLog(@"Warning: Replace `^(NSInvocation *invocation) { ... }` with `%@`.", OCMBlockDeclarationForInvocation(anInvocation));
59+
return;
60+
}
61+
62+
// This handles both the ^{} case and the ^(SelfType *a, Arg1Type b, ...) case.
63+
NSMethodSignature *invocationMethodSignature = [anInvocation methodSignature];
64+
NSInvocation *blockInvocation = [NSInvocation invocationWithMethodSignature:blockMethodSignature];
65+
NSUInteger invocationNumberOfArguments = [invocationMethodSignature numberOfArguments];
66+
if(blockNumberOfArguments != 1 && blockNumberOfArguments != invocationNumberOfArguments)
67+
{
68+
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature has wrong number of arguments. %d vs %d", (int)invocationNumberOfArguments, (int)blockNumberOfArguments];
69+
}
70+
id target = [anInvocation target];
71+
72+
// In the ^{} case, blockNumberOfArguments will be 1, so we will just skip the whole for block.
73+
for(NSUInteger argIndex = 1; argIndex < blockNumberOfArguments; ++argIndex)
74+
{
75+
// Set arg1 to be "localSelf".
76+
// Note that in a standard NSInvocation this would be SEL, but blocks don't have SEL args.
77+
if(argIndex == 1)
78+
{
79+
[blockInvocation setArgument:&target atIndex:1];
80+
continue;
81+
}
82+
const char *blockArgType = [blockMethodSignature getArgumentTypeAtIndex:argIndex];
83+
const char *invocationArgType = [invocationMethodSignature getArgumentTypeAtIndex:argIndex];
84+
NSUInteger argSize;
85+
NSGetSizeAndAlignment(blockArgType, &argSize, NULL);
86+
NSMutableData *argSpace = [NSMutableData dataWithLength:argSize];
87+
void *argBytes = [argSpace mutableBytes];
88+
[anInvocation getArgument:argBytes atIndex:argIndex];
89+
if(!OCMIsObjCTypeCompatibleWithValueType(invocationArgType, blockArgType, argBytes, argSize) && !OCMEqualTypesAllowingOpaqueStructs(blockArgType, invocationArgType))
90+
{
91+
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Arg %d is `%@` vs `%@`.", (int)argIndex, OCMObjCTypeForArgumentType(blockArgType), OCMObjCTypeForArgumentType(invocationArgType)];
92+
}
93+
[blockInvocation setArgument:argBytes atIndex:argIndex];
94+
}
95+
[blockInvocation invokeWithTarget:block];
96+
NSUInteger blockReturnLength = [blockMethodSignature methodReturnLength];
97+
if(blockReturnLength > 0)
4198
{
42-
block(anInvocation);
99+
// If there is a return value, make sure that it matches the expected return type.
100+
const char *blockReturnType = [blockMethodSignature methodReturnType];
101+
NSUInteger invocationReturnLength = [invocationMethodSignature methodReturnLength];
102+
const char *invocationReturnType = [invocationMethodSignature methodReturnType];
103+
if(invocationReturnLength != blockReturnLength)
104+
{
105+
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Return type is `%@` vs `%@`.", OCMObjCTypeForArgumentType(blockReturnType), OCMObjCTypeForArgumentType(invocationReturnType)];
106+
}
107+
NSMutableData *argSpace = [NSMutableData dataWithLength:invocationReturnLength];
108+
void *argBytes = [argSpace mutableBytes];
109+
[blockInvocation getReturnValue:argBytes];
110+
if(!OCMIsObjCTypeCompatibleWithValueType(invocationReturnType, blockReturnType, argBytes, invocationReturnLength) && !OCMEqualTypesAllowingOpaqueStructs(blockReturnType, invocationReturnType))
111+
{
112+
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Return type is `%@` vs `%@`.", OCMObjCTypeForArgumentType(blockReturnType), OCMObjCTypeForArgumentType(invocationReturnType)];
113+
}
114+
[anInvocation setReturnValue:argBytes];
43115
}
44116
}
45117

Source/OCMock/OCMBoxedReturnValueProvider.m

+6-20
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,7 @@
1616

1717
#import "OCMBoxedReturnValueProvider.h"
1818
#import "NSValue+OCMAdditions.h"
19-
#import "OCMFunctionsPrivate.h"
20-
19+
#import "NSMethodSignature+OCMAdditions.h"
2120

2221
@implementation OCMBoxedReturnValueProvider
2322

@@ -28,11 +27,11 @@ - (void)handleInvocation:(NSInvocation *)anInvocation
2827
NSGetSizeAndAlignment([returnValueAsNSValue objCType], &valueSize, NULL);
2928
char valueBuffer[valueSize];
3029
[returnValueAsNSValue getValue:valueBuffer];
30+
NSMethodSignature *signature = [anInvocation methodSignature];
31+
const char *returnType = [signature methodReturnType];
32+
const char *returnValueType = [returnValueAsNSValue objCType];
3133

32-
const char *returnType = [[anInvocation methodSignature] methodReturnType];
33-
34-
if([self isMethodReturnType:returnType compatibleWithValueType:[returnValueAsNSValue objCType]
35-
value:valueBuffer valueSize:valueSize])
34+
if([signature isMethodReturnTypeCompatibleWithValueType:returnValueType value:valueBuffer valueSize:valueSize])
3635
{
3736
[anInvocation setReturnValue:valueBuffer];
3837
}
@@ -43,21 +42,8 @@ - (void)handleInvocation:(NSInvocation *)anInvocation
4342
else
4443
{
4544
[NSException raise:NSInvalidArgumentException
46-
format:@"Return value cannot be used for method; method signature declares '%s' but value is '%s'.", returnType, [returnValueAsNSValue objCType]];
45+
format:@"Return value cannot be used for method; method signature declares '%s' but value is '%s'.", returnType, returnValueType];
4746
}
4847
}
4948

50-
- (BOOL)isMethodReturnType:(const char *)returnType compatibleWithValueType:(const char *)valueType value:(const void *)value valueSize:(size_t)valueSize
51-
{
52-
/* Same types are obviously compatible */
53-
if(strcmp(returnType, valueType) == 0)
54-
return YES;
55-
56-
/* Special treatment for nil and Nil */
57-
if(strcmp(returnType, @encode(id)) == 0 || strcmp(returnType, @encode(Class)) == 0)
58-
return OCMIsNilValue(valueType, value, valueSize);
59-
60-
return OCMEqualTypesAllowingOpaqueStructs(returnType, valueType);
61-
}
62-
6349
@end

0 commit comments

Comments
 (0)