Improve Factory Provider with libffi #102

drodriguez · 2013-11-15T16:41:18Z

The Factory Provider should allow to type a lot less of code. Currently the user has to manually provide a lot of code blocks for each of the factory methods. It maybe possible to do more work automatically for the user, at least for the simple cases.

Example

For a class and a factory like the following:

@interface Payment : NSObject
- (instancetype)initWithCreditService:(id<CreditService>)creditService
                          authService:(id<AuthService>)authService
                            startDate:(NSDate *)date
                                amout:(NSUInteger)amount;
@end

@protocol PaymentFactory <NSObject>
@property (nonatomic, strong, readonly) id<CreditService> creditService;
@property (nonatomic, strong, readonly) id<AuthService> authService;
+ (Payment *)paymentWithStartDate:(NSDate *)startDate
                           amount:(NSUInteger)amount;
@end

I want to be able to generate the PaymentFactory implementation with the following
definition.

- (id)paymentFactory {
  return [TyphoonFactoryProvider withProtocol:@protocol(PaymentFactory) dependencies:^(TyphoonDefinition *definition) {
    [definition injectProperty:@selector(creditService)];
    [definition injectProperty:@selector(authService)];
  } returningType:[Payment class]];
}

Using some conventions (which are pretty usual), the implementation of paymentWithStartDate:amount: should be a little bit of argument shuffling and forwarding to the Payment class.

Explanation

For this simple factories we need first to map the relation between the factory method and the constructor to be used.

@selector(paymentWithStartDate:amount:) ---> @selector(initWithCreditService:authService:startDate:amount:)

We will also need a way to map the relation between input arguments/properties and the initializer arguments.

1st argument -> property "creditService"
2nd argument -> property "authService"
3rd argument -> 1st input argument
4th argument -> 2nd input argument

(There will be another kind of argument “constant”)

The argument types must be the same for input and initializer, so the code doesn’t do any transformation.

Something like this could be the internal API (that might be also exposed):

[[definition factoryMethod:@selector(paymentWithStartDate:amount:) type:[Payment class]]
    initWithCreditService:TyphoonFactoryInjectProperty(creditService)
    authService:TyphoonFactoryInjectProperty(authService)
    startDate:TyphoonFactoryInjectArgument(0)
    amout:TyphoonFactoryInjectArgument(1)];

(Those “functions” are actually macros which will create the right classes, something like TyphoonFactoryProviderInvocationArgument. Also, I will use a proxy to allow the
inline definition of the arguments. I can also implement the more Typhoon-esque way, if people prefer that:)

