Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autowire idea #178

Closed
alexgarbarev opened this issue Feb 19, 2014 · 38 comments
Closed

Autowire idea #178

alexgarbarev opened this issue Feb 19, 2014 · 38 comments
Labels

Comments

@alexgarbarev
Copy link
Contributor

Hi folks,

I have some idea how to implement autowite with syntax:

@interface Knight

@property (nonatomic, strong, typhoon_injected) NSString *foo; //property attribute
@property (nonatomic, strong) NSString *foo TYPHOON_INJECTED; // like UI_APPEARANCE_SELECTOR
@property (nonatomic, strong) TYPHOON_INJECTED NSString *foo; // like IBOutlet

@end

Since we cannot add our own property attributes or clang attributes - all keywords above are just empty defines:

#define TYPHOON_INJECTED
//or
#define typhoon_injected

Because we cannot obtain these marks at runtime I thought that we can collect all these marks by our own 'parser', lets call it TyphoonPreprocessor.

It should be fast objective-c written utility (with caching in 'derived data' processed filepaths to avoid processing not changed files).

Result of TyphoonPreprocessor can be XML file in typhoon format. XML should has definitions with TYPHOON_INJECTED properties. For example:

<component class="Knight" key="knight_autowire">
    <property name="foo" />
</component>

Next step is coping result XML into application bundle - this step can be similar to Pods-resources.sh script.

Final step is loading TyphoonXmlComponentFactory with autowiring XML and re-register all definitions to user-created TyphoonComponentFactory.

Disadvantages

  • Extra build phase in user's project (can be added automatically by spec.prepare_command)
  • Longer build&install time (can be improved by caching - parsing only changed files, also we can decide to parsing only '.h' files - they are much smaller)
  • Longer Typhoon initialization process - because xml parsing and re-registering definitions

Advantages

  • Nice looking 'typhoon_injected' property attribute for auto-wiring
  • Ability to add another preprocess macroses
  • auto-wiring with same way as TyphoonXmlComponentFactory. Easy to debug by viewing result XML.
  • typhoon_injected tags can be used in future to write Typhoon Xcode plugin for smart autocomplete

What do you think? Make sense?

@jasperblues
Copy link
Member

Very good analysis.

  • The generated definitions would have to be merged in to any existing definitions. (support manual and auto-wiring simultaneously).
  • What do you mean about spec.prepare_command to add a new build phase? I've got a library here to manipulate Xcode project files: https://github.com/jasperblues/XcodeEditor
  • Do you think the benefits outweigh the drawbacks. We've been hesitant to add any pre-processing or code-gen before.

@jasperblues
Copy link
Member

  • Ah you mean using CocoaPods to install the build steps. That would be quite good.

@jasperblues
Copy link
Member

There are Objective-C grammars for Yacc, but I think the fastest way to parse would be to use Clang's Active Syntax Tree (AST).

Even if it would be cool to literally shave that Yacc ;)

. . . . actually what am I thinking!? . . shouldn't need a syntax tree just to match a macro in the header.

@jasperblues
Copy link
Member

Which syntax do people prefer?

I like this one: @Property (nonatomic, strong, typhoon_injected) NSString *foo;

@alexgarbarev
Copy link
Contributor Author

I tried to use AST, but property in AST is

-ObjCPropertyDecl 0x102001290 <line:17:1, col:59> foo 'int *' readwrite nonatomic strong

for this declaration:

@property (strong, nonatomic, typhoon_injected) NSString *foo;

Aslo I think that AST slower that 'just parse' - it analys code, type checking etc..

May be you can test something else or Yacc for parsing

@alexgarbarev
Copy link
Contributor Author

Do you think the benefits outweigh the drawbacks. We've been hesitant to add any pre-processing or code-gen before.

I don't like to add build phase or preprocessor (especially for tiny frameworks) but it is only way to 'extend' objective-c by our own directives.

benefits outweigh the drawbacks

I don't like auto-wire (don't want dependency on typhoon in code, want to keep all definitions in one place), but if someone likes it - ask him what is more important nice syntax or no-extra-phase.

@jasperblues
Copy link
Member

@ratkins @notjosh Would you care to comment on this ticket?

