coffeescript6 / discuss

A place to discuss the future of CoffeeScript
162 stars 4 forks source link

CS2 Discussion: Output: Classes #22

Closed rattrayalex closed 7 years ago

rattrayalex commented 8 years ago

This is a core feature that we should rally around submitting to jashhkenas/coffeescript in the near term.

Does anyone know of current leading efforts to accomplish this in the community? Perhaps we can lend a hand there.

kirly-af commented 8 years ago

I don't think we'll have any issue keeping class properties as:

class foo
    @bar: 42

... currently just compiles to the straightforward:

(function() {
  var foo;

  foo = (function() {
    function foo() {}
    foo.bar = 42;
    return foo;
  })();
}).call(this);

Regarding getters/setters, I've not been able to find old comments from @jashkenas, but as far as I remember, these were considered a bad part of JS because it obfuscates what's happening: the class user might think he's just accessing a property whereas some side effects are actually occurring.

Concerning the question if we actually need to support these to be compatible with es classes, I'd say no, but I might be wrong.

GeoffreyBooth commented 8 years ago

@kirly-af remember we’re trying to compile into the class keyword now, not function. How would you compile your example into an ES class?

One other point I should mention is that at least as far as classes are concerned, I think we’re trying to avoid adding sugar atop ECMAScript’s definition, since ES classes are in very active development. I think this came up in the discussion about decorators, which are currently just in the proposal stage for ES2017; in general we want to avoid adding features until they’re not just an approved spec but actually implemented in Node and/or a few browsers. Basically we want to avoid getting in this situation again, where we add something and then ECMAScript implements it differently and we need to make a breaking change to conform with their spec. So in general the goal is to output to the spec as it is now, as supported by Node, and not try to build on top of it with extra CS-only features. I can see us making exceptions for old CS features that ES now fails to support, like class properties; but we don’t want to be adding new class “features.”

kirly-af commented 8 years ago

For the example I gave, just replace the function foo() declaration by class foo and it will work just fine.

I don't think it's going to be something likely to change very often, the only concern I can think about is whether we should make these enumerable or not. Currently es2015 class methods are not enumerable, but I don't know if we should do the same for properties. In the public class fields proposal (only on stage 2) it seems these actually ARE enumerable (see 1.4.v).

This behavior might be the only breaking change we would have, though I completely understand this goal to avoid ANY possible bc. It's just frustrating to feel like we're removing a feature, to finally add it back slightly changed YEARS later. But you're right, that's probably the best for CS which suffered enough from being bloated.

kirly-af commented 8 years ago

Related:

GeoffreyBooth commented 8 years ago

@kirly-af I haven’t studied these issues enough to answer in such detail. Perhaps if you don’t have time to dig into the code just yet but do have time to help in other ways, you could take a pass at defining the syntax for the new CS2 class? Like my Polygon example above, but with every possible feature we’re trying to support in one example, in both CoffeeScript and ECMAScript. That would help us a lot.

alangpierce commented 8 years ago

I haven't been following this discussion too closely, but FYI, I wrote up a plan for class support in decaffeinate a while back and implemented most of it today. Here's a description of the semantics: https://github.com/decaffeinate/decaffeinate/issues/397

My notes talk through various cases that you may find interesting. But like I mention, even with the various ways to handle edge cases, the transform isn't 100% correct, particularly because it needs to reorder the statements to put methods and non-methods together, and I'm sure there are various other little differences.

GeoffreyBooth commented 8 years ago

Thanks for that @alangpierce. To be clear, we're not aiming to preserve backward compatibility for classes in CS2. But it's good to know regardless what wouldn't be supported.

Why create a new method initClass to define the properties in, rather that declaring them as the first lines of the constructor?

Also I think it might be more common than you're assuming to have multiple classes in the same file with identical properties. In the CoffeeScript codebase itself, nodes.coffee has two dozen or so classes all with a children property.

lydell commented 8 years ago

Why create a new method initClass to define the properties in, rather that declaring them as the first lines of the constructor?

Because SomeClass.initClass() will be run once, while the constructor is run every time the class is instantiated.

alangpierce commented 8 years ago

@GeoffreyBooth

Sorry, after looking again, I realize I didn't end up showing class fields/properties in the example. Something like children: ['name'] in the class body ends up as this.prototype.children = ['name'] in the initClass method. (The constructor is untouched.)

Why create a new method initClass to define the properties in, rather that declaring them as the first lines of the constructor?

I'm implementing the CoffeeScript behavior of putting class fields on the prototype rather than on the instance. This ends up being important for Backbone classes to work right, I think: in JS, you always need to call your superclass constructor before you have a chance to attach any instance properties, and I ran into some cases where the superclass constructor expected everything to already be set. The way to have properties (or something like them) on a class instance before the constructor is called is to put them on the prototype.

Also, CoffeeScript classes allow running arbitrary other code when the class (not each instance) is first instantiated, so initClass is where that goes.

Also I think it might be more common than you're assuming to have multiple classes in the same file with identical properties.

My approach doesn't have trouble with class fields/properties with the same name across classes, so the children thing you mentioned is actually fine. My assumption is that class-scoped constants/variables with conflicting names in the same file are rare. In nodes.coffee, the only class-scoped constants/variables I could find are CONVERSIONS and INVERSIONS, which are unique names in the file.

mrmowgli commented 8 years ago

I'm still going through the implications of ES6 classes and interactions with CS classes. I'll probably add to the wiki in this repository and then ask for any more comments. I get the feeling that this has a lot of edge cases, in particular with the way CS classes override or extend ES classes. I am also seeing additional keywords like static being used, and also issues with establishing the inheritance chain.

The video that @rattrayalex posted earlier was actually very useful, since it covers some of the ways the original parser cheats while building the AST.

kirly-af commented 8 years ago

So, I'll post here my ideas concerning the new CS classes features/syntax:

Basic approach

Pretty much a straight-forward compilation: static methods, super... Everythings is ES2015-ish apart from the addition of class properties.

class Foo
  constructor: (val) ->
    @prop = val

  @someClassMethod: ->

  @someClassProp: 'propValue'

class Bar extends Foo
  constructor: ->
    super 'bar'
    @prop2 = 'someVal'
    super.someClassMethod()

That would compile to:

let Foo;
let Bar;

Foo = (function() {
  class Foo {
    constructor(val) {
      this.prop = val;
    }
    static someClassMethod() {
    }
  }
  Foo.someClassProp = 'propValue'
  return Foo;
})();

Bar = (function() {
  class Bar extends Foo {
    constructor() {
      super('bar');
      this.prop2 = 'someVal';
      super.someClassMethod();
    }
  }
  return Bar;
})();

Preserve executable body

For that approach, all credit goes to @connec . Here we attempt to keep the possibility to execute code in class definitions.

class Foo

  someVar = 0
  someStatement someVar

  contructor: ->
    @prop = 'someVal'
    useClosure someVar

  anotherStatement someVar
  # ...

would compile to:

let Foo;

Foo = (function() {
  let someVar;

  class Foo {
    constructor() {
      this.prop = 'someVal';
      useClosure(someVar);
    }
  }

  someVar = 0;
  someStatement(someVar);
  anotherStatement(someVar);
  return Foo;
})();

Going forward: Class Metaprogramming

In CS we can take advantage of executable bodies to have some metaprogramming possibilities. While being a cool feature, keeping it involves manually defining properties.

someState = true

class Foo
  if someState
    someMethod: -> false
  else
    someMethod: -> true
    @someClassProp: 'someVal'
    @someClassMethod: -> null

would compile to:

let _defineProperty = function(target, key, value, enumerable) {
   value.enumerable || false;
   value.configurable = true;
   value.writable = true;
   Object.defineProperty(target, key, value);
};
let Foo;

Foo = (function() {
  let someState;

  class Foo {}

  someState = false;
  if (someState) {
    _defineProperty(Foo.prototype, "someMethod", function someMethod() {
      return true;
    }, true);
  }
  else (someState) {
    _defineProperty(Foo.prototype, "someMethod", function someMethod() {
      return false;
    }, true);
    Foo.someClassProp = 'someVal';
    _defineProperty(Foo, "someClassMethod", function someClassMethod() {
      return null;
    }, true);
  }
  return Foo;
})();

This is more or less the Babel handles class properties.

Please share your thoughts. I think 2 first steps are feasable, not sure we'd like to support the 3rd one though.

Also, @alangpierce, you said something some time ago that confused me:

CS class fields are on the prototype, JS class fields are assigned to the instance once the constructor finishes

I don't really see what you were talking about. Could you provide an example ?

mrmowgli commented 8 years ago

Still going through everything, but I ran into this reference of Node class support and versions. Handy reference: http://node.green/#class

connec commented 8 years ago

I got a bit carried away "adding some [tests] to exercise the ordering" and created a WIP PR @ https://github.com/jashkenas/coffeescript/pull/4330. I don't intend for this to clash with anything anyone else is doing, but hopefully it can highlight some of the potential issues and pitfalls.

Some salient points:

svicalifornia commented 8 years ago

Another way that ES6 classes (at least as compiled to ES5 by Babel) differ from CS classes is that in ES6, an extend hook on the superclass will be called when it is subclassed.

Cross-post from https://github.com/jashkenas/coffeescript/pull/4330:

When using Objection.js Model with CoffeeScript, I had to add a line of boilerplate after each CS subclass definition to call Model's extend hook:

Model = require('objection').Model
class MyModel extends Model
  ...

Model.extend MyModel

Babel compiles ES6 subclasses in such a way that the hook was called in the resulting ES5 code, so the explicit (and redundant) call to extend 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:

  1. Create a CS subclass with executable class bodies, static properties, and instance properties.
  2. Call the extend hook (if there is one) when extending an ES class, as Babel-compiled code does.
  3. Make super work properly with the ES6 superclass.

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.)

If CS 1.x generates CS subclasses, and CS 2.0 generates ES classes and subclasses, then I agree with @jashkenas that a separate esclass keyword is unnecessary and likely to cause confusion.

GeoffreyBooth commented 8 years ago

Trying to push along https://github.com/jashkenas/coffeescript/pull/4330: can anyone write a test that, if it were to pass, would prove that CoffeeScript could extend a React class such that CoffeeScript can interoperate well with React?

As far as I can tell the big interoperability concern with classes is that ES classes cannot currently be extended in CoffeeScript; and the most high-profile case where this is an issue is React. I would love a failing test where, if we were to get it to pass, it would prove that our interoperability problem with React was solved.

Please don’t write the test such that it actually imports React, just create the simplest possible ES class to demonstrate the issue. There could be a test/importing/es_class.js file that exports an ES class (using CommonJS) and inside test/classes.coffee there would be a test that extends the ES class and does something with it, similar to whatever people are trying to do with React (or Objection.js or other) classes.

mitar commented 7 years ago

I really love how CoffeeScript has classes and I do not really have much opinion on to what is it compiled internally. But I do love one thing from ES6 classes: that you can define instance properties inside the class body. I love that:

class Foo {
  bar = 3;
}

Is compiled to:

var Foo = function Foo() {
  this.bar = 3;
};

Because otherwise doing an instance-based assignment you can do only inside a constructor, which sometimes makes it much less cleaner to read. I wanted this feature so many times, and on the other hand I have never seen a need for executable class bodies which I could not simply resolve with creating dynamic classes at runtime.

lydell commented 7 years ago

But I do love one thing from ES6 classes: that you can define instance properties inside the class body.

That's not ES6, but a proposal for a future specification.

mitar commented 7 years ago

That's not ES6, but a proposal for a future specification.

Oh, sorry. It works with Babel, so I just assumed it is part of E2016 already. Do you know which Babel rule it is/more about this proposal/its name?

In any case, I love this feature and I would propose that CoffeeScript has it even it does not become part of JavaScript.

Edit: found it: https://babeljs.io/docs/plugins/transform-class-properties/

GeoffreyBooth commented 7 years ago

