Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[CS2] Compile class constructors to ES2015 classes#4354

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
GeoffreyBooth merged 32 commits intojashkenas:2fromconnec:es2015-classes-new
Jan 13, 2017
Merged

[CS2] Compile class constructors to ES2015 classes#4354

GeoffreyBooth merged 32 commits intojashkenas:2fromconnec:es2015-classes-new
Jan 13, 2017

Conversation

connec
Copy link
Collaborator

@connecconnec commentedNov 6, 2016
edited
Loading

OK, I've pushed what I believe is a more-or-less "ready" version of classes. I've adopted a similar approach to the previous PR, whereby valid methods are hoisted into an ES2015 class declaration, and other expressions are left as-is. I've also made it so that classes are compiled "bare", without an enclosing IIFE, where possible.

I've standardised the hoisting thing a little bit.hoist is now a method onBase, meaning any node can be hoisted. Callinghoist mutates the source node and returns a new target node, which should be put in the tree at the desired output location. When the target is compiled, it returns a fragments placeholder. When the source is compiled, it updates the target's fragments. Finally,Block#compileRoot will expand all placeholder fragments before returning the result.

I've not touchedsuper outside constructors as that adds quite a bit of surface area and would be better handled separately, imo. The downside of this is that, for now, derived classes always need an enclosing IIFE. Handlingsuper is something I'd be happy to look at as a subsequent PR, unless someone else wants to tackle it.

There is one remaining annoyance, which is the assignment. This is needed to allowLiteral to be referenced in the second example below (and throughout the CS source). We could remove this, which would mean that classes created in expressions can only be referenced by the result of the expression (e.g.exports.Foo = class Foo could only be referenced asexports.Foo). I've erred on the side of compatibility for now, as it's easy to take this out later if we so choose.

I've not added any test, the following would be good to have. Any test contributions would be great.

  • @ params with/withoutsuper, including errors
  • bound functions with/withoutsuper, including errors
  • super in arrow functions in constructors
  • super and external constructors
  • ... any more anyone can think of

As a final note, the diff looks fairly large as I ended up rewriting theClass node, however a lot of the logic hasn't changed, its just been moved around to better line-up with the new usage of the node. I felt this was better than continuing to stretch the previousClass out of shape. Apologies to reviewers.

classPolygonconstructor: (@height,@width)->area:->@calcArea()calcArea:->@height*@width
varPolygon;Polygon=classPolygon{constructor(height,width){this.height=height;this.width=width;}area(){returnthis.calcArea();}calcArea(){returnthis.height*this.width;}};
exports.Literal=classLiteralextendsBaseconstructor: (@value)->compileNode: (o)->super
varLiteral;exports.Literal=Literal=(function(superClass){classLiteralextendssuperClass{constructor(value){super(...arguments);this.value=value;}compileNode(o){returnLiteral.__super__.compileNode.call(this, ...arguments);}};Literal.__super__=superClass.prototype;returnLiteral;})(Base);
classAnimalconstructor: (@name)->move: (meters)->alert@name+" moved#{meters}m."classSnakeextendsAnimalmove:->alert"Slithering..."super5classHorseextendsAnimalmove:->alert"Galloping..."super45sam=newSnake"Sammy the Python"tom=newHorse"Tommy the Palomino"sam.move()tom.move()
// Generated by CoffeeScript 2.0.0-alphavarAnimal,Horse,Snake,sam,tom;Animal=classAnimal{constructor(name){this.name=name;}move(meters){returnalert(this.name+(" moved "+meters+"m."));}};Snake=(function(superClass){classSnakeextendssuperClass{move(){alert("Slithering...");returnSnake.__super__.move.call(this,5);}};Snake.__super__=superClass.prototype;returnSnake;})(Animal);Horse=(function(superClass){classHorseextendssuperClass{move(){alert("Galloping...");returnHorse.__super__.move.call(this,45);}};Horse.__super__=superClass.prototype;returnHorse;})(Animal);sam=newSnake("Sammy the Python");tom=newHorse("Tommy the Palomino");sam.move();tom.move();

Supersedes#4330.

@GeoffreyBooth
Copy link
Collaborator

@connec how does this compare with#4330? Is this a replacement, or are you breaking up the classes effort into multiple smaller PRs?

I and@mrmowgli, at least, would like to also work on classes. I’ve invited you both tomy repo, if you’d like to work on a branch there. Or perhaps you’d like to invite us to your repo? Or how would you propose collaborating on this?

@connec
Copy link
CollaboratorAuthor

Sorry, to be clear, this supersedes#4330, and is the 'final' implementation of what was discussed there. namely:

  • Compile class definitions to an ES2015class, rather than a named function.
  • Compileclass A extends B to ES2015extends.
  • Compilesuper in constructors to ES2015super calls.
  • Methods, non-constructorsuper, executable class bodies, etc. are all unaffected.

Some sample of the output:

classTitleextendsComponent@selfClosing=false###  Initialise a `Title` component with some text.###constructor: (@text)->render:->super"<h1>#{@text}</h1>"
varTitle;Title=(function(superClass){classTitleextendssuperClass{/*    Initialise a `Title` component with some text.     */constructor(text){super(...arguments);this.text=text;}}Title.__super__=superClass.prototype;Title.selfClosing=false;Title.prototype.render=function(){returnTitle.__super__.render.call(this,"<h1>"+this.text+"</h1>");};returnTitle;})(Component);

@GeoffreyBooth
Copy link
Collaborator

@connec#4353 has been merged.

I would like to add some more tests. Should I submit a PR against your branch?

jsClass.placeholder.updateFragments result
result

#### Javascript Classes
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we should call this “ECMAScript Classes” orESClass. The phrase “JavaScript class” for me implies afunction-based construct, like what’s generated by the CoffeeScript 1.xclass keyword; as opposed to the ES2015+ output.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

I agree, will update.

@svicalifornia
Copy link

I may be late to the party here (just got the link from the closing of#4330), but adding an implicit call tosuper only in narrow circumstances is unintuitive and unexpected behavior, the likes of which would rightly be criticized as obtuse CoffeeScript Magic gone awry.

Since callingsuper is required to createthis in ES2015, I think it would be far better to let developers callsuper explicitly, and to prevent the use of@ arguments if necessary, with a clear error message explaining why.

@connec
Copy link
CollaboratorAuthor

@svicalifornia nothing's been decided on that front yet, this is just what I thought was a sensible choice.

To be clear: the implementation here will add an implicit call tosuper unless an explicit one exists.

I'm a fan of@ params in constructors since it eliminates having to write out the variable names twice. On top of that, I can see an implicitsuper as quite a nice feature, since if you extend a class and write a constructor, youmust call super in order to use the class. And half the time the arguments you give don't matter, making it tantamount to boilerplate in a lot of cases. If youdo care about the arguments, you'll of course want to provide an explicitsuper, and CS won't let you use@ params in that case.

@GeoffreyBoothGeoffreyBooth added this to the2.0.0 milestoneNov 8, 2016
@GeoffreyBooth
Copy link
Collaborator

@connec I would like to add more tests to this. I can submit a pull request, or transfer your branch to my repo where others can collaborate on it. What is your preference?

@connec
Copy link
CollaboratorAuthor

Sorry@GeoffreyBooth, I've been a bit swamped this week. I've added you as a collaborator as I'd like to stay fully in the loop for this change.

@GeoffreyBoothGeoffreyBooth changed the titleCompile class constructors to ES2015 classes[WIP] Compile class constructors to ES2015 classesNov 11, 2016
@GeoffreyBooth
Copy link
Collaborator

I did some experiments usinghttps://github.com/balupton/es6-javascript-class-interop. I forked it, patched it to work with the current version of Node and its dependencies, and swapped out itscoffee-script for the CoffeeScript of this branch. Seehttps://github.com/GeoffreyBooth/es6-javascript-class-interop/tree/test-coffeescript-2-classes-branch.

So what’s interesting is that in the original, 2 tests fail of 9:

classes interop ➞ es5 extending es5 ✔classes interop ➞ es5 extending es6 ✘classes interop ➞ es5 extending coffee ✔classes interop ➞ es6 extending es5 ✔classes interop ➞ es6 extending es6 ✔classes interop ➞ es6 extending coffee ✔classes interop ➞ coffee extending es5 ✔classes interop ➞ coffee extending es6 ✘classes interop ➞ coffee extending coffee ✔

In my version, 2 tests still fail, but one of them is different:

classes interop ➞ es5 extending es5 ✔classes interop ➞ es5 extending es6 ✘classes interop ➞ es5 extending coffee ✘classes interop ➞ es6 extending es5 ✔classes interop ➞ es6 extending es6 ✔classes interop ➞ es6 extending coffee ✔classes interop ➞ coffee extending es5 ✔classes interop ➞ coffee extending es6 ✔classes interop ➞ coffee extending coffee ✔

So basically, leaving CoffeeScript aside for a moment, ES5 “classes” cannot extend ES2015 classes. Hence, the CoffeeScript 1.xfunction-based class can be “extended” in ES5; but the CoffeeScript 2class-based class cannot. I think this is what we want, but it’s a potential breaking change that we should probably note: in CoffeeScript 2, a CoffeeScript class can extend an ES2015class, but no longerbe extended in ES5. Is there a better way to phrase/explain this?@connec?

I guess this is the core of why we’re making this change in the first place. If ES5 functions could extend/inherit from ES2015 classes, then CoffeeScript 1.x’s classes wouldn’t need changing. But what, if anything, is lost by breaking downstream ES5 code’s ability to extend a CoffeeScript class?

@GeoffreyBooth
Copy link
Collaborator

To take the test fromthe top of#4233, in CoffeeScript 1.x:

./bin/coffee -be "\`class A {}\`; class B extends A; console.log new B"TypeError: Class constructor A cannot be invoked without 'new'

This branch:

./bin/coffee -be "\`class A {}\`; class B extends A; console.log new B"B {}

So I think this should fix#4233 unless there’s something else I’m missing.

@connec
Copy link
CollaboratorAuthor

connec commentedNov 12, 2016
edited
Loading

@GeoffreyBooth yes, in fact thees5 extending coffee failure indicates we're now aligned with ES6 behaviour :)

That said, the way babel does it seems to work too: they workaround it by callingConstructor.__proto__ rather thanConstructor when extending, e.g.

// class B extends AfunctionB(){(A.__proto__||Object.getPrototypeOf(A)).apply(this,arguments)}

I'm pretty regretful I didn't notice that before! That would be an even easier way of fixing compatibility, though it would add a dependency onObject.__proto__ orObject.getPrototypeOf, and so might still be no good for CS1.x.

@connec
Copy link
CollaboratorAuthor

connec commentedNov 12, 2016
edited
Loading

In short, two options:

  • If we want to compile to ES2015classes, it means the user will be responsible for extending them if they continue to use ES5. Thankfully this won't effect babel users since babel already works around the issue.

OR

  • Change howsuper compiles in constructors to match babel's pattern, document that CS now depends on__proto__ orObject.getPrototypeOf.

The latter approach would give coffee full class compatibility, however it would mean we're no longer usingclass. Personally I do prefer that approach, given that usingclass wasn't as simple as I'd hoped it would be.

@connec
Copy link
CollaboratorAuthor

connec commentedNov 12, 2016
edited
Loading

Clearly I've not had enough coffee. Babel's approach also only works with ES5 classes 😢

SO, that at least means this still seems the best approach for future compatibility.

In the interest of providing options, wecould compile to one of the ES5 variants inthis thread:

A=(function(superClass){functionA(){_this=newsuperClass(...arguments)Object.setPrototypeOf(_this,A.prototype)// compile `this`/`@` references to `_this`return_this}Object.setPrototypeOf(superClass,A)Object.setPrototypeOf(superClass.prototype,A.prototype)returnA})(B)

This would be a simpler compilation, and would make all thecoffee class compatibility tests pass. It would still depend on ES2015 for the splat syntax.

@GeoffreyBooth
Copy link
Collaborator

SO, that at least means this still seems the best approach for future compatibility.

“This” meaning, the current implementation in this PR? Yes, I think so. Especially if by “future compatibility” you mean whatever other ES class features we decide to support in CoffeeScript. I can easily see adding support forget andset andstatic, for example. For these things we should just accept that we’re going to output theclass keyword, and build from there. I think it’s a mistake to try to replicate Babel and find creative ways to try to compile down to ES5 while still preserving everything. If people need to output ES5, let them pipe CoffeeScript’s output through Babel. That shouldn’t be our job.

Needing to be able to extend an ES2015 class or CoffeeScript class in ES5 code is probably such an edge case at this point that we shouldn’t worry about it. The only scenario I can think of is a Node module meant for general use, likecoffee-script incidentally. Such Node modules should simply make sure that they output a folder like CoffeeScript’slib that includes ES5 output. Or don’t. Node has supportedclass fully since version 4, so maybe it’s past time to worry about lack ofclass support in Node.

If you can find a way to make all 9 of balupton’s tests pass, that would be heroic, and maybe such a solution could be merged intomaster. That would close#4330 for the 1.x branch, and spare many people from needing to ever upgrade to CoffeeScript 2. I’m not sure it’s worth the effort though.

I want to add a few more tests, and invite others to look at this PR, and then I think it’s probably ready to merge. Then we can release our first alpha of CoffeeScript 2 (!) and discuss where to go from there.

@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commentedNov 12, 2016
edited
Loading

Okay, a little test. I took the simple class from theMDN example:

classPolygon{constructor(height,width){this.height=height;this.width=width;}getarea(){returnthis.calcArea();}calcArea(){returnthis.height*this.width;}}

and rewrote it as CoffeeScript. Note that I just have to ignore theget for now:

classPolygonconstructor: (@height,@width)->area:->@calcArea()calcArea:->@height*@width

Per this PR, this becomes:

varPolygon;Polygon=(function(){classPolygon{constructor(height,width){this.height=height;this.width=width;}}Polygon.prototype.area=function(){returnthis.calcArea();};Polygon.prototype.calcArea=function(){returnthis.height*this.width;};returnPolygon;})();

Soarea doesn’t make much sense without theget, but this does work, assuming you callarea() instead of justarea, i.e. instead ofnew Polygon(3, 3).area you usenew Polygon(3, 3).area(). If there are other JavaScript libraries out there that depend on getters and setters—Ember?—we’ll have to add support forget andset.

Another thing we should discuss is the reliance onprototype. Perhaps we shouldn’t compile toprototype, e.g. thePolygon.prototype.area = line above, if we don’t have to. In other words, for a class that has no executable body, which I think is probably most classes, we could just output the method syntax like the MDN original. I get that that adds complexity to the compiler, as now you have to code two ways of doing things, but readable output that strays as little as possible from the author’s intent is one of our goals.

@@ -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']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why it’s safe to removeC::? This compiles toC.prototype in both this PR and CoffeeScript 1.x. I see that the test uses this with anew class, so presumably that’s why this test fails now; but why does it fail?

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

This fails because the test is attempting to assign to all those accesses, and theprototype property of ES classes is non-writable.

@@ -738,21 +721,21 @@ test "#2599: other typed constructors should be inherited", ->
ok (new Derived) not instanceof Base
ok (new Base) not instanceof Base

test "#2359: extending native objects that use other typed constructors requires defining a constructor", ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This all looks fine, but do you mind explaining what changed here?

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

So, from what I can tell, the main reason ES classes have been implemented the way they are is to fully support extending native objects. The test here therefore became invalid, i.e. you no longer need a constructor to extend native objects.

I decided to adjust the test to check that native objects could be extended with and without a constructor, rather than removing it.

@GeoffreyBoothGeoffreyBooth changed the title[WIP] Compile class constructors to ES2015 classes[CS2] Compile class constructors to ES2015 classesNov 12, 2016
@connec
Copy link
CollaboratorAuthor

“This” meaning, the current implementation in this PR? Yes, I think so. Especially if by “future compatibility” you mean whatever other ES class features we decide to support in CoffeeScript.

That's exactly what I mean, sorry for not being clearer.

If you can find a way to make all 9 of balupton’s tests pass, that would be heroic, and maybe such a solution could be merged into master.

Unfortunately one of those failures is an incompatibility in ES itself (extending ES6 classes in ES5), so there's nothing we can do about that. The CS ones could be made to pass with some acrobatics, like above. I may take a stab at that, though I'm a bit worried the approach might interfere with 'factory' constructors that return non-instances.

Another thing we should discuss is the reliance on prototype. Perhaps we shouldn’t compile to prototype, e.g. the Polygon.prototype.area = line above, if we don’t have to.

You're right that this would be clearer. In fact in all cases except where dynamic keys are involved, we have the option of usingclass. However, from my experience with the previous PR, implementing this in a sane way would be a fairly major undertaking, and makes sense to me as a progressive enhancement on what's in this PR.

@GeoffreyBooth
Copy link
Collaborator

You're right that this would be clearer. In fact in all cases except where dynamic keys are involved, we have the option of usingclass. However, from my experience with the previous PR, implementing this in a sane way would be a fairly major undertaking, and makes sense to me as a progressive enhancement on what's in this PR.

Agreed. Sorry, I thought I wrote that but I guess I forgot. 😄 I feel like we’re likely to want to implementget andset (and while we’re at it, we might as well implementstatic), and using the method syntax in more cases could be an improvement done along with that. It’s not just being used for dynamic keys, though, right—isn’t it also used to support executable class bodies?

Thanks for your answers to my questions on the tests. I can’t think of any other tests to add.

@jashkenas,@lydell,@mrmowgli, anyone else: any notes on this PR before we merge?

@connec
Copy link
CollaboratorAuthor

@GeoffreyBooth it is in fact mostly dynamic keys. Without dynamic keys you can reorder the executable statements after the class definition without affecting the result. However, when dynamic keys are used, it's possible for the executable class body to modify the values of the keys, meaning the reordering could result in a different result.

In the below example, the result will be different depending on whether the executable statements go before or after theclass, and both are 'incorrect' with respect to the original intention:

classAa='foo'"#{a}":->console.log'foo called'a='bar'"#{a}":->console.log'bar called'# class A {#   [`${a}`] () {#     console.log('foo called')#   }#   [`${a}`] () {#     console.log('bar called')#   }# }

Without going crazy with code flow analysis etc., it would be impossible to detect conflicts like that, so dynamic keys are incompatible with executable class bodies, and would have to be moved outside theclass.

For non-dynamic content there is no such conflict, and we can safely shift all the executable statements to before/after the class.

classA@mixin SomeMixinf:->@functionFromMixin()# class A {#   f () {#     return this.functionFromMixin()#   }# }# (function () {#   this.mixin(SomeMixin)# }).call(A)

@GeoffreyBooth
Copy link
Collaborator

I guess you could just as easily say it’s mostly executable class bodies, or rather, a class that has both an executable class bodyand dynamic keys. Only in the case that has both do we need to useprototype. As yourclass A { [${a}] () example shows, ES supports dynamic keys; so we could simply output those inline like[${a}] (). It’s only this narrow edge case do we need to engage in acrobatics . . . I wonder if it’s worth it. Executable class bodies aren’t that popular to begin with, and dynamic keys probably even less so. We could simply throw an error if they’re used together.

code.push @makeCode '}'
code.push @makeCode "\n#{@tab}#{@name}.__super__ = #{@parent}.prototype;" if @parent

[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this method always returning an empty array, or am I confused?

Copy link
CollaboratorAuthor

@connecconnecNov 13, 2016
edited
Loading

Choose a reason for hiding this comment

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

It is! This is part of the 'hack' to hoist the constructor, whilst also ensuring it has the correct scope. Consider:

classA@a=1

In CS1.x this becomes:

A=(function(){A.a=1functionA(){}returnA;})()

This works fine due to named function hoisting. Unfortunatelyclasses are not hoisted, so the same approach won't work. We also can't just move the node to the top, because the scope must correspond to the constructors position in the body, e.g.:

classA@a=1a=1constructor:->a=2

Thea in the constructor should reference the outer scope, but if we move theclass above the@a = 1 then the constructor will have the wrong scope.

I couldn't find a mechanism in the compiler for dealing with this, so I resorted to compiling a 'placeholder' node in the target position, and leaving the constructor node in the source position. Once compilation is done, the placeholder is swapped out for the fragments compiled for the constructor.

In this function, rather than returning the fragments, they are being attached to the placeholder (@placeholder.fragments) for later substitution, and so for this location no fragments are returned.

Regrettably this still leaves a blank line in the source where the constructor was. Now I think about it, the same approach could be used here: return a 'special' fragment that can be spliced out later.

If you have any suggestions for dealing with this (namely, compiling a node with a scope that's not available until further down the tree), or ways of clarifying what's here, I'd gladly hear them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the very least, please add a comment near the returned empty array that explains this.

Look at my commit where I removed extra empty lines. Could you put some code there that helps with this?

@GeoffreyBooth
Copy link
Collaborator

FYI there’s something in ECMAScript called “class public fields” that’s in Stage 2.Examples. It was inspired bythis.

It’s basically an executable class body, but restricted to class property variable assignments.

@jashkenas
Copy link
Owner

This looks more or less fine to me — I just have a couple of comments:

  • In Nodes, do we want a secondEsClass class? Why not just make it one thing?
  • Adding a mechanism to the nodes for better rearrangement/placement than the current placeholder array might be nicer.
  • Ditto for the bit that searches for and moves a comment. That's the kind of tree-walking that's not going to be fun to maintain in the future.

In order to be usable at runtime, an extended ES class must call `super`OR return an alternative object. This check prevented the latter case,and checking for an alternative return can't be completed staticallywithout control flow analysis.
@connec
Copy link
CollaboratorAuthor

connec commentedJan 8, 2017
edited
Loading

So many edge cases...

Another fun one withsuper in params:

classAclassBextendsAconstructor: (@a= (super ;@a))->

The promise of@ parameters is that oncethis is available, the param will be available, except it won't be assigned yet. If we fix the cache variable with an IIFE, an approximate compilation might be:

classA{}classBextendsA{constructor(a=((()=>{varrefref=super(...arguments)this.a=a// can't use `a` as it's not readyreturnref})(),this.a)){}}

I guess the assignment ofa just shouldn't be there.

@connec
Copy link
CollaboratorAuthor

How I think we should handle those cases:


super in@ param

This should count as an occurrence ofthis beforesuper, and should throw a compiler error.

classextends (class)constructor: (@a=super)->

super in regular param, with leading@ param

Thiscould work fine, but looks likethis beforesuper.

newclassextends (class)constructor: (@a,b=super)->
newclassextends(class{}){constructor(a,b=(()=>{varrefref=super(...arguments)this.a=areturnref})()){}}

It could also justthrow a "this beforesuper", since it certainly looks like one.

super in regular param, with following@ param

This can't work, as thesuper call needs to reference a subsequent parameter, which is undefined. We could detect this, or else the runtime will throw.

newclassextends (class)constructor: (a=super,@b)->
newclassextends(class{}){constructor(a=(()=>{varrefref=super(...arguments)this.b=breturnref})(),b){}}

super in regular param with no@ params

This should work fine.

newclassextends (class)constructor: (a,b=super,c)->

There are two bugs in the current implementation:

  • Handling the expansion ofsuper when it appears in params, due to the cache variable being defined in the function body. The fix I'm working to implement is to wrap thesuper expansion in an IIFE when it occurs within an@ parameter. This is almost ready to push, just needs some tests.
  • Fixing "this beforesuper" check to also check params. This will likely require some shenanigans with the parameter handling, but will hopefully be ready soon.

@lydell
Copy link
Collaborator

What about never allowingsuper in parameter defaults whatsoever?

@connec
Copy link
CollaboratorAuthor

@lydell that's certainly an option, and it's realistically not likely to happen with idiomatic code. I honestly didn't expect it to work when I tried it, but figured I should see if we could support it before ruling it out.

More than happy to go with `super` is not allowed in parameter defaults until there's a compelling use-case otherwise.

@GeoffreyBooth
Copy link
Collaborator

GeoffreyBooth commentedJan 9, 2017
edited
Loading

I'm wondering why we need to allow function calls in parameter lists. As CoffeeScript's own compiler output shows, such code could always be rewritten such that the call is in the function body. It also doesn't strike me as something that happens often, so if we ban it we won't be breaking too much existing code.

@connec
Copy link
CollaboratorAuthor

That's another option, though I could imagine a situation like:

classUsernextId=do->currentId=0->++currentIdconstructor: (@id=nextId())->

Yet another option would be to reverting back to CS' old parameter defaults behaviour, which would work predictably in the above cases.


Of the ones we've mentioned, my preference is either banningsuper in params (simple, low maintenance), or making the changes I outlined above to allow working variants (more special cases == more maintenance).

If we eventually support class properties in the same way Babel does, this issue would crop up again. I had hoped that someone from Babel would chime in on the issues I created with some wisdom, but I guess it's a pretty extreme edge case.

@mrmowgli
Copy link
Contributor

Interesting. My tendency would be to ban it in this pull request, and possibly open another issue outlining the edge cases and discuss on CS6-Discuss.

Chris Connelly added2 commitsJanuary 10, 2017 09:00
There are many edge cases when combining 'super' in parameter defaultswith @-parameters and bound functions (and potentially propertyinitializers in the future).Rather than attempting to resolve these edge cases, 'super' is nowexplicitly disallowed in constructor parameter defaults.
@-parameters can't be assigned unless 'super' is called.
@connec
Copy link
CollaboratorAuthor

OK, I've tidied up the rules aroundsuper and parameters now:

  • Disallowsuper in constructor parameter defaults at compile time.
  • Disallow@-params in constructors withoutsuper at compile time.

I reckon this is ready to merge. No doubt there will be a few kinks in it, but we're not going to find them unless this gets used in some real projects (myyaml-js project is the only one using it atm).

Once it's approved I would quite like to rebase it on to2 to tidy up the merge, there are currently a few duplicate commits etc., but I don't want to rebase whilst people might still be playing with the branch (already had plenty of fun with that 😄).

@jashkenas
Copy link
Owner

I think you're taking the right approach with disallowing the fancy option to start. We can always add it later if there's real demand for it.

Thanks for seeing this through, and for the heroic effort,@connec.

Looks good to merge to the2 branch to me.

connec, jamiter, emschwar, and mrmowgli reacted with hooray emoji

@connec
Copy link
CollaboratorAuthor

connec commentedJan 11, 2017
edited
Loading

I've rebased, for that fresh commit log feeling.

@GeoffreyBooth and@mrmowgli - I've revoked your write access to prevent shenanigans, since it's ready to merge. If we need to make changes I can add you back in!

@GeoffreyBooth
Copy link
Collaborator

To follow up on my example from above, the MDNPolygon is now compiled into:

varPolygon;Polygon=classPolygon{constructor(height,width){this.height=height;this.width=width;}area(){returnthis.calcArea();}calcArea(){returnthis.height*this.width;}};

which looks pretty damn good.

@connec, what remains to be done, that you wanted to follow up in a second PR? Improvements tosuper? Edge cases?

@GeoffreyBoothGeoffreyBooth merged commit8d81804 intojashkenas:2Jan 13, 2017
GeoffreyBooth added a commit to GeoffreyBooth/coffeescript that referenced this pull requestJan 13, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jashkenasjashkenasjashkenas left review comments

@svicaliforniasvicaliforniasvicalifornia left review comments

@GeoffreyBoothGeoffreyBoothGeoffreyBooth left review comments

@mrmowglimrmowglimrmowgli left review comments

@lydelllydelllydell left review comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

9 participants
@connec@GeoffreyBooth@svicalifornia@jashkenas@lydell@greghuc@TheBITLINK@mitar@mrmowgli

[8]ページ先頭

©2009-2025 Movatter.jp