Options:

  • Stick with the current typhoon autowiring: https://github.com/typhoon-framework/Typhoon/wiki/Autowiring . . . (until such a time that there are real annotations in Objective-C)
  • Use the proposed new style and have Typhoon generate the property dynamically. (The property must be @dynamic. You can't override the property or touch the getter/setter). You can't specify the factory, we have to assume a default factory.
  • Use the proposed new style, but it requires a pre-processor. This would be installed by CocoaPods.

@alexgarbarev
Copy link
Contributor Author

Using @dynamic is not recommended and can have side effects for all non-typhoon dynamic properties.

Also there is a lot of runtime works

  • scan all classes for all properties;
  • replace all getter-setters of all dynamic properties.
  • check property type in getter method; - check that definition exists for property type in getter.

Also all injected properties become lazy - can be unexpected for users.

Send from iPhone

20.02.2014, â 13:16, Jasper Blues notifications@github.com íàïèñàë(à):

@ratkins @notjosh Would you care to comment on this ticket?

Options:

Stick with the current typhoon autowiring: https://github.com/typhoon-framework/Typhoon/wiki/Autowiring . . . (until such a time that there are real annotations in Objective-C)

Use the proposed new style and have Typhoon generate the property dynamically. (The property must be @dynamic. You can't override the property or touch the getter/setter).

Use the proposed new style, but it requires a pre-processor. This would be installed by CocoaPods.


Reply to this email directly or view it on GitHub.

@alexgarbarev
Copy link
Contributor Author

Also @dynamic way can't specify factory - only default factory usage..

20.02.2014, â 13:16, Jasper Blues notifications@github.com íàïèñàë(à):

@ratkins @notjosh Would you care to comment on this ticket?

Options:

Stick with the current typhoon autowiring: https://github.com/typhoon-framework/Typhoon/wiki/Autowiring . . . (until such a time that there are real annotations in Objective-C)

Use the proposed new style and have Typhoon generate the property dynamically. (The property must be @dynamic. You can't override the property or touch the getter/setter).

Use the proposed new style, but it requires a pre-processor. This would be installed by CocoaPods.


Reply to this email directly or view it on GitHub.

@jasperblues
Copy link
Member

Agree regarding dynamic. @ratkins, @notjosh we don't recommend this approach - just listed to be complete.

Usually you want to resolve dependencies up-front, and fail early. Except in the case of say, transitioning from one view controller to another. For that we have:

Couple of course with: Typhoon Scopes

@alexgarbarev
Copy link
Contributor Author

Let's use

@property (nonatomic, strong, inject) NSString *foo;

for all injected properties just for documentation purpose

@jasperblues
Copy link
Member

inject vs injected. which is better?

@alexgarbarev
Copy link
Contributor Author

I used inject to be same as 'copy' 'retain' - not 'copied', 'retained'

@jasperblues
Copy link
Member

I used inject to be same as 'copy' 'retain' - not 'copied', 'retained'

Ah, yes, of course.

@notjosh
Copy link

notjosh commented Feb 20, 2014

Catching up. +1 on the feature! Super valuable :)

If I'm going to bikeshed, I prefer the IBOutlet-esque syntax of

@property (nonatomic, strong) TYPHOON_INJECTED NSString *foo;

I think it's more familiar to anyone that's used IB that "this gets compiled out", whereas the inject property attribute is less clear, and less common. Though, I suppose it'd be more readable to conform to IBOutlet, like TyphoonInjected or similar.

@alexgarbarev
Copy link
Contributor Author

I agree that

@property (nonatomic, strong) TyphoonInjected NSString *foo;

or just

@property (nonatomic, strong) Injection NSString *foo;

more clear for users, because IBOutlet is same as Injected but by xibs. So it is place for Injection tag.

@notjosh on feature you mean using attribute for self-documentation or preprocessor way?

@notjosh
Copy link

notjosh commented Feb 20, 2014

@alexgarbarev I meant the preprocessing. ✨Magical✨ autowiring via a preprocessor will be amazing.

@alexgarbarev
Copy link
Contributor Author

My vote for documentation purpose instead of magical preprocessor

@ratkins
Copy link

ratkins commented Feb 20, 2014

I agree with @notjosh, I like

@property (nonatomic, strong) Injected NSString *foo;

I would also say Injected in preference to TyphoonInjected so other frameworks could latch on to the "annotation". This would assuage concerns of the framework "infecting" your code—you want it declaratively documented that you are using dependency injection, but how you do so is not a concern of the person reading said code at the point where they're just looking at the property declarations.

@jasperblues
Copy link
Member

Thanks @ratkins.

How about we follow (as we did with the original autowiring feature) @jonreid 's approach of:

  • Make it namespaced as: TyphoonInjected
  • If you #define typhoon_shorthand, you can just use 'injected'

This would allow the tag to catch on if it wanted to, at the same time as give some safety.

@jasperblues
Copy link
Member

@ratkins Did you have a vote on the 2nd table?

  • Magic pre-processor?
  • Documentation / tool-support

@jasperblues
Copy link
Member

Votes:

Style Votes
@Property(nonatomic, strong, typhoon_injected) Thing *foo; 0
@Property(nonatomic, strong) TYPHOON_INJECTED Thing* foo 4 (Aleksey, Jasper, Josh, Robert)
@Property (nonatomic, strong) NSString *foo TYPHOON_INJECTED; 0
Purpose Votes
For documentation. And IDE tools can hook into this* 2 (Aleksey, Jasper)
Pre-processor annotation (behave like a Java annotation, by generating runtime pars-able data 1 (Josh)

* I imaged with an IDE tool we can do the following:

  • Tool-tip what the property being injected will be
  • Crtl-click though to the definition.
  • Warn if not defined. Also provide the option to define.

@AlexDenisov
Copy link

Hi @jasperblues.
IMHO, @property(nonatomic, strong) TYPHOON_INJECTED Thing* foo or similar is the best solution.
I've thought about such feature for my project, and figured out that under this macro you can use __attribute__((annotation("Foo"))), which allows you to do some interesting stuff with compiler-like tool (doesn't matter would it be flex, clang-c or handwritten tool).
In case of (nonatomic, injected) you can use only empty macro-definition (btw, AppCode produces warnings for this code).

@jasperblues
Copy link
Member

Thanks @AlexDenisov !!! This is very valuable input. . . (and great work on Blood and Magic, btw)

@jasperblues
Copy link
Member

@AlexDenisov In fact, we were inspired and impressed by BM's annotated properties . . . but thought the pre-processor approach might work better for Typhoon.

@AlexDenisov
Copy link

In fact, we were inspired and impressed by BM's annotated properties

:shy: 😄

btw, I've started small project for analysing projects that use BM. I'm working on it just from academical interest, but I think that it might be useful for you too, at least used approaches.
So, I will notify you as soon as it will be ready.

@jasperblues
Copy link
Member

We'll look forward to hearing more about that. Is it already on GitHub?

@AlexDenisov
Copy link

No, it's still in my private repo.

@literator
Copy link
Contributor

Hi guys,

It's already outvoted, but I would definitely go with TyphoonInjected syntax. I agree with all of arguments, plus attribute-like syntax looks just bad in IDE.
Purpose table? Vote for documentation.

General note.
That's great idea to simulate annotations in ObjC but one of the main reasons I would choose Typhoon is to keep my code clean from any external frameworks additions.

@AlexDenisov
Copy link

@literator, as I know there is no possibility to deal with macro definitions via runtime, so this feature will affect only on visual representation and, maybe, on some tools.
I just mean that it's optional.
@jasperblues, am I right?

@jasperblues
Copy link
Member

@AlexDenisov, Yes you're right - it will be just optional.

Well, actually we proposed two ideas:

  1. Use a pre-processor to generate the information that we need, and copy it the application bundle (just like a lot of C++ reflection frameworks). Install via CocoaPods. . . we were hesitant about this idea already, but it does have some merits, hence the debate.
  2. Even if we don't proceed with the above idea, perhaps there's some value in using it for code-clarity and documentation? On top of this, we can have the IDE run the same task that the pre-processor would have and do things like:

⋅⋅* warn if the injection didn't happen.
⋅⋅* ask if you want to define the inject and/or code-gen it for you.
⋅⋅* link to / tool-tip the injection

@jasperblues
Copy link
Member

@literator do you mean like this?

@property(nonatomic, strong) TyphoonInjected Thing *foo;

prefer it to this?

@property(nonatomic, strong) TYPHOON_INJECTED Thing *foo;

@literator
Copy link
Contributor

@jasperblues for me TyphoonInjected looks nicer, however TYPHOON_INJECTED syntax will be more consistent with Apple's annotations like UI_APPEARANCE_SELECTOR, NS_AVAILABLE_IOS and others that you can find in properties declarations, so I would vote on that one.

@jasperblues
Copy link
Member

Do we annotate:

  • Only writable properties that are injected using property-style injection?
  • Or also read-only properties that are set via an initializer?

Follow the behavior of Spring's @required annotation here? (Need to check what that behavior is).

@AlexDenisov
Copy link

@jasperblues, that's what I meant here
But this project discontinued in favor of Clang plugin.

@jasperblues
Copy link
Member

Great work Alexey! A trove of useful stuff there.

@AlexDenisov
Copy link

But this project discontinued in favor of Clang plugin.

Btw, it's also discontinued, because Apple's clang doesn't support plugins :(
http://railsware.com/blog/2014/02/28/creation-and-using-clang-plugin-with-xcode

@jasperblues
Copy link
Member

If I had to have a macro-based solution, I'd want it to look like this #252 (great work from Aleksey)

  • 'Annotation' is in the header file where the property is declared.

. . this feature is currently on trunk if you want to try it out. We'll be posting docs soon. It will deprecate Typhoon's old autowiring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants