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

Use of Array.prototype for __slice throws on Array declaration #1973

Closed
jed opened this issue Dec 26, 2011 · 15 comments
Closed

Use of Array.prototype for __slice throws on Array declaration #1973

jed opened this issue Dec 26, 2011 · 15 comments
Labels

Comments

@jed
Copy link

jed commented Dec 26, 2011

This CoffeeScript

class Array
  [@...]

compiles to this JavaScript

var Array,
  __slice = Array.prototype.slice;

Array = (function() {

  function Array() {}

  __slice.call(Array);

  return Array;

})();

which throws because Array is undefined here.

Wouldn't it be safer to use [].slice instead, since it will always work, or at least reverse the order of declaration?

@michaelficarra
Copy link
Collaborator

Yeah, we should probably reverse the order of declaration.

@satyr
Copy link
Collaborator

satyr commented Dec 26, 2011

Order doesn't matter:

js> new function(){ var __slice = Array.prototype.slice, Array }
typein:1: TypeError: Array is undefined

@michaelficarra
Copy link
Collaborator

Hah, whoops. Can't believe that slipped my mind.

@jed
Copy link
Author

jed commented Dec 26, 2011

Ah, of course. Thanks @satyr.

Personally, I think [] is the best place to get the Array prototype, but I suppose you could also do this:

__slice = function(){ return this.Array.prototype.slice }()

@michaelficarra
Copy link
Collaborator

@jashkenas: Do you think this is one of those issues that's so pathological it's not even worth fixing? I'm thinking that may be so, especially since the fix negatively effects both the size and execution time of every other program.

Another possible fix is to initialise the top-level scope object with all of the built-in constructors and possibly all of the other predefined globals.

@jashkenas
Copy link
Owner

@michaeficarra: What's the perf hit of [].slice these days?

@geraldalewis
Copy link
Contributor

What's the perf hit of [].slice these days?

[].slice appears to be slightly slower than Array.prototype.slice

The difference is fairly negligible and the alternative proposal:

__slice = function(){ return this.Array.prototype.slice }()

has holes: this in that IIFE is really "null", which is coerced to the Global object only in non-strict mode, so it wouldn't be viable.

Another possible fix is to initialise the top-level scope object with all of the built-in constructors and possibly all of the other predefined globals.

This is interesting -- weak sandboxing. Seems like a lot of overhead for a limited use-case though (this issue seems to be about user-defined classes with the same name as native Globals that CoffeeScript uses internally -- is that right?).

@michaelficarra
Copy link
Collaborator

Illustrating @geraldalewis's point:

$ node
> (function(){ "use strict"; __slice = function(){ return this.Array.prototype.slice }(); return __slice; }())
TypeError: Cannot read property 'Array' of undefined
    at repl:1:62
    at repl:1:85
    at repl:1:107
    at REPLServer.eval (repl.js:80:21)
    at Interface.<anonymous> (repl.js:182:12)
    at Interface.emit (events.js:67:17)
    at Interface._onLine (readline.js:162:10)
    at Interface._line (readline.js:426:8)
    at Interface._ttyWrite (readline.js:603:14)
    at ReadStream.<anonymous> (readline.js:82:12)

@shesek
Copy link

shesek commented Jan 7, 2012

[].slice appears to be slightly slower than Array.prototype.slice

Since its ran only once and assigned into __slice (rather than being used directly) I don't think a slight performance difference is important here (and its really slight).

__slice = function(){ return this.Array.prototype.slice }()
has holes: this in that IIFE is really "null", which is coerced to the Global object only in non-strict mode, so it wouldn't be viable.

Can't we just (global||window).Array.prototype.slice?

@geraldalewis
Copy link
Contributor

Another option could be to modify Scope's find method to return true for any global var. CoffeeScript wouldn't declare Array then, since it's already implicitly declared. This shouldn't be a problem for built-ins, but I feel like maintaining an accurate list of all host objects as well, for every platform, would be tricky (maybe there aren't as many as I think there may be). In fact, that different environments have different globals makes this idea non-viable, right?

I think we have two good options:

  1. Modify the utility methods indexOf, hasProp, and slice to use Array and Object literals instead of their respective class names.
__slice = [].slice
  • Slightly less readable (Array.prototype.slice is maybe more explicit than[].slice)
  • Measurable but negligible performance impact
  • Saves a few chars?
  1. wontfix. It's a genuine bug and obviously a real issue, but the cure may be worse than the disease.

Edit: Oops - quite right @shesek - I meant [].slice. Thanks - fixed :)

@shesek
Copy link

shesek commented Jan 7, 2012

[].prototype.slice? An array instance doesn't have its prototype set to anything. Perhaps you meant [].__proto__.slice (or Object.getPrototypeOf([]).slice)? (which aren't supported in all browsers)

@jashkenas
Copy link
Owner

I think that [].slice should be fine.

@michaelficarra
Copy link
Collaborator

Me too.

@michaelficarra
Copy link
Collaborator

@jed: fixed by the above commit

@jed
Copy link
Author

jed commented Jan 11, 2012

awesome. many thanks, @michaelficarra.

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

No branches or pull requests

6 participants