fat arrow functions bound before calls to super render constructors invalid

[smurp]

decaffeinate is producing the wrong JavaScript based on my CoffeeScript input:

class SuperClass
  constructor: () ->
    @a = 1
class SubClass extends SuperClass
  fatArrowFunction: () =>
    @b = 1
    return

I get this output:

/*
 * decaffeinate suggestions:
 * DS002: Fix invalid constructor
 * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
 */
class SuperClass {
  constructor() {
    this.a = 1;
  }
}
class SubClass extends SuperClass {
  constructor(...args) {
    this.fatArrowFunction = this.fatArrowFunction.bind(this);
    super(...args);
  }

  fatArrowFunction() {
    this.b = 1;
  }
}

Here's what I expect it to be instead:

class SuperClass {
  constructor() {
    this.a = 1;
  }
}
class SubClass extends SuperClass {
  constructor(...args) {
    super(...args);
    this.fatArrowFunction = this.fatArrowFunction.bind(this);
  }

  fatArrowFunction() {
    this.b = 1;
  }
}

[eventualbuddha]

Previously decaffeinate would produce code that worked around this when using Babel or TypeScript to transpile to ES5 or lower, but Babel changed in a way that broke the workaround. Given that the workaround didn't work when using real ES6 classes, I just decided to remove it in 14ab797.

Note that the output you posted does indicate that you should fix the invalid constructor. Most of the time you can simply change it manually to be as you wrote it, but it's possible that you'd been relying on the ordering that bound the method first, so that's how it leaves it.

[smurp]

Just popping a meta, that we might speak of how to deal with issues which are issues but which have a complex background....

Isn't it more appropriate to leave the issue open rather than, in effect, say WontFix about it? Maybe someone else might want to give it a whack.

BTW, thanks for all your work on decaffeinate! Much appreciated.

[eventualbuddha]

That's a fair point, though the only way I can think of to fix this is to use ES5-style classes. That's almost certainly more work than anyone is willing to put in.

[smurp]

I know nothing about decaffeinate's internals -- forgive me -- but given that decaffeinate introduces the .bind(this) lines itself, can't it have control over whether they come before the call to super?

[eventualbuddha]

@smurp I noticed in looking into this that CS2 binds methods after calling super to be compatible with ES6 classes. I submitted #2143 to fix that. Once that's in, you can do --use-cs2 to opt into that behavior. It may change other things too, though.

[grassick]

I'm still seeing this one in version 6.1.8 with option --use-cs2. See attached input and output:

Failed conversion.zip

e.g.:

 constructor(props) {
      this.handleAnswerChange = this.handleAnswerChange.bind(this);
      this.handleEntryDataChange = this.handleEntryDataChange.bind(this);
      this.handleAdd = this.handleAdd.bind(this);
      this.handleRemove = this.handleRemove.bind(this);
      this.handleCellChange = this.handleCellChange.bind(this);
      this.handleSort = this.handleSort.bind(this);
      this.renderEntry = this.renderEntry.bind(this);
      super(props);

      this.state = {
        validationErrors: {}  // Map of "<rowindex>_<columnid>" to validation error
      };
    }

Is there some reason the fix isn't being used in this case?

[softgripper]

Same issue as above (with 6.1.10)

Having a CoffeeScript sub class, that calls super, is putting all the binds before the super call, regardless of the --use-cs2 flag.

valid-and-invalid.zip

I have ~=480 classes to migrate.

Currently looking at doing our own "coffeescript > compile" conversion due to this problem (like bulkdecaffeinate without the decaffineate).

Invalid output

define (require) ->
  Layout = require 'backbone.layoutmanager'
  class View extends Layout
    constructor: (options) ->
       super options

    enable: () =>
      return

/*
 * decaffeinate suggestions:
 * DS002: Fix invalid constructor
 * DS102: Remove unnecessary code created because of implicit returns
 * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
 */
define(function(require) {
  let View;
  const Layout = require('backbone.layoutmanager');
  return (View = class View extends Layout {
    constructor(options) {
       this.enable = this.enable.bind(this);
       super(options);
     }

    enable() {
    }
  });
});

Valid output

define (require) ->
  Layout = require 'backbone.layoutmanager'
  class View extends Layout

    enable: () =>
      return

/*
 * decaffeinate suggestions:
 * DS102: Remove unnecessary code created because of implicit returns
 * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
 */
define(function(require) {
  let View;
  const Layout = require('backbone.layoutmanager');
  return (View = class View extends Layout {

    constructor(...args) {
      super(...args);
      this.enable = this.enable.bind(this);
    }

    enable() {
    }
  });
});

[danwashusen]

@softgripper and I spent some time debugging yesterday and came up with the following fix for the issue he documents:
sixtydigits@b43dbde

As noted in the commit message, this change assumes you're always using --use-cs2. I'd say more work is required before it could be merged into decaffinate proper. However decaffeinate is now working as expected for our use case...

[eventualbuddha]

🎉 This issue has been resolved in version 6.1.11 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