Fix #1403: Variables inconsistently masked inside of do

[zdenko]

Fixes #1403.
It seems that For::pluckDirectCall isn't needed.

Examples from #1403

for dummy in [0...1]
  masked = 10
  do -> alert masked = masked + 1
alert 'a line needs to be here to trigger the issue'

for (dummy = i = 0; i < 1; dummy = ++i) {
  masked = 10;
  (function() {
    return alert(masked = masked + 1);
  })();
}
alert('a line needs to be here to trigger the issue');

---

for dummy in [0...1]
  notMasked = 10
  do -> alert notMasked += 1
alert 'a line needs to be here to trigger the issue'

for (dummy = j = 0; j < 1; dummy = ++j) {
  notMasked = 10;
  (function() {
    return alert(notMasked += 1);
  })();
}
alert('a line needs to be here to trigger the issue');

[GeoffreyBooth]

Before we jettison pluckDirectCall entirely, can someone explain why it was added in the first place? From my Git diving I see that @satyr added it in 4e39e2e; @satyr, do you mind sharing what was on your mind back in 2010? The commit message was “made dynamic calls at the last of for expression be defined outside”.

I did some testing, and on master pluckDirectCall is currently called by the following tests, all in comprehensions.coffee:

• Ensure that the closure wrapper preserves local variables.
• Even a convoluted one.
• Scoped loop pattern matching.
• Issue Loop variables are captured in closures; others are not #948. Capturing loop variables.
• Comprehensions over function literals.

I pulled those tests into their own file and compiled them into JS using both master and this branch. Here’s the diff.

A minimal case seems to be this:

for i in [1..3] then do -> console.log i

On master:

var fn, i, j;

fn = function() {
  return console.log(i);
};
for (i = j = 1; j <= 3; i = ++j) {
  fn();
}

Per this PR:

var i, j;

for (i = j = 1; j <= 3; i = ++j) {
  (function() {
    return console.log(i);
  })();
}

Both versions print 1, 2, 3, which might not be obvious at first; the reference to i is consistent no matter where the do function is put. But the two outputs illustrate the difference pluckDirectCall makes, which is to shift a do-wrapped function outside of the for loop, presumably for scope reasons. Sounds a lot like “made dynamic calls at the last of for expression be defined outside.” So . . . why? What is gained by creating a temporary variable fn and assigning this do function to it, and calling fn inside the for loop rather than leaving the function itself there?

4e39e2e has this test added, in obsolete CoffeeScript syntax:

# If the last expression is dynamic call,
# define it outside and pass loop variables to it.
fs = for i from 0 to 2 when i > 0
  void
  do ->
    {callee} = arguments
    -> [i, callee]
[one, two] = (f() for f in fs)
eq one[0], 1
eq two[0], 2
eq one[1], two[1]

Even converting this into modern syntax:

fs = for i in [0..2] when i > 0
  do ->
    {callee} = arguments
    -> [i, callee]
[one, two] = (f() for f in fs)
eq one[0], 1
eq two[0], 2
eq one[1], two[1]

none of these tests pass, assuming I converted it correctly. Perhaps pluckDirectCall is a relic of those times, when scope and comprehensions behaved differently?

[vendethiel]

@GeoffreyBooth my memory is a bit fuzzy because it's been a long time... But AFAIK not defining the function inside the loop is an optimization (see coco's wiki page "Additions").
As to why the test passes: I don't remember everything, but I think at some point do auto-captured loop variables(?). I think that's why the test passed.

[vendethiel]

Also, paging @satyr about some reasoning from 2010 is probably not going to work...

[GeoffreyBooth]

Ah, an optimization makes sense. Well, it's not really worth keeping if it's buggy, which it appears to be; and it's an optimization the user could do themselves. If they want the function defined before the loop and referenced in the loop, they could write their code as such.

So this looks good to me. @vendethiel, any notes? And have we lost @satyr?

[vendethiel]

LGTM. As for @satyr, I think their last commit was a few years back, so we'd have to wait for a while.

[connec]

This may lead to non-trivial performance degradations for code using the old for ... then do -> construct:

for a in [1, 2, 3] then do ->
  setTimeout (-> console.log a), 1000