// Alternative Typhoon-esque way?
[[definition factoryMethod:@selector(paymentWithStartDate:amount:)
  type:[Payment class]
  withInitializer:^(TyphoonFactoryInitializerBlock *initializer) {
    initializer.selector = @selector(initWithCreditService:authService:startDate:amount:);
    [initializer injectWithProperty:@selector(creditService)];
    [initializer injectWithProperty:@selector(authService)];
    [initializer injectWithArgumentAtIndex:0];
    [initializer injectWithArgumentAtIndex:1];
}];

(This is very similar to TyphoonInitializerBlock, but it cannot be the same, so that’s my main reason to not follow Typhoon-esque patterns for this, but it can share a lot of code, but need two totally different set of valid “injections”. Anyway, that’s a lot more typing than the block, so people will probably not use it).

After finishing the definition block, there will be a couple of checks (number of factory methods defined, methods existing in protocols, initializer existing in classes, right number of arguments…). All those checks will fail at runtime, probably.

Automatic wiring

The simplest case is one factory method and one initializer. We could have supposed that the input arguments are in the same order than the output arguments, but since the properties of a class do not specify the order in which they are returned, we cannot use the order for the first arguments. In the end is better, because we use a more general solution, which solves a lot more situations.

The idea is “parsing” the usual conventions for the initializer and the factory methods used in Cocoa. We will have a set of rules that will be used. If you follow other rules, or your factory method/initializer needs another name, no problem, you can always use the “manual” mapping definition.

For the factory methods we will remote everything until the first “With” and convert every atom of the selector to lower camelCase removing the colons (so paymentWithStartDate:amount: will end up as @[@"startDate", @"amount"]). For the initializer is the same, but we take out the “initWith” prefix (so initWithCreditService:authService:startDate:amount: ends up as @[@"creditService", @"authService", @"startDate", @"amount"]).

At this moment, we will try to create the mappings comparing the name of the atoms. If there is still mappings to fill, we will then look at the property names (that way and argument with the same name as a property will override the property, which I think is expected).

In our running example, first we will assign input argument at index 0 with output argument at index 2, and input argument at index 1 with output argument at index 3. Since two arguments are still not set, we will look at the properties and fill property “creditService” with output argument at index 0, and “authService” with output argument at index 1.

For the case of several factory methods and initializer, I suppose one can follow the same pattern, testing which one of the initializers fits better the factory method. However I going to left that as a low priority requirement.

As you can see the “conventions” are not that strange (if your own factory methods and initializers use different namings, you have bigger problems). Using the same name for the properties and the selector atoms is not that strange, either.

Ending notes, questions & comments

I will start developing in drodriguez/Typhoon following the branch factory-providers. I will probably keep an ongoing pull request in this repository, so we all can comment.

We need libffi. I have found that https://github.com/zwaldowski/libffi-iOS offers a podspec, so I will need to include a dependency in Typhoon podspec. I suppose that’s not a problem (I don’t want to namespace all libffi).

Any questions, suggestions, ideas, or things I might overlook?

Tasks

Integrate with libffi (https://github.com/zwaldowski/libffi-iOS)
Create classes supporting definition of the mappings
Implement libffi code to forward arguments to the right functions.
Implement heuristics for automatic wiring of simple cases.
Implement heuristics for automatic wiring of complex cases (optional).

The text was updated successfully, but these errors were encountered:

jasperblues · 2013-11-16T01:37:24Z

Brilliant work Daniel! I would love to see Typhoon support this.

jasperblues · 2013-11-16T02:12:44Z

Oops. . . I was going to say I liked the first more compact definition, but having re-read it, I think the Typhoon-esque one makes more sense, at least for the sake of uniformity.

You can decide though - whatever you think is best.

drodriguez · 2013-11-16T11:49:29Z

OK, I will go with Typhoon-esque version.

About the question in the previous version of the comment (I have it in the email)...

In fact, for the Typhoon factories without arguments (the v1.0 feature) we use the term 'initializer' to specify both that or a factory method. . . this works fine, but could be confusing. . . This new factory feature would be able to deprecate that right? Because you could also define a factory with zero argument mappings?

I think of this as a “plugin” for Typhoon. Like Assisted Injection is for Typhoon (Dagger, for example, do not have this feature). I will not like to deprecate anything, if possible. Anyway, maybe there is some confusion here, because I used definition on the code blocks, but there are two definition at work here: for the “dependencies” block it is the standard TyphoonDefinitionBlock while the other one is a TyphoonAssistedFactoryDefinitionBlock with no methods in common with the former.

jasperblues · 2013-11-16T12:46:39Z

Sounds great! I'm really looking forward to this feature.

Hey, just another note. . . in the original discussions we talked about using the Assembly interface itself as the factory . . for example instead of defining a normal method like

-(id) knight
{
   //do definition here
}

we define a factory method right there on the assembly, eg:

- (id) paymentWithStartDate:(NSDate *)startDate amount:(NSUInteger)amount
{
    //do definition here
}

You don't have to do it this way of course, it was just something we were thinking about.

It took a way longer than I expected. Basically a lot of files that use to sit at Tests/ now sit at the root of the project (.ruby-version, Gemfile, Podfile...), also the Pods directory has moved. The Podfile contains two more targets for the static library which point to the podspec (to have access to libffi). There is some minor changes in the ARCHS, HEADERS and LDFLAGS of the main project and the testing projects. I hope I haven't broken anything. There is a hack in the post_install hook of the Podfile to avoid Cocoapods from overwritting some libffi iOS headers with their OS X versions. I might need to send this patch to either Cocoapods or libffi, because I think projects depending on Typhoon might have problems. Also: it seems that OCMockito and OCHamcrest have been updated. (appsquickly#102)

Modify TyphoonFactoryProvider to delegate the actual work of creating the method from the protocol to TyphoonAssistedFactoryMethodCreator. TyphoonAssistedFactoryDefinition stores subclasses of TyphoonAssistedFactoryMethod instead of the previous array pair. The block are now stored in TyphoonAssistedFactoryMethodBlock, where the actual work is done. Similarly, TyphoonAssistedFactoryMethodInitializer holds the new code to configure the mapping between the factory method and the initializer. A lot of little classes help (TAFInjectedParameter, TAFParameterInjectedWithProperty, TAFParameterInjectedWithArgumentIndex...). The TAFMethodInitializerCreator is currently unimplemented. (appsquickly#102)

TAFMethodInitializerCreator will be split in several smaller files. InitializerCreator creates TAFMethodClosures and stores them in a static global NSDictionary. The closures need to live while the methods can be called and could be a memory "leak" if the user decides to remove those methods or entire classes from the runtime. Not very usual, so I think the solution is "good enough". TAFMethodClousure is where the work is done. The class describes both the factory method and the initializer to libffi, and creates a closure that ties them together. Many lines are taken and slightly adapted from Michael Ash amazing MABlockClosure. There is a small "proof of concept" test for the InitializerCreator. Fix a copy/paste bug in the TAFMethodCreator convenience method. TAFMethodBlockCreator and TAFMethodInitializerCreator share the code to look for a ObjC method description. Return the TAFMethodInitializer parameter descriptions sorted according to their parameterIndex. Update Pods. TODO: lot of error checking TODO: lot of commenting (appsquickly#102)

drodriguez · 2013-11-25T16:45:06Z

Well, after a little fighting with libffi, and understanding how many pointers pointing to pointers you can create without going crazy, I think I have created the first “complete” solution.

There is a little explanation inside the code, and in the commit message, but maybe it is a little bit rough at this moment. I will provide more tests, documentation and comments when I have some more time.

Problems:

libffi CocoaPods do not support iOS 64 bits, but I think they have ARM64 support, so maybe is a little modification.
I haven’t actually tested this in iOS… nor x86 architecture (which at this point in time is not that important). Only x86_64. I don't think libffi will be broken for those architectures, but it will be nice to test it… maybe when the test suite for this part is more complete.
MABlockClosure code is BSD, while Typhoon is Apache. I think there is no incompatibility, as long as we include the copyright and licensing terms of the original code (BSD vs. Apache).

drodriguez · 2013-11-25T16:49:37Z

Sorry for not comming back earlier. About your comment of having parameters in the assembly methods: I am not aiming for that, which will probably imply huge changes all around Typhoon. My idea of factories are more “external” to the assembly, and I actually think I haven’t had the need of modifying Typhoon at all until now. I don’t think my code could be useful for those kind of methods, either, becuase the only thing that happens inside the assembly is the wiring of the methods, not the invocation of the methods themselves.

jasperblues · 2013-11-26T01:40:38Z

Great work Daniel! . . this has been a really tricky feature, but it will be so useful.

Re "the only thing that happens inside the assembly is the wiring of the methods/components" - yes that is true, although we do use the assembly interface to pull components from the factory - using a very simple re-route from method name to componentForKey(methodName) . . .

jasperblues · 2013-11-28T03:17:32Z

Where is the code for this? I'd like to try it out.

drodriguez · 2013-11-28T09:29:23Z

It’s on my repository https://github.com/drodriguez/Typhoon/tree/factory-provider , since I expect to rebase before finishing and I didn’t want to do that in the main repo.

jasperblues · 2013-11-28T10:07:14Z

Ok - found it. . . Looking great!

jasperblues · 2013-11-28T10:07:34Z

ps: We'll make the new shared Typhoon org page on Github soon.

Create separate file for TyphoonAssistedFactoryMethodClosure. Validation of initializer and parameters during the construction of the closure. Should catch some typing mistakes. Use parameterIndex instead of the iteration index for filling the arguments of the closure. Fix a leak in typesWithEncodingString:count:. Use allocate: instead of calloc. Use the factory method name instead of the selector name to create the keys for the closure cache. You can have several factory methods calling the same initializer, but only one factory method definition. Added more complete TyphoonAssistedFactoryMethodClosureTest, testing invocation with parameters and properties more or less in isolation. TyphoonFactoryProviderTest duplicates the protocols before using them, so the factory implementations are different for blocks and initializers. There is a new test that does the same as the blocks one, but with initializers. (appsquickly#102)

A lot of code that used to be in TyphoonFactoryProvider is now in TyphoonAssistedFactoryCreator and its subclasses (one for one factory block, one for a definition block, and one for the future implicit with a result type). The subclasses are supposed to generate a block that returns a TyphoonAssistedFactoryDefinition, which the super class will use in case the class doesn't exists yet (using a block to not instantiate the definition eagerly). TODO: the implicit creation strategy. (appsquickly#102)

(appsquickly#102)

Recover libxml headers in OS X target. While including libffi as Cocoapod I must have forgot to include this back in the OS X target (it is there in the iOS target). A couple of imports to avoid warnings and to correctly compile in iOS. Marking the temporal allocations array of the closure as unused and with precise lifetime. The unused is necessary to avoid a warning, the precise lifetime might not be necessary, but I fear the optimizer might optimize the variable away and break everything. (appsquickly#102)

@Private

The method GuessFactoryMethodForProtocol moves from the OneFactory to the base class, since the implicit creator also needs it. The _protocol ivar moves from the @Private to @Protected in the Private.h header, so the implicit creator can access it. (appsquickly#102)

Added a bunch of bogus init methods to the PaymentImpl, to better test the guessing of the initializer for the factory method. (appsquickly#102)

drodriguez · 2013-12-01T23:43:59Z

I’m dealing with the problem libffi pod had when using both OS X and iOS in the same project. My proposal is at zwaldowski/libffi-iOS#1 and at atgreen/libffi#61. Anyway, it will be good to also wait for at atgreen/libffi#60 (already merged) to be released, which includes ARM64 support for libffi (even if I cannot test on a real device). Hopefully both changes get released in the next libffi version.

jasperblues · 2013-12-01T23:57:59Z

Sounds good. . .

I can test it out on-device for you - let me know when you're ready. I didn't have any iOS7 iphone-sized devices, so I'm getting a 5s in a few days . . .

drodriguez · 2013-12-04T21:26:05Z

I just found out that we were a little bit too eager using libffi, when Apple provides NSInvocation for almost the same thing. I have done a quick test, and I have it working with NSInvocation, which is great because I don’t think that Apple will break NSInvocation in any of the platforms.

Most of the code works, but I have to redo the branch, because I started it changing everything to support CocoaPods also in the main project, not only in test.

jasperblues · 2013-12-04T23:38:04Z

Ha! oops - it looks like I sent you on a "wild goose chase" there. . . When we talked about using the TyphoonAssembly interface as a factory provider itself it looked as though we'd probably need libffi. .

I tried doing it with NSInvocation but ran into some problems. . . I don't recall what they were now. . Maybe it would have actually worked.

In any case - fantastic work!

I'm glad its almost there. I hope you at least found the libffi trip useful. . I still plan to check out what you did there because its quite interesting. .

Sorry again for sending you on the scenic route!

Modify TyphoonFactoryProvider to delegate the actual work of creating the method from the protocol to TyphoonAssistedFactoryMethodCreator. TyphoonAssistedFactoryDefinition stores subclasses of TyphoonAssistedFactoryMethod instead of the previous array pair. The block are now stored in TyphoonAssistedFactoryMethodBlock, where the actual work is done. Similarly, TyphoonAssistedFactoryMethodInitializer holds the new code to configure the mapping between the factory method and the initializer. A lot of little classes help (TAFInjectedParameter, TAFParameterInjectedWithProperty, TAFParameterInjectedWithArgumentIndex...). The TAFMethodInitializerCreator is currently unimplemented. (appsquickly#102)

TAFMethodInitializerCreator will be split in several smaller files. InitializerCreator creates TAFMethodClosures and stores them in a static global NSDictionary. The closures need to live while the methods can be called and could be a memory "leak" if the user decides to remove those methods or entire classes from the runtime. Not very usual, so I think the solution is "good enough". TAFMethodClousure is where the work is done. The class describes both the factory method and the initializer to libffi, and creates a closure that ties them together. Many lines are taken and slightly adapted from Michael Ash amazing MABlockClosure. There is a small "proof of concept" test for the InitializerCreator. Fix a copy/paste bug in the TAFMethodCreator convenience method. TAFMethodBlockCreator and TAFMethodInitializerCreator share the code to look for a ObjC method description. Return the TAFMethodInitializer parameter descriptions sorted according to their parameterIndex. Update Pods. TODO: lot of error checking TODO: lot of commenting (appsquickly#102)

Create separate file for TyphoonAssistedFactoryMethodClosure. Validation of initializer and parameters during the construction of the closure. Should catch some typing mistakes. Use parameterIndex instead of the iteration index for filling the arguments of the closure. Fix a leak in typesWithEncodingString:count:. Use allocate: instead of calloc. Use the factory method name instead of the selector name to create the keys for the closure cache. You can have several factory methods calling the same initializer, but only one factory method definition. Added more complete TyphoonAssistedFactoryMethodClosureTest, testing invocation with parameters and properties more or less in isolation. TyphoonFactoryProviderTest duplicates the protocols before using them, so the factory implementations are different for blocks and initializers. There is a new test that does the same as the blocks one, but with initializers. (appsquickly#102) Conflicts: Pods/Pods.xcodeproj/project.pbxproj

A lot of code that used to be in TyphoonFactoryProvider is now in TyphoonAssistedFactoryCreator and its subclasses (one for one factory block, one for a definition block, and one for the future implicit with a result type). The subclasses are supposed to generate a block that returns a TyphoonAssistedFactoryDefinition, which the super class will use in case the class doesn't exists yet (using a block to not instantiate the definition eagerly). TODO: the implicit creation strategy. (appsquickly#102) Conflicts: Pods/Pods.xcodeproj/project.pbxproj

(appsquickly#102)

Recover libxml headers in OS X target. While including libffi as Cocoapod I must have forgot to include this back in the OS X target (it is there in the iOS target). A couple of imports to avoid warnings and to correctly compile in iOS. Marking the temporal allocations array of the closure as unused and with precise lifetime. The unused is necessary to avoid a warning, the precise lifetime might not be necessary, but I fear the optimizer might optimize the variable away and break everything. (appsquickly#102)

@Private

The method GuessFactoryMethodForProtocol moves from the OneFactory to the base class, since the implicit creator also needs it. The _protocol ivar moves from the @Private to @Protected in the Private.h header, so the implicit creator can access it. (appsquickly#102)

Added a bunch of bogus init methods to the PaymentImpl, to better test the guessing of the initializer for the factory method. (appsquickly#102)

Not depending on libffi and depending on Apple forwarding mechanism assure us that they are not going to break it badly suddenly. The code is also a lot simpler. (appsquickly#102)

drodriguez · 2013-12-05T00:45:05Z

Well, it was not a wild goose chase, because it actually worked. And I have to say that the scenic route was beautiful. libffi is small but powerful. I left that work in another branch in my repository. I don't think it deserves to be “forgotten”.

Let me do a re-check when I am a little more awake, and I will ask for the pull request.

jasperblues · 2013-12-08T03:07:41Z

Let's close this and raise new specific issues for the new TyphoonFactoryProvider

Removing one more restriction from the TyphoonFactoryProvider. Now you can have factories with several methods implicitly matched with the best init methods from the return type. The matching is done using the Stable Marriage algorithm, a little modified to left init methods without matching, if needed. See #102 Also: - Updated the documentation. - Fixed a bug where the dependency properties could not be nil (strange for them to be, but anyway).

ghost assigned drodriguez Nov 15, 2013

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Nov 29, 2013

Split TyphoonFactoryProviderTest too.

653b1f3

(appsquickly#102)

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Nov 29, 2013

Factory Creator Implicit tests.

0dcc8cf

Added a bunch of bogus init methods to the PaymentImpl, to better test the guessing of the initializer for the factory method. (appsquickly#102)

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Dec 5, 2013

Split TyphoonFactoryProviderTest too.

9cb0b02

(appsquickly#102)

drodriguez added a commit to drodriguez/Typhoon that referenced this issue Dec 5, 2013

Factory Creator Implicit tests.

8a893c7

Added a bunch of bogus init methods to the PaymentImpl, to better test the guessing of the initializer for the factory method. (appsquickly#102)

drodriguez mentioned this issue Dec 5, 2013

Factory provider #114

Merged

jasperblues closed this as completed Dec 8, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Factory Provider with libffi #102

Improve Factory Provider with libffi #102

drodriguez commented Nov 15, 2013

jasperblues commented Nov 16, 2013

jasperblues commented Nov 16, 2013

drodriguez commented Nov 16, 2013

jasperblues commented Nov 16, 2013

drodriguez commented Nov 25, 2013

drodriguez commented Nov 25, 2013

jasperblues commented Nov 26, 2013

jasperblues commented Nov 28, 2013

drodriguez commented Nov 28, 2013

jasperblues commented Nov 28, 2013

jasperblues commented Nov 28, 2013

drodriguez commented Dec 1, 2013

jasperblues commented Dec 1, 2013

drodriguez commented Dec 4, 2013

jasperblues commented Dec 4, 2013

drodriguez commented Dec 5, 2013

jasperblues commented Dec 8, 2013

Improve Factory Provider with libffi #102

Improve Factory Provider with libffi #102

Comments

drodriguez commented Nov 15, 2013

Example

Explanation

Automatic wiring

Ending notes, questions & comments

Tasks

jasperblues commented Nov 16, 2013

jasperblues commented Nov 16, 2013

drodriguez commented Nov 16, 2013

jasperblues commented Nov 16, 2013

drodriguez commented Nov 25, 2013

drodriguez commented Nov 25, 2013

jasperblues commented Nov 26, 2013

jasperblues commented Nov 28, 2013

drodriguez commented Nov 28, 2013

jasperblues commented Nov 28, 2013

jasperblues commented Nov 28, 2013

drodriguez commented Dec 1, 2013

jasperblues commented Dec 1, 2013

drodriguez commented Dec 4, 2013

jasperblues commented Dec 4, 2013

drodriguez commented Dec 5, 2013

jasperblues commented Dec 8, 2013