[CS2] Compile all super calls to ES2015 super #4424

connec · 2017-01-19T18:43:46Z

Super quick first pass. This breaks using super in non-methods, meaning several tests are failing. Self-compilation still works.

As I mentioned in coffeescript6/discuss#22 (comment), the main decision to be made here is around support for super in non-class methods. Opinions from @jashkenas, @GeoffreyBooth, @lydell or anyone else very welcome!

Here are a couple of the options:

Go all in with ES2015, drop support for super outside of methods in class initializers. This would clean up some code in the compiler for tracking the classes of methods, special handling of prototype assignments in class bodies, etc.
Why not both? If the super call is in a method in a class initializer, compile it to an ES2015 super, otherwise, compile it as we do now. This would just add a couple of special cases in the handling of SuperCall nodes.
Try and devise some best of both worlds approach. Compile class method to ES2015. Since we can now use ES5+ method, we could drop __super__, method class recording, etc. and replace it all with Object.getPrototypeOf(this.constructor). This could optionally turn super into a fully runtime feature that will compile anywhere, and would provide clean ES2015 output.

Personally, I'm leaning towards 1. for the significant impact I expect it will have on the compiler, and the fact that, realistically, the prototype-based approach is gonna be rare in an ES2015 world. If really, really needed, super in a dynamic method can still be specified explicitly (e.g. BaseClass::method.apply(this, arguments)).

If the compatibility break of such a move is too great, I'd be interested in exploring 3. I think that 2. would not be popular as it would force us to continue using class IIFEs to ensure correct parent class caching, etc. meaning 'ugly' JS output.

mitar · 2017-01-19T18:46:25Z

I think calling super inside anonymous functions inside methods can help sometimes with logic structure. So I would vote to keep existing support and not be as strict as ES2015.

lydell · 2017-01-19T18:53:04Z

I'm all for option 1. See #3796.

kirly-af · 2017-01-19T19:17:44Z

I personally never used super outside of classes, so 1 is completely fine to me.
However 3 would be an understandable choice to avoid breaking changes + nicer output.

jashkenas · 2017-01-19T19:18:50Z

I think since this is already the big breaking change release, option #1 is the way to go.

connec · 2017-01-19T20:40:18Z

OK, I'll go for option 1., and we can discuss about @jashkenas' comment as a separate new feature.

@mitar calling super within anonymous functions is possible, so long as they are arrow functions.

class A {
  method () { console.log('A#method') }
}
class B extends A {
  constructor () {
    (() => super())()
  }
  method () {
    (() => super.method())()
    console.log('B#method')
  }
}
new B().method() // A#method
                 // B#method

GeoffreyBooth · 2017-01-20T06:01:28Z

I also think we should go with option 1, since we can always add the more elaborate versions later.

This PR has a bug though I think. Take this input:

class A
  foo: ->
    console.log 'woohoo!'

class B extends A
  bar: ->
    super.foo()

This branch produces:

var A, B;

A = class A {
  foo() {
    return console.log('woohoo!');
  }

};

B = class B extends A {
  bar() {
    return super.bar(...arguments).foo();
  }

};

but it should produce:

var A, B;

A = class A {
  foo() {
    return console.log('woohoo!');
  }

};

B = class B extends A {
  bar() {
    return super.foo();
  }

};

connec · 2017-01-20T08:41:47Z

Not a bug - that's how CoffeeScript's super works. See #2638 for some discussion on this.

Interestingly, it seems that super of this form used to be planned for ES: https://esdiscuss.org/topic/referencing-super. I can't find any reference to when it was dropped.

Personally, I think being able to look up arbitrary methods on the superclass is an undesirable feature. An overridden method shouldn't need to be aware that other methods may be overridden. I.e. if bar wants to call foo, it should call this.foo; if B#bar wants to specifically call A#foo, it should call A::foo.

It also encourages people to think that super is an entity of its own, perhaps an object containing references to methods of the superclass, but it is not (in ES discussions it was proposed to mean this, which would have meant that super.foo and a = super, a.foo would target different methods!).

jashkenas · 2017-01-20T15:11:57Z

Not a bug - that's how CoffeeScript's super works. See #2638 for some discussion on this.

Interestingly, it seems that super of this form used to be planned for ES: https://esdiscuss.org/topic/referencing-super. I can't find any reference to when it was dropped.

Personally, I think being able to look up arbitrary methods on the superclass is an undesirable feature. An overridden method shouldn't need to be aware that other methods may be overridden. I.e. if bar wants to call foo, it should call this.foo; if B#bar wants to specifically call A#foo, it should call A::foo.

It also encourages people to think that super is an entity of its own, perhaps an object containing references to methods of the superclass, but it is not (in ES discussions it was proposed to mean this, which would have meant that super.foo and a = super, a.foo would target different methods!).

I agree with this (naturally), and I think I've made that argument in these pages before.

But! If we're switching over to ES6 semantics for classes for CS2, shouldn't we switch over to ES6 semantics for super at the same time?

GeoffreyBooth · 2017-01-20T15:44:18Z

A super.foo() like my example wasn't just planned; it's implemented. Paste my "should output" example into Chrome console to see for yourself. We need to support this type of super.

connec · 2017-01-20T15:51:47Z

Well, I'd argue that with classes we've changed very few semantics - we've adopted, by necessity, the class {} syntax (and associated baggage 😅 ), but have avoided removing any significant CS features.

@GeoffreyBooth it's the other form that was planned not implemented (super()), sorry.

I'm not sure I agree that we need to support it. I feel like we're just adding needless redundancy since 99%¹ of uses are just super.<current function>, and any other case is almost certainly a sign of bad design, or a contrived example for a test case. I wish ES Discuss was more searchable so I could figure out why they chose this approach.

Whilst maintaining that it would be best not to, we could make both uses live side-by-side. If we do go down that route we might want to talk about letting 'bare super' go, and mandating regular function syntax (i.e. super arguments... would replace bare super).

1. Made up.

GeoffreyBooth · 2017-01-20T16:12:15Z

Using super to call a different method on the parent is one of the examples on MDN's super page, and it doesn't seem contrived. Regardless of what we do with "bare" super, we need to support this usage too.

jashkenas · 2017-01-20T16:21:10Z

I think that once again, it'll be a step backwards — but we should make it work like JS super works. Keeping different semantics will just breed confusion when people try to use it, and will be a running source of new opened tickets asking for it to be changed.

connec · 2017-01-20T16:55:54Z

OK, lets add super.method.

Next question is bare super - super.method makes it look like super is something that hosts superclass versions of methods, maybe, whereas in CS it is in fact a method call and in ES it's a syntax error. Since we're adopting super.method, I suggest we also adopt a SyntaxError for bare super. This eliminates the ambiguity observed by @GeoffreyBooth above.

To outline the suggested compilation:

class Base
class SuperExamples extends Base
  # Should throw a SyntaxError along the lines of "'super' must be called or accessed"
  bareSuper: -> super

  # Should compile to `super.shortSuper()`
  shortSuper: -> super()

  # Should compile to `super.esSuper()`
  esSuper: -> super.esSuper()

jashkenas · 2017-01-20T17:17:59Z

Yes, we should make it work like ES super — in all respects.

connec · 2017-01-20T17:30:15Z

It looks like I was hasty to say "it's a 'syntactic form' that is only valid within a class initializer" - it seems it's also valid within methods in object initializers:

({
  foo () { super.foo() } // OK
})
({
  foo: function () { super.foo() } // SyntaxError: 'super' keyword unexpected here
})

Of course we don't currently compile object assigns with method values to that syntax. I had planned to look into that at some point to clean up the code I had to put in nodes.Code for the class work, so might investigate that.

jashkenas · 2017-01-20T17:51:54Z

But how are you supposed to get that "method" back onto the prototype from outside?

