Push var down to first variable use when possible

[edemaine]

trafficstars

This is a portion of my typescript branch that I spent the last few days working on, implementing the plan laid out in https://github.com/jashkenas/coffeescript/issues/5377#issuecomment-1019537087 : push the var declaration of a variable down to its first clean assignment if there is one. The result is generally cleaner code (many fewer variables declared in top var block), and much better automatic types when the code is presented to TypeScript. It's a step toward #5377.

Examples of clean assignments:

• x = 7 → var x = 7
• [x, y] = array → var [x, y] = array
• {x, y} = point → var {x, y} = point
• [{x, y}, {a, b}] = [point, pair] → var [{x, y}, {a, b}] = [point, point2];

Unexamples of unclean assignments (so var won't try to be inserted here; if this is the only assignment, var will remain at top):

• [{x, y}, {x: x2, y: y2}] = [point, point2]
• {x = 5} = obj
• x += 5
• if match = re.exec string
• console.log x = 5

Note for reviewer: "clean" is referred to as isDeclarable in the code.

Only the first assignment to a variable is modified. The only exception is if there is a joint assignment that has some undeclared variables in addition to some already declared variables. Some examples:

CoffeeScript	JavaScript output from this PR
x = 5 x = 10	var x = 5; x = 10;
x = 5 [x, y] = array	var x = 5; var [x, y] = array;

While the duplicate var isn't pretty, I think it's rare, and it's allowed. eslint would complain about the output, but eslint complains about all sorts of things in CoffeeScript's output; it's better to run eslint on the CS AST.

There's also special handling of for loops and ranges. (This took the longest to get right.) Examples: (comments added for explanation)

CoffeeScript	JavaScript output from this PR
for x from it console.log x	for (var x of it) { console.log(x); }
for x, y of object console.log x, y	for (var x in object) { var y = object[x]; console.log(x, y); }
for x in [1..100] console.log x	for (var i = 1, x = i; i <= 100; x = ++i) { console.log(x); }
for x in array console.log x	for (var i = 0, len = array.length; i < len; i++) { var x = array[i]; console.log(x); }
for x, i in array console.log x	for (var j = 0, i = j, len = array.length; j < len; i = ++j) { var x = array[i]; console.log(x); }
squares = for x in array x ** 2	var x; // needs to stay here so visible outside loop var squares = (function() { var results = []; for (var i = 0, len = array.length; i < len; i++) { x = array[i]; results.push(x ** 2); } return results; })();
powers = for x, i in array x ** i	var i, x; // needs to stay here so visible outside loop var powers = (function() { var j, len; // can't declare in for loop or would shadow i var results = []; for (j = 0, i = j, len = array.length; j < len; i = ++j) { x = array[i]; results.push(x ** i); } return results; })();

As you can see in the more complicated examples, there are some tricky issues where it's crucial to avoid shadowing by excess var. I think I got them all right; it was very hard to pass all the tests. But it's possible that more tests should be done/added.

This PR also refactors Scope to ensure all variables have positions mapping, so we can do faster searching for variables, and a new get method for retrieving various data about a variable. This makes it easier to keep track of whether a variable has already had a var declaration during some assignment.

[edemaine]

There are some browser tests failing, caused by this bug in babel-plugin-minify-dead-code. I guess var is tricky to implement after all. 😦

[STRd6]

I'm also excited about this, anything I can do to help?

[phil294]

I noticed the following translation, not mentioned above or in the test cases:

[ x = '', y = '' ] = []
x = x.slice(0)

becomes

var y;

[x = '', y = ''] = [];

var x = x.slice(0);

is this a bug or intended? If I didn't read you wrong, this should have been var x, y instead.

[edemaine]

@phil294 I can see why that would happen. The code must be interpreting var [x = '', y = ''] = []; as an invalid declaration, so it doesn't do var there, leaving x and y needing declaration elsewhere. x = x.splice(0) is a valid place to put the x declaration and so it puts it there. y doesn't have an assignment so it gets hoisted. This is all valid thanks to automatic var hoisting.

However, var [x = '', y = ''] = []; is a valid declaration, so I should tweak the test to just put the var there. Some assignments are not valid places to put var though (for example, if x = 5); in those cases, the next assignment to x will be where the var ends up. We could alternatively decide that, if the first assignment isn't a valid place for var, then the var always gets hoisted to the top -- but that's not how this branch works currently.

[phil294]

@edemaine Thanks for the clarification!

---

As you know, this PR branch has been in use in CoffeeSense for several months now, and indeed successfully so.

Today however, I have found an actual bug for the first time (I think). It does not happen with the normal exports, but only with the browser compiler version. Here's reproduction code, which I ran with Deno because Node/modules gave some cryptic errors which I couldn't be bothered with.

import { compile } from "./node_modules/coffeescript/lib/coffeescript-browser-compiler-modern/coffeescript.js"
let coffee = 'a = 1\n[1..a]'
let response = compile(coffee, { sourceMap: true, bare: true })
console.log(response.js)

output:

var a = 1;

(function() {
  var results = [];
  for (var i = 1; 1 <= a ? i <= a : i >= a; 1 <= a ? i++ : i--){ results.push(i); }
  return results;
}).apply(thisundefined);

note the thisundefined at the end.

Please, there is no need to investigate, this is a rare case with low priority for me or any user, I think. I just thought it might be helpful for developing this branch, should you ever get back to it - maybe it's also already covered above.

Thank you again for your efforts.