Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

possible to shadow _arg in parameter list #1574

Closed
MichaelBlume opened this Issue Aug 4, 2011 · 11 comments

Comments

Projects
None yet
8 participants
Contributor

MichaelBlume commented Aug 4, 2011

func = ({alpha:a,beta:b}, _arg) ->
  alert a

func {alpha: 3, beta: 5}, 20

should alert 3, alerts undefined.

Contributor

MichaelBlume commented Aug 4, 2011

compiles to:

(function() {
  var func;
  func = function(_arg, _arg) {
    var a, b;
    a = _arg.alpha, b = _arg.beta;
    return console.log(a);
  };
  func({
    alpha: 3,
    beta: 5
  }, 20);
}).call(this);
Contributor

geraldalewis commented Aug 4, 2011

Same is true for @-params that are reserved keywords:

(@case,_case)->

...compiles to

(function(_case, _case) {
  this["case"] = _case;
});

Fix in #1543 -- #1002 identical params (revised) . That pull was closed since prohibiting duplicate param names depends on CoffeeScript's adoption of use strict. However, I agree that compiler-generated param names should never clash with user-defined.

Fixing
User-defined param names need to be added to scope before param names are generated for complex parameters, so the formal parameter list needs to be processed twice. Also, in the case of reserved @-params, Scope#freeVariable should be used to generate the param name, not simply prepending an underscore.

Owner

jashkenas commented Aug 5, 2011

This is strictly speaking not exactly a bug ... we've been using _ prefixed names for temporary variables to avoid name clashes ... we're never going to be able to avoid all name clashes in the presence of a with or eval. But that said, there's been some growing support for doing two-pass variable naming ... which means that we'd be able to drop the _ prefix altogether, and be reasonably sure that names are unique.

Collaborator

michaelficarra commented Aug 5, 2011

+1 for two-pass variable naming (or some other approach that accomplishes the same thing)

+1, and here's the example that led me here:

t = (value for value in this when not (value in _results))

allows reliance on _results (or, similarly, _i and the like), which is a bad idea, and thus shouldn't be allowed to happen.

almost commented Jul 25, 2012

How about making the internal variables used reserved in some way? You could issue a warning or an error when they're used. It's the unexpected behaviour that's the problem, much more so than not being able to use certain words as variable names.

Collaborator

michaelficarra commented Jul 25, 2012

@almost: Ideally, you should be able to use any variable name you wish, as long as it doesn't conflict with a CS keyword. We already do something like you suggest, though: https://github.com/jashkenas/coffee-script/blob/5d7a83468abce3abe5ac236342201dd3ce6653a7/src/lexer.coffee#L580-581

almost commented Jul 25, 2012

I agree that ideally you should be able to. But right now you can't, and it's not obvious that you can't. I think it would be a good idea to add the extra special variables that CS uses into that list of reserved words. I guess it would break code that uses those variable names in contexts where they are allowed, but code like that is already skating on thin ice.

Collaborator

michaelficarra commented Jul 25, 2012

@almost: submit a pull request.

Just started. What led me here was this:

foo = (_results for _results in [1..2]) #OK
(_this) => @ #NOT OK
// generated
var foo, _results,
  _this = this;

foo = (function() {
  var _i, _results1;
  _results1 = [];
  for (_results = _i = 1; _i <= 2; _results = ++_i) {
    _results1.push(_results);
  }
  return _results1;
})();

(function(_this) {
  return _this;
});

Try Coffeescript link

What concerned me more was the inconsistency (was _this overlooked?). If there's a certain group of keywords to avoid, that's fine. But it looks like sometimes there's a safety net and sometimes not.

lydell added a commit to lydell/coffee-script that referenced this issue Jan 10, 2015

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.

lydell added a commit to lydell/coffee-script that referenced this issue Jan 10, 2015

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.
Collaborator

lydell commented Jan 13, 2015

Fixed by #3784.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment