`for` variable shadowing with destructuring #2273

Closed
satyr opened this Issue Apr 21, 2012 · 9 comments

5 participants

@satyr
Collaborator
$ coffee -bce 'v = 0; -> for v in a then'
// Generated by CoffeeScript 1.3.1
var v;

v = 0;

(function() {
  var v, _i, _len, _results;
  _results = [];
  for (_i = 0, _len = a.length; _i < _len; _i++) {
    v = a[_i];  }
  return _results;
});

$ coffee -bce 'v = 0; -> for [v] in a then'
// Generated by CoffeeScript 1.3.1
var v;

v = 0;

(function() {
  var _i, _len, _results;
  _results = [];
  for (_i = 0, _len = a.length; _i < _len; _i++) {
    v = a[_i][0];
  }
  return _results;
});

Note the lack of inner var v in the second.

See also: #1552

@geraldalewis

I think #643 definitely needs to be reexamined now (especially after reading #238). See my comment here.

@michaelficarra
Collaborator

Yep, we both acknowledged that problem in #1552. The solution apparently isn't acceptable, though.

@geraldalewis

I wasn't claiming credit :); the conversation above my comment was addressing the issue by proposing new functionality (ensuring iterators were not exposed). I'm arguing for simply not special-casing comprehensions within nested functions.

@michaelficarra
Collaborator

Oh, me either. I was just making it clear that others have shared the same opinion, but there's pushback from @jashkenas. Pulling the declaration in would solve both problems (accidental shadowing, as your comment pointed out, and exposure of the iterator).

@geraldalewis

Ahh -- gotcha :) -- the commuter rail tends to re-contextualize everything in a weird light ;)

@jashkenas
Owner

Perhaps my pushback is mere confusion. What's the desired change here exactly?

I'm arguing for simply not special-casing comprehensions within nested functions.

... that indeed sounds like what we should be doing.

@michaelficarra
Collaborator

@jashkenas: Gerald's comment, compiled:

(function() {
  var outerVar;
  outerVar = "outer";
  return function() {
    return console.log(outerVar);
  };
});

(function() {
  var outerVar;
  outerVar = "outer";
  return function() {
    var outerVar, _i, _len, _ref, _results;
    console.log(outerVar);
    _ref = [0];
    _results = [];
    for (_i = 0, _len = _ref.length; _i < _len; _i++) {
      outerVar = _ref[_i];
      _results.push(0);
    }
    return _results;
  };
});
@jashkenas
Owner

Yes -- that seems wrong. We shouldn't be shadowing there ... why are we?

@jashkenas jashkenas closed this in afdcdcf Apr 24, 2012
@jashkenas jashkenas added a commit that referenced this issue Apr 24, 2012
@jashkenas Adding a test for #2273 e433098
@brettkiefer

This change just broke some of my code, and I was surprised by it. I think the reason to do it the old way (pre- afdcdcf) is that it is natural to think of a list comprehension as a shortcut for defining an iterator function and calling it on a list of values, which makes what you're calling the loop variable names actually the names of the PARAMETERS to the iterator.

I realize that's not the implementation, but it seems like a more natural mental model for what a list comprehension actually is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment