Introducing a less noisy way to describe definitions (TyphoonDefinitionProxy)

[nickynick]

Hey guys!

We have a pretty big project that uses Typhoon extensively - that is, we have about 20 assemblies each containing dozens of definitions. This is a sizeable amount of code which, quite frankly, doesn't always look friendly. Consider this example taken from the real project:

- (PXSession *)session {
    return [TyphoonDefinition withClass:[PXSession class] configuration:^(TyphoonDefinition *definition) {
        [definition useInitializer:@selector(initWithCore:requestSender:imageCacheFactory:URLSessionManager:reachability:tutorialScenarioManager:) parameters:^(TyphoonMethod *initializer) {
            [initializer injectParameterWith:[self core]];
            [initializer injectParameterWith:[self requestSender]];
            [initializer injectParameterWith:self];
            [initializer injectParameterWith:[self URLSessionManager]];
            [initializer injectParameterWith:[self reachability]];
            [initializer injectParameterWith:[self.tutorialAssembly scenarioManager]];
        }];

        [definition injectProperty:@selector(analyticsManager) with:[self.analyticsAssembly analyticsManager]];
    }];    
}

Definitely not the worst piece of code, but it's far from being readable because arguments are separated from the initializer. It also makes it harder for changing and refactoring, and so on.

So I figured out it can be done in a cleaner way :) Consider this:

- (PXSession *)session {
    PXSession *session = [[PXSession typhoonDefinition] initWithCore:[self core]
                                                       requestSender:[self requestSender]
                                                   imageCacheFactory:self
                                                   URLSessionManager:[self URLSessionManager]
                                                        reachability:[self reachability]
                                             tutorialScenarioManager:[self.tutorialAssembly scenarioManager]];

    session.analyticsManager = [self.analyticsAssembly analyticsManager];

    return session;
}

This looks exactly like your normal code. Also, a compiler gets to do it job properly in checking for methods and properties.

I've made an implementation for this, introducing TyphoonDefinitionProxy class. It's pretty straightforward: it collects invocations and converts them to definition configuration blocks, which are later used to build a proper definition. I had to add a check for this new class in the place where TyphoonAssemblyDefinitionBuilder fetches a definition - not the best thing to do, but unfortunately I can't see a better way to do so because there is no protocol for this and I don't feel authorized to add it :) I also added a couple of tests, not sure if I chose a correct place for them though.

This same machinery can be used for factory definitions too - not done yet, but if you like the whole thing, I don't see why not.

The only caveat for this comes from the limitation Typhoon currently has - that is, lacking support for scalars. You can see in the tests that I have to casts objects to scalar integers, and this is uncool. To make things worse, it doesn't work for scalar properties at all, because of the magic of automatic unboxing. I don't see why there is a such limitation in the first place - TyphoonRuntimeArguments can be improved to support scalar arguments, and I'm willing to do so in a separate PR. :)

Please tell me if any part of this needs some extra work, I'd be glad to polish it :)

[etolstoy]

Hi, @nickynick! Thanks for your contribution!

At a glance your way of describing definitions looks very interesting - many people bewared Typhoon because of the unfamiliar syntax.

I'll try to find some time in the following days to have a deeper look at your implementation.

[alexgarbarev]

Yes, that's looks great! Thanks for this idea and implementation. The only side effect I can see - mess of two styles of definitions syntax in same assembly (since this syntax can't be used for special definitions like TyphoonFactoryDefinition, TyphoonOptionDefinition etc).. but maybe we can add some "proxy" wrapper to these special definitions to match this amazing new syntax.. @jasperblues, your thoughts?

[nickynick]

Thanks for the comments guys :)

@alexgarbarev Yeah, that's a valid point. As I mentioned, factory definitions come off naturally with this - basically you'd have an instance typhoonDefinition category method in addition to the class one. I guess I'll try to figure out what can be done with the rest. They seem to be less common though, judging from our experience at least - haven't had to use anything but plain withClass: and a couple of withFactory: ones.

[jasperblues]

@jasperblues, your thoughts?

I love it!

had to add a check for this new class in the place where TyphoonAssemblyDefinitionBuilder fetches a definition - not the best thing to do, but unfortunately I can't see a better way to do so because there is no protocol for this and I don't feel authorized to add it :)

What kind of protocol were you thinking of?

I also added a couple of tests, not sure if I chose a correct place for them though.

Those places were good choices for top-level integration tests.

This same machinery can be used for factory definitions too - not done yet, but if you like the whole thing, I don't see why not.

We'll definitely need that if this is to become the recommended approach in place of the 'older' style.

TyphoonRuntimeArguments can be improved to support scalar arguments, and I'm willing to do so in a separate PR. :)

That would be good, if you could pull it off.

Great work - look forward to using this on projects.

On a mostly unrelated note, I wonder if it would be possible to provide the same kind of API, using only closures - no reflection, proxies, runtime, etc. I'm thinking of a Swift friendly version.

[alexgarbarev]

@nickynick

TyphoonRuntimeArguments can be improved to support scalar arguments, and I'm willing to do so in a separate PR. :)

This limitation here, because Typhoon sends placeholder objects instead of real argument at build phase (It's TyphoonInjectionByRuntimeArgument). If we define scalar arguments in method signature, clang will be confused and treat object as scalar - ARC will be broken and you'll get BAC_ACCESS crash. If you able to fix that somehow - I would like to see how! :+1:

[nickynick]

@jasperblues Thanks for the feedback! We gonna start a new project soon and I'm definitely looking forward to using this too :)

What kind of protocol were you thinking of?

Maybe something like this:

@protocol TyphoonDefinitionBuilder <NSObject>

- (TyphoonDefinition *)buildTyphoonDefinition;

@end

Both TyphoonDefinition and TyphoonDefinitionProxy would conform to it, the former returning self. This way, it can be assumed that objc_msgSend_InjectionArguments should always return id<TyphoonDefinitionBuilder>. Btw, I'm not entirely sure I understand what happens right now if TyphoonAssemblyDefinitionBuilder gets anything but TyphoonDefinition - there are non-aborting checks for the class of returned object as far as I can see.

Those places were good choices for top-level integration tests.

Good! Do you think there's anything else to be tested?

[nickynick]

@alexgarbarev Ah indeed, you are right! It probably might still be possible, but not with the current setup with prebuilt definitions. :confused:

---

Well, this leaves us in a sort of a trouble... However, here's one workaround I came up with after some further investigation.

The thing we want to be able to do is this:

- (Knight *)myKnightWithDamselsRescued:(NSNumber *)damselsRescued {
    Knight *knight = [[Knight typhoonDefinition] init];
    knight.hasHorseWillTravel = YES;
    knight.damselsRescued = damselsRescued.unsignedIntegerValue;
    return knight;
}

knight.hasHorseWillTravel = YES works just fine with a little improvement to TyphoonRuntimeArguments. The main problem, obviously, is damselsRescued.unsignedIntegerValue, because we are calling a method on TyphoonInjectionByRuntimeArgument which doesn't like it:

- (id)forwardingTargetForSelector:(SEL)aSelector
{
    [NSException raise:NSInvalidArgumentException
        format:@"You can't call a method on the runtime argument being passed in. It has to be passed in as-is"];
    return nil;
}

There is a good reason for this, because generally it's totally out of our control. However, what if we allow (and swallow) invocations of methods ending with Value? This is a common NSValue method naming pattern, ubiquitous for both stock and category usages. Edit: even better, we can also check that the invocation method signature returns a primitive type.

This is not a squeaky clean solution, but imo it's decent because it covers our needs entirely and doesn't do much damage. What do you think guys?

[nickynick]

I did some further investigation and came to a conclusion that there is a fundamental problem. Basically, we can't get away without supporting primitive arguments. Consider something like this:

@interface Foo : NSObject

- (instancetype)initWithCharValue:(char)bar;

@end

// in assembly:

- (id)fooWithCharValue:(NSNumber *)number
{
    return [[Foo typhoonDefinition] initWithCharValue:number.charValue];
}

As we build this definition, TyphoonInjectionByRuntimeArgument is passed in place of number, and let's say it forwards .charValue to simply return self. So far so good.

The problem is: by the time we get to forward initWithCharValue: method in our definition proxy, the argument will already be truncated to the size of char, because Foo's method signature is used. So there's no way to recover an injection object. Now that's too bad. :(

---

I guess I have a crazy idea how to pull it off but it's super weird :) I'll open a separate issue to discuss it there. Btw, do you guys by any chance have a Slack channel or anything like that? Perhaps it would be more convenient.

[alexgarbarev]

@nickynick yes, good research. That similar problem like I had when tried to support primitives as is.. Looks like we can't inject primitives with that kind of API, since original method signature used.

Another potential problem - passing block arguments ( I mean something like dispatch_block_t ). Have you tried to inject that using your new API? We had that problem before.. solved via id TyphoonInjectionWithRuntimeArgumentAtIndexWrappedIntoBlock(NSUInteger argumentIndex)

[nickynick]

As per #468, adding support for primitive arguments the current Typhoon way turned out to be a dead end. However, I wonder if there's anything that keeps from doing a whole new approach I'd like to elaborate below.

What if we ditch much of the TyphoonDefinition stuff we currently have and replace it with this:

+ (id)withInitializer:(id (^)())initializer
           injections:(void (^)(id object))injections
        configuration:(void (^)(TyphoonDefinition *definition))configuration;

An example usage:

- (Knight *)knightWithDamselsRescued:(NSUInteger)damselsRescued
{
    return [TyphoonDefinition withInitializer:^{
        return [[Knight alloc] initWithQuest:[self defaultQuest] damselsRescued:damselsRescued];
    } injections:^(Knight *knight) {
        knight.hasHorseWillTravel = YES;
        knight.favoriteDamsels = @[ @"Mary" ];

        [knight setFoobar:[self foobar] andHasHorse:YES friends:nil];

        [knight giddyUp];
    }];
}

Basically, we allow user to provide an arbitrary code to create an instance and to setup it. No more prebuilt injection lists.

So, how is this stuff supposed to work? Behind the scenes, there is a global per-thread coordinator object orchestrating what this withInitializer:injections:configuration: method does. It is either of these:

• invokes initializer block and returns the created instance;
• gets a current instance from the coordinator and invokes injections block on it, returns the instance back;
• creates and returns a TyphoonDefinition object configured with configuration block - this is where scope and stuff like that may be configured.

There's still a component factory which does the same stuff it's doing now - maintaining call stack & built object pools, and such. When the factory needs to build a new instance, it calls an assembly method two times - once to go for "initializer" route and once to go for "injections" route. By splitting initializer and injections, we are able to support circular dependencies in the same fashion as it's done now.

This approach solves a problem with primitive arguments, gives a simpler API, and also has a better performance. However, it's a major revamp, and I'm not sure if I forgot any potential pitfalls. What do you think?

[nickynick]

Gently bumping this discussion :)

I've been thinking about implementing this latest thing. I think there is a nice way to make it work without actually breaking everything. I'd move some common stuff to the new <TyphoonDefinition> protocol, which then would be implemented by two classes: the current TyphoonDefinition, and the new "arbitrary code" definition. Would be so good and useful :)

[nickynick]

Closing this in favor of #475.