-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Comments
This is not a bug. There is no way to use the |
@michaelficarra I can see several ways; as mentioned in the “observations” above. This is most certainly a bug, if CoffeeScript doesn't generate a 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. (= |
@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. |
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 |
Please demonstrate. I'd love to be proven wrong here. |
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:
… 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. |
I'd be for a message in the documentation, but it would have to emphasise how minor an issue this is. |
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. |
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. |
@erisdiscord exactly the kind of thing I'd have written. I'm so proud of this someone. :) |
Related ECMAScript-Regrets issue: DavidBruant/ECMAScript-regrets#28 |
For posterity -- @ELLIOTTCABLE ... how on earth did you run into this in your project's code? |
@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.) |
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 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. |
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-constructedObject
screws up the native constructors.The same general thing can happen with any native constructor that operates on objects with
[[Class]]
other than"Object"
:Observations
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 beingString
or such), or at run-time (more generated code, never good, but much more robust) for checking whether we can't use thenew 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-awaythis
-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!)The text was updated successfully, but these errors were encountered: