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

FR: Provide curried versions of functions #200

Closed
raveclassic opened this issue Aug 14, 2017 · 27 comments
Closed

FR: Provide curried versions of functions #200

raveclassic opened this issue Aug 14, 2017 · 27 comments

Comments

@raveclassic
Copy link
Collaborator

raveclassic commented Aug 14, 2017

Hi!

I have just found myself currying by hand yet another function like insert/remove from StrMap in fp-ts and Lens#modify in monocle-ts to use it with compose and pipe.

Here's the question therefore - are you going to include curried versions of these helpers? Or maybe replace current with curried and export uncurried with a postfix like it's done in sanctuary:

export const remove_ = <A>(k: string, d: StrMap<A>): StrMap<A> => { ... }
export const remove: (k: string) => <A>(d: StrMap<A>) => StrMap<A> = curry(remove_);
@raveclassic raveclassic changed the title FE: Provide curried versions of functions FR: Provide curried versions of functions Aug 14, 2017
@gcanti
Copy link
Owner

gcanti commented Aug 14, 2017

My policy so far is simple

  • do not curry...
  • ...unless type inference and/or correctness can be improved

Example

import { Applicative } from '../src/Applicative'
import { HKT } from '../src/HKT'
import { option, some } from '../src/Option'
import { right } from '../src/Either'

// curried version
declare function traverse1<F>(F: Applicative<F>): <A, B>(f: (a: A) => HKT<F, B>, ta: Array<A>) => HKT<F, Array<B>>

// x: HKT<"Option", number[]>
export const x1 = traverse1(option)(a => some(a), [1, 2, 3])
// export const x2 = traverse1(option)(a => right(a), [1, 2, 3]) // error :)

// uncurried version
declare function traverse2<F, A, B>(F: Applicative<F>, f: (a: A) => HKT<F, B>, ta: Array<A>): HKT<F, Array<B>>

// x3: HKT<"Option" | "Either", number[]>
export const x3 = traverse2(option, a => right(a), [1, 2, 3]) // no error :(

I'm definitely open to suggestions in order to change the policy (in general I would avoid API duplications though)

@raveclassic
Copy link
Collaborator Author

Well, at the beginning I was trying to change an object through lens multiple times and noticed that all modifying functions on every structure (Array, StrMap etc.) are uncurried as well as Lens#modify so at first the code looked like:

type Entity = {
  id: string,
  foo: string
};
type State = {
  ids: string[],
  entities: StrMap<Entity>
};
const ids = Lens.fromProp<State, 'ids'>('ids');
const entities = Lens.fromProp<State, 'entities'>('entities');

//signature is aligned with redux reducers (that's why state goes first)
const removeFromState = (state: State, id: string): State => compose(
  state => ids.modify(ids => filter(id_ => id_ !== id, ids), state),
  state => entities.modify(entities => remove(id, entities), state)
)(state);

But it could be:

const removeFromState = (state: State, id: string): State => compose(
  ids.modify(filter(id_ => id_ !== id)), //curried modify from Lens and curried filter from Array
  entities.modify(remove(id)) //curried modify from Lens and curried remove from StrMap
)(state);

Curried version looks much more cleaner. Also I don't think that performance penalty needed for currying is critical, but if it is, curried versions can be changed to uncurried with postfix anytime.

Also it looks like API is a bit inconsistent if some functions are uncurried to be aligned with javascript defaults and some are not to fix TS type inference. I think it would be better to preserve consistency throughout the codebase.

What do you think? If it looks like a massive breaking change then maybe you could export curried versions with prefix/postfix? I saw curriedConcat in Array.

@SimonMeskens
Copy link
Collaborator

Also it looks like API is a bit inconsistent if some functions are uncurried to be aligned with javascript defaults and some are not to fix TS type inference

I recently mentioned this as well and it breaks compatibility with Ramda too. I plan on looking into a solution, but until type inference gets better, it's a bit of a hairy issue.

@gcanti
Copy link
Owner

gcanti commented Aug 15, 2017

I think it would be better to preserve consistency throughout the codebase

So do I, the point is consistency with respect to which policy? Current APIs are consistent to the base policy mentioned above

APIs are never curried unless type safety is compromised

