search

faylang / fay

A proper subset of Haskell that compiles to JavaScript
https://github.com/faylang/fay/wiki
BSD 3-Clause "New" or "Revised" License
1.29k stars 86 forks source link

Runtime fayToJs - defer automatic_function fayToJs call until after argu... #426

Closed ryachza closed 9 years ago

ryachza commented 9 years ago

...ment application; #409

bergmark commented 9 years ago

Thanks for the pull request!

It is unclear to me what the effect of this change is. Are you able to provide a test case?

ryachza commented 9 years ago

Regarding the effect, the original would run fayToJs on the result of the application of the first argument, and subsequent applications would pass a jsToFay converted arguments into a "JS" function, which would then call jsToFay on the "Fay" argument, etc.

The proposed change should have no effect on curried application since it simply moves the application outside the loop and should be equivalent in the arguments.length == 1 case. It also does not currently appear that calling a nullary function regardless is supported, since the initial jsToFay call results in it's value and the application would throw.

Regarding a test case, I had used the scenario in #409. If you could perhaps direct me to the/any existing tests regarding uncurried application I could look into extending it/them.

bergmark commented 9 years ago

I forgot to reply to this, sorry!

The existing tests for the --strict flag are part of the special cased "compile" tests https://github.com/faylang/fay/blob/master/tests/Compile/StrictWrapper.hs and you can add stuff there. Note that the file has an explicit export list for the functions to expose under Strict, not really sure why I added the list since it has caught me off guard more than once and I'm not sure if it does any good.

if #409 is fixed by this that sounds like a good enough test case to me, simply include it in that test file.

bergmark commented 9 years ago

It's possible that this is the fix for a regression @sebastiaanvisser noticed (that we never managed to make a test case out of).

ryachza commented 9 years ago

Sure I will take a look though it may not be until this weekend. As far as I could tell from reviewing/debugging the runtime it is only an issue for lists/tuples and only when other than the first argument since other data types appeared to have the same representation in Fay/JS and the first argument was already handled properly. I will extend the file referenced with some such cases (zipWith, for example, used in #409 is a perfect fit).

bergmark commented 9 years ago

Thanks!

I might make a release before that, but I can release this separately since it's "only" a bug fix.

ryachza commented 9 years ago

In that case I will make sure to get something together tonight or tomorrow. When are you planning to make a release?

bergmark commented 9 years ago

It really doesn't matter to me whether I make one or to releases this week :-) I just generally don't want to make two releases with API breaking changes close together, so no need for you to rush! I don't have a time plan.

ryachza commented 9 years ago

Added test cases for combinations of one/two raw/wrapped list/tuple.

bergmark commented 9 years ago

Rebased and merged as 93f4f91, thanks!