Avoid free variable conflicts within descendant scopes instead of global

Instead of a global `referencedVars` option that is computed at the
lexer level (via all IDENTIFIER tokens), recurse through the AST to mark
each scope with all variables used within it and descendant scopes.

Likely also more efficient via `Set`s instead of `Array`s.

Also fix some missing `children` in `For` nodes, which otherwise caused
this rewrite to fail.

Fixes jashkenas#4865 by causing parameter renaming in fewer (more predictable)
scenarios.

Author: edemaine

lib/coffeescript/coffeescript.js

@@ -93,12 +93,6 @@ exports.compile = compile = withPrettyErrors (code, options = {}) ->
 
   tokens = lexer.tokenize code, options
 
-  # Pass a list of referenced variables, so that generated variables won’t get
-  # the same name.
-  options.referencedVars = (
-    token[1] for token in tokens when token[0] is 'IDENTIFIER'
-  )
-
   # Check for import or export; if found, force bare mode.
   unless options.bare? and options.bare is yes
     for token in tokens
@@ -127,6 +121,7 @@ exports.compile = compile = withPrettyErrors (code, options = {}) ->
     ast.tokens = tokens
     return ast
 
+  nodes.findReferencedVars()
   fragments = nodes.compileToFragments options
 
   currentLine = 0

lib/coffeescript/nodes.js

@@ -361,6 +361,10 @@ exports.Base = class Base
       else
         return true if children.replaceInContext match, replacement
 
+  findReferencedVars: (scopes) ->
+    @eachChild (child) ->
+      child.findReferencedVars scopes
+
   invert: ->
     new Op '!', this
 
@@ -515,9 +519,13 @@ exports.Root = class Root extends Base
     super()
 
     @isAsync = (new Code [], @body).isAsync
+    @referencedVars = new Set
 
   children: ['body']
 
+  findReferencedVars: (scopes = []) ->
+    super scopes.concat @
+
   # Wrap everything in a safety closure, unless requested not to. It would be
   # better not to generate them in the first place, but for now, clean up
   # obvious double-parentheses.
@@ -532,7 +540,7 @@ exports.Root = class Root extends Base
     [].concat @makeCode("(#{functionKeyword}() {\n"), fragments, @makeCode("\n}).call(this);\n")
 
   initializeScope: (o) ->
-    o.scope = new Scope null, @body, null, o.referencedVars ? []
+    o.scope = new Scope null, @body, null, @referencedVars
     # Mark given local variables in the root scope as parameters so they don’t
     # end up being declared on the root block.
     o.scope.parameter name for name in o.locals or []
@@ -1172,6 +1180,9 @@ exports.IdentifierLiteral = class IdentifierLiteral extends Literal
       name: @value
       declaration: !!@isDeclaration
 
+  findReferencedVars: (scopes) ->
+    scope.referencedVars.add @value for scope in scopes
+
 exports.PropertyName = class PropertyName extends Literal
   isAssignable: YES
 
@@ -3904,6 +3915,7 @@ exports.Code = class Code extends Base
     @isGenerator = no
     @isAsync     = no
     @isMethod    = no
+    @referencedVars = new Set
 
     @body.traverseChildren no, (node) =>
       if (node instanceof Op and node.isYield()) or node instanceof YieldReturn
@@ -3921,7 +3933,11 @@ exports.Code = class Code extends Base
 
   jumps: NO
 
-  makeScope: (parentScope) -> new Scope parentScope, @body, this
+  findReferencedVars: (scopes) ->
+    super scopes.concat @
+
+  makeScope: (parentScope) ->
+    new Scope parentScope, @body, this, @referencedVars
 
   # Compilation creates a new scope unless explicitly asked to share with the
   # outer scope. Handles splat parameters in the parameter list by setting
@@ -5342,7 +5358,7 @@ exports.For = class For extends While
     @addBody body
     @addSource source
 
-  children: ['body', 'source', 'guard', 'step']
+  children: ['body', 'name', 'index', 'source', 'guard', 'step']
 
   isAwait: -> @await ? no
 

lib/coffeescript/repl.js

@@ -38,8 +38,6 @@ replDefaults =
       if tokens.length >= 1 and tokens[tokens.length - 1].generated and
          tokens[tokens.length - 1].comments?.length isnt 0 and "#{tokens[tokens.length - 1][1]}" is ''
         tokens.pop()
-      # Collect referenced variable names just like in `CoffeeScript.compile`.
-      referencedVars = (token[1] for token in tokens when token[0] is 'IDENTIFIER')
       # Generate the AST of the tokens.
       ast = CoffeeScript.nodes(tokens).body
       # Add assignment to `__` variable to force the input to be an expression.
@@ -49,7 +47,8 @@ replDefaults =
       isAsync = ast.isAsync
       # Invoke the wrapping closure.
       ast    = new Root new Block [new Call ast]
-      js     = ast.compile {bare: yes, locals: Object.keys(context), referencedVars, sharedScope: yes}
+      ast.findReferencedVars()
+      js     = ast.compile {bare: yes, locals: Object.keys(context), sharedScope: yes}
       if transpile
         js = transpile.transpile(js, transpile.options).code
         # Strip `"use strict"`, to avoid an exception on assigning to

lib/coffeescript/scope.js

@@ -91,7 +91,8 @@ compiler-generated variable. `_var`, `_var2`, and so on...
         index = 0
         loop
           temp = @temporary name, index, options.single
-          break unless @check(temp) or temp in @root.referencedVars
+          break unless @check(temp) or @referencedVars.has(temp) or
+            @shared and @parent.referencedVars.has(temp)
           index++
         @add temp, 'var', yes if options.reserve ? true
         temp