square / Cleanse

Lightweight Swift Dependency Injection Framework
Other
1.79k stars 90 forks source link

Building with Swift 4 Blocked on SR-6108 #67

Closed sebastianv1 closed 6 years ago

sebastianv1 commented 6 years ago

Reference: https://bugs.swift.org/browse/SR-6108

It appears there is a bug in Swift as a result of SE-0110 (stronger type safety for tuples) that affects our BinderArities methods. Cleanse fails to compile tests because of the following compiler error:

error: ambiguous use of 'to(file:line:function:factory:)'
            binder.bind().to(factory: StructWithDependencies.init)
                   ^
Cleanse.BindToable:9:17: note: found this candidate
    public func to<P_1>(file: StaticString = #file, line: Int = #line, function: StaticString = #function, factory: @escaping (P_1) -> Self.Input) -> Cleanse.BindingReceipt<Self.Input>
                ^
Cleanse.BindToable:37:17: note: found this candidate
    public func to<P_1, P_2, P_3, P_4, P_5>(file: StaticString = #file, line: Int = #line, function: StaticString = #function, factory: @escaping (P_1, P_2, P_3, P_4, P_5) -> Self.Input) -> Cleanse.BindingReceipt<Self.Input>

For some reason, swift cannot disambiguate between the two generic functions. There is a workaround listed in the JIRA from the swift project, that will resolve the issue if the parameter names of each generic function are unique. So in our case, changing factory: @escaping... to something more unique for each arity such as factory1: @escaping..., factory2: @escaping.

I don't think this is an acceptable solution for Cleanse so we may have to get creative on this one. Thoughts @holmes @mikelikespie

akoskomuves commented 6 years ago

Hi guys! Is there any update about this issue?

sebastianv1 commented 6 years ago

@akoskomuve No update yet. I'll look into it this week and get an answer back to you

mikelikespie commented 6 years ago

:/ this issue concerns me. I honestly don't know of a good, easy solution for this. @sebastianv1 maybe time to open up the swift compiler source :)

sebastianv1 commented 6 years ago

From the Swift JIRA ticket: "Unfortunately you'd have to use a workaround for now, by adding labels as suggested or changing function names, ideally we'd have the second overload to match unambiguously but we need to do more refactoring of function type representation to get there."

This is a little concerning since it's not clear which version of swift it will be fixed in. @mikelikespie We could contribute in fixing the issue which would require some heroics, or brainstorm a new API for the factory functions? Both aren't great

mikelikespie commented 6 years ago

:( may be worth asking if they have any ideas on what the scope of the patch would be.

This basically breaks the core of cleanses DSL and there’s not a clean way to fix.

Probably the simplest workaround is just renaming the to method with the arity of 1 to to1 since that’s the one that is ambiguous with all the others.

Anyways this still sucks cuz the nice thing is that right now one can just change the constructor signature of a binding and it automatically matches to correct variant of to. And I feel without that, one loses a lot of the benefits of this library.

I’m not sure if there’s a way we could circumvent these limitations with some sort of reflection use. Also open to proposals.

Other thoughts: maybe there’s another workaround by having the compiler deprioritize the 1 variant with an attribute of sorts (e.g. @deprecated) or adding a return value that has to go unused.

eduardbosch commented 6 years ago

So Cleanse does not work with Swift 4 and above right now, right?

I wanted to test it out, but I could not manage to use it with swift 4.1

If it should work, I can post my problems in another issue.

Thanks

sebastianv1 commented 6 years ago

@eduardbosch Correct, Cleanse does not work with Swift4.x+ right now. I'll be working on a potential fix/change soon since we will also need Cleanse on swift4 soon.

LeeWong commented 6 years ago

@sebastianv1 Do you know when the support for Swift4.x+ will be in place? Thank you.

sebastianv1 commented 6 years ago

@LeeWong Working on a fix this week. Hoping to open a PR by the end of the week

eduardbosch commented 6 years ago

Thanks @sebastianv1

I'm currently using DIP https://github.com/AliSoftware/Dip which works with Swift 4.x. I'll try Cleanse when it works with Swift 4.x too 😉

sebastianv1 commented 6 years ago

I've spent a fair amount of time thinking through a number of workarounds that balances minimizing how much we change the core API of cleanse (specifically the to clause that is causing the issue) and reverting back to the normal API easy when the generic function ambiguity issue is resolved in swift.

Right now, one potential solution is to change each of the labels from factory to their corresponding arity count such as arity1-factory arity2-factory etc. This is a bit of a naive approach and would increase the cognitive load of using the API since one would have to match up the number of dependencies with the correct arity function label name. However, this does make reverting back to the normal API very easy with a simple search and replace. Would like to get input from y'all about this approach and if there are any other ideas: @mikelikespie @eduardbosch @holmes @LeeWong

mikelikespie commented 6 years ago

Hmm I think that would ruin a lot of the convenience that cleanse provides and may even make it not worth added complexity of DI system.

What about just renaming the 1-arity to function to to1 which will disambiguate the tuple version of functions. I think you may even be able to make a 1-arity version have @available(deprecated, instead=to1 with the name to which will help migration.

Still, with the swift compiler behaving as it does for swift4, I’d probably have not designed cleanse this way. Takes away a lot of the utility. Maybe you can get a swift patch upstream to make this a viable DI framework again?

sebastianv1 commented 6 years ago

@mikelikespie There still exists arity-0 that causes ambiguity. I played around with this idea and the compiler wants to use the 0-arity function instead of the 6th-arity for say Hamburger.init with 6 dependencies for some reason.

mikelikespie commented 6 years ago

Does hamburger have a 0 argument constructor too? What happens if 0th arity is renamed?