jashkenas · 2017-01-20T17:53:59Z

Man, it just keeps getting worse and worse...

connec · 2017-01-20T17:55:57Z

Heh, I was just writing a similar example in node, and got the same result. The MDN example shows using it via Object.setPrototypeOf.

Object.setPrototypeOf(a, Lion)
new a().speak()

connec · 2017-01-20T18:02:56Z

This works:

function A () {}
A.prototype = {
  foo () { console.log('A#foo') }
}

function B () {}
B.prototype = {
  foo () {
    super.foo()
    console.log('B#foo')
  }
}

Object.setPrototypeOf(B.prototype, A.prototype)
new B().foo() // A#foo
              // B#foo

So, it seems that super can only appear in 'initializer method literals', and can only target initializer method literals. Otherwise there's a runtime error.

Edit: It seems the methods must be within their original object as well, the following doesn't work:

A.prototype.foo = ({ foo () { console.log('A#foo') } }).foo
...
B.prototype.foo = ({ foo () { super.foo() ; console.log('B#foo') } }).foo

Object.setPrototypeOf(B.prototype, A.prototype)
new B().foo() // TypeError: (intermediate value).foo is not a function

jashkenas · 2017-01-20T18:13:01Z

This works ...

No, as you demonstrate below, there's still no way to add methods to an existing prototype and make use of super. That's the "normal" ES3 ability that we're losing in ES6.

connec · 2017-01-20T19:04:13Z

So it basically seems that super is 'half-bound' at compile time. The prototype can be changed but the 'home object' (as it's referred to in the spec) cannot.

base   = { foo () { return 'base' } }
child1 = { foo () { return super.foo() + ' -> child1' } }
child2 = { foo () { return super.foo() + ' -> child2' } }

try {
  child1.foo()
} catch (e) {
  console.log(e) // TypeError: (intermediate value).foo is not a function
}

Object.setPrototypeOf(child1, base)
console.log(child1.foo()) // base -> child1

Object.setPrototypeOf(child2, base)
console.log(child2.foo()) // base -> child2

Object.setPrototypeOf(child2, child1)
console.log(child2.foo()) // base -> child1 -> child2

method = child2.foo
console.log(method()) // base -> child1 -> child2

function A () {}
A.prototype.method = child2.foo
console.log(new A().method()) // base -> child1 -> child2

connec · 2017-01-26T00:34:16Z

OK, I've pushed some more updates that adds support for super with accessors! See this test and the following one for some contrived examples. @GeoffreyBooth your example now indeed compiles to:

// Generated by CoffeeScript 2.0.0-alpha
var A, B;

A = class A {
  foo() {
    return console.log('woohoo!');
  }

};

B = class B extends A {
  bar() {
    return super.foo();
  }

};

I've also fixed a couple of incidental bugs that were exposed by these changes, including:

Soaked super invocations. There was existing code in Call to deal with soaked super invocations, but as far as I can tell the parse tree would never actually evaluate to a soaked call due to bare super taking precedence (i.e. super?() was interpreted as super()?()).
Generating bound functions for the closures that wrap statement expressions when the expression contains super. This also means super can now be used in constructors within try expressions, etc. There is an edge case for statement expressions containing both super and yield, because bound functions cannot be generators (i.e. try (super; yield)).
Special-case handling to support class @A extends A when compiled as a simple class. There was a test for this that started breaking once classes with parents no longer needed an IIFE wrapper. To comply with the tests, some extra handling was put in place to wrap the class in an IIFE if the parent's name matches the class' name. I wouldn't be opposed to dropping this, as it seems like it would be an unusual case, and the workaround is pretty awkward.

Edit: Looks like this test was added here to fix an infinite recursion bug. The error now would be a (still surprising) ReferenceError: A is not defined, because the second A refers to the not yet defined class. Any thoughts @jashkenas ?
Throw a compile error when calling super with new. Previously, the new would simply not be emitted! From my experiments it seems super with new is syntactically valid JS, but it always fails at runtime with "unsupported 'super' reference", so I've made it a compile error.

GeoffreyBooth · 2017-01-26T01:10:59Z

Awesome work! Assuming all the tests pass, is there anything else to be done for super?

Aside from adding support for static, get and set, is there anything else to be done for classes?

connec · 2017-01-26T09:55:15Z

@GeoffreyBooth I think we need to have a discussion about static. CS already supports static with @name [:=] assignments, which is far more idiomatic within the language (no special syntax). From a JS dev perspective it's also a straightforward conversion (static => this).

Regarding super, I need to review some of the new class tests. At least one of them was testing that super in try blocks won't work, except I believe they should now, yet the test is still passing.

GeoffreyBooth · 2017-01-26T15:52:07Z

We can discuss static, get and set in another thread. What I'm wondering is when the 2 branch is ready for release as the first alpha. It sounds like as soon as you're done checking these tests and perhaps fixing some issues, we're good to release?

jashkenas · 2017-01-26T16:07:34Z

@GeoffreyBooth I think we need to have a discussion about static. CS already supports static with @name [:=] assignments, which is far more idiomatic within the language (no special syntax).

I'd love to keep this sort of thing, and avoid the word static if possible. I think it's a terrible word for the feature, and is just legacy baggage from Java.

connec · 2017-01-26T18:12:01Z

@GeoffreyBooth I think that would be sufficient, yeh. I'll review those tests asap and double check there's nothing else we can throw out.

Also if @lydell or @jashkenas (or anyone with more familiarity with the grammar) have any comments on the grammar changes they'd be very welcome.

lydell · 2017-01-26T18:21:57Z

src/grammar.coffee

-  Super: [
-    o 'SUPER',                                  -> new SuperCall
-    o 'SUPER Arguments',                        -> new SuperCall $2
+    o 'SUPER OptFuncExist Arguments',           -> new SuperCall LOC(1)(new Super), $3, $2


Just double-checking: Was the 'SUPER Arguments' case redundant because of 'SUPER OptFuncExist Arguments'?

What about the 'SUPER' case?

Whoa there — I don't know about all of the ES6 grammar rules and wrinkles ... but wouldn't it make more sense to bring SUPER in as a Value, instead of special-casing all of the different ways you might access it?

Edit: Nevermind -- looking more than just cursorily, you did exactly that!

Yeh, so SUPER Arguments has been directly subsumed by SUPER OptFuncExists Arguments (which allows super?() to parse).

The SUPER case has been dropped as, in light of supporting super with accessors, it becomes confusing whichever way we spin it:

Keep bare super as a super call to the method of the same name, forwarding arguments. This is fine until we have to deal with super.foo, which now looks ambiguous: is it super.foo as it appears, or super().foo? What about (super).foo? Tbh this use of super is useful enough that I'd be happy to leave it in, but it does lead to a number of ways of doing the same thing with super.

Allow bare super, and handle it the same way super() is handled (compiler error in a constructor, and otherwise a reference to super.<current method>). This would be safe and consistent, except it could lead to subtle bugs and misunderstandings for people upgrading to CS2 from CS1.

SO, a bare super will no longer parse. Unfortunately the error message isn't very friendly, and any tips to improve that would be great!

I see, makes sense. 👍

GeoffreyBooth · 2017-01-31T03:32:04Z

@lydell did you have any further notes on this or is it ready to merge?

lydell

Just a few really minor things that shouldn't hold up a merge.

lydell · 2017-01-31T06:17:49Z

test/error_messages.coffee

+
+test "soaked 'super' in constructor", ->
+  assertErrorFormat 'class extends A then constructor: -> super?()', '''
+    [stdin]:1:38: error: Unsupported reference to 'super'


What about illegal instead of unsupported? (The latter almost sounds like this is a TODO comment, doesn't it?)

Hrm, I copied the current node.js error message verbatim. I suspect that might be because if is a bit of a TODO in JS (given past discussions on ES about allowing bare super, amongst other things).

Since we're explicitly banning it, happy to change to "illegal" 👍

I see. It’s fine either way then, I guess.

I've decided to leave this as Unsupported for consistency with JS.

lydell · 2017-01-31T06:18:20Z

test/error_messages.coffee

+
+test "bare 'super' is no longer allowed", ->
+  assertErrorFormat 'class extends A then constructor: -> super', '''
+    [stdin]:1:35: error: unexpected ->


We should add a TODO comment about improving this error message..

lydell · 2017-01-31T06:19:50Z

test/regexps.coffee

-      eq 2, super/ b/g
+      eq 2, super() / b/g
+      eq 2, super()/b/g
+      eq 2, super()/ b/g
      eq true, super /b/g


This line still has a bare super? Also, I'm not sure if these tests makes any sense any longer when the parentheses are needed?

I'm not sure either tbh, I guess it depends whether we consider "super()" a 'callable token'. The test on this line is still valid, though, although I guess it's testing general parens-less calling, rather than anything regex-specific (though, I guess it's checking that a regex in that format is parsed as an arguments...).

I've not made any change for this. This test is pretty large, and seems concerned with testing the parsing of regexen vs. division. This section was added to check that things worked properly when the preceding node is a SuperCall, so from that point of view the test is the same (except now it could just as easily use any other function besides super).

I see. I think this test makes sense the way it is, then. 👍

connec · 2017-01-31T10:50:30Z

I'll aim to address those last comments and review the class super tests this evening.

GeoffreyBooth · 2017-02-02T20:02:10Z

@connec along with making the changes discussed recently, can you please update https://github.com/jashkenas/coffeescript/wiki/%5BWIP%5D-Breaking-changes-in-CoffeeScript-2? Once the info about breaking changes for classes is in there, I think I'll move it into the docs proper.

This breaks using `super` in non-methods, meaning several tests are failing. Self-compilation still works.

`super` can only be called directly in a method, or in an arrow function.

This behaviour worked 'for free' when the parent reference was being cached by the executable class body wrapper. There now needs to be special handling in place to check if the parent name matches the class name, and if so to cache the parent reference.

This removes syntax support for 'bare' super calls, e.g.: class B extends A constructor: -> super `super` must now always be followed with arguments like a regular function call. This also removes the capability of implicitly forwarding arguments. The above can be equivalently be written as: class B extends A constructor: -> super arguments...

`super` with following accessor(s) is now compiled to ES2015 equivalents. In particular, expressions such as `super.name`, `super[name]`, and also `super.name.prop` are all now valid, and can be used as expected as calls (i.e. `super.name()`) or in expressions (i.e. `if super.name? ...`). `super` without accessors is compiled to a constructor super call in a constructor, and otherwise, as before, to a super call to the method of the same name, i.e. speak: -> super() ...is equivalent to speak: -> super.speak() A neat side-effect of the changes is that existential calls now work properly with super, meaning `super?()` will only call if the super property exists (and is a function). This is not valid for super in constructors.

This fixes a bug in the previous super handling whereby using the `new` operator with a `super` call would silently drop the `new`. This is now an explicit compiler error, as it is invalid JS at runtime.

This was mostly code for tracking the source classes and variables for methods, which were needed to build the old lookups on `__super__`.

connec · 2017-02-04T15:01:07Z

@GeoffreyBooth / @lydell I've responded to the review issues. I've left some additional TODOs for the class tests that were added in #4354, rather than trying to tackle them in this PR (all the tests pass, but some should now 'fail' that were testing for broken things that now work...).

In short, I think this is ready to go.

lydell · 2017-02-04T15:09:00Z

Good job, @connec!

connec mentioned this pull request Jan 19, 2017

CS2 Discussion: Output: Classes coffeescript6/discuss#22

Closed

This was referenced Jan 23, 2017

CS2 Discussion: Output: Destructuring coffeescript6/discuss#69

Closed

Redesign coffeescript.org? #4370

Closed

GeoffreyBooth changed the title ~~Compile all super calls to ES2015 super~~ [CS2] Compile all super calls to ES2015 super Jan 26, 2017

GeoffreyBooth added this to the 2.0.0 milestone Jan 26, 2017

lydell reviewed Jan 26, 2017

View reviewed changes

lydell approved these changes Jan 31, 2017

View reviewed changes

Chris Connelly added 10 commits February 4, 2017 14:35

Compile all super calls to ES2015 super

8254bf4

This breaks using `super` in non-methods, meaning several tests are failing. Self-compilation still works.

Use bound functions for IIFEs containing super

56aa57b

`super` can only be called directly in a method, or in an arrow function.

Fix tests broken by compiling ES2015 super

b261534

Prevent calling super methods with new

299dab5

This fixes a bug in the previous super handling whereby using the `new` operator with a `super` call would silently drop the `new`. This is now an explicit compiler error, as it is invalid JS at runtime.

Clean up some old super handling code

bd0285f

This was mostly code for tracking the source classes and variables for methods, which were needed to build the old lookups on `__super__`.

Add TODO to improve bare super parse error

5ea2427

Add some TODOs to improve some of the class tests

32ca5f3

GeoffreyBooth merged commit 396bd4f into jashkenas:2 Feb 4, 2017

GeoffreyBooth mentioned this pull request Feb 4, 2017

CS2 Discussion: Output: Super - CS vs ES6 semantics coffeescript6/discuss#68

Closed

connec deleted the es2015-super branch February 8, 2017 14:51

This was referenced Feb 19, 2018

CS2 Discussion: Output: Classes #4922

Closed

CS2 Discussion: Output: Super - CS vs ES6 semantics #4961

Closed

CS2 Discussion: Output: Destructuring #4962

Closed

[CS2] Compile all super calls to ES2015 super #4424

[CS2] Compile all super calls to ES2015 super #4424

Conversation

connec commented Jan 19, 2017

mitar commented Jan 19, 2017

lydell commented Jan 19, 2017

kirly-af commented Jan 19, 2017

jashkenas commented Jan 19, 2017

connec commented Jan 19, 2017

GeoffreyBooth commented Jan 20, 2017

connec commented Jan 20, 2017

jashkenas commented Jan 20, 2017

GeoffreyBooth commented Jan 20, 2017

connec commented Jan 20, 2017 • edited Loading

GeoffreyBooth commented Jan 20, 2017 • edited Loading

jashkenas commented Jan 20, 2017

connec commented Jan 20, 2017

jashkenas commented Jan 20, 2017

connec commented Jan 20, 2017 • edited Loading

jashkenas commented Jan 20, 2017

jashkenas commented Jan 20, 2017

connec commented Jan 20, 2017

connec commented Jan 20, 2017 • edited Loading

jashkenas commented Jan 20, 2017

connec commented Jan 20, 2017

connec commented Jan 26, 2017 • edited Loading

GeoffreyBooth commented Jan 26, 2017

connec commented Jan 26, 2017

GeoffreyBooth commented Jan 26, 2017 • edited Loading

jashkenas commented Jan 26, 2017

connec commented Jan 26, 2017

lydell Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

jashkenas Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Jan 31, 2017

lydell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connec commented Jan 31, 2017

GeoffreyBooth commented Feb 2, 2017

connec commented Feb 4, 2017

lydell commented Feb 4, 2017

connec commented Jan 20, 2017 •

edited

Loading

GeoffreyBooth commented Jan 20, 2017 •

edited

Loading

connec commented Jan 20, 2017 •

edited

Loading

connec commented Jan 20, 2017 •

edited

Loading

connec commented Jan 26, 2017 •

edited

Loading

GeoffreyBooth commented Jan 26, 2017 •

edited

Loading

lydell Jan 26, 2017 •

edited

Loading

jashkenas Jan 26, 2017 •

edited

Loading