See also: https://github.com/jashkenas/coffeescript/pull/4354#issuecomment-263477788

mrmowgli commented 7 years ago

Ok, I have finished getting through the existing class tests, and a lot of the current rules are covered well. I will be adding roughly thirty or so tests. One thing was very "interesting" to me:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/species

It's definitely referred to in the core MDN docs, and Symbols are an area we will probably have to support in iterators and classes.

I'm not entirely sure I understand why this exists, if anyone knows I would love to hear a use case?


class C {
  ident() {return 'Class C!';}  
};

class MyArray extends Array {
  // Overwrite species to the parent Array constructor
  static get [Symbol.species]() { return C; }
  ident() {return 'Class MyArray!';}
}

var a = new MyArray(1,2,3);
var mapped = a.map(x => x * x);

console.log(mapped instanceof C); // true
console.log(mapped instanceof MyArray); // false
console.log(mapped instanceof Array);   // false
console.log(MyArray.prototype); // MyArray
console.log(a.ident()); // Class MyArray!
console.log(mapped.ident()); // Class C!
console.log(mapped);  // C { '0': 1, '1': 4, '2': 9 }
triskweline commented 7 years ago

I found this article slightly more helpful than MDN: http://www.keithcirkel.co.uk/metaprogramming-in-es6-symbols/#symbolspecies

GeoffreyBooth commented 7 years ago

@mrmowgli: I will be adding roughly thirty or so tests.

You, sir, are my hero.

mrmowgli commented 7 years ago

Also super is an interesting dilemma. This is a core feature of CS and I would want to keep it pretty much as is, however super in ES6 has the following 'problems':

and likely more that I don't know about yet. Almost all of the tests for classes would need to be rewritten if super really tries to emulate the ES6 super(). One of the biggest divergent areas is subclassing of member functions. CS6 doesn't seem to allow calling super for anything besides constructors.

I have no idea what @connec is putting together for super, but I suspect the final version will be split between generating the "official" CS6 when super is used correctly, ie in a constructor and with parens, and a tokenized version of super that continues to behave as the existing CS base. I don't think this needs to break any existing CS code.

mrmowgli commented 7 years ago

@henning-koch - Interesting article. I suspect Symbol.species in this format will end up being used quite a bit.

Which means that we probably need to add get/set fairly soon to classes.

connec commented 7 years ago

@mrmowgli regarding super:

Firstly, the way super is compiled in CS, roughly, is that a "super reference" is determined based on the containing method's name and context, some current examples:

Context.name = -> super             # doesn't compile
Context.prototype.name = -> super   # super reference: Context.__super__.name

class Context then @name = -> super # super reference: Context.__super__.constructor.name
class Context then name: -> super   # super reference: Context.__super__.name

# constructor is not special
Context.prototype.constructor = -> super   # super reference: Context.__super__.constructor
class Context then constructor: -> super   # super reference: Context.__super__.constructor

In https://github.com/jashkenas/coffeescript/pull/4354 (the active ES class PR) the only change for super is the last example above, all others have been left alone.

class Context then constructor: -> super # super reference: super

Ordinarily, the "super reference" is compiled with a .apply(this, arguments) or a .call(this, arg1, arg2, ...) depending on how super is called (bare super vs. super(arg1, arg2), etc). This doesn't work in an ES constructor because super in a constructor seems to be a very restricted syntactic form, rather than an 'auto' reference like arguments, hence when compiling a constructor super call there's a special case when the super reference is simply "super".

this doesn't exist in an extended class until super() is called.

That is an interesting case indeed. The way I chose to handle it in my PR is to add an implicit super to all derived constructors unless there's already an explicit one, e.g.:

class A
class B extends A then constructor: ->
# ...
#   class B extends superClass {
#     constructor() {
#       super(...arguments);
#     }
#   }
# ...

class A
class B extends A then constructor: -> super()
# ...
#   class B extends superClass {
#     constructor() {
#       super();
#     }
#   }
# ...

The reason I chose to do this is because it's the most reasonable way I could think of to deal with @-params and bound methods. Consider:

class B extends A
  constructor: (@name) ->

  greet: ->
    console.log "Hi #{@name}"

class C extends A
  constructor: (name) ->
    super()
    @name = name

  greet: =>
    console.log "Hi #{@name}"

Without an implicit super, class B is clearly broken: it doesn't call super so attempting to access its internal state will always fail. However, we can't simply add an explicit super as CS will compile the @-param into an assignment at the top of the constructor:

class B extends A {
  constructor (name) {
    this.name = name
    // super() - this is already too late
  }
}

C is also broken, though slightly more subtly: although it doesn't have @-params or reference this before calling super, it does make use of bound methods (greet: => ...). CS compiles method bindings into calls at the top of the constructor:

class C extends B {
  constructor (name) {
    this.greet = bind(this.greet, this)
    super() // - this is also too late
  }
}

Given these cases, there are basically 3 options:

  1. Disallow @-params in constructors and disable bound instance methods in extended classes (serious blow to backwards compatibility and utility, imo).
  2. Insert @-param assignments and method binding calls after the first super in a derived constructor. I haven't really explored this option, but I'm concerned that performing the insertion would be non-trivial and error-prone if super was used as part of an expression (e.g. result = super() is a valid expression, as is result = [super(), this.name]). I expect it could be done but have no doubt it would generate pretty ugly code.
  3. Add an implicit super, at the top of the function, to constructors with @-params or bound methods, and disallow explicit super in these cases. This preserves the original compilation of @-params and bound methods and results it more predictable behaviour, imo.

I went with an extension of 3., and decided to add an implicit super in all cases, which is easier to explain and understand. If a user strongly cares about how super is called, they would need to forgo using @-params or bound methods.

connec commented 7 years ago

Sorry for the wall of text! Figured I'd try and explain the current status as fully as I could.

Having written that all out, I'm starting to feel like option 2. should be explored a little more. I guess the idea behind that would be to generate code that made it seem like @-param assignments and method bindings occurred as part of the call to super.

// class A
// class B extends A
//   constructor: (@foo) ->
//     console.log super(), @foo, @bar
//   bar: =>
class A {}
class B extends A {
  constructor (foo) {
    var ref;
    console.log((ref = super(), this.foo = foo, this.bar = this.bar.bind(this), ref), this.foo, this.bar)
  }

  bar () {}
}

This would behave reasonably well I think, as attempting to access @name or @greet before calling super would throw this is undefined, so from the user's perspective it doesn't matter that @name hasn't been assigned yet.

mrmowgli commented 7 years ago

Ok, my bad: Super CAN be called in other methods, and super isn't required except in constructors for this:


var A = class A {
  constructor (token) {
    this.token = token;
    console.log('Token in base was: ', token); 
  }  
  objectLocal(origin) {
    console.log('The '+ origin + ' called in Parent method.'); 
  }
}

var B = class B extends A {
  constructor (token){
    super(token);
    this.token = token;
    console.log('Secondly, token `'+ this.token + '` was called in child.');  
  }
  objectLocal() {
    console.log('Overriding objectLocal now with "super"'); 
    console.log('Current token: ' + this.token) 
    super.objectLocal(this.token)
  }
}

let n = new B('coffee-script');
n.objectLocal();

/* Output:
  Token in base was:  coffee-script
  Secondly, token `coffee-script` was called in child.
  Overriding objectLocal now with "super"
  Current token: coffee-script
  The coffee-script called in Parent method.
*/
GeoffreyBooth commented 7 years ago

@connec @mrmowgli, fantastic heroic work on https://github.com/jashkenas/coffeescript/pull/4354. Now that that’s merged in, what work related to classes remains to be done?

mrmowgli commented 7 years ago

I realize we didn't create browser specific tests, however I believe we are expecting Babel to cover the bases there. Other than that it seems like it's mostly edge cases surrounding super(), and those need actual usage to shake out a bit. I'm of the opinion we should go ahead with the release, the tests are comprehensive and everything works far better than I originally expected. @connec did an amazing job!

Realistically I believe we should start trying to cover lower priority items like get/set, species, and new modifiers like new.target . I don't believe these should hold up a 2.0 release however.

connec commented 7 years ago

The only thing we probably want to finish off for 2 is super in regular methods. Ultimately this is optional (things will work as they are), however it would be a pretty big win for output clarity and reducing complexity in the compiler.

Unless someone else picks it up I will work on this once I make some time.

GeoffreyBooth commented 7 years ago

@mrmowgli the tests really only cover Node, aside from the browser-specific ones. We treat Node as more or less our reference JavaScript runtime to test against, for JavaScript features that should behave identically in Node and browser-based runtimes.

@connec what do you mean by super in regular methods? Can you give some examples of current output and desired output? And how soon do you think you might have more time?

connec commented 7 years ago

CS source

class Base
  method: ->
    console.log 'Base#method'

class Child extends Base
  method: ->
    super
    console.log 'Child#method'

Current CS2 output

// Generated by CoffeeScript 2.0.0-alpha
var Base, Child;

Base = class Base {
  method() {
    return console.log('Base#method');
  }

};

Child = (function(superClass) {
  class Child extends superClass {
    method() {
      Child.__super__.method.call(this, ...arguments);
      return console.log('Child#method');
    }

  };

  Child.__super__ = superClass.prototype;

  return Child;

})(Base);

Desired CS2 output

// Generated by CoffeeScript 2.0.0-alpha
var Base, Child;

Base = class Base {
  method() {
    return console.log('Base#method');
  }

};

Child = class Child extends Base {
  method() {
    super.method(...arguments);
    return console.log('Child#method');
  }

};

I could have time any day now 😄 Watch this space at the weekend...

connec commented 7 years ago

There will be some 'tough choices' to make for this, I expect. I've not explored it much yet but if we wanted to clean up properly we would effectively lose our current support for super in assignment-based classes, e.g.

Base = ->
Base::method = -> console.log 'Base#method'

Child = ->
Child extends Base

Child::method = ->
  super
  console.log 'Base#method'

Some alternatives:

I'll flesh all these out a bit once I started looking at it.

GeoffreyBooth commented 7 years ago

We already removed some tests that assumed a class was really a function (i.e. the pre-ES2015 class implementation). Your example that begins with Base = -> assumes that implementation, which I think should be considered unsupported in CS2. I think we should only allow super within methods that belong to classes, and in CS2 the only way to create a class is to use the class keyword.

When you get a chance, you should start documenting the class-related breaking changes in the wiki. You’re far more familiar with what’s changed than I am.

jashkenas commented 7 years ago

ES6 super doesn't work in methods added to the class prototype from other locations? Seriously? That's insane.

Edit: Regardless — moving to ES6 classes for CS2 means that you take the lumps that come with. We should definitely not still have the original system still lying around in the codebase for the CS2 release.

connec commented 7 years ago

Nope @jashkenas, it's a 'syntactic form' that is only valid within a class initializer.

connec commented 7 years ago

I think it's interesting that we could opt to compile super into Object.getPrototypeOf(this)[method] in basically any context. Only cost is the extra call to Object.getPrototypeOf, rather than caching it somewhere (such as __super__, currently).

connec commented 7 years ago

Threw together https://github.com/jashkenas/coffeescript/pull/4424 with the bare bones.

jashkenas commented 7 years ago

Nope @jashkenas, it's a 'syntactic form' that is only valid within a class initializer.

It's just madness. It straightjackets class construction unnecessarily, and makes ugly lots of interesting patterns for mixing methods into classes, or composing traits. One step forward, three steps back.

I think it's interesting that we could opt to compile super into Object.getPrototypeOf(this)[method] in basically any context. Only cost is the extra call to Object.getPrototypeOf, rather than caching it somewhere (such as super, currently).

That seems like a pretty nice CoffeeScript feature that would improve upon ES6 classes.

GeoffreyBooth commented 7 years ago

Closed via https://github.com/jashkenas/coffeescript/pull/4424. For further changes or improvements upon CS2 classes, please open new issues.

coffeescriptbot commented 6 years ago

Migrated to jashkenas/coffeescript#4922