Skip to content

Generating functions from comprehensions doesn't close over variables.. #423

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

Closed
foot opened this issue Jun 10, 2010 · 9 comments
Closed

Comments

@foot
Copy link

foot commented Jun 10, 2010

TEST:
[f1, f2]: (() -> n) for n in [1,2]
ok f1() is 1 # Fails (f1() is 2)
ok f2() is 2 # Passes

Coffee generates:

(function(){
  var _a, f1, f2;
  // Generating functions from a comprehension
  _a = (function() {
    var _b, _c, _d, _e, n;
    _b = []; _d = [1, 2];
    for (_c = 0, _e = _d.length; _c < _e; _c++) {
      n = _d[_c];
      _b.push((function() {
        return n;
      }));
    }
    return _b;
  })();
  f1 = _a[0];
  f2 = _a[1];
})();

but should generate something like this probably?

(function(){
  var _a, f1, f2;
  // Generating functions from a comprehension
  _a = (function() {
    var _b, _c, _d, _e;
    _b = []; _d = [1, 2];
    for (_c = 0, _e = _d.length; _c < _e; _c++) {
      (function(){
        var n = _d[_c];
        _b.push((function() {
          return n;
        }));
      })();
    }
    return _b;
  })();
  f1 = _a[0];
  f2 = _a[1];
})();

Ta

@jashkenas
Copy link
Owner

I'm afraid that this is a feature... Comprehensions are supposed to have the same speed and roughly the same semantics as the equivalent JavaScript for loop. So, like for loops, variables will not be closed over between iterations.

If you'd like them to, I'd recommend using forEach or map, if you're on Node.js or an ECMA5. Otherwise, you can save the variable by using an external function:

returns: (x) -> -> x
[f1, f2]: returns n for n in [1, 2]

@foot
Copy link
Author

foot commented Jun 11, 2010

Ahhh bummer that sucks!

If it ever comes up again I'm totally +1 in favour in breaking with semantics for this case! For perf.. could test for anon function at compile time and only wrap in this case? Though messy semantics grrr.

But thanks for the response! I think using map makes the code even clearer anyway.
[rsin, rcos]: [Math.sin, Math.cos].map((f) -> (a) -> Math.round(r * f(a)))
Cheers

@jashkenas
Copy link
Owner

That's nice, or you can keep it simple:

rsin: (n) -> Math.round r * Math.sin n
rcos: (n) -> Math.round r * Math.cos n

But yep, performance is the main thing here. It actually would be possible to test for function literals in the body of the loop... Do you think that would be desirable in all circumstances?

@tcr
Copy link

tcr commented Jun 11, 2010

"returns (variable at time of invocation)" to create an implicit closure isn't that bad of a feature. Is there any comparable keyword in any language?

Edit: Or something like:

nodes: for k, v of hash
    closure -> "Key: " + k + ", Value: " + v
alert(node()) for node in nodes

@hen-x
Copy link

hen-x commented Jun 11, 2010

I think closing over loop variables is a great idea, and testing for function literals in the loop body would be a solid way to decide whether or not to capture them.
Is there any situation where one might define a function inside a loop, but reasonably rely on the loop variables not being captured?

@jashkenas
Copy link
Owner

Alright folks -- I think we have this one licked. Take a close look at this test case, because I want to make sure we have the right semantics here:

# Ensure that the closure wrapper preserves local variables.
obj: {}

for method in ['one', 'two', 'three']
  obj[method]: ->
    "I'm " + method

ok obj.one()   is "I'm one"
ok obj.two()   is "I'm two"
ok obj.three() is "I'm three"


# Even when referenced in the filter.
list: ['one', 'two', 'three']

methods: for num in list when num isnt 'two'
  -> num

ok methods.length is 2
ok methods[0]() is 'one'
ok methods[1]() is 'three'

Here's the patch (includes some formatting):

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

Closing the ticket.

@foot
Copy link
Author

foot commented Jun 14, 2010

Cool! Awesome stuff.

I couldn't think of or find any situations where you wouldn't want this behavior in the end. I'm still in support of the patch.

Did come across some interesting links though:

  • Discussing block-scope and potential design choices and benefits behind it:
    http://lambda-the-ultimate.org/node/1775
    They mention python behaves similarly which was a shock to me, so I checked it and...

    In [1]: f1, f2 = [ lambda: x for x in (1,2) ]

    In [2]: assert(f1() is 1)

    AssertionError Traceback (most recent call last)

Wowo! And in Firefox 3.6.3:

<script type="application/javascript;version=1.8">
  var fs = [ (function(){ return n; }) for each (n in [1,2]) ];
  console.log(fs[0]()); // 2
  console.log(fs[1]()); // 2
</script> 

So I guess this patch is more of a feature than a bug fix. (cool feature though.)

@hen-x
Copy link

hen-x commented Jun 14, 2010

@jashkenas: that test case definitely looks right.

@foot : thanks for the illuminating language-design links! As a spectator on this issue tracker, it's great being able to place this stuff in context.

@TrevorBurnham
Copy link
Collaborator

I've raised an issue to discuss the removal of this feature, which has proven problematic: Issue 959.

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

5 participants