Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new operator misbehavior #619

Closed
satyr opened this issue Aug 16, 2010 · 15 comments
Closed

new operator misbehavior #619

satyr opened this issue Aug 16, 2010 · 15 comments

Comments

@satyr
Copy link
Collaborator

satyr commented Aug 16, 2010

There is a common misconception regarding new in JS, producing code like this:

bar = new require("foo").Bar;

The intent here is to create a Bar instance, but fails because the RHS evaluates as (new require("foo")).Bar.

CoffeeScript doesn't correct this situation, but makes it worse:

$ bin/coffee --no-wrap -pe 'new require("foo").Bar'
new require("foo").Bar();

I can think of three compile options:

// leave it as is
new require("foo").Bar

// clarify the actual evaluation order
(new require("foo")).Bar

// adopt simpler (but different from JS) behavior for `new`
new (require("foo").Bar)
@TrevorBurnham
Copy link
Collaborator

Wow, that's fascinating. So you're saying that every major JS implementation evaluates

new A.B

as

(new A).B

rather than what I would expect:

new (A.B)

I'm very surprised by this.

@StanAngeloff
Copy link
Contributor

FM moment:

class1 = ->
  @name = 'class1'

class1.class2 = ->
  @name = 'class2'

factory = (arg) ->
  return { class2: class1.class2 }

obj1   = new class1
obj2_1 = new class1.class2
obj2_2 = new factory('dummy').class2
obj2_3 = new (factory('dummy')).class2
obj2_4 = new (factory('dummy').class2)

alert obj1.name    # class1
alert obj2_1.name  # class2
alert obj2_2.name  # undefined
alert obj2_3.name  # class2
alert obj2_4.name  # class2

I bet this has something to do with Coffee adding () after the last property access.

@jashkenas
Copy link
Owner

After messing around with it, I don't believe that the initial premise of this ticket is correct.

bar = new require("foo").Bar

... works correctly in both JavaScript and CoffeeScript, not the way that you assert it does. Stan's test case got screwy because he wasn't returning this from constructor functions, but rather, other objects. If the test case is adjusted, it produces this result:

class1 = ->
  @name = 'class1'
  this

class1.class2 = ->
  @name = 'class2'
  this

factory = (arg) ->
  return { class2: class1.class2 }

obj1   = new class1
obj2_1 = new class1.class2
obj2_2 = new factory('dummy').class2
obj2_3 = new (factory('dummy')).class2
obj2_4 = new (factory('dummy').class2)

puts obj1.name    # class1
puts obj2_1.name  # class2
puts obj2_2.name  # class2
puts obj2_3.name  # class2
puts obj2_4.name  # class2

Which is all well and good. Closing the ticket as n/a.

@satyr
Copy link
Collaborator Author

satyr commented Aug 17, 2010

TrevorBurnham: Wow, that's fascinating. So you're saying that every major JS implementation evaluates new A.B as (new A).B

Not quite.

new A.B     // new (A.B)()
new A().B   // (new A()).B
new A.B().C // (new (A.B)()).C

@satyr
Copy link
Collaborator Author

satyr commented Aug 17, 2010

jashkenas: ... works correctly in both JavaScript and CoffeeScript

Not quite. Your test case doesn't produce the same result in JS.

class1 = function() {
  this.name = 'class1';
  return this;
};
class1.class2 = function() {
  this.name = 'class2';
  return this;
};
factory = function(arg) {
  return {
    class2: class1.class2
  };
};

obj1 = new class1;
obj2_1 = new class1.class2;
obj2_2 = new factory('dummy').class2;
obj2_3 = new (factory('dummy')).class2;
obj2_4 = new (factory('dummy').class2);

puts = require('sys').puts;
puts(obj1.name);   # class1
puts(obj2_1.name); # class2
puts(obj2_2.name); #
puts(obj2_3.name); # class2
puts(obj2_4.name); # class2

Note the 3rd being blank because obj2_2 === class2 in that expression.

@jashkenas
Copy link
Owner

Sure, that's fine -- CoffeeScript fixes this broken JS behavior by compiling the 3rd line like so:

obj2_2 = new factory('dummy').class2();

Note the trailing parentheses. This makes the output correct.

@satyr
Copy link
Collaborator Author

satyr commented Aug 17, 2010

jashkenas: obj2_2 = new factory('dummy').class2();

This exhibits a classic JS pitfall--doesn't [[Construct]] class2, but [[Call]]s class2 with new factory('dummy') as this value.

Try appending:

puts obj2_2
puts obj2_2.class2

to your test case.

@jashkenas
Copy link
Owner

ugh, how is that possible? ... Fuck me, indeed. satyr got any documentation links for this behavior?

@satyr
Copy link
Collaborator Author

satyr commented Aug 17, 2010

http://ecma262-5.com/ELS5_Section_11.htm#Section_11.2
Can't find an article offhand that particularly addresses this.

@jashkenas
Copy link
Owner

I've been toying with this a bit, and it's really as nasty as satyr describes. I need some help with this piece of it. Say you want to construct a new object from a require'd property, and you want to pass arguments to the constructor. Is this possible to write in JavaScript?

var instance  = new require("foo").Bar(arg1, arg2);

With any combination of parentheses?

@satyr
Copy link
Collaborator Author

satyr commented Aug 21, 2010

you want to construct a new object from a require'd property, and you want to pass arguments to the constructor

var instance  = new (require("foo").Bar)(arg1, arg2);

@jashkenas
Copy link
Owner

The patch is now on master, to fix the behavior to do the expected thing ... not the JavaScript thing:

http://github.com/jashkenas/coffee-script/commit/e7834de92974f997e692dd1c11f735a71078666f

Thanks, satyr, for bringing up this issue. It's a nasty JS gotcha that I hadn't suffered before, and was completely unaware of. It's wonderful that we're in a position to be able to do something about it.

Edit: Nevermind -- the cure was worse than the disease. Reverted.

@satyr
Copy link
Collaborator Author

satyr commented Aug 28, 2010

wontfix...?

@michaelficarra
Copy link
Collaborator

This is still an issue. I'm not sure why it's closed.

$ coffee -ne 'new A.B().C'
Block
  Op new
    Value
      Call
        Value "A"
          Access "B"
      Access "C"

Fixed in CoffeeScriptRedux :) (and Coco and LiveScript...). Reopening.

edit: Also, check this out:

$ coffee -bep 'new A[0..0][0]'
new A.slice(0, 1)[0];

Eek. Not good.

@satyr
Copy link
Collaborator Author

satyr commented Jun 29, 2012

This is still an issue.

How? Seems working to me. The AST looking incorrect is less important.

$ coffee -bce 'new A.B().C'
// Generated by CoffeeScript 1.3.3

new A.B().C;

Also, check this out:

$ coffee -bep 'new A[0..0][0]'
new A.slice(0, 1)[0];

#1189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants