Fix default constructor for subclasses of native objects

[epidemian]

I was trying to subclass Error to handle different kind of errors, and got some pretty weird results:

class MyError extends Error
console.log new MyError instanceof MyError # -> false

False? WTF?

Now, after digging a bit and asking about this in Stack Overflow i could figure out why is it that i need to write...

class MyError extends Error then constructor: -> super

... in order for new MyError instanceof MyError to work properly. You can read the SO question if you don't know why, but, basically, the generated JS constructor changed, in CoffeeScript 1.3, from this:

function MyError() {
  MyError.__super__.constructor.apply(this, arguments);
}

to this:

function MyError() {
  return MyError.__super__.constructor.apply(this, arguments);
}

Which is equivalent to calling Error.apply(this, arguments) which returns a new Error object instead of the this that was passed to it, and that is what finally gets returned when you do new MyError.

Now, i can understand the motivation for introducing this behavior in CoffeeScript 1.3.1 (it seems that new A not instanceof A is even a desired behavior sometimes), but i think this behavior has more problems than benefits.

First, it makes subclassing native objects inconsistent, as with "normal" CoffeeScript classes you don't need to define a constructor in the subclasses for them to work:

class Base
  constructor: -> 
    console.log 'Base class constructed'

class Derived extends Base

console.log new Derived instanceof Base # prints 'Base class constructed' and 'true'

Also, as noted in issue #1966, it also breaks subclasses of native objects completely if a constructor is not defined in the subclass:

class CustomArray extends Array
  isFat: -> @length > 5

new CustomArray(3,2,1).isFat() # -> TypeError: Object 3,2,1 has no method 'isFat'

Now, the need for subclassing native objects like Array is something that can be criticized, yes. But i think that subclassing Error is not so far fetched; shouldn't class MyError extends Error Just Work™? :D

Was the behavior of the generated JS constructor changed just to support returning an explicit object (different from this) from a constructor? If that is the case, why is that supported at all? I mean, why would someone want to return a different object from a CoffeeScript constructor? You can always write a JS-style function for that...

TrollCtor = ->
  lol: 'problem?'

console.log new TrollCtor instanceof TrollCtor # -> false! yeah!

But i don't see the point in allowing this (at least not when taking the problems mentioned above in consideration) inside CoffeeScript little nice syntactic sugar for classes, a syntactic sugar that is meant to make writing classes and inheritance easier.

So i open this issue mainly to request 080ed2e to be reverted, but i'm also really interested in knowing other people's opinion on this matter :D

[paulmillr]

I totally agree, there is absolutely zero need in Sacrificing these great features to make external constructors work better. +10

[jashkenas]

@michaelficarra -- how do you feel about this? I'll just briefly reiterate my general antipathy towards returning "other typed" objects from constructors...

[michaelficarra]

@jashkenas: It seems like a bug. The default constructor of a sub-class shouldn't return the result of the super application. It should be noted that this doesn't contradict anything from #1970 or #1966. This is just a related issue.

[epidemian]

@michaelficarra Doesn't it contradict #2111?

[michaelficarra]

Sure, yeah. But I don't think we actually want the behaviour that was being requested there. We just happened to already have that behaviour. And hopefully we will correct it here. I think we just need to add a unless @parent to line 926. Pull requests (with tests!) welcome.

edit: After thinking about it again for a minute, it's probably not as simple as I originally thought. It's something like if not @parent or @body.expressions.length. Something like that.

[epidemian]

@michaelficarra Also, this. If the default constructor of a subclass doesn't return the result of super, then that linked example (which relies on these ill typed constructors that return other things than what's expected) doesn't work.

[michaelficarra]

@jashkenas: Changing this behaviour may cause some users' code to break in a pretty sneaky way. I definitely think it's worth fixing, though. Minor version bump when this lands?

[jashkenas]

Ok ... but why would you want to not makeReturn if you happen to be a subclass, and always makeReturn if you don't happen to have a parent class? Isn't that bizarre?

[epidemian]

It's awesome to see the general support of this idea! =D

I'll try to get my hands on this in the weekend if i have time, but the code on nodes.coffee kinda scares me a bit hehe.

@michaelficarra i've just noticed that you have a Kickstarter project to make a better CoffeeScript compiler and it was successfully funded just yesterday. Congratulations man! And good luck with that!

[epidemian]

Well, i've been playing with this a bit. The problem indeed comes from the behavior of ensureConstructor, but i don't know if it's so easy to fix.

If the return is added only when the class does not extend another class (i.e. change @ctor.body.makeReturn() to @ctor.body.makeReturn() unless @parent), then all the tests still pass and extending native objects works as expected:

test "#2359: instanceof should work when extending native objects", ->
  class MyError extends Error
  ok new MyError instanceof MyError

But it adds a quite nasty inconsistency: "other typed" (i like this term =P) external constructors don't work as expected in subclasses, nor do inherited "other typed" constructors. This (new) tests fail:

test "'other typed' external constructors should still work in subclasses", ->
  ctor = -> {}
  class A then constructor: ctor
  class B extends A
  ok (new B) not instanceof A
  ok (new B) not instanceof B

test "'other typed' constructors should be inherited", ->
  class A then constructor: -> return {}
  class B extends A
  ok (new B) not instanceof A
  ok (new B) not instanceof B

(note that both these tests pass on the current master)

Both of these inconsistencies, as i see it, arise from trying to deal with these "other typed" constructors, which i also think is a questionable behavior.

I personally don't see how we could fix this issue (and i'm using the word "fix" just because it has been marked as a bug) and maintain the behavior of "other typed" constructors consistent.

If the @ctor.body.makeReturn() is removed altogether, the only test that breaks on the current master is:

test "#1966: external constructors should produce their return value", ->
  ctor = -> {}
  class A then constructor: ctor
  ok (new A) not instanceof A

Which is the one that deals with "other typed" external constructors.

I personally would prefer to get rid of "other typed" constructors altogether: even forbid returning anything other than undefined from a constructor (i.e. an early empty return). But that'd be a breaking change for code that relies on these hacks. What do you think?

[jashkenas]

I personally would prefer to get rid of "other typed" constructors altogether

I concur. Let's remove 'em.

[JanMiksovsky]

Wait, this would remove the "return" statement from the default constructor?

That would be problematic for the QuickUI framework, which now depends on that feature for concise UI component creation. (See http://quickui.org/docs/CoffeeScript.html for an explanation of how you easily create UI component subclasses with CoffeeScript's "class" syntax.)

It turns out that there's at least one very commonly-used JavaScript class that makes heavy use of an "other typed" constructor: jQuery. As described in http://blog.quickui.org/2012/06/07/jquery-fn-init/, jQuery's constructor supports both a static form (without "new") and a normal form (with "new").

QuickUI components are, by design, subclasses of the jQuery class. This means that any UI component created in QuickUI can be directly manipulated using jQuery methods such as $.show(), $.animate(), $.css(), and so on. For this to work, QuickUI component class constructors have to be able to return a value of a type other than that class. (Specifically, the constructor for class Foo needs to be able return an instance of a helper class called Foo.fn.init.)

Since coming across CoffeeScript last year, it's been a goal of mine to let UI developers create new QuickUI component classes as concisely as possible in CoffeeScript. QuickUI's earliest support for CoffeeScript required that each component class include a boilerplate constructor that did the "return" required here. Experience proved that even requiring a single line of boilerplate in a class was something of a pain: it was all too easy to forget the magic line, and the consequence of omitting that line was unfortunately not something that made it obvious where the bug was. Furthermore, that line stuck out like a sore thumb in otherwise a highly streamlined CoffeeScript class definition. With CoffeeScript 1.3.1, the boilerplate constructor could be dropped, and UI component code in CoffeeScript looked much nicer. (For a more interesting sample of web app UI written in QuickUI and CoffeeScript, see http://quickui.org/docs/contacts.html.)

Subclassing jQuery represents an extreme edge case but, to QuickUI at least, it's a highly useful thing to do. From the perspective of a QuickUI developer, it would be a significant loss to drop the implicit "return" from the default constructor generated by CoffeeScript.

[epidemian]

@JanMiksovsky Thanks for the interesting reply. After some time trying to understand what $.sub does and why subclassing jQuery is so tricky, i think i now understand why you were relying on the behaviour of introduced in CoffeeScript 1.3.1: you need the base-class constructor to return a special type and override the behaviour of the sub-class constructor, is that it? (by the amount of comments on QuickUI's sub.coffee i imagine that this was no easy task :)

It's really unfortunate that that same behaviour is what's causing this issue when extending native objects :(

[jashkenas]

Fix should be merged now.

[epidemian]

Maybe this should be labelled as "wontfix" instead of "fixed" now that #2599 is reverted in case anyone wanders into this problem in the future.

[xixixao]

@epidemian Is there no way to fix this and keep the other-typed constructor behavior?

[epidemian]

@xixixao, technically, yes, it should be possible to fix this particular issue while keeping some compatibility with other-typed constructors.

(Disclaimer: this issue is very old; i had to dig through various issues and comments to re-understand some of it, so i might very well be missing many things here...)

The first commit for #2359 (e46b129) only fixed this issue actually; without removing the ability to return other objects from constructors. Having only that fix, however, would mess up other-typed constructors in another way: they wouldn't be inheritable.

# CoffeeScript < 1.5.0 or >=1.6.0
class Base
  constructor: -> return {}

class Derived extends Base

(new Derived) not instanceof Derived # => true
(new Derived) not instanceof Base # => true
(new Base) not instanceof Base # => true

So that would introduce a new inconsistency to the language.

I think that, if other typed constructors (or, actually, constructors that return something other than this) are part of the language, then it makes sense for them to be inheritable, just like normal constructors are. As a result of that, then this particular issue would just be a logical consequence of that design decision†: native object's constructors, when called with .apply(this, arguments), will return a different object than this, so if you inherit them, doing new YourErrorSubclass will not return an instance of YourErrorSubclass.

---

†: An unfortunate consequence at that. But just like ['10', '10', '10'].map(parseInt) is an unfortunate consequence of some questionable JS design decisions (Array#map passing anything other than the array elements to the callback, and being able to call functions with any number of arguments in general), and special-casing .map(parseInt) to make it do something useful is not worth it if it's going to make it inconsistent with the rest of the language, i think that special-casing extends Error and extends Array (and any other native object that has this particular behaviour) to "fix" them at the expense of language consistency is not worth it either.

[xixixao]

Just to make sure: Why can't

class A extends Error
  constructor: -> super

be the implementation when no constructor is declared?

[epidemian]

Because doing that would, for example, break code like QuickUI (which @JanMiksovsky mentioned a couple of messages back in this thread), or any other code that relies on constructors that return other objects being inherited.

Would you prefer to allow having other typed constructors (or external constructors) in superclasses but, at the same time, not have them inherited in subclasses?

That would cause this inconsistency:

class Something
  constructor: ->
    return new SomethingElse()

class SubSomething extends Something
  # implied constructor: -> super

(new Something) instanceof Something # => false
(new Something) instanceof SomethingElse # => true (as "expected", because Something has an other typed constructor)
(new SubSomething) instanceof SubSomething # => true
(new SubSomething) instanceof SomethingElse # => false (it didn't inherit the constructor of Something, even if we didn't override it!)

Now, don't get me wrong: i'm not advocating for the use of other-typed constructors here. I think that "feature" of new (and the general concept of new being a special construct instead of a method) really sucks. But people use it; and rely on it. So it seems it is here to stay. You can check #2728 for examples of how other-typed constructors are used (and for a reason why i wanted to have this marked as wontfix and why i'm not very keen on opening this can of worms again hehe).