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

[CS2] Output ES2015 arrow functions, default parameters, rest parameters #4311

Merged
merged 48 commits into from
Oct 26, 2016

Conversation

GeoffreyBooth
Copy link
Collaborator

WIP. Just thought I’d get the ball rolling and try to establish a precedent for merging into the 2 branch. I’ve only spent a few minutes on this, and I was surprised how easy it was to get => to output as =>; although I get the sense that there are a lot of considerations to => that I’m not considering. Please enlighten me.

Only two tests fail with this patch, but I’m not sure that they should pass:

test "`new` works against bare function", ->
  eq Date, new ->
    eq this, new => this
    Date

test "#2009: don't touch `` `this` ``", ->
  nonceA = {}
  nonceB = {}
  fn = null
  (->
    fn = => this is nonceA and `this` is nonceB
  ).call nonceA
  ok fn.call nonceB

@lydell actually commented back when he wrote the second test that it would fail when we output =>. So maybe it’s time to remove that test?

As for the first one, I get the sense that the test was written expecting the arrow function to be wrapped by an IIFE. I presume that the point of outputting => for => is to get our output closer to its input, and therefore we shouldn’t be dropping unexpected wrapper functions into the output:

./bin/coffee -bpe 'foo = (bar) => console.log bar, @'
var foo;

foo = (bar) => {
  return console.log(bar, this);
};

Is this correct? Or should this test pass somehow?

There must be lots of additional code scattered across the repo that can be removed now that we needn’t do so much work to parse =>. What cleanup needs to be done?

@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Sep 20, 2016
@lydell
Copy link
Collaborator

lydell commented Sep 20, 2016

new (function () {}) // Valid JavaScript
new (() => {}) // Runtime error.

Both tests should be removed, in my opinion.

Are there no tests that include both fat arrows and parameter splats? I'd expect that to fail, since splats are implemented with arguments, which is not available in fat arrows.

a = (args...) => args

@connec
Copy link
Collaborator

connec commented Sep 20, 2016

Are there no tests that include both fat arrows and parameter splats? I'd expect that to fail, since splats are implemented with arguments, which is not available in fat arrows.

There's probably a whole debate to be had about whether we really want to output ES2015 arrow functions. I guess we do, but perhaps we should add special handling for arguments? Perhaps not since it's very 'magical'. But then users need to remember that there's a concrete difference between -> and => beyond the value of this.

@connec
Copy link
Collaborator

connec commented Sep 20, 2016

Just another thought (forgive me if this is already handled in the PR): would we want to special-case single-JS-statement fat arrow functions and generate shorthand methods in that case?

a = => console.log 'hello'
# var a = () => console.log('hello')

a = => console.log 'hello' ; console.log 'world'
# var a = () => {
#   console.log('hello')
#   console.log('world')
# }

@lydell
Copy link
Collaborator

lydell commented Sep 20, 2016

@connec arguments is not a problem. Here’s a lazy solution:

a = (..._arguments) => {
  ...
}

@connec
Copy link
Collaborator

connec commented Sep 20, 2016

OK, I guess I was just wondering aloud whether or not we should continue to deal with arguments in fat arrow functions or whether we should adopt ES6 behaviour (arguments is undefined).

E.g. what should this CS produce:

() => arguments

Currently it compiles to:

function() {
  return arguments;
}

With this change:

() => {
  return arguments;
}

Ultimately, would we want to catch calls to arguments in bound functions and set them up to instead output:

(..._arguments) => {
  return _arguments;
}

@lydell
Copy link
Collaborator

lydell commented Sep 20, 2016

I think it makes sense to simply disallow arguments in fat arrow functions.

@bjmiller
Copy link

I vote no for outputting es6 fat arrows. Original CS fat arrows are objectively better, and there is no advantage to having es6 arrows in the compiled code.

Can everyone please stop trying to add es6 things to the compiled output just because they're new?

@lydell
Copy link
Collaborator

lydell commented Sep 20, 2016

@bjmiller I would be very interesting to hear what makes CS fat arrows better! :)

@GeoffreyBooth
Copy link
Collaborator Author

Re the one-line syntax for arrow functions, I think we shouldn't bother. We don't output if foo then bar() or a = { b: 1 } as one-liners.

Re arguments, one of the goals of the 2.x effort is to align more closely with ECMAScript. Part of that involves no more sugar to “correct” ECMAScript’s oversights as we may perceive them, primarily because ES is evolving so fast and they might fix said oversight themselves soon enough and we don't want our polyfill to behave differently than their new syntax. In the case of arguments specifically, we should just follow the spec: if they left it out, presumably they have their reasons, and it's no more difficult in CoffeeScript to need to know that arrow functions lack arguments than it is to know the same in JavaScript.

@atg
Copy link

atg commented Sep 20, 2016

My intuition of CoffeeScript functions is that -> has a free this but => has a bound this. That's it. It's kind of weird for arguments to depend on the whether this was bound or not. If you are going to ban arguments then ban it from both or neither.

@lydell
Copy link
Collaborator

lydell commented Sep 20, 2016

I wouldn't mind "banning" arguments altogether – in fact, I don't think I've ever used in in CoffeeScript, since we have splat parameters.

@atg
Copy link

atg commented Sep 20, 2016

I personally would not mourn the loss of arguments, however removing it has a high potential to break existing code. ¯_(ツ)_/¯

My view is that it's desirable for -> and => in CoffeeScript to behave as identically as possible, and not to introduce too many surprises or gotchas. The current (ES3) implementation performs perfectly in this regard. It binds this and otherwise works exactly the same as ->.

In some ways, ES6 arrows are worse. You can't yield inside an ES6 arrow, so CoffeeScript would have to fall back to the current implementation to continue to support that combination of features.

So I agree with @bjmiller there's really nothing wrong with the way things currently work, and it has the added benefit of being compatible with ES3 and ES5 as well.

@GeoffreyBooth
Copy link
Collaborator Author

If you want to create a bound generator function after this change, you would do so the same way in CoffeeScript as you would in ECMAScript: just declare self = this in the enclosing function, like the old days. This doesn’t strike me as terrible. Likewise, having to write (args...) => instead of referencing the magic arguments object also doesn’t strike me as terrible.

What’s “wrong” with current arrow functions is that they unnecessarily deviate from the ES2015 spec. Now that JavaScript has arrow functions, we should output arrow functions. Anything else is counterintuitive.

@lydell
Copy link
Collaborator

lydell commented Sep 21, 2016

I was confused in my previous comments. arguments is not disallowed in ES arrow functions. They refer to the outside arguments (just like this).

$ ./bin/coffee -e 'console.log(((a...) -> a) 1, 2, 3)'
[ 1, 2, 3 ]

$ ./bin/coffee -e 'console.log(((a...) => a) 1, 2, 3)'
[]

The second example returns an empty array, because of the arguments from CoffeeScript's IIFE wrapper.

I'm surprised that there were no tests for this! (Both examples above should of course log the same thing: [1, 2, 3]).

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Sep 21, 2016

To complete @lydell’s example, here’s the ES2015 equivalent:

console.log(((...a) => a)(1, 2, 3))
// [1, 2, 3]

So there is a way to access a limitless number of arguments in an arrow function, so long as we make sure to compile ... into ES2015 ... in any parameters (e.g. args... to ...args). See also.

That just leaves the generator issue. Is it really so common to make a bound function generator? Is self = this in the outer scope not good enough for what I presume to be an uncommon use case?

@GeoffreyBooth GeoffreyBooth changed the title Output ES2015 arrow functions [wip] Output ES2015 arrow functions Sep 25, 2016
@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Sep 25, 2016

Okay @lydell, I did some work on splats/rest parameters. I could do a lot more to eliminate now-unnecessary code, but I just want to get this working first. Anyway, as of my latest commit your example:

a = (args...) => args

becomes

var a;

a = (...args) => {
  return args;
};

Since we’re not converting args... into ES5 code involving arguments anymore, we needn’t worry about arguments within arrow functions. This also fixes your other example:

./bin/coffee -e 'console.log(((a...) => a) 1, 2, 3)'
[ 1, 2, 3 ]

An interesting disadvantage of the ES2015 rest parameter syntax is that the rest parameter must be the last parameter. So now we have two failing tests:

  test/function_invocation.coffee:173
    mult = function(x, ...mids, y) {
                              ^
SyntaxError: Rest parameter must be last formal parameter

  test/functions.coffee:124
    arrayEq([0, 1], (function(...splat, _, _1) {
                                      ^
SyntaxError: Rest parameter must be last formal parameter

That SyntaxError is thrown by Node. I guess this should just become part of our syntax as well that you can’t use ... except in the last parameter?

This also raises the question of what to do about expansions in parameter lists:

./bin/coffee -bpe '(a, ..., b) => splat'
(() => {
  var a, b;
  a = arguments[0], b = arguments[arguments.length - 1];
  return splat;
});

@lydell
Copy link
Collaborator

lydell commented Sep 25, 2016

Yes, splats have to be the last thing in ES. My first idea for handling that (rather than dropping support) was to do something like:

f = (a, b, args..., c, d) ->
f = function (a, b, c, d, ...args) {
  [c, d] = args.slice(-2);
  args = args.slice(0, -2);
};

Also, could you merge master into 2 and then rebase this branch on 2? It’s impossible to review this PR now with so much crap in the diffs.

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Sep 25, 2016

The diff should be sensible now.

Your example doesn’t work for me:

f = function (a, b, c, d, ...args) {
  [c, d] = args.slice(-2);
  args = args.slice(0, -2);
  console.log(a, b, args[0], c, d);
};

f(1, 2, 3, 4, 5)
// 1 2 undefined 5 undefined

args, at the start of the function, is just [5]. So args.slice(-2) returns [5], thereby setting d to undefined. We can’t use args as a substitute for arguments.

We can, however, recreate arguments (this works for arrow functions too):

f = function (a, b, c, d, ...args) {
  let arguments = [a, b, c, d].concat(args);
  [c, d] = arguments.slice(-2);
  args = arguments.slice(2, -2);
  console.log(a, b, ...args, c, d);
};

f(1, 2, 3, 4, 5)
// 1 2 3 4 5
f(1, 2, 3, 4, 5, 6, 7)
// 1 2 3 4 5 6 7

I guess the question is whether we should. This strikes me as a lot of extra JavaScript to be outputting. I suppose we should do it to prevent breaking backward compatibility, but it feels messy. It’s also so much more difficult to figure out: the slice lines require knowing how many arguments precede and follow the splat, so that you know the indices to slice by. The [c, d] part requires knowing which arguments follow the splat. It’s just a lot of processing, prone to edge cases I can’t imagine, for a feature that I can’t imagine people use all that often.

@GeoffreyBooth
Copy link
Collaborator Author

Better version without intermediate variable:

f = (a, b, args..., c, d) =>
  console.log a, b, args..., c, d

becomes:

f = (...args) => {
  [a, b] = args.slice(0, 2);
  [c, d] = args.slice(-2);
  args = args.slice(2, -2);
  console.log(a, b, ...args, c, d);
}

And f(1, 2, 3, 4, 5, 6, 7) still prints 1 2 3 4 5 6 7.

This is quite complicated to produce, so the question still stands: is this a feature we want to support?

Likewise with expanding parameters:

f = (a, ..., b) =>

This isn’t even mentioned in the docs, though it’s in the grammar. Are we keeping this too?

@lydell
Copy link
Collaborator

lydell commented Sep 26, 2016

Sorry about that broken compilation example. It sure was a "something like." Here's a version that I've actually tested:

var f;

f = function (a, b, c, d, ...args) {
  [c, d, ...args] = args.slice(-2).concat([c, d], args.slice(0, -2))
}

Regarding that it is more complicated to compile: Yes, it is. But at the same time, that's what we've been doing here for like half a decade now.

Regarding whether the feature is useful or not: I can't really think of a case (from the top of my mind) when I've used a splat parameter somewhere else than at the end. However, I have used it in [first, middle..., last] = someArray.

Regarding a, ..., b: Yes, it is missing from the documentation. That's a shame. The feature is not very old (it was added in 1.7.0), and is useful for getting the last item of an array: [..., last] = someArray. That's used quite a bit in CoffeeScript's source code. When it comes to compiling, it's no different than a, args..., b, except that args won't be available for the user to use.

Regarding your suggested compilation: (...args) => means that we loose .length of the function. That's true for todays' compiled output too, which is a shame. Redux preserves .length. I think we should, too, when we have the chance.

Summary:

  • I've never missed non-last-splat-parameter when writing ES2015, but I have missed it in array destructuring.
  • I believe that dropping support for splats-anywhere-in-the-list will be a major backwards incompatibility hurdle for people.
  • We already have lots of code for dealing with it.
  • My suggested output is just one extra line of code.

@lydell
Copy link
Collaborator

lydell commented Sep 26, 2016

Here's an example with parameter defaults:

// f = (a = 1, b = 2, args..., c = 3, d = 4) ->
var f;

f = function (a = 1, b = 2, c, d, ...args) {
  [c = 3, d = 4, ...args] = args.slice(-2).concat([c, d], args.slice(0, -2))
  console.log(a, b, args, c, d)
}

And here's an example with destructuring:

// f = ({a = 1} = {}, b, args..., c, d) ->
var f;

f = function ({a = 1} = {}, b, c, d, ...args) {
  [c, {d = 4} = {}, ...args] = args.slice(-2).concat([c, d], args.slice(0, -2))
  console.log(a, b, args, c, d)
}

@GeoffreyBooth
Copy link
Collaborator Author

I was only referring to function parameters. Limiting splats to the last argument in a function parameter list, the way ES does, doesn’t mean we would need to stop using splats in any position in destructuring.

Assuming splats elsewhere are untouched, are you still sure we want to support splats at any function parameter position? And expansions at any function parameter position?

@lydell
Copy link
Collaborator

lydell commented Sep 26, 2016

I've never been sure :)

Personally, I wouldn't mind dropping splats at any position in parameter lists. And dropping expansion in parameter lists.

The only "bad" thing is that array destructuring and "parameter list destructuring" would no longer be equivalent. The following would no longer be true:

f = (<params>) ->
# equivalent to:
f = (args...) ->
  [<params>] = args

Couldn't we use the same code in both cases?


Another route we could take, is to:

  • Remove expansion.
  • Make splats work like in ES everywhere.
  • Flip the ... to the ES side (...args instead of args... – this regularly trips me up when switching between the languages ;) )

@GeoffreyBooth
Copy link
Collaborator Author

@connec @jashkenas @lydell Not to short-circuit the discussion in coffeescript6/discuss#51, but can we merge in this PR as is? And if we ultimately decide to go the other way regarding null, I can submit another PR. I don’t feel strongly either way on that question, but I want to get this change merged in so that the classes PR can start using it.

@GeoffreyBooth GeoffreyBooth changed the title Output ES2015 arrow functions, default parameters, rest parameters [CS2] Output ES2015 arrow functions, default parameters, rest parameters Oct 23, 2016
@lydell
Copy link
Collaborator

lydell commented Oct 23, 2016

If merging this helps you who actually work on the 2 branch, then go for it, I'd say. The 2 branch doesn't need be be as stable as master.

@GeoffreyBooth
Copy link
Collaborator Author

So I just tried running the tests in this branch under --harmony mode, and I got a surprise. This test fails only in harmony (generators.coffee:55):

test "bound generator", ->
  obj =
    bound: ->
      do =>
        yield this
    unbound: ->
      do ->
        yield this
    nested: ->
      do =>
        yield do =>
          yield do =>
            yield this

It only fails in harmony mode because we’re not running generators.coffee except in harmony mode, because back when generators were added to CoffeeScript that’s the only way they could be run. Now with Node 6 the --harmony is unnecessary, and will be removed as soon as #3757 lands.

Anyway the reason this test fails is because of the yield this inside a bound function. For a simpler example, this:

f = => yield this

compiles (via this PR) to this:

f = () => {
  return (yield this);
};

which Node doesn’t like:

  return (yield this);
                ^^^^
SyntaxError: Unexpected token this

because arrow functions cannot be generators. So this is another breaking change as a result of this PR. And I need to find a way to check that yield is inside a bound function as opposed to a regular function, and throw an error in the former case.

The workaround, ironically enough, is to just use a regular function and the old self = this hack:

self = this
f = -> yield self

… add check to throw error if `yield` appears inside a bound (arrow) function
@GeoffreyBooth
Copy link
Collaborator Author

Fixed. Everyone okay with this change? I suppose the alternative is to fall back to the CS 1.x output for bound functions in the event of a bound generator function, but I feel like that will cause more debugging trouble than it’s worth, by potentially introducing inconsistencies between bound functions with and without yield. Hopefully bound generator functions aren’t so common, and we’re only aligning ourselves with ES here by forcing people to do the self = this rigamarole for bound generator functions. I’ve added this to the breaking changes wiki document.

@lydell, others, is there anything I’m missing here? If not, please let me know and I’ll merge this PR into 2.

assertErrorFormat 'f = => yield this', '''
[stdin]:1:5: error: bound (fat arrow) functions cannot contain 'yield'
f = => yield this
^^^^^^^^^^^^^
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will this look like if the function is multi-line? Is it better to underline yield only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just underlines whatever the first line of the function is, starting at the =>:

./bin/coffee -bpe 'f = =>
quote>   1
quote>   2
quote>   yield this'
[stdin]:1:5: error: bound (fat arrow) functions cannot contain 'yield'
f = =>
    ^^

@GeoffreyBooth
Copy link
Collaborator Author

Updated. Now it only underlines the yield line (assuming I understand what o.scope.method refers to):

./bin/coffee -bpe 'f = =>
  1
  2
  yield this'
[stdin]:4:3: error: yield cannot occur inside bound (fat arrow) functions
  yield this
  ^^^^^^^^^^

@GeoffreyBooth
Copy link
Collaborator Author

@lydell any other notes?

@lydell
Copy link
Collaborator

lydell commented Oct 26, 2016

No :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants