Fix #1500, #1574, #3318: Name generated vars uniquely

Any variables generated by CoffeeScript are now made sure to be named to
something not present in the source code being compiled. This way you can no
longer interfere with them, either on purpose or by mistake. (#1500, #1574)

For example, `({a}, _arg) ->` now compiles correctly. (#1574)

As opposed to the somewhat complex implementations discussed in #1500, this
commit takes a very simple approach by saving all used variables names using a
single pass over the token stream. Any generated variables are then made sure
not to exist in that list.

`(@a) -> a` used to be equivalent to `(@a) -> @a`, but now throws a runtime
`ReferenceError` instead (unless `a` exists in an upper scope of course). (#3318)

`(@a) ->` used to compile to `(function(a) { this.a = a; })`. Now it compiles to
`(function(_at_a) { this.a = _at_a; })`. (But you cannot access `_at_a` either,
of course.)

Because of the above, `(@a, a) ->` is now valid; `@a` and `a` are not duplicate
parameters.

Duplicate this-parameters with a reserved word, such as `(@case, @case) ->`,
used to compile but now throws, just like regular duplicate parameters.
This commit is contained in:
Simon Lydell 2015-01-10 23:04:30 +01:00
parent 23a691ae87
commit 8ab15d7372
15 changed files with 202 additions and 119 deletions

View File

@ -40,13 +40,25 @@
};
exports.compile = compile = withPrettyErrors(function(code, options) {
var answer, currentColumn, currentLine, extend, fragment, fragments, header, js, map, merge, newLines, _i, _len;
var answer, currentColumn, currentLine, extend, fragment, fragments, header, js, map, merge, newLines, token, tokens, _i, _len;
merge = helpers.merge, extend = helpers.extend;
options = extend({}, options);
if (options.sourceMap) {
map = new SourceMap;
}
fragments = parser.parse(lexer.tokenize(code, options)).compileToFragments(options);
tokens = lexer.tokenize(code, options);
options.referencedVars = (function() {
var _i, _len, _results;
_results = [];
for (_i = 0, _len = tokens.length; _i < _len; _i++) {
token = tokens[_i];
if (token.variable && token[1].charAt(0) === '_') {
_results.push(token[1]);
}
}
return _results;
})();
fragments = parser.parse(tokens).compileToFragments(options);
currentLine = 0;
if (options.header) {
currentLine += 1;
@ -236,8 +248,8 @@
}
return tag;
},
setInput: function(tokens) {
this.tokens = tokens;
setInput: function(_at_tokens) {
this.tokens = _at_tokens;
return this.pos = 0;
},
upcomingInput: function() {

View File

@ -139,6 +139,7 @@
})();
}
tagToken = this.token(tag, id, 0, idLength);
tagToken.variable = !forcedIdentifier;
if (poppedToken) {
_ref4 = [poppedToken[2].first_line, poppedToken[2].first_column], tagToken[2].first_line = _ref4[0], tagToken[2].first_column = _ref4[1];
}

View File

@ -417,7 +417,7 @@
o.indent = o.bare ? '' : TAB;
o.level = LEVEL_TOP;
this.spaced = true;
o.scope = new Scope(null, this, null);
o.scope = new Scope(null, this, null, o.referencedVars);
_ref2 = o.locals || [];
for (_i = 0, _len = _ref2.length; _i < _len; _i++) {
name = _ref2[_i];
@ -517,8 +517,8 @@
exports.Literal = Literal = (function(_super) {
__extends(Literal, _super);
function Literal(value) {
this.value = value;
function Literal(_at_value) {
this.value = _at_value;
}
Literal.prototype.makeReturn = function() {
@ -617,8 +617,8 @@
return [this.makeCode(this.val)];
};
function Bool(val) {
this.val = val;
function Bool(_at_val) {
this.val = _at_val;
}
return Bool;
@ -628,8 +628,8 @@
exports.Return = Return = (function(_super) {
__extends(Return, _super);
function Return(expression) {
this.expression = expression;
function Return(_at_expression) {
this.expression = _at_expression;
}
Return.prototype.children = ['expression'];
@ -850,8 +850,8 @@
exports.Comment = Comment = (function(_super) {
__extends(Comment, _super);
function Comment(comment) {
this.comment = comment;
function Comment(_at_comment) {
this.comment = _at_comment;
}
Comment.prototype.isStatement = YES;
@ -875,9 +875,9 @@
exports.Call = Call = (function(_super) {
__extends(Call, _super);
function Call(variable, args, soak) {
this.args = args != null ? args : [];
this.soak = soak;
function Call(variable, _at_args, _at_soak) {
this.args = _at_args != null ? _at_args : [];
this.soak = _at_soak;
this.isNew = false;
this.isSuper = variable === 'super';
this.variable = this.isSuper ? null : variable;
@ -1046,9 +1046,9 @@
exports.Extends = Extends = (function(_super) {
__extends(Extends, _super);
function Extends(child, parent) {
this.child = child;
this.parent = parent;
function Extends(_at_child, _at_parent) {
this.child = _at_child;
this.parent = _at_parent;
}
Extends.prototype.children = ['child', 'parent'];
@ -1064,8 +1064,8 @@
exports.Access = Access = (function(_super) {
__extends(Access, _super);
function Access(name, tag) {
this.name = name;
function Access(_at_name, tag) {
this.name = _at_name;
this.name.asKey = true;
this.soak = tag === 'soak';
}
@ -1093,8 +1093,8 @@
exports.Index = Index = (function(_super) {
__extends(Index, _super);
function Index(index) {
this.index = index;
function Index(_at_index) {
this.index = _at_index;
}
Index.prototype.children = ['index'];
@ -1116,9 +1116,9 @@
Range.prototype.children = ['from', 'to'];
function Range(from, to, tag) {
this.from = from;
this.to = to;
function Range(_at_from, _at_to, tag) {
this.from = _at_from;
this.to = _at_to;
this.exclusive = tag === 'exclusive';
this.equals = this.exclusive ? '' : '=';
}
@ -1214,8 +1214,8 @@
Slice.prototype.children = ['range'];
function Slice(range) {
this.range = range;
function Slice(_at_range) {
this.range = _at_range;
Slice.__super__.constructor.call(this);
}
@ -1240,8 +1240,8 @@
exports.Obj = Obj = (function(_super) {
__extends(Obj, _super);
function Obj(props, generated) {
this.generated = generated != null ? generated : false;
function Obj(props, _at_generated) {
this.generated = _at_generated != null ? _at_generated : false;
this.objects = this.properties = props || [];
}
@ -1379,10 +1379,10 @@
exports.Class = Class = (function(_super) {
__extends(Class, _super);
function Class(variable, parent, body) {
this.variable = variable;
this.parent = parent;
this.body = body != null ? body : new Block;
function Class(_at_variable, _at_parent, _at_body) {
this.variable = _at_variable;
this.parent = _at_parent;
this.body = _at_body != null ? _at_body : new Block;
this.boundFuncs = [];
this.body.classBody = true;
}
@ -1566,11 +1566,11 @@
exports.Assign = Assign = (function(_super) {
__extends(Assign, _super);
function Assign(variable, value, context, options) {
function Assign(_at_variable, _at_value, _at_context, options) {
var forbidden, name, _ref2;
this.variable = variable;
this.value = value;
this.context = context;
this.variable = _at_variable;
this.value = _at_value;
this.context = _at_context;
this.param = options && options.param;
this.subpattern = options && options.subpattern;
forbidden = (_ref2 = (name = this.variable.unwrapAll().value), __indexOf.call(STRICT_PROSCRIBED, _ref2) >= 0);
@ -1875,14 +1875,10 @@
_ref5 = this.params;
for (_k = 0, _len2 = _ref5.length; _k < _len2; _k++) {
p = _ref5[_k].name;
if (!(!(param instanceof Expansion))) {
continue;
}
if (p["this"]) {
p = p.properties[0].name;
}
if (p.value) {
o.scope.add(p.value, 'var', true);
if (!(param instanceof Expansion)) {
if (p.value) {
o.scope.add(p.value, 'var', true);
}
}
}
splats = new Assign(new Value(new Arr((function() {
@ -1935,7 +1931,7 @@
uniqs = [];
this.eachParamName(function(name, node) {
if (__indexOf.call(uniqs, name) >= 0) {
node.error("multiple parameters named '" + name + "'");
node.error("multiple parameters named " + name);
}
return uniqs.push(name);
});
@ -1997,11 +1993,11 @@
exports.Param = Param = (function(_super) {
__extends(Param, _super);
function Param(name, value, splat) {
var _ref2;
this.name = name;
this.value = value;
this.splat = splat;
function Param(_at_name, _at_value, _at_splat) {
var name, _ref2;
this.name = _at_name;
this.value = _at_value;
this.splat = _at_splat;
if (_ref2 = (name = this.name.unwrapAll().value), __indexOf.call(STRICT_PROSCRIBED, _ref2) >= 0) {
this.name.error("parameter name \"" + name + "\" is not allowed");
}
@ -2014,16 +2010,14 @@
};
Param.prototype.asReference = function(o) {
var node;
var name, node;
if (this.reference) {
return this.reference;
}
node = this.name;
if (node["this"]) {
node = node.properties[0].name;
if (node.value.reserved) {
node = new Literal(o.scope.freeVariable(node.value));
}
name = "at_" + node.properties[0].name.value;
node = new Literal(o.scope.freeVariable(name));
} else if (node.isComplex()) {
node = new Literal(o.scope.freeVariable('arg'));
}
@ -2045,11 +2039,7 @@
name = this.name;
}
atParam = function(obj) {
var node;
node = obj.properties[0].name;
if (!node.value.reserved) {
return iterator(node.value, node);
}
return iterator("@" + obj.properties[0].name.value, obj);
};
if (name instanceof Literal) {
return iterator(name.value, name);
@ -2199,8 +2189,8 @@
}
};
While.prototype.addBody = function(body) {
this.body = body;
While.prototype.addBody = function(_at_body) {
this.body = _at_body;
return this;
};
@ -2514,9 +2504,9 @@
exports.In = In = (function(_super) {
__extends(In, _super);
function In(object, array) {
this.object = object;
this.array = array;
function In(_at_object, _at_array) {
this.object = _at_object;
this.array = _at_array;
}
In.prototype.children = ['object', 'array'];
@ -2588,11 +2578,11 @@
exports.Try = Try = (function(_super) {
__extends(Try, _super);
function Try(attempt, errorVariable, recovery, ensure) {
this.attempt = attempt;
this.errorVariable = errorVariable;
this.recovery = recovery;
this.ensure = ensure;
function Try(_at_attempt, _at_errorVariable, _at_recovery, _at_ensure) {
this.attempt = _at_attempt;
this.errorVariable = _at_errorVariable;
this.recovery = _at_recovery;
this.ensure = _at_ensure;
}
Try.prototype.children = ['attempt', 'recovery', 'ensure'];
@ -2630,8 +2620,8 @@
exports.Throw = Throw = (function(_super) {
__extends(Throw, _super);
function Throw(expression) {
this.expression = expression;
function Throw(_at_expression) {
this.expression = _at_expression;
}
Throw.prototype.children = ['expression'];
@ -2653,8 +2643,8 @@
exports.Existence = Existence = (function(_super) {
__extends(Existence, _super);
function Existence(expression) {
this.expression = expression;
function Existence(_at_expression) {
this.expression = _at_expression;
}
Existence.prototype.children = ['expression'];
@ -2681,8 +2671,8 @@
exports.Parens = Parens = (function(_super) {
__extends(Parens, _super);
function Parens(body) {
this.body = body;
function Parens(_at_body) {
this.body = _at_body;
}
Parens.prototype.children = ['body'];
@ -2894,10 +2884,10 @@
exports.Switch = Switch = (function(_super) {
__extends(Switch, _super);
function Switch(subject, cases, otherwise) {
this.subject = subject;
this.cases = cases;
this.otherwise = otherwise;
function Switch(_at_subject, _at_cases, _at_otherwise) {
this.subject = _at_subject;
this.cases = _at_cases;
this.otherwise = _at_otherwise;
}
Switch.prototype.children = ['subject', 'cases', 'otherwise'];
@ -2979,8 +2969,8 @@
exports.If = If = (function(_super) {
__extends(If, _super);
function If(condition, body, options) {
this.body = body;
function If(condition, _at_body, options) {
this.body = _at_body;
if (options == null) {
options = {};
}

View File

@ -5,8 +5,8 @@
repeat = require('./helpers').repeat;
exports.OptionParser = OptionParser = (function() {
function OptionParser(rules, banner) {
this.banner = banner;
function OptionParser(rules, _at_banner) {
this.banner = _at_banner;
this.rules = buildRules(rules);
}

View File

@ -17,8 +17,8 @@
exports.Rewriter = (function() {
function Rewriter() {}
Rewriter.prototype.rewrite = function(tokens) {
this.tokens = tokens;
Rewriter.prototype.rewrite = function(_at_tokens) {
this.tokens = _at_tokens;
this.removeLeadingNewlines();
this.closeOpenCalls();
this.closeOpenIndexes();

View File

@ -1,16 +1,18 @@
// Generated by CoffeeScript 1.8.0
(function() {
var Scope, extend, last, _ref;
var Scope, extend, last, _ref,
__indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; };
_ref = require('./helpers'), extend = _ref.extend, last = _ref.last;
exports.Scope = Scope = (function() {
Scope.root = null;
function Scope(parent, expressions, method) {
this.parent = parent;
this.expressions = expressions;
this.method = method;
function Scope(_at_parent, _at_expressions, _at_method, _at_referencedVars) {
this.parent = _at_parent;
this.expressions = _at_expressions;
this.method = _at_method;
this.referencedVars = _at_referencedVars;
this.variables = [
{
name: 'arguments',
@ -91,7 +93,11 @@
reserve = true;
}
index = 0;
while (this.check((temp = this.temporary(name, index)))) {
while (true) {
temp = this.temporary(name, index);
if (!(this.check(temp) || __indexOf.call(Scope.root.referencedVars, temp) >= 0)) {
break;
}
index++;
}
if (reserve) {

View File

@ -3,8 +3,8 @@
var LineMap, SourceMap;
LineMap = (function() {
function LineMap(line) {
this.line = line;
function LineMap(_at_line) {
this.line = _at_line;
this.columns = [];
}

View File

@ -44,7 +44,17 @@ exports.compile = compile = withPrettyErrors (code, options) ->
if options.sourceMap
map = new SourceMap
fragments = parser.parse(lexer.tokenize code, options).compileToFragments options
tokens = lexer.tokenize code, options
# Pass a list of referenced variables, so that generated variables won't get
# the same name. Since all generated variables start with an underscore only
# referenced variables also starting with an underscore are passed, as an
# optimization.
options.referencedVars = (
token[1] for token in tokens when token.variable and token[1].charAt(0) is '_'
)
fragments = parser.parse(tokens).compileToFragments options
currentLine = 0
currentLine += 1 if options.header

View File

@ -156,6 +156,7 @@ exports.Lexer = class Lexer
else tag
tagToken = @token tag, id, 0, idLength
tagToken.variable = not forcedIdentifier
if poppedToken
[tagToken[2].first_line, tagToken[2].first_column] =
[poppedToken[2].first_line, poppedToken[2].first_column]

View File

@ -320,7 +320,7 @@ exports.Block = class Block extends Base
o.indent = if o.bare then '' else TAB
o.level = LEVEL_TOP
@spaced = yes
o.scope = new Scope null, this, null
o.scope = new Scope null, this, null, o.referencedVars
# Mark given local variables in the root scope as parameters so they don't
# end up being declared on this block.
o.scope.parameter name for name in o.locals or []
@ -1361,7 +1361,6 @@ exports.Code = class Code extends Base
o.scope.parameter param.asReference o
for param in @params when param.splat or param instanceof Expansion
for {name: p} in @params when param not instanceof Expansion
if p.this then p = p.properties[0].name
if p.value then o.scope.add p.value, 'var', yes
splats = new Assign new Value(new Arr(p.asReference o for p in @params)),
new Value new Literal 'arguments'
@ -1386,7 +1385,7 @@ exports.Code = class Code extends Base
o.scope.parameter fragmentsToText params[i]
uniqs = []
@eachParamName (name, node) ->
node.error "multiple parameters named '#{name}'" if name in uniqs
node.error "multiple parameters named #{name}" if name in uniqs
uniqs.push name
@body.makeReturn() unless wasEmpty or @noReturn
code = 'function'
@ -1431,9 +1430,8 @@ exports.Param = class Param extends Base
return @reference if @reference
node = @name
if node.this
node = node.properties[0].name
if node.value.reserved
node = new Literal o.scope.freeVariable node.value
name = "at_#{node.properties[0].name.value}"
node = new Literal o.scope.freeVariable name
else if node.isComplex()
node = new Literal o.scope.freeVariable 'arg'
node = new Value node
@ -1451,9 +1449,7 @@ exports.Param = class Param extends Base
# `name` is the name of the parameter and `node` is the AST node corresponding
# to that name.
eachName: (iterator, name = @name)->
atParam = (obj) ->
node = obj.properties[0].name
iterator node.value, node unless node.value.reserved
atParam = (obj) -> iterator "@#{obj.properties[0].name.value}", obj
# * simple literals `foo`
return iterator name.value, name if name instanceof Literal
# * at-params `@foo`

View File

@ -17,10 +17,11 @@ The `root` is the top-level **Scope** object for a given file.
Initialize a scope with its parent, for lookups up the chain,
as well as a reference to the **Block** node it belongs to, which is
where it should declare its variables, and a reference to the function that
it belongs to.
where it should declare its variables, a reference to the function that
it belongs to, and a list of variables referenced in the source code
and therefore should be avoided when generating variables.
constructor: (@parent, @expressions, @method) ->
constructor: (@parent, @expressions, @method, @referencedVars) ->
@variables = [{name: 'arguments', type: 'arguments'}]
@positions = {}
Scope.root = this unless @parent
@ -84,7 +85,10 @@ compiler-generated variable. `_var`, `_var2`, and so on...
freeVariable: (name, reserve=true) ->
index = 0
index++ while @check((temp = @temporary name, index))
loop
temp = @temporary name, index
break unless @check(temp) or temp in Scope.root.referencedVars
index++
@add temp, 'var', yes if reserve
temp

View File

@ -6,6 +6,7 @@
# * Destructuring Assignment
# * Context Property (@) Assignment
# * Existential Assignment (?=)
# * Assignment to variables similar to generated variables
test "context property assignment (using @)", ->
nonce = {}
@ -405,3 +406,34 @@ test "#2181: conditional assignment as a subexpression", ->
false && a or= true
eq false, a
eq false, not a or= true
test "#1500: Assignment to variables similar to generated variables", ->
_len = 0
x = ((_results = null; i) for i in [1, 2, 3])
arrayEq [1, 2, 3], x
eq 0, _len
for x in [1, 2, 3]
f = ->
_i = 0
f()
eq 'undefined', typeof _i
_ref = 2
x = _ref * 2 ? 1
eq x, 4
eq 'undefined', typeof _ref1
x = {}
_base = -> x
_name = -1
_base()[-_name] ?= 2
eq x[1], 2
eq _base(), x
eq _name, -1
f = (@a, _at_a, a) -> [@a, _at_a, a]
arrayEq [1, 2, 3], f.call scope = {}, 1, 2, 3
eq 1, scope.a
doesNotThrow -> CoffeeScript.compile '(@_slice...) ->'

View File

@ -431,3 +431,19 @@ test "unclosed regexes", ->
a #{""" ""#{if /[/].test "|" then 1 else 0}"" """}
^
'''
test "duplicate function arguments", ->
assertErrorFormat '''
(foo, bar, foo) ->
''', '''
[stdin]:1:12: error: multiple parameters named foo
(foo, bar, foo) ->
^^^
'''
assertErrorFormat '''
(@foo, bar, @foo) ->
''', '''
[stdin]:1:13: error: multiple parameters named @foo
(@foo, bar, @foo) ->
^^^^
'''

View File

@ -116,8 +116,13 @@ test "@-parameters: automatically assign an argument's value to a property of th
((@prop...) ->).call context = {}, 0, nonce, 0
eq nonce, context.prop[1]
# the argument should still be able to be referenced normally
eq nonce, (((@prop) -> prop).call {}, nonce)
# the argument should not be able to be referenced normally
code = '((@prop) -> prop).call {}'
doesNotThrow -> CoffeeScript.compile code
throws (-> CoffeeScript.run code), ReferenceError
code = '((@prop) -> _at_prop).call {}'
doesNotThrow -> CoffeeScript.compile code
throws (-> CoffeeScript.run code), ReferenceError
test "@-parameters and splats with constructors", ->
a = {}
@ -213,6 +218,16 @@ test "reserved keyword as parameters", ->
eq 2, b
eq 3, c
test "reserved keyword at-splat", ->
f = (@case...) -> @case
[a, b] = f(1, 2)
eq 1, a
eq 2, b
test "#1574: Destructuring and a parameter named _arg", ->
f = ({a, b}, _arg, _arg1) -> [a, b, _arg, _arg1]
arrayEq [1, 2, 3, 4], f a: 1, b: 2, 3, 4
test "#1844: bound functions in nested comprehensions causing empty var statements", ->
a = ((=>) for a in [0] for b in [0])
eq 1, a.length

View File

@ -72,19 +72,14 @@ test "duplicate formal parameters are prohibited", ->
# a Param can also be a splat (...) or an assignment (param=value)
# the following function expressions should throw errors
strict '(_,_)->', 'param, param'
strict '(_,@_)->', 'param, @param'
strict '(_,_...)->', 'param, param...'
strict '(@_,_...)->', '@param, param...'
strict '(_,_ = true)->', 'param, param='
strict '(@_,@_)->', 'two @params'
strict '(_,@_ = true)->', 'param, @param='
strict '(@case,@case)->', 'two @reserved'
strict '(_,{_})->', 'param, {param}'
strict '(@_,{_})->', '@param, {param}'
strict '({_,_})->', '{param, param}'
strict '({_,@_})->', '{param, @param}'
strict '(_,[_])->', 'param, [param]'
strict '([_,_])->', '[param, param]'
strict '([_,@_])->', '[param, @param]'
strict '(_,[_]=true)->', 'param, [param]='
strict '(_,[@_,{_}])->', 'param, [@param, {param}]'
strict '(_,[_,{@_}])->', 'param, [param, {@param}]'
@ -95,6 +90,12 @@ test "duplicate formal parameters are prohibited", ->
strict '(0:a,1:a)->', '0:param,1:param'
strict '({0:a,1:a})->', '{0:param,1:param}'
# the following function expressions should **not** throw errors
strictOk '(_,@_)->'
strictOk '(@_,_...)->'
strictOk '(_,@_ = true)->'
strictOk '(@_,{_})->'
strictOk '({_,@_})->'
strictOk '([_,@_])->'
strictOk '({},_arg)->'
strictOk '({},{})->'
strictOk '([]...,_arg)->'
@ -113,7 +114,6 @@ test "`delete` operand restrictions", ->
strict 'a = 1; delete a'
strictOk 'delete a' #noop
strict '(a) -> delete a'
strict '(@a) -> delete a'
strict '(a...) -> delete a'
strict '(a = 1) -> delete a'
strict '([a]) -> delete a'