fantasyland / daggy

Library for creating tagged constructors.
MIT License
700 stars 29 forks source link

Make constructors curried #17

Closed notgiorgi closed 7 years ago

notgiorgi commented 7 years ago

It would be convenient if constructors were curried. (Not by default but when they take less arguments then supposed, like R.curry)

Example:

const Person = daggy.tagged('Person', ['name', 'age'])

const lift2 = (f, x, y) = x.map(f).ap(y)

// this won't work, because Person constructor isn't curried:
lift2(Person, Just('Rick Sanches'), Nothing)

I'll try to send PR if you like the idea.

notgiorgi commented 7 years ago

Also because of rest parameters, constructors are not curry-able easily:

this won't work, because R.curry uses fn.length to determine arity:

const Person = R.curry(daggy.tagged('Person', ['name', 'age']))

so we have to do this:

const _Person = daggy.tagged('Person', ['name', 'age'])
const Person = x => y => Person(x, y)

// or manually set arity
const Person = R.curryN(2, daggy.tagged('Person', ['name', 'age']))

in case of taggedSum, its much more struggle, because you have to extract every ctor and curry them each.

safareli commented 7 years ago

When we are generating constructor functions:

18:  const typeRep = (0, (...args) => makeValue(fields, proto, args))
47:    typeRep[tag] = (...args) => makeValue(fields, tagProto, args)

We can write a function in a way R.curryN is written:

const mkCtr = (fields, proto) => {
  switch(fields.length) {
    case 1: return function(a) { return makeValue(fields, tagProto, arguments) }
    case 2: return function(a,b) { return makeValue(fields, tagProto, arguments) }
    case 3: ... // do this up to 10
    default: Object.defineProperty(
      function(){ return makeValue(fields, tagProto, arguments),
      "length",
     { value: 5 })
    }
}

and change those lines to:

18:  const typeRep = mkCtr(fields, proto)
47:  typeRep[tag] = mkCtr(fields, tagProto)

We should test if it's better performance wise to pass arguments(it might have bigger memory footprint) or [a,b,...] to makeValue.

What you think on performance impact of this change @davidchambers @Avaq

Avaq commented 7 years ago

Intuitively I would think this may increase performance, or at least not reduce it by much. All you're doing is replacing a variadic wrapper function with a fixed-arity wrapper function (fixed arity functions are often a bit faster). The switch statement which determines the fixed arity should not have a big performance footprint, and only happens during the definition of the constructor, unlikely to be in a hot-path.

notgiorgi commented 7 years ago

@safareli what does that default case do?

safareli commented 7 years ago

@notgiorgi if someone creates type with more them 10 arguments (like 20 or whatever) then default variadic function will be provided with length mutated using defineProp (which will be slower in general, so for common cases we should use fixed arity functions)

@Avaq my main concern was difference in passing arguments vs creating new array, as it's stored in created object. if arguments has bigger memory footprint than we should create array instead. (not sure but I think storing arguments would have smaller memory footprint as it's special object which is created all the time and should be optimised by vms, but I might be hardly wrong.)

Avaq commented 7 years ago

@safareli In my experience, using arguments is usually faster.

safareli commented 7 years ago

Also we only expect the stored array to be just ArrayLike so we can just pass in arguments.

@notgiorgi would you like to open PR?

notgiorgi commented 7 years ago

if someone creates type with more them 10 arguments (like 20 or whatever) then default variadic function will be provided with length mutated using defineProp (which will be slower in general, so for common cases we should use fixed arity functions)

Yep, but { value: 5} confused me, shouldn't it be {value: fields.length}?

I think storing arguments would have smaller memory footprint as it's special object which is created all the time and should be optimised by vms

I think thats a legit assumption.

So to sum it up, we're gonna make ctors generated by daggy curriable by 3-rd party libs.

I'll get into it and send PR later this week.

gwn commented 5 years ago

Hi everybody, Are constructors still curried or did it break with new features/fixes since the merging of this PR? Or maybe I'm missing something. Below is what I'm experiencing with Daggy v1.3.0:

> const Foo = daggy.tagged ('Foo', ['x', 'y'])
undefined
> Foo (3)
TypeError: Expected 2 arguments, got 1
    at makeValue (/home/gwn/playground/fp/node_modules/daggy/src/daggy.js:114:13)
    at /home/gwn/playground/fp/node_modules/daggy/src/daggy.js:154:47
safareli commented 5 years ago

@gwn The error is expected https://github.com/fantasyland/daggy/blob/master/test/daggy.js#L19-L29 The pr hasn't made constructors curried. it made them "curriable", which means they have length set to number of expected arguments, so you can use R.curry (instead of R.curryN).