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

Passing splats to constructors screws up native constructors #3033

Closed
ELLIOTTCABLE opened this issue Jun 13, 2013 · 15 comments
Closed

Passing splats to constructors screws up native constructors #3033

ELLIOTTCABLE opened this issue Jun 13, 2013 · 15 comments
Labels

Comments

@ELLIOTTCABLE
Copy link
Contributor

Spent a couple hours banging my head against something, before managing to reduce it to the following test-case. It looks like CoffeeScript's approach of apply()ing a constructor against a newly-constructed Object screws up the native constructors.

func = (a, b, args...) ->
   s = new String args...
   s.toString()

func 'foo', 'bar', {}

TypeError: String.prototype.toString is not generic
  at String.toString (native)
  at func (/Users/elliottcable/Code/cord/test.coffee:11:4, <js>:39:14)
  at Object.<anonymous> (/Users/elliottcable/Code/cord/test.coffee:13:1, <js>:42:3)
  at Object.<anonymous> (/Users/elliottcable/Code/cord/test.coffee:60:4)
  at Module._compile (module.js:456:26)

The same general thing can happen with any native constructor that operates on objects with [[Class]] other than "Object":

func = (a, b, args...) ->
   d = new Date args...
   d.toString()

func 'foo', 'bar', 1, 26, 1989

TypeError: this is not a Date object.
  at Date.toString (native)
  at func (/Users/elliottcable/Code/cord/test.coffee:17:4, <js>:39:14)
  at Object.<anonymous> (/Users/elliottcable/Code/cord/test.coffee:19:1, <js>:42:3)
  at Object.<anonymous> (/Users/elliottcable/Code/cord/test.coffee:44:4)
  at Module._compile (module.js:456:26)

Observations

func = function() {
  var a, args, b, s;

  a = arguments[0], b = arguments[1], args = 3 <= arguments.length ? __slice.call(arguments, 2) : [];
  s = (function(func, args, ctor) {
    ctor.prototype = func.prototype;
    var child = new ctor, result = func.apply(child, args);
    return Object(result) === result ? result : child;
  })(String, args, function(){});
  return s.toString();
};

child in the transpiled source is always going to be an "object" type with [[Class]] of "Object". We need a method, either at compile-time (fragile, checking for the constructor-name in the source being String or such), or at run-time (more generated code, never good, but much more robust) for checking whether we can't use the new function(){} style.

Here's one, tentative, possibility, that would have to be checked against various real-world implementations: replacing the noöp function(){} with the constructor we're given, but only when we can tell what that constructor is (as far as I know, all the native constructors are safe to be called twice, once to simply generate a throw-away this-argument with the correct [[Class]]), as we don't want to apply that effect to users' custom constructors (not all of them will be safe to be called twice!)

@michaelficarra
Copy link
Collaborator

This is not a bug. There is no way to use the new operator with a nondeterministic number of arguments. You can simulate the desired behaviour for any constructor you can create, but there are special steps defined in the spec for native constructors that you can't specify in JS code, such as setting the [[Class]] or [[PrimitiveValue]] internal properties.

@ELLIOTTCABLE
Copy link
Contributor Author

@michaelficarra I can see several ways; as mentioned in the “observations” above. This is most certainly a bug, if CoffeeScript doesn't generate a SyntaxError for new Constructor args..., then it's implying it supports that (not to mention that it obviously generates special code for that!); and if it supports that, then it's extremely unintuitive for it to fail when passed certain constructors.

Either implement a way to support all constructors, or make it a syntax error and include a mention in the documentation. o_O

@michaelficarra unrelated, but you don't seem to have a Twitter or to be on IRC, so I'll ask here: you're in Chicago? We should meet up sometime. (=

@michaelficarra
Copy link
Collaborator

@ELLIOTTCABLE: Only non-native constructors are supported. How did you expect a splat to work with a native constructor when you used it?

There's no way to support the native constructors, as I've shown. This doesn't mean that the feature isn't useful. There's no reason we should make it a syntax error.

@ELLIOTTCABLE
Copy link
Contributor Author

Again, “no way” is more than a little extremist; there's plenty of ways. I understand, however, if none of them are desirable or relevant to CoffeeScript.

Then, at the very least, let's make this issue a request to document that fact? It's anything but obvious to a newcomer why new Foo rest_of_args... would work, but then new Function rest_of_args... wouldn't. I can understand the restrictions, but it took even me a solid hour of digging to track my bug down to that very non-obvious interaction (passing a native constructor to a function that previously had only been handling custom constructors, in my case.)

@michaelficarra
Copy link
Collaborator

Again, “no way” is more than a little extremist; there's plenty of ways

Please demonstrate. I'd love to be proven wrong here.

@ELLIOTTCABLE
Copy link
Contributor Author

