-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[wip] Compile classes to ES2015 #4330
Conversation
@@ -225,6 +225,7 @@ exports.Base = class Base | |||
# `if`, `switch`, or `try`, and so on... | |||
exports.Block = class Block extends Base | |||
constructor: (nodes) -> | |||
super() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ES2015 derived constructors need explicit super
calls in order to access this
.
@
arguments raise an interesting issue, wherein the this
assignments would typically occur above an explicit super
call. To deal with this I'm unshifting
a super call if any @
arguments are present, and disallowing explicit super
in constructors with @
arguments. This is not ideal.
complex = @variable.isComplex() | ||
if @isNew | ||
fragments.push @makeCode 'new ' | ||
fragments.push @makeCode '(' if complex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wraps complex new
targets in parenthesis (e.g. new (a())
) to ensure there are no errors from calling constructors without new
(e.g. this test was throwing that error without this change).
preface = @superReference(o) + ".call(#{@superThis(o)}" | ||
if compiledArgs.length then preface += ", " | ||
preface = @superReference o | ||
if preface isnt 'super' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bare super
(e.g. in a constructor) isn't a function reference, and needs special treatment.
return [].concat @makeCode("#{ @superReference o }.apply(#{@superThis(o)}, "), | ||
splatArgs, @makeCode(")") | ||
reference = @superReference o | ||
if reference is 'super' or reference.indexOf('super.') is 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses ES2015 splats in super
calls. Hopefully these changes will be superseded by a change to ES2015 splats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's really no way to make a variadic super
call other than argument spread.
#{@tab}})("""), | ||
(@variable.compileToFragments o, LEVEL_LIST), | ||
@makeCode(", "), splatArgs, @makeCode(", function(){})") | ||
complex = @variable.isComplex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiles new
calls with splats to ES2015 splats. Again, a changing to ES2015 splats generally would supersede this.
@@ -753,7 +762,11 @@ exports.SuperCall = class SuperCall extends Call | |||
# method. | |||
superReference: (o) -> | |||
method = o.scope.namedMethod() | |||
if method?.klass | |||
if method?.isMethod and method?.ctor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superReference
is in a bit of a state now. In order to preserve behaviours such as super
in indirect methods and soaked super invocations, the old implementation hasn't been removed. If those were removed this would become much, much simpler.
# This allows `method.compileToFragments` to be called with the correct scope whilst still | ||
# being generated in the class body. | ||
compileToFragments = method.compileToFragments | ||
method.compileToFragments = (o) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the worst hack in the PR. It's being used to allow methods to be compiled with the correct scope with respect to the class body, whilst also being hoisted into the JsClass
. This is needed for the following to work:
class A
a = 1
foo: ->
a = 2 # changes `a` from the class scope
b = 2 # declares a new variable
b = 1
bar: ->
a = 2 # changes a from the class scope
b = 2 # changes b from the class scope
See the test.
Unfortunately, returning []
still leaves trailing semi-colons at the methods previous location, and this doesn't account for method comments, which are also left at their original location.
lhs = new Value(new ThisLiteral, [name]).compile o | ||
@ctor.body.unshift new Literal "#{lhs} = #{utility 'bind', o}(#{lhs}, this)" | ||
|
||
ctor?.body.unshift new Literal 'super(...arguments)' if @parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably just unshift new SuperCall
|
||
ctor?.body.unshift new Literal 'super(...arguments)' if @parent | ||
|
||
finalize: -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second part of the method hoisting hack. Once a class body has been compiled, finalize()
is called to inject the compiled methods into the JsClass
body.
if node instanceof Assign and node.variable.looksStatic name | ||
node.value.static = yes | ||
for node, i in child.expressions | ||
if node instanceof Assign and node.variable.base.value == name and node.value instanceof Code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer only detects static
methods to ensure that @prototype.a = ->
is captured as a class method.
@@ -1172,24 +1300,6 @@ exports.Class = class Class extends Base | |||
node instanceof Value and node.isString() | |||
@directives = expressions.splice 0, index | |||
|
|||
# Make sure that a constructor is defined for the class, and properly | |||
# configured. | |||
ensureConstructor: (name) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructor ensurance is now handled in JsClass
.
@addBoundFunctions o | ||
@body.spaced = yes | ||
@body.expressions.push lname | ||
@hoistDirectivePrologue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this has been reordered...
|
||
if @parent | ||
superClass = new IdentifierLiteral o.classScope.freeVariable 'superClass', reserve: no | ||
@body.expressions.unshift new Extends lname, superClass | ||
@body.expressions.unshift new Assign \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using the extend
utility, we are compiling an extends
in the JsClass
. However, old superReference
behaviour still depends on __super__
(unless there's a way to look that up in vanilla JS).
|
||
children: ['params', 'body'] | ||
|
||
isStatement: -> !!@ctor | ||
isStatement: -> !!@method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't working correctly (@method
vs. @isMethod
) 😨 Not sure what will be broken...
name.shift() if name[0].code is '.' | ||
code.push name... | ||
|
||
if @isDerivedCtor then for param in @params when param.name.this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derived constructors must call super
before using this
, which conflicts with @
arguments going at the top of the constructor.
I think the best way to deal with this would be to automatically call super
as the first statement in all derived constructors, disallowing it from ever being written explicitly. This would have implications with changing the arguments passed to superclasses and writing factory constructors that want to return
other instances.
@@ -449,7 +449,7 @@ test "#1591, #1101: splatted expressions in destructuring assignment must be ass | |||
|
|||
test "#1643: splatted accesses in destructuring assignments should not be declared as variables", -> | |||
nonce = {} | |||
accesses = ['o.a', 'o["a"]', '(o.a)', '(o.a).a', '@o.a', 'C::a', 'C::', 'f().a', 'o?.a', 'o?.a.b', 'f?().a'] | |||
accesses = ['o.a', 'o["a"]', '(o.a)', '(o.a).a', '@o.a', 'C::a', 'f().a', 'o?.a', 'o?.a.b', 'f?().a'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ES2015 classes, prototype
is read-only, so the C::
case won't work.
delete Function.prototype.new | ||
|
||
|
||
# test "Overriding the static property new doesn't clobber Function::new", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete TwoClass.new
won't work now as it's inherited from OneClass
. I wasn't sure how to update the test properly (e.g. delete OneClass.new
works but probably invalidates the test).
@@ -464,7 +466,7 @@ test "ensure that constructors invoked with splats return a new object", -> | |||
# Ensure that constructors invoked with splats cache the function. | |||
called = 0 | |||
get = -> if called++ then false else class Type | |||
new get() args... | |||
new (get()) args... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
()
was necessary to ensure the new
is called on the result of get
, otherwise you get a "constructors must be called with new" error.
@@ -481,6 +483,7 @@ test "`new` works against bare function", -> | |||
test "#1182: a subclass should be able to set its constructor to an external function", -> | |||
ctor = -> | |||
@val = 1 | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to avoid a "derived constructors must return an object or undefined" error.
workingArray = new WorkingArray | ||
ok workingArray instanceof WorkingArray | ||
eq 'yes!', workingArray.method() | ||
# test "#2359: extending native objects that use other typed constructors requires defining a constructor", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is broken now, since brokenArray instanceof BrokenArray
and brokenArray instanceof Array
with ES2015 inheritance :D
class C extends B | ||
m: -> super | ||
eq 5, (new C).m() | ||
# test "#1392 calling `super` in methods defined on namespaced classes", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is broken by the last case: new C.a
throws a "C.a is not a constructor". ES2015 class methods (static or not) can't be used as constructors.
# @a::m = -> super | ||
# eq 5, (new C.a).m() | ||
|
||
# test "dynamic method names and super", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test would pass if the indices in dynamic class methods were cached properly, but I had some difficulty implementing that correctly. E.g.
class A
"#{m}": ->
Currently compiles to:
class A {
[m] () {}
}
Should be
var _ref = m
class A {
[_ref] () {}
}
# m: -> super | ||
# eq 5, (new C).m() | ||
|
||
# test 'dynamic method names using class context', -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where executable class bodies and ES2015 classes are incompatible: when class-context variables are referenced in dynamic keys. The problem is that the class methods are all defined above the executable statements, so they're not available in the compiled class.
Possible solutions:
- Compile all dynamic keys to ye olde
.prototype
assignments. - Attempt to detect class-scoped variables in dynamic keys and compile them to
.prototype
assignments. - Give up on executable class bodies (sadpanda).
|
||
""" | ||
eq CoffeeScript.compile(input, bare: on), result | ||
# test "#3132: Place block-comments nicely", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment ordering is screwed up by class method hoisting.
|
||
b = new B | ||
b.foo => eq b._i, 3 | ||
# test "#1183: super + fat arrows", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super
in closures isn't working atm. We could compile to the old form if we detect a super
in a closure, or else we could compile closures in methods to new-style arrow functions (but that would have implications for scope).
Some thoughts on "Dynamic method names referencing variables that change in the class body is broken." I had hoped we might be able to detect problem cases to compile to a = 'hello'
foo = ->
[a, old] = ['world', a]
a
class Blah
# This uses a variable outside the class scope
"#{a}": ->
# Which gets mutated
foo()
"#{a}": ->
# class Blah {
# [a] () {}
# [a] () {}
# }
# ...not what we wanted I think the only way this could work is by compiling dynamic keys to |
Wow, nice work! I'm not actually 100% sure we have to drop executable class bodies. Is there a downside to declaring functions as |
@vendethiel they don't appear to be bound, I think the only difference is that they're not enumerable. I'm hoping there's some not too complicated way of identifying 'unsafe' dynamic assignments and/or restricting ESBs such that |
@carlmathisen |
When we were initially discussing keeping support for class properties, in my head I was assuming we would take the approach Babel does here, defining as part of the constructor. Is there a reason this PR’s |
@GeoffreyBooth I've been thinking about that, the only way the prototype pattern is 'better' is because it is consistent with method definitions: class A
property: 1
method: -> Compiling one to a prototype property and the other to an instance property could be argued to be confusing. However, there's no technical reason that we couldn't compile function literals to prototype properties, and anything else to a A = (function () {
class A {
constructor () {
this.property = 1
}
}
A.prototype.method = function () {}
return A
})() |
Actually, this gets complicated with object literals in executable class bodies 😢 class A
if something
property: 1
method: -> console.log 'something is true'
else
property: 2
method: -> console.log 'something is false' We could pull non-function properties in literals into the constructor... A = (function () {
var _ref
class A {
constructor () {
if (_ref) {
this.property = 1
} else {
this.property = 2
}
}
}
_ref = something
if (_ref) {
A.prototype.method = function () {
console.log('something is true')
}
} else {
A.prototype.method = function () {
console.log('something is false')
}
}
return A
})() |
Wow, I never realized the properties defined in class bodies were assigned to prototype the same way as functions. It makes sense and is consistent, but I feel this to be completely counterintuitive: class Foo
property: 'defaultVal'
method: ->
a = new Foo()
b = new Foo()
a.property = 'a'
b.property = 'b'
console.log a.property # 'a'
console.log b.property # 'b'
delete a.property
console.log a.property # 'defaultVal' I think it's worth considering to completely remove this feature as I'd doubt most users understand what it really does. I don't see real benefits to it. Idem for ES public class fields. I think it's quite useless feature. But that is just my opinion. @carlmathisen: btw this proposal only reached stage 2, so it's definitely not part of ES2017 (see your babel repl example includes the stage2 preset). |
Also, I'd say there should be at least an example on the docs with properties to show how that behave. |
Late to the party here, but agree with the earlier comment by @bjmiller. Can CS extend ES6 classes in such a way that the CS subclasses can use executable class bodies and other CS goodness? |
In addition to testing with React, you may want to consider testing with the Model class from Objection.js. Even though it's much less-known than React, it is intended as an abstract class to be subclassed, similarly to React's Component class. When using Objection.js with CoffeeScript, I had to add a line of boilerplate after each CS subclass definition to call Model's extend hook:
Babel compiles ES6 subclasses in such a way that the hook was called in the resulting ES5 code, so this explicit call was not necessary when using Babel. To achieve backward compatibility with CS code and derive from classes in ES libraries, it would be great if CS 1.x could detect and extend ES6 classes as follows:
Going the other direction — compiling CS classes to ES6 classes that can be extended by ES6 code — would be a breaking change and should probably be held for 2.0. (Extending from CS to ES subclasses is the much less common use case, given that most of the ES community doesn't care much for CoffeeScript. At least until CoffeeScript is modernized to full ES compatibility, we are much more likely to want to use ES classes than they are likely to want to use modules originally written in CoffeeScript.) |
@svicalifornia Thanks for these detailed notes. Do you have time to write any tests that would prove that the new I think for various reasons listed in coffeescript6/discuss#22 there’s no way to output the I don’t think ES being able to extends CS2-generated classes is an edge case that should be dismissed. Just think of any Node module written in CoffeeScript, that exports a class. We want those to work. |
@GeoffreyBooth Yes, you're right. I didn't mean to dismiss that use case of ES extending CS classes, and it should be part of the acceptance criteria for CS 2.0 compilation to ES But that is not a supported use case today with CS 1.x, so I think it's still fair to say that the other use case of CS 1.x extending ES classes properly is a higher priority. If CS 1.x code can make better use of ES classes, that would bring a lot of benefits to existing CS 1.x projects that either can't or won't make the leap to 2.0 right away. And perhaps we can achieve those benefits for CS 1.x before CS 2.0 is ready for production use, just as you made import/export available in CS 1.x. (Great job, btw!) Unfortunately, I do not currently have the bandwidth to contribute code, but I am happy that these efforts are underway, and I'll continue to offer ideas and feedback. |
@svicalifornia CoffeeScript 1.x is fixed at outputting ES5-compatible code, with the exceptions of modules and generators which are opt-in by typing those keywords in your code. If we wanted to be purists we would’ve limited those features to CS2, to keep things totally simple (CS1 = ES5, CS2 = ESNext) but that principle was violated with generators a few years ago and modules seemed to fall into the same category. CoffeeScript 1.x So we can’t change the output for If there’s some way to extend an ES class without needing to use the |
@GeoffreyBooth I never said that CS 1.x should output ES6 However, CS 1.x could and should do a better job of extending ES classes. When extending a class, CS 1.x should check whether the superclass is a CS class or an ES class. It should map I hope that the most prominent missing bits are simple enough that they can be added to CS 1.x while CS 2.0 is still being discussed, finalized, and implemented. |
Sure. Could you write up how all that could be done? Not the code to put in the compiler, but the general approach as to how to check whether it’s extending a CS class versus an ES class, etc. Maybe post it as an issue at https://github.com/coffeescript6/discuss/issues and it can become a new effort. |
This was one of my biggest disagreements with the CS6 direction. I felt that ES6 classes should be "fixed" to be more like original CS classes. When extending them, you can feature-test whether Object.getPrototypeOf and Object.getOwnPropertyNames exists, and use that to find all of the properties of a superclass to copy onto the new CS class. It would take some doing, but that can make all classes compatible with one another, Any CS code can extend any classes from ES6 or CS, and you can choose to improve the design thereafter with callable constructors, real private members, etc. Actually using ES6 classes binds you to their limitations, so CS classes can never have these features that people come around looking for. |
I agree with @bjmiller. Why should a good student take after bad ones :) |
Someone posted a table of ES class features and their support in popular JS engines. We should go down the list of features and figure out which ones are not currently handled well in CS 1.x, perhaps with priority to the features that would improve React compatibility. We don't need 100% all at once, but a plan (and some tests, as @GeoffreyBooth suggests) for iterative improvement. |
@svicalifornia Support is complete in Node: http://node.green/#class and in Edge, Firefox, and Chrome; mostly complete in Safari 9 and complete in Safari 10 and WebKit: http://kangax.github.io/compat-table/es6/#test-class. I think the main feature that imperils interoperability is the inability to extend an ES class. (#4233) We need to be able to do that, while still trying to preserve as many of CS classes’ features as possible. |
I remember a different table that just showed various features of class support, such as |
I'm working on this again now, it's getting pretty close but one thing that's a bit odd is derived constructors with class A extends B
constructor: (@a) -> With the current compilation, the Even adding an explicit Currently, my solution to this is to add an implicit bare constructor: (a) ->
this Will fail, whilst one like constructor: (@a) ->
this Will succeed, thanks to the implicit The two most sane solutions to this, I think, are:
Personally I think that option 2. is unworkable, due to the huge volume of codecitation needed that uses the convenient pattern of initialising instance fields with constructor arguments. Option 1.i. does require some additional explaining, however it should be transparent for the common cases of forgetting to call |
Superseded by #4354. |
I started looking at adding some tests for executable class bodies to check for regressions when we begin to compile ES2015 classes, and I ended up writing an implementation. Sorry if this clashes with anyone else's work for coffeescript6/discuss#22.
This is some unfinished, work in progress changes to compile CoffeeScript classes into ES2015 classes. I've only committed the source files, so if y ou want to run it you'll need to
cake build && cake build:parser
.The compiler successfully self hosts, however THERE ARE DEFINITELY A LOT OF BUGS AND THE CODE IS NOT PRETTY. It might be useful as prior art and/or to resolve some discussions on how CS should proceed with ES2015 classes.
I tried not to remove any CS functionality, including executable class bodies, soaked super invocations, etc. Quite a bit of code could be removed/cleaned up if they were removed.
I'll add some comments in-line, and later update this with a TL;DR of the significant issues.
See coffeescript6/discuss#22 for more context.
Top issues: