Skip to content

Commit

Permalink
Avoid free variable conflicts within descendant scopes instead of global
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
edemaine committed Jan 25, 2022
1 parent 6fd58ef commit fd0d180
Show file tree
Hide file tree
Showing 8 changed files with 62 additions and 50 deletions.
14 changes: 1 addition & 13 deletions lib/coffeescript/coffeescript.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 33 additions & 7 deletions lib/coffeescript/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 2 additions & 14 deletions lib/coffeescript/repl.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions lib/coffeescript/scope.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 1 addition & 6 deletions src/coffeescript.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -127,6 +121,7 @@ exports.compile = compile = withPrettyErrors (code, options = {}) ->
ast.tokens = tokens
return ast

nodes.findReferencedVars()
fragments = nodes.compileToFragments options

currentLine = 0
Expand Down
22 changes: 19 additions & 3 deletions src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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 []
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand Down
5 changes: 2 additions & 3 deletions src/repl.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/scope.litcoffee
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit fd0d180

Please sign in to comment.