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

rename `then` -> `flatMap` #213

Closed
tanner0101 opened this Issue Mar 21, 2018 · 19 comments

Comments

Projects
None yet
10 participants
@tanner0101
Copy link
Contributor

tanner0101 commented Mar 21, 2018

Swift's standard library makes extensive use of the map/flatMap pattern for working with generics. The basic concept behind this pattern is preventing generics from becoming nested (i.e., keeping them "flat").

The concept of map/flatMap can be represented abstractly like so:

before signature after
T<U> map(_: U -> V) T<V>
T<U> flatMap(_: U -> T<V>) T<V>

map is used to change the generic's wrapped type to another type. flatMap is used to change the generic's wrapped type to the wrapped type of another instance of the generic type.

There was some misconception surrounding flatMap and the old method on array that would remove optional elements. This was fixed by SE-0187, replacing the incorrectly labelled flatMap with compactMap. I believe that is a strong signal from the Swift team that map/flatMap is a more abstract concept for working with (and not nesting) generics, and not just a term associated with optionals.

If you look at NIO's .map method, the signature directly corresponds to usages of map in the standard library. 👍

extension EventLoopFuture {
    func map<U>(_ callback: (T) -> U) -> EventLoopFuture<U>
}

/// identical to...

extension T {
    func map<V>(_ callback: (U) -> V) -> T<V>
}

/// identical to...

extension Optional {
    func map<U>(_ transform: (Wrapped) throws -> U) rethrows -> U?
}

/// identical to...

extension Array {
    func map<T>(_ transform: (Element) throws -> T) rethrows -> [T]
}

If you look at NIO's .then method, the signature matches what is called .flatMap in other places.

extension EventLoopFuture {
    func then<U>(_ callback: (T) -> EventLoopFuture<U>) -> EventLoopFuture<U>
}

/// signature matches...

extension T {
    func flatMap<V>(_ callback: (U) -> T<V>)) -> T<V>
}

/// signature matches...

extension Optional {
    func flatMap<U>(_ transform: (Wrapped) throws -> U?) rethrows -> U?
}

/// signature matches...

extension Array {
    func flatMap<U>(_ transform: (Element) throws -> [U]) rethrows -> [U]
}

I think all uses of .then in NIO should be renamed (with deprecation fix-its) to flatMap to further enforce this pattern and make NIO fit in better with the Swift standard library.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 21, 2018

I think this makes sense... @weissi @John-Connolly @airspeedswift WDYT ? If all agree are you interested doing a PR @tanner0101 ?

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Mar 21, 2018

@normanmaurer yeah, definitely. :)

@helje5

This comment has been minimized.

Copy link
Contributor

helje5 commented Mar 21, 2018

I don't particularly care, but to me this feels wrong for two reasons:

a) flatMap just does a flatMap function on some entity. It is pure functional and there is no timing aspect to it. then attaches an event handler to the future, which may or may not be called. In other words, the then/timing aspect is the focus of the call, not the mapping aspect.

b) then is not always called, there can be multiple thens, and error thens, etc. This doesn't map to any
of the functional stuff.

But maybe asyncFlatMap or something? I would just keep then to be honest :-)

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Mar 21, 2018

then attaches an event handler to the future, which may or may not be called.

Same goes for the other cases. The function you pass to Optional.flatMap may or may not be called depending on whether the value is nil. The difference is when it is called, not if it is.

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Mar 21, 2018

@helje5 by your argument .map should be renamed to something else then because it also has an async nature to it.

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Mar 21, 2018

I am a bit conflicted. Consistency is good, but so is clarity. There isn't a clearly better name for Optional.flatMap (only bad verbose alternatives like mapAndUnwrap). then OTOH is a pretty nice name for what this particular method does.

@John-Connolly

This comment has been minimized.

Copy link
Contributor

John-Connolly commented Mar 21, 2018

I prefer .flatmap, I think its consistent with other monadic types that are commonly used in swift. The result type also uses this naming.

@helje5

This comment has been minimized.

Copy link
Contributor

helje5 commented Mar 21, 2018

@helje5 by your argument .map should be renamed to something else then because it also has an async nature to it.

I think I agree with this. As mentioned not only because it is async (the significance being on the temporal part), but also because you can attach multiple handlers in a non-functional way.

Guesswork: Presumably map is only not-named then because otherwise you would end up having to type the return values explicitly? (to disambiguate)

Two other suggestions which come to mind: thenMap and thenFlatMap? (but what about the error cases, thenMapIfError? - all this looks just wrong to me).
Or maybe something similar to lazy: async.map and async.flatMap. But again, what about the error cases? Or redo them to be actually functional and get passed a Result or Error/T?

As mentioned I don't particularly care, but it seems pretty weird to me to map functional conventions on something which is not composed in a functional way.

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Mar 21, 2018

Guesswork: Presumably map is only not-named then because otherwise you would end up having to type the return values explicitly?

This is historically true for our implementation: we split then up into two largely in order to help the Swift compiler do the right thing with type inference.


So I'm basically -0 on this. I don't care very much, and I buy the arguments in favour of changing. However, I also think that the name change is basically a bikeshed: the .then name is not a source of confusion, and the value of changing it to flatMap is probably less than the work required to get everyone to change .then to .flatMap.

But if everyone else wants to change it I certainly won't kick up a fuss.

@weissi

This comment has been minimized.

Copy link
Contributor

weissi commented Mar 21, 2018

Couldn’t have commented better than @airspeedswift . Why could have both then and flatMap? I don’t think deprecating then is a good call right now as it’s everywhere and then arguably sounds nicer.
Once Swift gains higher kinded types, we should obviously follow what the Applicative/Monad or whatever they’ll be called protocol mandate. But I don’t think there’s anything from with having the nicer name then

@helje5

This comment has been minimized.

Copy link
Contributor

helje5 commented Mar 21, 2018

But what about the reverse thing. Should map be called differently? Maybe now? 🤪

@tomerd

This comment has been minimized.

Copy link
Member

tomerd commented Mar 21, 2018

@vlm

This comment has been minimized.

Copy link
Contributor

vlm commented Mar 21, 2018

Having both .then and .flatMap is a source of confusion. Even having .map and .flatMap is a source of confusion, and you'll have a fun time explaining all that in the documentation. I prefer map+then.

@gps

This comment has been minimized.

Copy link
Contributor

gps commented Mar 23, 2018

FWIW, Ratpack has both then, and flatMap, but they do different things. They also have flatLeft, flatRight, etc. which I find quite useful.

IMO, something else worth considering is a problem that arises from having then do more than one thing. This is something that plagued PromiseKit, IMO. From http://promisekit.org/news/2018/02/PromiseKit-6.0-Released/:

With PromiseKit our then did multiple things, and we relied on Swift to infer the correct then from context. However with multiple line thens it would fail to do this, and instead of telling you that the situation was ambiguous it would invent some other error. Often the dreaded cannot convert T to AnyPromise. We have a troubleshooting guide to combat this but I believe in tools that just work, and when you spend 4 years waiting for Swift to fix the issue and Swift doesn’t fix the issue, what do you do? We chose to find a solution at the higher level.

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Mar 23, 2018

Right now then only does one thing, and I don't think there's any plan to move to only having then. We pulled then and map apart for good reason. 😄

@helje5

This comment has been minimized.

Copy link
Contributor

helje5 commented Mar 23, 2018

But don’t you think that map is a little confusing wrt the original question? Specifically considering that it really is a then and just named differently to pleaze the compiler?

(I‘m not really suggesting we should rename it, but maybe it is worth a consideration to create consistency „the other way around“ as originally proposed)

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Mar 23, 2018

I think map is a fine name, tbh. It doesn't have an asynchronous property, nor does it claim to: it takes a concrete value and maps it to a new concrete value.

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Mar 23, 2018

FWIW, it's very likely the swift std lib will get a future at some point, which will get bikeshod on evolution. Renaming stuff now is probably tempting fate for the new name to conflict with whatever the std lib implementation ends up being.

@tomerd tomerd added the discussion label Mar 23, 2018

@tanner0101

This comment has been minimized.

Copy link
Contributor Author

tanner0101 commented Mar 27, 2018

I'm OK sticking with then if it seems everyone likes that name. It's less inconsistent with the rest of stdlib, but more consistent with other promise implementations which may be more important.

I do agree that we should definitely not add any overloads to then. Swift's type system already has a hard enough time inferring the return type as is.

I'm fine closing this. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment