ClojureScript

Implement apply

Details

  • Type: Defect Defect
  • Status: Resolved Resolved
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Approval:
    Test

Description

The time for `apply` is nigh.

Is it as simple as emitting `afn.apply(ctx, args...)`?

Imported from github issue #11

Activity

Hide
Fogus added a comment -

Merged into master. Work performed by mf and ss

Show
Fogus added a comment - Merged into master. Work performed by mf and ss
Hide
Chouser added a comment -

Comment by richhickey, Wed Jun 15 05:35:44 2011:

Nope. Issues are:

laziness - apply is lazy when called with a lazy seq and a fn which consumes the & argument lazily
js apply expects an array, not a seq
we'd like our apply to work on all js functions, not just our fns

One approach:
Our variadic fns indicate they are such by the presence of applyTo (possibly by implementing IVariadicFn protocol? - probably not, as they are of type Function). Rather than handle all arities in applyTo, this applyTo need only handle the variadic method (if the fn is arity overloaded). The variadic fn further provides maxFixedArity. We'll need a boundedLength/count fn (which counts a seq, but only up to a limit). If we are putting applyTo and maxFixedArity on the function objects, they should be scoped names, e.g. cljs$lang&applyTo

The recipe then is:
check fn for applyTo, if not present dump the argseq into an array and call JS apply
else is variadic, call (bounded-count argseq (maxFixedArity fn))
if <= maxFixedArity, dump into array and call JS apply
else call applyTo

Show
Chouser added a comment - Comment by richhickey, Wed Jun 15 05:35:44 2011: Nope. Issues are: laziness - apply is lazy when called with a lazy seq and a fn which consumes the & argument lazily js apply expects an array, not a seq we'd like our apply to work on all js functions, not just our fns One approach: Our variadic fns indicate they are such by the presence of applyTo (possibly by implementing IVariadicFn protocol? - probably not, as they are of type Function). Rather than handle all arities in applyTo, this applyTo need only handle the variadic method (if the fn is arity overloaded). The variadic fn further provides maxFixedArity. We'll need a boundedLength/count fn (which counts a seq, but only up to a limit). If we are putting applyTo and maxFixedArity on the function objects, they should be scoped names, e.g. cljs$lang&applyTo The recipe then is: check fn for applyTo, if not present dump the argseq into an array and call JS apply else is variadic, call (bounded-count argseq (maxFixedArity fn)) if <= maxFixedArity, dump into array and call JS apply else call applyTo
Hide
Chouser added a comment -

Comment by richhickey, Fri Jun 24 05:25:53 2011:

maxFixedArity as implemented ends up being a code-removal blocker for gclosure compiler. I'm not sure if there is another impl approach that will work, or if we need another strategy. Also, let's please avoid dynamic defns in our code.

Show
Chouser added a comment - Comment by richhickey, Fri Jun 24 05:25:53 2011: maxFixedArity as implemented ends up being a code-removal blocker for gclosure compiler. I'm not sure if there is another impl approach that will work, or if we need another strategy. Also, let's please avoid dynamic defns in our code.
Hide
Chouser added a comment -

Comment by stuartsierra, Fri Jul 1 14:24:47 2011:

Laziness defeats me. The trick is making sure that every fn has an `applyTo` property, even if it only has one arity.

Show
Chouser added a comment - Comment by stuartsierra, Fri Jul 1 14:24:47 2011: Laziness defeats me. The trick is making sure that every fn has an `applyTo` property, even if it only has one arity.
Hide
Chouser added a comment -

Comment by stuartsierra, Mon Jul 4 12:35:00 2011:

There are 4 cases to deal with:

1. Single-arity function, non-variadic. These can stay as they are.
2. Multiple-arity function, non-variadic. These can stay as they are.
3. Single-arity variadic function.

  • Need to add `applyTo`
  • Only one code path for invocation
    4. Multiple-arity variadic function.
  • Need to add `maxFixedArgs` and `applyTo`
  • Different code paths when invoked with more or less than `maxFixedArgs`
  • There can be only one variadic fn body!

Case 3 is complicated by the fact that single-arity functions are currently emitted in such a way that additional properties cannot be set on the Function object.

Show
Chouser added a comment - Comment by stuartsierra, Mon Jul 4 12:35:00 2011: There are 4 cases to deal with: 1. Single-arity function, non-variadic. These can stay as they are. 2. Multiple-arity function, non-variadic. These can stay as they are. 3. Single-arity variadic function.
  • Need to add `applyTo`
  • Only one code path for invocation 4. Multiple-arity variadic function.
  • Need to add `maxFixedArgs` and `applyTo`
  • Different code paths when invoked with more or less than `maxFixedArgs`
  • There can be only one variadic fn body!
Case 3 is complicated by the fact that single-arity functions are currently emitted in such a way that additional properties cannot be set on the Function object.
Hide
Chouser added a comment -

Comment by fogus, Sat Jul 16 07:42:59 2011:

This may not make the 7/20 demo. There is a lot conspiring against a solid (and minifiable) implementation. Apply works as always, it's a little smarter about gen'ing the right code in the correct contexts, but it's still not lazy. I believe the problem lies elsewhere, but have not tracked the issue yet. I have a branch at 11-apply-mf if anyone is interested in exploring.

Show
Chouser added a comment - Comment by fogus, Sat Jul 16 07:42:59 2011: This may not make the 7/20 demo. There is a lot conspiring against a solid (and minifiable) implementation. Apply works as always, it's a little smarter about gen'ing the right code in the correct contexts, but it's still not lazy. I believe the problem lies elsewhere, but have not tracked the issue yet. I have a branch at 11-apply-mf if anyone is interested in exploring.
Hide
Chouser added a comment -

Comment by stuartsierra, Sat Jul 16 20:22:06 2011:

For one thing, I think the definition of `apply` should say `(<= (bounded-count args (inc fixed-arity)) fixed-arity)` instead of `(<= (bounded-count args fixed-arity) fixed-arity)`. As written now, the condition is always true.

Changing this on master breaks `apply` in some cases because `cljs$lang$applyTo` is not correctly defined.

Show
Chouser added a comment - Comment by stuartsierra, Sat Jul 16 20:22:06 2011: For one thing, I think the definition of `apply` should say `(<= (bounded-count args (inc fixed-arity)) fixed-arity)` instead of `(<= (bounded-count args fixed-arity) fixed-arity)`. As written now, the condition is always true. Changing this on master breaks `apply` in some cases because `cljs$lang$applyTo` is not correctly defined.
Hide
Chouser added a comment -

Comment by fogus, Sun Jul 17 07:15:53 2011:

Right. You'll notice I fixed the trivial case of apply to do (effectively) that in my branch, but ran out of time to find the source of full realization that it hitting us later on.

Show
Chouser added a comment - Comment by fogus, Sun Jul 17 07:15:53 2011: Right. You'll notice I fixed the trivial case of apply to do (effectively) that in my branch, but ran out of time to find the source of full realization that it hitting us later on.

People

  • Assignee:
    Fogus
    Reporter:
    Anonymous
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: