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

then methods type inferring issue #44

Closed
isaac-weisberg opened this issue Apr 2, 2018 · 13 comments
Closed

then methods type inferring issue #44

isaac-weisberg opened this issue Apr 2, 2018 · 13 comments

Comments

@isaac-weisberg
Copy link
Contributor

There are 3 methods titled then on a Promise object

public func then(on queue: ExecutionContext = DispatchQueue.main, _ onFulfilled: @escaping (Value) -> (), _ onRejected: @escaping (Error) -> () = { _ in }) -> Promise<Value>

public func then<NewValue>(on queue: ExecutionContext = DispatchQueue.main, _ onFulfilled: @escaping (Value) throws -> NewValue) -> Promise<NewValue>

public func then<NewValue>(on queue: ExecutionContext = DispatchQueue.main, _ onFulfilled: @escaping (Value) throws -> Promise<NewValue>) -> Promise<NewValue>

onFulfilled of the first method returns Void, of the second - NewValue, and the third - Promise<NewValue>, that's important.

If you were to write

Promise { fulfill, reject in
    fulfill(3)
}.then { result in
     return
}

then it's dandy, the Void is returned
If you were to write

Promise { fulfill, reject in
    fulfill(3)
}.then { result in
     return Promise(value: result)
}

then it's also dandy, the swiftc will figure out that the return type is Promise
However

Promise { fulfill, reject in
    fulfill(3)
}.then { result in
     return result
}

never just works on its own; there is an issue with type inference and compiler errors with "expected type Promise<_>"
The way you do it so it gets figured out is

Promise { fulfill, reject in
    fulfill(3)
}.then { result -> Int in
     return result
}

Then it's alright.

Observed in XCode 9.2 GM, Swift 4.0.2
Also observed in Swift 4.0.3 environment on Linux.

Possible solution

It may or may not help to rename the handler from onFulfilled to a different name such as onFulfilledWithValue in the method where handler returns NewValue.

@khanlou
Copy link
Owner

khanlou commented Apr 2, 2018

I wonder if this is because of the type inference on the value 3. It doesn't know if it's an Int, UInt, CGfloat, Int8, etc. Can you try it with an empty struct like struct MyStruct { }?

@isaac-weisberg
Copy link
Contributor Author

isaac-weisberg commented Apr 3, 2018

@khanlou oh no no, Int and 3 are just for reference
Here's a snippet from an actual project

var giftController: Promise<GiftIntroductionController> {
    imagePromise.then { userImage in
        return GiftIntroductionControllerBackend.make(for: gift, image: userImage)
    }.then { backend -> GiftIntroductionController in // Here, type will not be inferred if not specified explicitly
        let controller = UIStoryboard.main.instantiate(GiftIntroductionController.self)
        controller.prepare(for: backend)
        return controller
    }
}

N.B.: the return type of closure in then is not necessarily the same as the one and only parameter type, it just happened to be so in the snippet.

@khanlou
Copy link
Owner

khanlou commented Apr 3, 2018

In the example, the issue is that your block is longer than one line, so its type is not inferred. Via the readme:

As long as the block you pass to then is one line long, its type signature will be inferred, which will make Promises much easier to read and work with.

@khanlou
Copy link
Owner

khanlou commented Apr 5, 2018

@isaac-weisberg is this good to close now or is there still an issue?

@isaac-weisberg
Copy link
Contributor Author

Still an issue, planned to try to dig more this weekend, if you don't mind :)

@isaac-weisberg
Copy link
Contributor Author

🤔🤔🤔 I am struggling trying to replicate this behavior with Swift 4.1 on Linux

@isaac-weisberg
Copy link
Contributor Author

Alright, evidently, the issue is not with version of Swift, I have tuned test up a bit and it started to appear

@isaac-weisberg
Copy link
Contributor Author

Alright, so first wave of experiments is to be referred to as "done".

The compiler was ultimately confused by the following construct.

        let promise = Promise { fulfill, reject in
            fulfill(SomeStuff())
        }.then { value in
            return Promise { fulfill, reject in
                fulfill(Apple(str: "Heoooo"))
            }
        }.then { apple in
            return Bapple.make()
        }.then { bapple in
            print("Bapple is \(bapple)")
            return bapple
        }

About the last then closure, the output is.

PromiseTypeInferringTests.swift:49:20: error: unexpected non-void return value in void function
            return bapple

Observation 1: so we've got 3 then methods. One of them is generic with closure returning a Promise, another - generic with closure returning a value, creating a Promise itself, and third one - non-generic with closure returning Void. Since Void is a first-class type in Swift, just like functions, a set containing Void type is a strict subset of a set containing all possible types satisfying unconstrained generic type specifier.

Observation 2: third then method features an optional onRejected parameter (that has a default value), while the rest of the then methods don't feature such capabilities. From a logical standpoint, as well as the majority of use cases, a then method with onRejected handler is a done method. With the background of previous observation, this may or may not play some role in compiler's decisions, and if so, the implementations must especially be available both ways.

@isaac-weisberg
Copy link
Contributor Author

So, possible solution:

  1. Start treating Void as NewValue
  2. Remove then with callback that returns Void
  3. Make the old then with two callbacks the new done, optionally, for internal usage only.

@isaac-weisberg
Copy link
Contributor Author

isaac-weisberg commented Apr 7, 2018

Lol so I tried it

Good thing:

  1. it compiles.

Bad thing:

  1. If you had a Promise<Int> and then applied a then handler that returned Void, the type of returned promise is transformed to Promise<Void>

Extract from PromiseTests.swift

        let promise = Promise(value: "someString").then({ string in
            return string.count
        }).then({ count in
            return count*2
        }).then({ doubled in
            XCTAssertEqual(doubled, 20)
            expectation?.fulfill()
        })

Before the changes, type of promise is Promise<Int>, after - Promise<Void>, cause it chains.
@khanlou, I imagine, this is not the desired behavior but wait, it is in fact the desired behavior, obviously, because what we are actually doing is applying not a then handler, but the done handler; in other words, we explicitly specify that we are not chaining another action.

P.S.: one might be wondering "Hey, Isaac, but did this resolve your issue that you had with type inferring?". Answer is no 😂

@isaac-weisberg
Copy link
Contributor Author

So obviously, with this change to API, all usages of not-chaining then must be changed to done.
And, naturally, done must be public.

@isaac-weisberg
Copy link
Contributor Author

I have done this and it looks concise and beautiful but this did not resolve the issue. I will fiddle with is just a bit more, maybe it will work, but there is no reason to presume something is going to get fixed eventually in that regard.

@isaac-weisberg
Copy link
Contributor Author

        let promise = Promise { fulfill, reject in
            fulfill(SomeStuff())
        }.then { value in
            return Promise { fulfill, reject in
                fulfill(Apple(str: "Heoooo"))
            }
        }.then { apple in
            return Bapple.make()
        }.then { bapple in
            print("Bapple is \(bapple)")
            return bapple
        }

produces

PromiseTypeInferringTests.swift:47:16: error: unable to infer complex closure return type; add explicit type to disambiguate
        }.then { bapple in
               ^
                        -> (PromiseTypeInferringTests.Bapple)

Well, that was fun while it lasted :)

P.S.: Just in case, @khanlou, I understand the dramatic nature of changes that I used to propose and I have no hard feelings about the idea of having my PR rejected :) I can guess, the project wouldn't like to have such a drastic change to the API :)

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

No branches or pull requests

2 participants