Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reserved keywords as parameters maybe clobbered #3754

Closed
wants to merge 1 commit into from
Closed

Reserved keywords as parameters maybe clobbered #3754

wants to merge 1 commit into from

Conversation

josh
Copy link
Contributor

@josh josh commented Dec 7, 2014

I was looking at some of the strict tests added in 66eb186 (by @geraldalewis) and noticed some are generating invalid output. The test only checks if the code can be generated but not evaluated.

There was two different types of errors.

(@case, _case) ->

generates

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

So theres a bug in the variable renaming here.

The second is a little worse.

(@case, _case...) ->

generates

var __slice = [].slice;

(function() {
  var case, _case;
  _case = arguments[0], _case = 2 <= arguments.length ? __slice.call(arguments, 1) : [];
  this["case"] = _case;
});

An unused and reversed local is created for case.

I haven't looked into a fix yet, but I figured I'd open an patch with tests and maybe someone much more familiar will know what to do.

@michaelficarra
Copy link
Collaborator

This seems like just another duplicate of #1574 to me (which is basically a dupe of #1500). Needs two-pass variable naming. Also related: we have an open issue to disallow referencing @-params by the locals used in their compilation (e.g. (@a) -> a), but I can't find it right now.

@josh
Copy link
Contributor Author

josh commented Dec 8, 2014

Should I close this out in favor of the existing issues or leave open because it has tests?

@vendethiel
Copy link
Collaborator

I'd say leave it open because it has tests.

@jashkenas
Copy link
Owner

Should we merge the failing tests now? Or add to this PR first?

@michaelficarra
Copy link
Collaborator

Let's not merge failing tests. I've had problems with that in the past. Either the fix never comes and your build will fail forever or the tests won't actually be what you wanted (and you won't know this until you have an implementation).

@jashkenas
Copy link
Owner

Words of wisdom.

@vendethiel
Copy link
Collaborator

Well, these could be "negative tests" so we'd know when we fix these, or "TODO" (skipped tests) (e.g. which would not be run in usual run, but could be run with a special command)

@lydell
Copy link
Collaborator

lydell commented Dec 9, 2014

Mocha lets you mark tests as pending. It then reports X passed, Y failed and Z pending. FYI.

@lydell lydell mentioned this pull request Jan 10, 2015
@lydell
Copy link
Collaborator

lydell commented Jan 13, 2015

This was included in #3784.

@josh josh deleted the reserved-keyword-params branch February 26, 2015 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants