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

Bug with assigning function and calling it immediately. #2

Closed
fitzgen opened this issue Dec 26, 2009 · 8 comments
Closed

Bug with assigning function and calling it immediately. #2

fitzgen opened this issue Dec 26, 2009 · 8 comments

Comments

@fitzgen
Copy link

fitzgen commented Dec 26, 2009

This should assign the function to square and then call it with 4 as an argument, but when everything is said and done square is assigned the result of the function call.

cs> (square: x => x * x.)(4)
16
cs> square
16

Here is the equivalent (correct) js:

js> (square = function (x) { return x * x })(4)
16
js> square
function (x) {
    return x * x;
}

It doesn't seem like you are using the issue tracker right now, but I wasn't sure how else to notify you.

I'm not sure what the javascript that is getting compiled looks like, but I think it would be cool to have a function in the REPL like Common Lisp's "macroexpand" and "macroexpand-1" to see the JS code that was being generated.

Thanks a bunch,

Nick

@jashkenas
Copy link
Owner

Definitely using the issue tracker -- this is the right place.

For future reference, you can load "coffee-script" like any other Narwhal module. To check what a function compiles as from within the REPL, you can do this:

cs> coffee = require('coffee-script')
[object Object]
cs> coffee.compile("(square: x => x * x.)(4)")
square = function(x) {
  return x * x;
}(4);

In the REPL, things compile a little differently, without "var" keywords at the top level, or the function wrapper, in order to add every input line to global scope by default.

If you want to quickly check compilation, I'd use coffee-script -e "(square: x => x * x.)(4)"

Unfortunately, your problem is a little bit deeper than at first glance. Your parenthesis are getting ignored because they contain an assignment, which is considered to be a statement (because it might have to declare "var", which would be a syntax error in JS). You can't do (var square = function ... ). It would be great if we could fix this in CoffeeScript by pulling out the variable declaration outside the parenthesis, but the code generation doesn't support that at this point. It might also be better to throw a syntax error when parenthesis enclose a statement.

If you have ideas for how best to fix it, I'm all ears. Perhaps we can special case it, just for assignment, in the same manner as custom_return and custom_assign.

@fitzgen
Copy link
Author

fitzgen commented Dec 26, 2009

You're right this does get a little messy.

Off the top of my head, here is the best I can think up for special casing it:

js> coffee.compile("(square: x * x.)(4)")
var square;
(square = function (x) {
    return x * x;
})(4);

What do you think of that?

@jashkenas
Copy link
Owner

Yeah, that would be ideal, but it's going to get complicated. Assignment can't be used as an expression in the general case unless the variable is already declared and it's wrapped in parenthesis. Take this case:

1 + 2 + (func: => 3.)()

Right now it compiles into a JS syntax error:

1 + 2 + var func = function() {
  return 3;
}();

Ideally, it would compile into this:

var func;
1 + 2 + (func = function() {
  return 3;
})();

In order to accomplish this, the inner assignment would have to push back the variable declaration through the previous ParenNode and OpNode, to tuck it in as the expression prior to the current one. Either backtracking or a multi-pass compiler is required for that (I think). Neither of which we have at the moment.

Maybe something with throw :declaration, and catching it at the Expressions level....

@fitzgen
Copy link
Author

fitzgen commented Dec 26, 2009

I would say just throw an exception then. This is pretty edge case, and its not like CoffeeScript has to match JS one-to-one for every little detail.

Maybe this can be revisited after more important things have been completed? I think there definitely should be some type of alert to the user about the current ambiguities if they try to compile something like this, though.

@jashkenas
Copy link
Owner

Sure, you got it. I'll throw an exception if a parenthesis wraps a statement, instead of swallowing it silently. And leave this ticket open as an enhancement for future reference. If we could push back the declaration (ensuring it gets declared in proper scope), then all assignment could be used as an expression, which would be pretty cool.

@fitzgen
Copy link
Author

fitzgen commented Dec 26, 2009

Idea!

Instead of pushing back the "var" declaration, can we push it forward? I know that JS looks ahead for all vars declared and named functions (not "var foo = function ..." but "function foo () ....") and will actually read those first before executing any code. Thats why you can define functions inside other functions after the return! Very wierd.

Check out this code I just ran through firebug's console:

(function () {
    alert(1 + 2 + (foo = 3) + 4);
    alert(foo);
    alert(window.foo);
    var foo;
})();

Does this make it easier?

@jashkenas
Copy link
Owner

Thanks for pushing this issue. It made a real positive change to the language. I just pushed a commit that pulls out all variable declarations to the top of the current scope, allowing assignment to act as an expression. Pull from master and give it a try:

six: (one: 1) + (two: 2) + (three: 3)

Compiles into:

var one, six, three, two;
six = (one = 1) + (two = 2) + (three = 3);

Your original example:

(square: x => x * x.)(4)

Compiles into:

var square;
(square = function(x) {
  return x * x;
})(4);

As it turns out, we already had the Scope object handy, which knew exactly which variables were declared inside of it (as opposed to being declared within an external scope). This commit has the changes:

http://github.com/jashkenas/coffee-script/commit/9adf2e2d306bcd36eebd9e162adf6e30a374b9c8

Closing the ticket...

@fitzgen
Copy link
Author

fitzgen commented Dec 26, 2009

Don't thank me, I didn't do anything. Thank you!

stephank pushed a commit to stephank/coffee-script that referenced this issue May 14, 2011
alangpierce added a commit to alangpierce/coffeescript that referenced this issue Aug 6, 2016
Backport of jashkenas#4291 .

In f609036, a line was changed from
`if length > 0 then (length - 1) else 0` to `Math.max 0, length - 1`. However,
in some cases, the `length` variable can be `undefined`. The previous code would
correctly compute `lastCharacter` as 0, but the new code would compute it as
`NaN`. This would cause trouble later on: the end location would just be the end
of the current chunk, which would be incorrect.

Here's a specific case where the parser was behaving incorrectly:
```
a {
  b: ->
    return c d,
      if e
        f
}
g
```

The OUTDENT tokens after the `f` had an undefined length, so the `NaN` made it
so the end location was at the end of the file. That meant that various nodes in
the AST, like the `return` node, would incorrectly have an end location at the
end of the file.

To fix, I just reverted the change to that particular line.

Also add a test that tokens have locations that are in order
This issue was closed.
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

2 participants