Improve andDo: type safety and simplify usage.#470
Open
dmaclach wants to merge 1 commit intoerikdoe:masterfrom
Open
Improve andDo: type safety and simplify usage.#470dmaclach wants to merge 1 commit intoerikdoe:masterfrom
andDo: type safety and simplify usage.#470dmaclach wants to merge 1 commit intoerikdoe:masterfrom
Conversation
a77b540 to
8b0dc63
Compare
7d25e6c to
ba2c837
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current
andDoblocks 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_unretainedon arguments, or risk overretaining/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 andgives 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 codeverifies 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, ...)wherereturnTypeand the argument listmatch 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
NSLoga warning about the deprecated block type, and will attempt tosuggest a good block declaration to replace it with. They would look something like this:
This changes code that previously would have looked like this:
to
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.