(well except some mistakes like Functor.voidRight that I'd like to fix)

We can definitely change the policy, or change any API, name or module, if the ergonomics can be improved.

However I'll refuse any change which leads to lesser type safety (which is a key point of this library)

@raveclassic
Copy link
Collaborator Author

raveclassic commented Aug 15, 2017

However I'll refuse any change which leads to lesser type safety (which is a key point of this library)

I understand this clearly and do agree but I don't get what kind of regression are you talking about mentioning curried functions. Your example with traversables shows that curried version is better then uncurried in type checking and emits compile-time error.

traverse1(option)(a => right(a), [1, 2, 3]) // error :)
traverse2(option, a => right(a), [1, 2, 3]) // no error :(

Do I get it wrong?

If you mean that functions with broken type inference should not be exported at all then the way to export uncurried versions with a postfix fits even better because such function can be dropped in a particular case. Or the lib can fully drop all uncurried versions for the sake of consistency. Though with a light perfomance penalty.

@gcanti
Copy link
Owner

gcanti commented Aug 15, 2017

Your example with traversables shows that curried version is better then uncurried in type checking and emits compile-time error

Indeed! Sorry, I expressed myself poorly, my example was just to show and justify the rationale behind my choice, i.e. a mix of uncurried (the "standard") and curried functions (the exceptions) and why I find it consistent with the goal of this library (type safety first of all), not against your proposal :)

@raveclassic
Copy link
Collaborator Author

Ah I see :)
So the question's left: to be or not to be to curry or not to curry?
It is extremely handy to use curried versions with compose/pipe but I know it's a massive breaking change. What do you think?

@gcanti
Copy link
Owner

gcanti commented Aug 15, 2017

Well, the good thing about a type system is that refactorings are simpler. I don't mind to have breaking changes if they are well justified (by improved type safety or improved ergonomics)

We could put up a list of breaking changes we all agree upon. Perhaps some pratical examples would also help to focus on the new policy.

Also, if we are going to publish a breaking change release, then I'd love to fix some type safety issues (like in voidLeft / voidRight mentioned above)

In other words we can work on a "clean-up" release, which hopefully will fix all my arguable choices and bring us closer to v1.0.0

@raveclassic raveclassic mentioned this issue Aug 15, 2017
60 tasks
@gcanti
Copy link
Owner

gcanti commented Aug 16, 2017

I'll start from the most important change with respect to type safety

Type constraint rule. If there is a type constraint then a function must be curried, at least in its type constraint arguments

Motivations

Type safety. For example the current uncurried version of Functor.voidLeft is unsafe

import { voidLeft } from '../src/Functor'
import { option } from '../src/Option'
import { right } from '../src/Either'
voidLeft(option, right(1), 2) // no error

Ergonomics

It will be easy to specialize a generic function like Functor.voidLeft or Functor.lift

import { lift } from '../src/Functor'
import { option } from '../src/Option'

const optionLift = lift(option)

@gcanti
Copy link
Owner

gcanti commented Aug 16, 2017

What about Binary operations?

There are plenty of them: Setoid.equals, Semigroup.concat, Alt.alt etc..

Are the majority of usages saturated?

import { equals, some } from '../src/Option'
import { setoidNumber } from '../src/Setoid'

const optionEquals = equals(setoidNumber) // <= Type constraint rule

optionEquals(some(1), some(2)) // <= typical usage?
// or?
optionEquals(some(1))(some(2))

@sledorze
Copy link
Collaborator

sledorze commented Aug 16, 2017

For reference, new v8 engine (Turbofan) performance characteristic (includes currying)
(https://www.nearform.com/blog/node-js-is-getting-a-new-v8-with-turbofan/)

@gcanti
Copy link
Owner

gcanti commented Aug 16, 2017

@raveclassic Or we can tackle this issue the other way around: let's say we go full curried, i.e. the policy would be dead simple: all functions are completely curried, always. Any cons? For example awkwardness, e.g. myApi(a)(b)(c)(d)? Performances? Bad stack traces? Other?

@sledorze
Copy link
Collaborator

sledorze commented Aug 16, 2017

one can derive a curried version from a unccurried, performant code.
one cannot derive a unccurried (performant) code from a curried version.

one can derive a type correct code from a type correct code.
one cannot derive a type correct code from a type incorrect code.

from my understanding, performant, uncurried code can sometimes be incorrect.

also we want to have correct, type safe code.

Therefore, by deduction, I would think that the best would be to have:

  • performant, uncurried version of all functions but the one which a not type safe.
  • additionnally, curried version of all functions.

Is it correct or am I missing something?

@raveclassic
Copy link
Collaborator Author

raveclassic commented Aug 16, 2017

@sledorze

new v8 engine (Turbofan) performance characteristic

Nice! Seems like currying doesn't affect performance notably.

Is it correct or am I missing something?

AFAICS that's correct and that's why I'd suggest keeping original perfomant uncurried functions with a postfix like it's done in sanctuary.js and provide handy curried versions with a name without a postfix.
Like uncurried remove_ and curried remove. Still if there's a type-checking regression in an uncurried function then only curried should be exported like in the case of voidLeft.

@gcanti IMO it's really that simple - if we curry, then we do it fully. Even binary functions. I don't see any reasonable cons except that you've mentioned. But the profit of currying outweighs them.

@gcanti
Copy link
Owner

gcanti commented Aug 16, 2017

@sledorze That is correct, but I'm afraid you end up with a mess. I'm wary of duplicated APIs. And what about type classes and instances?

Type classes

export interface Functor<F> {
  readonly URI: F
  map<A, B>(f: (a: A) => B, fa: HKT<F, A>): HKT<F, B>
  map<A, B>(f: (a: A) => B): (fa: HKT<F, A>) => HKT<F, B>
}

I'm not even sure it compiles...

(Ok, I tried.. it doesn't compile)

Instances

// Either.ts
export function getSetoid<L, A>(setoid: Setoid<A>): Setoid<Either<L, A>> {
  return {
    equals: (x, y) => fx.equals(setoid, fy) // uncurried version
  }
}

export function getCurriedSetoid<L, A>(setoid: Setoid<A>): Setoid<Either<L, A>> {
  return {
    equals: x => y => fx.equals(setoid, fy) // curried version
  }
}

@raveclassic I'm leaning towards the "curry all the things" policy, it's the only clean and consistent solution (with perhaps some notable exceptions, like compose or pipe)

I'd prefer to not clutter the codebase with two (o more) versions of everything (a priori).

I'd love to have one and only one well-typed version of each API.

Then, if you hit a (real, benchmarked) performance problem, you can always optimise that particular computation by hand. Or, in the face of a verified full-blown problem, we can even provide an additional uncurried API (a posteriori)

@raveclassic
Copy link
Collaborator Author

@gcanti Yep, sounds good! Actually I'm not getting hit by any performance problems so I'm ok with the only one (curried) version.

@sledorze
Copy link
Collaborator

@gcanti that's good to me

@raveclassic raveclassic mentioned this issue Aug 16, 2017
68 tasks
@gcanti
Copy link
Owner

gcanti commented Aug 18, 2017

Wow the first phase took longer than I thought.

Once #212 is merged I think we could talk about the next steps, any ideas?

EDIT: FYI both fp-ts#master and fp-ts#FR/migrate_to_curried compile fine with typescript@rc (2.5)

@raveclassic
Copy link
Collaborator Author

@gcanti Sorry, I'm away from my laptop for the weekend. If you feel you can merge a PR - do it :) We only need to track what's left uncurried

@raveclassic
Copy link
Collaborator Author

Anyway until TS provides proper type inference I think we could go as is for now and pay more attention for doc generation of required quality.

All in all, thanks for the help, it's a large step towards improvement of the lib!

@SimonMeskens
Copy link
Collaborator

Great work guys!

@raveclassic
Copy link
Collaborator Author

So, as #202 is merged, we can close this!

@gcanti
Copy link
Owner

gcanti commented Aug 21, 2017

@raveclassic I'm going to rebase #199. Then I'll publish a fp-ts@next version in order to gather some early feedback

@gcanti
Copy link
Owner

gcanti commented Aug 21, 2017

Released

@gcanti
Copy link
Owner

gcanti commented Sep 1, 2017

@raveclassic how is going with fp-ts@next and monocle-ts@next? Any (negative) feedback?

@raveclassic
Copy link
Collaborator Author

@gcanti The flight is nominal :) No problems were found.

Actually I'm missing some methods on array like find :: Predicate a -> Array a -> Option a, just have no time to add them.

@gcanti
Copy link
Owner

gcanti commented Sep 1, 2017

@raveclassic Thanks. Both officially released.

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

4 participants