I'll summarize the approaches that seem obvious to me, though I don't have time to write code at this moment. If you're having trouble understanding, I'd be happy to take a stab at putting this issue off, and writing some example code another time:

  • Compile-time: As mentioned above, a fragile approach, but better than the current inobvious and undocumented operation: check if the constructor being is being invoked through a reference-name known to refer to a native constructor by default
  • Run-time: Check if the func.name matches a list of known native-constructors; if so, invoke the constructor once with no arguments, then check the {}.toString to see if it's truly constructing a known [[Class]]. If so, pass that instance (with the correct [[Class]]) into apply(), to prevent incorrect-type errors. If not, proceed with the current implementing, assuming the constructor's something else with the same name.
  • Error-checking: If attempting the construction throws a TypeError, re-run it with a nullary first-argument to apply
  • Evaluatory: eval-compile a wrapping-function on the fly that invokes the given constructor with an actual new and the appropriate number of arguments, wrap it in a function that takes that number of arguments, then apply that wrapper

… and so, and on, and so on. Not saying any of them are good ideas; as I said, none of them may be desirable or relevant to CoffeeScript, and that's fine. At this point, all I'm suggesting is a change in documentation for newcomers.

@michaelficarra
Copy link
Collaborator

@ELLIOTTCABLE:

  • The compile-time and run-time tests are no more reliable than not having a test. They can still fail in similarly rare scenarios. NotTheNativeStringConstructorIPromise = String. function String(){}.
  • I don't understand the error-checking one. The construction doesn't throw an error, it just doesn't construct the object in a way that you want, setting specific internal properties on the object used as the constructor's context.
  • You're right about the eval method, I didn't think about that. But obviously that's not an option.

I'd be for a message in the documentation, but it would have to emphasise how minor an issue this is.

@erisdev
Copy link

erisdev commented Jun 14, 2013

Yeahhh, it's unfortunate that we're stuck with this problem, but it's not something CoffeeScript can work around in a nice way. I just try to avoid situations where I need to call native constructors with an unknown number of arguments. But if you really need to, you can do something horrifying like this:

createNativeObject = (ctor, args) ->
  argNames = ("$#{i}" for i in [0...args.length])
  factory = Function('ctor', argNames, "return new ctor(#{argNames})")
  factory ctor, args...

that sure was fun to write. It's also slow as all get out.

@erisdev
Copy link

erisdev commented Jun 14, 2013

Here, somebody (definitely not me) made something really gross to show off a couple variants that cache the generated factory function and you should never use them. Just as an exercise in futility, of course. Futility and perversion.

@ELLIOTTCABLE
Copy link
Contributor Author

@erisdiscord exactly the kind of thing I'd have written. I'm so proud of this someone. :)

@michaelficarra
Copy link
Collaborator

Related ECMAScript-Regrets issue: DavidBruant/ECMAScript-regrets#28

@jashkenas
Copy link
Owner

For posterity -- @ELLIOTTCABLE ... how on earth did you run into this in your project's code?

@ELLIOTTCABLE
Copy link
Contributor Author

@jashkenas a quick stab at metaprogrammatic testing of some code I was throwing together. If your eyes melt from reading this code, I'm not liable. ;)

describe '(aping a String)', ->
   mimic = (first, second, constructee...)-> (method)-> ->
      a = new first constructee...; b = new second constructee...
      expect(a[method] arguments...).to.be(b[method] arguments...)
      return expect(a[method] arguments...)

   it 'constructs arbitrary arguments via their #toString', ->
      mimic(Cord, String, {}        )('toString')().to.be '[object Object]'
      mimic(Cord, String, 42        )('toString')().to.be '42'
      mimic(Cord, String, [1, 2, 3] )('toString')().to.be '1,2,3'

      thingie = { toString: -> return 'whee' }
      mimic(Cord, String, thingie )('toString')().to.be 'whee'

(Using Mocha and expect.js, rather obviously, I suppose.)

@notslang
Copy link

I actually managed to hit this issue on a (somewhat) reasonable piece of code that converts a non-standard date string (from the WordPress JSON API) into a regular Date object. The format returned by model.get('date') looks like 2014-07-16 15:55:00.

model.set 'date', new Date(model.get('date').split(/[-\s:]/)...)

In this case I don't really need a variable number of args, so I was able to do:

[a, b, c, d, e, f] = model.get('date').split(/[-\s:]/)
model.set 'date', new Date(a, b, c, d, e, f)

...I think adding a warning to the compiler for when someone tries use a splat on something that looks like a native constructor would be nice.

Also, it seems like this should work:

model.set 'date', new Date(model.get('date').split(/[-\s:]/)[0..5]...)

...since it states explicitly how many args are being passed, but getting that to actually work is probably way more work than it's worth.

@vendethiel
Copy link
Collaborator

@slang800 related: #2183

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