Skip to content

Commit

Permalink
Improve andDo: type safety and simplify usage.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dmaclach committed Jan 2, 2021
1 parent c81c481 commit 6e0096b
Show file tree
Hide file tree
Showing 16 changed files with 950 additions and 176 deletions.
6 changes: 0 additions & 6 deletions Source/OCMock.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
objects = {

/* Begin PBXBuildFile section */
031E50581BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */; };
031E50591BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */; };
0322DA65191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */; };
0322DA66191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */; };
0322DA6919118B4600CACAF1 /* OCMVerifier.h in Headers */ = {isa = PBXBuildFile; fileRef = 0322DA6719118B4600CACAF1 /* OCMVerifier.h */; settings = {ATTRIBUTES = (Public, ); }; };
Expand Down Expand Up @@ -477,7 +475,6 @@
030EF0B814632FD000B04273 /* OCMock.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = OCMock.h; sourceTree = "<group>"; };
030EF0DC14632FF700B04273 /* libOCMock.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libOCMock.a; sourceTree = BUILT_PRODUCTS_DIR; };
030EF0E114632FF700B04273 /* OCMockLib-Prefix.pch */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "OCMockLib-Prefix.pch"; sourceTree = "<group>"; };
031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMBoxedReturnValueProviderTests.m; sourceTree = "<group>"; };
0322DA64191188D100CACAF1 /* OCMockObjectVerifyAfterRunTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMockObjectVerifyAfterRunTests.m; sourceTree = "<group>"; };
0322DA6719118B4600CACAF1 /* OCMVerifier.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OCMVerifier.h; sourceTree = "<group>"; };
0322DA6819118B4600CACAF1 /* OCMVerifier.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = OCMVerifier.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -765,7 +762,6 @@
8BF73E52246CA75E00B9A52C /* OCMNoEscapeBlockTests.m */,
03B316271463350E0052CD09 /* OCMStubRecorderTests.m */,
037ECD5318FAD84100AF0E4C /* OCMInvocationMatcherTests.m */,
031E50571BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m */,
03B316211463350E0052CD09 /* OCMConstraintTests.m */,
8B11D4B62448E2E900247BE2 /* OCMCPlusPlus98Tests.mm */,
8B11D4B92448E53600247BE2 /* OCMCPlusPlus11Tests.mm */,
Expand Down Expand Up @@ -1544,7 +1540,6 @@
8BF73E53246CA75E00B9A52C /* OCMNoEscapeBlockTests.m in Sources */,
8B11D4B72448E2E900247BE2 /* OCMCPlusPlus98Tests.mm in Sources */,
2FA28AB33F01A7D980F2C705 /* OCMockObjectDynamicPropertyMockingTests.m in Sources */,
031E50581BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down Expand Up @@ -1637,7 +1632,6 @@
buildActionMask = 2147483647;
files = (
3C76716C1BB3EBC500FDC9F4 /* TestClassWithCustomReferenceCounting.m in Sources */,
031E50591BB4A56300E257C3 /* OCMBoxedReturnValueProviderTests.m in Sources */,
8B3786A924E5BD6400FD1B5B /* OCMFunctionsTests.m in Sources */,
03C9CA1E18F05A84006DF94D /* OCMArgTests.m in Sources */,
036865651D3571A8005E6BEE /* OCMQuantifierTests.m in Sources */,
Expand Down
4 changes: 4 additions & 0 deletions Source/OCMock/NSMethodSignature+OCMAdditions.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,8 @@
- (NSString *)fullTypeString;
- (const char *)fullObjCTypes;

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

@end
5 changes: 5 additions & 0 deletions Source/OCMock/NSMethodSignature+OCMAdditions.m
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,9 @@ - (const char *)fullObjCTypes
return [[self fullTypeString] UTF8String];
}

- (BOOL)isMethodReturnTypeCompatibleWithValueType:(const char *)valueType value:(const void *)value valueSize:(size_t)valueSize
{
return OCMIsObjCTypeCompatibleWithValueType([self methodReturnType], valueType, value, valueSize);
}

@end
18 changes: 16 additions & 2 deletions Source/OCMock/OCMBlockCaller.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,24 @@

@interface OCMBlockCaller : NSObject
{
void (^block)(NSInvocation *);
id block;
}

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

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

Expand Down
80 changes: 76 additions & 4 deletions Source/OCMock/OCMBlockCaller.m
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
*/

#import "OCMBlockCaller.h"

#import "NSMethodSignature+OCMAdditions.h"
#import "OCMFunctionsPrivate.h"
#import "NSInvocation+OCMAdditions.h"

@implementation OCMBlockCaller

-(id)initWithCallBlock:(void (^)(NSInvocation *))theBlock
-(id)initWithCallBlock:(id)theBlock
{
if ((self = [super init]))
{
Expand All @@ -37,10 +39,80 @@ -(void)dealloc

- (void)handleInvocation:(NSInvocation *)anInvocation
{
if (block != nil)
if(!block)
{
return;
}
NSMethodSignature *blockMethodSignature = [NSMethodSignature signatureForBlock:block];
NSUInteger blockNumberOfArguments = [blockMethodSignature numberOfArguments];
if(blockNumberOfArguments == 2 && strcmp([blockMethodSignature getArgumentTypeAtIndex:1], "@\"NSInvocation\"") == 0)
{
// This is the deprecated ^(NSInvocation *) {} case.
if([blockMethodSignature methodReturnLength] != 0)
{
[NSException raise:NSInvalidArgumentException format:@"NSInvocation style `andDo:` block for `-%@` cannot have return value.", NSStringFromSelector([anInvocation selector])];
}

void (^theBlock)(NSInvocation *) = block;
theBlock(anInvocation);
NSLog(@"Warning: Replace `^(NSInvocation *invocation) { ... }` with `%@`.", OCMBlockDeclarationForInvocation(anInvocation));
return;
}

// This handles both the ^{} case and the ^(SelfType *a, Arg1Type b, ...) case.
NSMethodSignature *invocationMethodSignature = [anInvocation methodSignature];
NSInvocation *blockInvocation = [NSInvocation invocationWithMethodSignature:blockMethodSignature];
NSUInteger invocationNumberOfArguments = [invocationMethodSignature numberOfArguments];
if(blockNumberOfArguments != 1 && blockNumberOfArguments != invocationNumberOfArguments)
{
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature has wrong number of arguments. %d vs %d", (int)invocationNumberOfArguments, (int)blockNumberOfArguments];
}
id target = [anInvocation target];

// In the ^{} case, blockNumberOfArguments will be 1, so we will just skip the whole for block.
for(NSUInteger argIndex = 1; argIndex < blockNumberOfArguments; ++argIndex)
{
// Set arg1 to be "localSelf".
// Note that in a standard NSInvocation this would be SEL, but blocks don't have SEL args.
if(argIndex == 1)
{
[blockInvocation setArgument:&target atIndex:1];
continue;
}
const char *blockArgType = [blockMethodSignature getArgumentTypeAtIndex:argIndex];
const char *invocationArgType = [invocationMethodSignature getArgumentTypeAtIndex:argIndex];
NSUInteger argSize;
NSGetSizeAndAlignment(blockArgType, NULL, &argSize);
NSMutableData *argSpace = [NSMutableData dataWithLength:argSize];
void *argBytes = [argSpace mutableBytes];
[anInvocation getArgument:argBytes atIndex:argIndex];
if(!OCMIsObjCTypeCompatibleWithValueType(invocationArgType, blockArgType, argBytes, argSize) && !OCMEqualTypesAllowingOpaqueStructs(blockArgType, invocationArgType))
{
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Arg %d is `%@` vs `%@`.", (int)argIndex, OCMObjCTypeForArgumentType(blockArgType), OCMObjCTypeForArgumentType(invocationArgType)];
}
[blockInvocation setArgument:argBytes atIndex:argIndex];
}
[blockInvocation invokeWithTarget:block];
NSUInteger blockReturnLength = [blockMethodSignature methodReturnLength];
if(blockReturnLength > 0)
{
// If there is a return value, make sure that it matches the expected return type.
const char *blockReturnType = [blockMethodSignature methodReturnType];
NSUInteger invocationReturnLength = [invocationMethodSignature methodReturnLength];
const char *invocationReturnType = [invocationMethodSignature methodReturnType];
if(invocationReturnLength != blockReturnLength)
{
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Return type is `%@` vs `%@`.", OCMObjCTypeForArgumentType(blockReturnType), OCMObjCTypeForArgumentType(invocationReturnType)];
}
NSMutableData *argSpace = [NSMutableData dataWithLength:invocationReturnLength];
void *argBytes = [argSpace mutableBytes];
[blockInvocation getReturnValue:argBytes];
if(!OCMIsObjCTypeCompatibleWithValueType(invocationReturnType, blockReturnType, argBytes, invocationReturnLength) && !OCMEqualTypesAllowingOpaqueStructs(blockReturnType, invocationReturnType))
{
block(anInvocation);
[NSException raise:NSInvalidArgumentException format:@"Block style `andDo:` block signature does not match selector signature. Return type is `%@` vs `%@`.", OCMObjCTypeForArgumentType(blockReturnType), OCMObjCTypeForArgumentType(invocationReturnType)];
}
[anInvocation setReturnValue:argBytes];
}
}

@end
26 changes: 6 additions & 20 deletions Source/OCMock/OCMBoxedReturnValueProvider.m
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#import "OCMBoxedReturnValueProvider.h"
#import "OCMFunctionsPrivate.h"
#import "NSValue+OCMAdditions.h"

#import "NSMethodSignature+OCMAdditions.h"

@implementation OCMBoxedReturnValueProvider

Expand All @@ -28,11 +28,11 @@ - (void)handleInvocation:(NSInvocation *)anInvocation
NSGetSizeAndAlignment([returnValueAsNSValue objCType], &valueSize, NULL);
char valueBuffer[valueSize];
[returnValueAsNSValue getValue:valueBuffer];
NSMethodSignature *signature = [anInvocation methodSignature];
const char *returnType = [signature methodReturnType];
const char *returnValueType = [returnValueAsNSValue objCType];

const char *returnType = [[anInvocation methodSignature] methodReturnType];

if([self isMethodReturnType:returnType compatibleWithValueType:[returnValueAsNSValue objCType]
value:valueBuffer valueSize:valueSize])
if([signature isMethodReturnTypeCompatibleWithValueType:returnValueType value:valueBuffer valueSize:valueSize])
{
[anInvocation setReturnValue:valueBuffer];
}
Expand All @@ -43,22 +43,8 @@ - (void)handleInvocation:(NSInvocation *)anInvocation
else
{
[NSException raise:NSInvalidArgumentException
format:@"Return value cannot be used for method; method signature declares '%s' but value is '%s'.", returnType, [returnValueAsNSValue objCType]];
format:@"Return value cannot be used for method; method signature declares '%s' but value is '%s'.", returnType, returnValueType];
}
}

- (BOOL)isMethodReturnType:(const char *)returnType compatibleWithValueType:(const char *)valueType value:(const void *)value valueSize:(size_t)valueSize
{
/* Same types are obviously compatible */
if(strcmp(returnType, valueType) == 0)
return YES;

/* Special treatment for nil and Nil */
if(strcmp(returnType, @encode(id)) == 0 || strcmp(returnType, @encode(Class)) == 0)
return OCMIsNilValue(valueType, value, valueSize);

return OCMEqualTypesAllowingOpaqueStructs(returnType, valueType);
}


@end
Loading

0 comments on commit 6e0096b

Please sign in to comment.