AliSoftware / Dip

Simple Swift Dependency container. Use protocols to resolve your dependencies and avoid singletons / sharedInstances!
MIT License
978 stars 75 forks source link

register function is broken in Swift 4 #172

Closed jmartinesp closed 6 years ago

jmartinesp commented 7 years ago

Hello, and thanks for Dip, I've been using it for quite a while and I love it so far.

Today I updated my project to Swift 4 and I found out that some container.register { ... } calls cannot be resolved by the compiler.

container.register { TestClass($0) } // Works fine
container.register { TestClass() } // Nope

The error shown is Ambiguous use of 'register(_:type:tag:factory:)'. I guess because of this change, when the register function is used and no argument is ever used inside the closure, the compiler won't be able to choose between all the 0, 1, 2, 3... args register functions in RuntimeArguments.swift.

As far as I know you still haven't updated the library to work with Swift 4, but I just wanted to report this so it's a known issue for future versions.

ilyapuchka commented 7 years ago

Yes, it's most likely related to this change, but the idea was that it should not break anything. That's sad.

jmartinesp commented 7 years ago

I think I might have fixed it by wrapping the args in an extra pair of (), like this:

// Original
@discardableResult public func register<T, A, B>(_ scope: ComponentScope = .shared, type: T.Type = T.self, tag: DependencyTagConvertible? = nil, factory: @escaping (A, B) throws -> T) -> Definition<T, (A, B)> {
    return register(scope: scope, type: type, tag: tag, factory: factory, numberOfArguments: 2) { container, tag in try factory(container.resolve(tag: tag), container.resolve(tag: tag)) }
  }

// "Fixed"
@discardableResult public func register<T, A, B>(_ scope: ComponentScope = .shared, type: T.Type = T.self, tag: DependencyTagConvertible? = nil, factory: @escaping ((A, B)) throws -> T) -> Definition<T, (A, B)> {
    return register(scope: scope, type: type, tag: tag, factory: factory, numberOfArguments: 2) { container, tag in try factory((container.resolve(tag: tag), container.resolve(tag: tag))) }
  }

So far everything seems to be working fine, I will report if I find anything odd.

kaosdg commented 7 years ago

@Arasthel have you gotten any further on this? I've done what you suggested, however calling register with no arguments still gives me the ambiguous reference error.

jmartinesp commented 7 years ago

This is what mi RuntimeArguments file looks like: https://gist.github.com/Arasthel/38434fccf17e78fcf370f4724d86c2ff

It works fine for me, both with 0, 1 and several arguments.

karlpuusepp commented 7 years ago

@kaosdg I managed to get it working by only changing the single-argument overload from (A) throws -> Void to ((A)) throws -> Void.

I'd guess the compiler is not sure if Void works as A or not, but adding extra parentheses seems to resolve the ambiguity.

ilyapuchka commented 7 years ago

I don't think turning several factory arguments to one tuple argument is a good solution for that.

kaosdg commented 7 years ago

About to put this aside for a few days, but here's where I am so far:

I can get it to the point where the framework compiles, but one unit test gives me trouble with the following test: testThatItCallsResolvedDependenciesBlockWhenResolvingByTypeForwarding()

container.register { ServiceImp1() }

This one line fails with Ambiguous use of 'register(_:type:tag:factory:)' whereas all other calls similar to this compile without issues. (my branch is here: https://github.com/kaosdg/Dip/tree/swift4.0 )

I followed a bit of the discussion on SE-0110 and saw this: https://lists.swift.org/pipermail/swift-evolution-announce/2017-June/000386.html

which leads me to believe there may be some wonkiness with how the Swift 4 compiler is behaving at this point

matkuznik commented 7 years ago

Hi all. Do anyone found proper solution? I was able to successfully implement @rerelease's proposition and all tests pass.

@kaosdg you need to update one more register function, see this diff

I can agree with @ilyapuchka that this is not the best solution, but for now only one or not?

karlpuusepp commented 7 years ago

Since SE-0110 was deferred I would assume this is a compiler bug?

The ambiguity can be trivially reproduced in a playground.

func overload<T>(_ arg: () -> T) {

}

func overload<T, A>(_ arg: (A) -> T) {

}

overload { // compiler error here
    return 10
}

Again, wrapping (A) with extra parentheses fixes it.

stevenkramer commented 7 years ago

I implemented @rerelease's fix in https://github.com/stevenkramer/Dip/commit/34dd0a90911e0708a453c5b06a696dd4bdbe2ab3

So far so good.

ilyapuchka commented 6 years ago

On Xcode 9 I don't see any compilation errors or tests failures, code snippet provided by @rerelease also does not result in compiler error. So I guess the breaking change in swift compiler was fixed.

jonasman commented 6 years ago

I get this error using XCode 9 GM