Return value from function breaks constructor

[weepy]

Problem: if the return value from a function is an array or an object, the creation of new objects will be incorrect. E.g.

A: x,y => this.array = [x,y]
a: new A(1,2) 
# a is [1,2], not a class of A

This is not a problem for classic JS as it can choose not to return anything. However CS has implicit return values.

Some Solutions:

1. Put null or this at the bottom of constructors.
2. Use a special keyword to denote constructors (e.g. class or constructor or ==>).
3. override the new operator in some way:

   function A(x,y) { return this.array = [x,y] } 
   a = new (function() { A.apply(this, Array.prototype.slice.call(arguments,0)) } )(1,2)
   

Notes 1 was noted to be ugly or error prone 2 was felt to be ok, although it seemed silly to have an a keyword to just turn off implicit returns in functions.

[alsonkemp]

I'm strongly in favor of solution 3. Would be great to have the same functionality as in JS. And I like your way of coding it better than I did mine...

[jashkenas]

Solution three is a beautiful idea. I don't fully understand its mysteries, because I can't seem to implement it. Simple cases like yours work fine, but the tests fail completely -- I think wrapping it in a closure doesn't inherit the prototype chain of the inner class.

If you think you can help, check out the "fix_new" branch, and try to fix the tests ("rake test"), or let me know what you think is going wrong.

[weepy]

is there a way of getting tests to say what failed ? Right now all I've got is

  1) Failure:
test_execution_of_coffeescript(ExecutionTest)
    [./test/unit/test_execution.rb:17:in `test_execution_of_coffeescript'
     ./test/unit/test_execution.rb:13:in `each'
     ./test/unit/test_execution.rb:13:in `test_execution_of_coffeescript']:
 is not true.

and a narwhal error

TypeError: Cannot find function func in object [object Object]. (/Users/jonahfox/src/coffee-script/lib/coffee_script/narwhal/lib/coffee-script.js#67(eval)#1)

[weepy]

The code I initially wrote was just an idea. Here's some code that actually works :

A = function(x,y) {
  return this.array= [x,y]
}
A.prototype.sum = function() {
  return this.array[0] + this.array[1]
}
a = new A(1,2) 
console.log(a)  // => [1,2]

var klass_constuctor = function(klass) {
    var constr = function() {
      var args = Array.prototype.slice.call(arguments,0)
      klass.apply(this, args)
    }
    constr.prototype = klass.prototype
    return constr
  }

_A = klass_constuctor(A)
_a = new _A(1,2)
console.log(_a, _a.array, _a.sum()) // => object Array, [1,2], 3

[weepy]

just realized that that isn't going to work very well. further operations to A won't get applied to _A, and any properties are A aren't currently get applied. There are work arounds, but I don't recommend this route now.

"==>" FTW

[weepy]

ok so how about this !

A = function(x,y) {
  var ret = this.array = [x,y];
  if(arguments.callee != this.constructor) return ret
}
new A(1,2); // => Object array=[2]
A(1,2) // => [1,2]

[jashkenas]

Cool idea, but there are two problems -- we'd have to do the check for every function (distasteful), and arguments.callee is deprecated in ECMAScript 5.

[alsonkemp]

Solid. I've been poking around with the code, too, because there's got to be a way to do this! Your code looks pretty good, though it prevents constructors from ever returning a value. This seems like a fine tradeoff since I'm not sure when a constructor would ever not return 'this'. And even if there's some bizarre little corner case, CoffeeScript can reflect the 99.99% case rather than adopting weirdness in order to support the 0.01% case.

Doh. Just saw Jeremy's note... But we're on the right track.

[weepy]

ok then ! We're saved by our named constructor

A = function A(x,y) {
  var ret = this.array = [x,y];
  if(A != this.constructor) return ret
}

Doesn't fix the check for every function though. Could have a keyword to skip it if you were concerned about performance ??! Eg: fast_square: x ==> x*x

[jashkenas]

Nice, the idea of adding this check, only when the first letter of the function name is capitalized, is awfully appealing.

[weepy]

high five to that ! So the upshot of that would mean that : 1) You can have functions with capital letters if you want. 2) constructors without capital letters 'might' break (if last return value is array or object), so should be avoided ?

[alsonkemp]

[doh... gotta be faster with my comments!]

http://stackoverflow.com/questions/103598/why-was-the-arguments-callee-caller-property-deprecated-in-javascript

Looks as we should be okay, though. I've added uniqueifying stuff to the function name.

function __coffee_script_construct_A(x, y) {
    var ret = this.array = [x, y];
    if (__coffee_script_construct_A != this.constructor) return ret;
}

The check is distasteful, but doesn't have much effect on performance (1000000 new instances => 15s without, 17s with check).

Weepy: good stuff.

[jashkenas]

Weepy:

1. Yep, functions with capital letters wouldn't break, they'd just get an extra check.
2. It can be a CoffeeScript convention to have constructors start with capital letters -- it's already good style, and this is just extra incentive to use it.

[alsonkemp]

FWIW: I really don't think that the little check is an issue. It's a teeny price to pay to keep JS and CS aligned. There's no performance difference (on FF 3.7a) for this simple test case. My vote would be to add the check to every function.

Also, recall that kamatsu uses lower case constructors and isn't alone.

http://github.com/jashkenas/coffee-script/issues/#issue/17/comment/100320

http://github.com/jashkenas/coffee-script/issues/#issue/17/comment/100664

http://github.com/jashkenas/coffee-script/issues/#issue/17/comment/100721

[jashkenas]

Yeah, I'm a bit torn about it, but you won't need it on 95+% of functions, and while the performance isn't a problem, the code clarity is a problem, and the code-size bloat is a real problem for JavaScript.

[weepy]

You'd still be able to use other libraries that had constructors that are lowercase. It's a good pattern anyway to use capital letters for constructors and as it's new code that you'd be writing, I don't think others would particularly have a problem with it.

[noonat]

I'm a fan of constructor:

• It's explicit, much more apparent to new Coffee users what's going on.
• It's good to know when a function should be used with new, instead of called directly
• I think that ==> is dangerous even for experienced Coffee users, as it doesn't necessarily distinguish itself from => when scanning code.
• If users want to use custom functions as constructors in a non-standard way, they can still explicitly return the object
• It doesn't rely as much on magic

[jashkenas]

Alright, this is now resolved on master. I don't think that it's worth tagging a function with a long word ("constructor"), just to avoid writing a short one ("this"). But I do want to protect folks from mysterious errors when constructors work improperly, so master now has weepy's special check added to functions that start with a Capital Letter:

Horse: name => this.name: name

Into:

Horse = function Horse(name) {
  var __a;
  __a = this.name = name;
  return Horse === this.constructor ? this : __a;
};

Closing the ticket... If you'd like to use lowercase constructors, against the style convention, just put a this at the end.

[liamoc]

Right, I am going to make a branch that un-does this feature as I feel it to be against the spirit of this language.

We should under no circumstances affect dynamic semantics by enforcing a naming convention.

Introducing a style convention on top of existing JS style is something I would oppose strongly.

[liamoc]

This is where you are compensating for one divergence from JS semantics (implicit return) with another massive divergence from JS semantics (adding checks to capitalized functions), when you could just add a feature that makes it convergent again, such as a keyword.

[jashkenas]

The divergences from JS semantics are all pointing in the same direction --> everything should be usable as an expression, to the greatest extent possible. I agree with you that the most distasteful part of this "fix" is the capital-letter sniffing. Are you proposing the constructor keyword, which just suppresses the implicit return, or simply having a CoffeeScript convention of placing a this as the last line of constructor functions?

[liamoc]

hm, I agree that constructor is sort of ickishly long, but seeing as significant indentation lends itself to top-heavy declarations, I would prefer to have a keyword at the top rather than have to stick a weird value at the bottom.

It still doesn't sit right with me, but i'd go with constructor, but keep options open for changing that in the future.. there has to be a better, terser notation for it, but the only thing I can think of is changing the => operator to something else for constructors, which is arguably worse

[liamoc]

(by the way, i am kamatsu, i changed my username)

[jashkenas]

My problem with constructor:

Tortoise: How do you create a constructor function in CoffeeScript?

Achilles: As in JavaScript, any function can be used as a constructor ... but you should probably tag the function declaration with the "constructor" keyword.

Tortoise: Why?

Achilles: Well, you don't really need to use it, unless your constructor returns an object that isn't this. If you return another object, then you're not going to get the instance you expect when you call new func()

Tortoise: So I could just return this myself, and it would work.

Achilles: Yes.

Tortoise: So what's the point of having the "constructor" keyword?

Achilles: So that everyone doesn't have to have this conversation.

It's a keyword that's like bubble wrap. It only makes sense to use when you don't understand what's going on. If you understand the JavaScript, you'll probably prefer to return this yourself.

[alsonkemp]

My big beef with 'constructor' is that it's error prone. Forgetting to use the notation will lead to nasty, hard-to-find bugs.

Again, I advocate for adding the 'new' check to every function... It will increase the JS slightly, but it'd make the CS and JS behavior pretty much identical.

[jashkenas]

Reopening this ticket because it needs some more thought. I'm ever-more-often running into situations where the constructor safety check doesn't work successfully, and am starting to think that we either need to make it bulletproof, or just ditch it and say that you always have to put this as the last line of a constructor.

My use case is this quickie inheritance helper function that I'm using for the CoffeeScript-in-CoffeeScript code generation (del is a delete function that returns the value):

inherit: (parent, props) ->
  klass: del(props, 'constructor')
  klass extends parent
  (klass.prototype[name]: prop) for name, prop of props
  klass

Which lets you define subclasses without having to repeatedly assign to the prototype, like so:

CallNode: inherit Node, {

  constructor: (variable, args) ->
    ...

  compile: (options) ->
    ...

}

But those constructors, sadly, don't get the check, even though they wind up with a capitalized name in the end. I'm leaning towards removing the check -- we've exhausted the options pretty well in this conversation so far -- what do y'all think?

[liamoc]

I agree

[weepy]

Seems reasonable - in which case, it comes down to education. For advanced users, in fact you only need to return something that's not and object or an array. But I think returning this makes sense for a constructor - so we should push that message.

[StanAngeloff]

Just throwing ideas here: how about making extends behave in this manner:

B extends A: (name) ->
    @name: name
    super  # Calling and returning value from parent's constructor

should compile to this:

var B;
B = function B(name) {
    this.name = name;
    return A.call(this);
}
B.__superClass__ = A.prototype;
B.prototype = new A();
B.prototype.constructor = B;

when you want a prototype that doesn't extend another class, you would still use:

A extends Class  # built-in keyword

should compile to:

var A;
A = function A() {
    return this;
}

and optionally to define a constructor, in which case you must return this:

A extends Class: ->
    @initialized: 1
    @

A::move: -> alert('Moving, a moment please...')

should compile to (something like):

var A;
A = function A() {
    this.initialized = 1;
    return this;
}

A.prototype.move = function move() {
    return alert('Moving, a moment please...');
}

So to sum it up, a new way to define prototype objects where Coffee will always return this when a constructor was not given. When one is available, as per weepy's comment, you must know what you are doing.

[liamoc]

Why must "Class" be a built in keyword? And why use "Class" when it is not a class at all but an object?

[zmthy]

I don't really understand what you've achieved. You still have to drop in a this when there's no extension, and A extends Class is exactly the same as A: -> this is it not? Am I missing something?

[StanAngeloff]

@Tesco: what I am trying to archive is to make it explicit. When it is explicit, you know what to watch out for, i.e., the return this at the end. This should make it easier for Coffee to figure that out too.

@liamoc: I realise that, just trying to make it more convenient for people that are not used to prototype based programming. A lot of my colleagues and friends still think JavaScript is a true OO language.

From both of your comments, I gather the idea wasn't well received, but then again, reason it's called an idea, Lol.

[liamoc]

Making things more convenient? Or more confusing? Trying to hide JS's prototypal nature will only make things worse. Also, prototypal OO is in no way not a "true OO" paradigm. It's been around since about as long as the class approach.

[jashkenas]

Stan -- It's a nice idea, don't worry about it. This is why it's great to have folks from different backgrounds contributing to the language, to see proposals with different flavors.

I've removed the safety check on master, which may break your current code, so be careful. There was only a single test to fix (and it was testing inheritance anyway), so it probably won't be too difficult to upgrade.

I think that the new party line should be something like: In CoffeeScript, functions return their values. When you're writing a constructor, make sure to return the new instance of the object.

Node: (children) ->
  @children: children
  this

Which will be documented. Closing the ticket...

[jashkenas]

As of Issue #210, CoffeeScript has classes.