vars in for loop declarations should be declared in upper-scope. #145

Open
graingert opened this Issue Jul 4, 2016 · 6 comments

Projects

None yet

2 participants

@graingert
graingert commented Jul 4, 2016 edited
function logTheLengthOfTheArrayForEachElementInTheArrayAsync() {
  var messages = [0, 1, 2];
  for (var i = 0; i < messages.length; i++) {
    function logMessage() {
      console.log(i)
    }
    setTimeout(logMessage)
  }
}

logTheLengthOfTheArrayForEachElementInTheArrayAsync();

prints:

3
3
3

but:

function logTheLengthOfTheArrayForEachElementInTheArrayAsync() {
  const messages = [0, 1, 2];
  for (let i = 0; i < messages.length; i++) {
    function logMessage() {
      console.log(i)
    }
    setTimeout(logMessage)
  }
}

logTheLengthOfTheArrayForEachElementInTheArrayAsync();

prints:
0
1
2

@graingert
graingert commented Jul 4, 2016 edited

Vars should be hoisted, then converted to let

function logTheLengthOfTheArrayForEachElementInTheArrayAsync() {
  const messages = [0, 1, 2];
  let i;
  for (i = 0; i < messages.length; i++) {
    function logMessage() {
      console.log(i)
    }
    setTimeout(logMessage)
  }
}

logTheLengthOfTheArrayForEachElementInTheArrayAsync();
@nene
Collaborator
nene commented Jul 7, 2016

Thanks for reporting.

The problem is that it's no way to tell whether the function logMessage is invoked synchronously or asynchronously. For example the following code works fine with either var or let because forEach happens to be synchronous:

for (let i=0; i<10; i++) {
  messages.forEach(function(msg){ console.log(msg + i); });
}

So to solve it we'll have to detect it in a more generic fashion:

  • Does a loop create a closure over variable defined with let?

In which case we'd leave the variable as var - currently Lebab does not perform any moving-around of variable declarations (implementing the latter would be a larger feature of its own).

@graingert Could you come up with a more real-life examples? The current one looks more like the code was buggy to start with.

@graingert

Rather than leaving the variable as var you could just manually move it to
the hoisted position and convert it into a let.
On 7 Jul 2016 13:43, "Rene Saarsoo" notifications@github.com wrote:

Thanks for reporting.

The problem is that it's no way to tell whether the function logMessage is
invoked synchronously or asynchronously. For example the following code
works fine with either var or let because forEach happens to be
synchronous:

for (let i=0; i<10; i++) {
messages.forEach(function(msg){ console.log(msg + i); });
}

So to solve it we'll have to detect it in a more generic fashion:

  • Does a loop create a closure over variable defined with let?

In which case we'd leave the variable as var - currently Lebab does not
perform any moving-around of variable declarations (implementing the latter
would be a larger feature of its own).

@graingert https://github.com/graingert Could you come up with a more
real-life examples? The current one looks more like the code was buggy to
start with.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#145 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAZQTO2KDdd2vrOJ3CAvinXFuCRc4qrtks5qTPR4gaJpZM4JESyv
.

@nene
Collaborator
nene commented Jul 7, 2016

As I said, currently the let transform does not perform any such moving-around of declarations. There are several other scenarios where such moving would be needed, but at the moment these are left as var.

That would be a feature to be implemented separately from fixing this bug, but I'm not really sure it's a good thing to do. This kind of code is probably better to be manually rewritten - with the help of warnings #136 it should soon become simpler to do so.

@nene nene added the bug label Jul 7, 2016
@graingert

Instead of moving it you could wrap the for loop in a new scope.

{
  let i;
  for (i in ...){}
}

On 7 Jul 2016 14:06, "Rene Saarsoo" notifications@github.com wrote:

As I said, currently the let transform does not perform any such
moving-around of declarations. There are several other scenarios where such
moving would be needed, but at the moment these are left as var.

That would be a feature to be implemented separately from fixing this bug,
but I'm not really sure it's a good thing to do. This kind of code is
probably better to be manually rewritten - with the help of warnings #136
#136 it should soon become
simpler to do so.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#145 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAZQTD1ByttH3sHhUA_VF661a3nizPGgks5qTPnGgaJpZM4JESyv
.

@nene
Collaborator
nene commented Jul 7, 2016

Introducing a plain block would be an even stranger code. And I would still consider it as "moving" the declaration. I would rather declare the variable in original parent block.

But the main priority currently is to not do a transform that could introduce a bug. Properly transforming all the edge cases is not the main priority.

@nene nene added a commit that referenced this issue Aug 2, 2016
@nene nene Reference loop variable issue from README
Refs #145
7f5ab3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment