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

[CS2] Nodes, Scope: Assignment in dynamic method names #4425

Closed
connec opened this issue Jan 19, 2017 · 2 comments
Closed

[CS2] Nodes, Scope: Assignment in dynamic method names #4425

connec opened this issue Jan 19, 2017 · 2 comments
Labels
Milestone

Comments

@connec
Copy link
Collaborator

connec commented Jan 19, 2017

class A
  "#{name = 'hello'}": ->
new A

compiles to

var A;

A = class A {
  [`${(name = 'hello')}`]() {}

};
new A // ReferenceError: name is not defined
@connec connec changed the title [CS2 [CS2] Assignment in dynamic method names Jan 19, 2017
@connec
Copy link
Collaborator Author

connec commented Jan 20, 2017

There is a separate error case related to class scope, which is attempting to use anything assigned in an executable class body within a class dynamic method name, e.g.:

class A
  getName = -> 'm'
  "#{getName()}": ->

which currently compiles to

var A;

A = (function() {
  var getName;

  class A {
    // note that getName hasn't been assigned yet
    [`${getName()}`]() {}

  };

  getName = function() {
    return 'm';
  };

  return A;

})();

I discussed this at some length in the classes PRs, but couldn't settle on an appropriate solution. Options include:

  1. Leave it as is, let users wishing to leverage dynamic method names figure out the subtleties.
  2. Compile methods with dynamic names as prototype assignments. This gives a consistent scope and execution order, but would break super and obfuscate output.
  3. Attempt to detect invalid cases, or only allow clearly valid cases (e.g. "only contains references to identifiers in the parent scope, that aren't assigned in the executable body scope"). Not sure how far we could get with this before we'd need significant control flow analysis.
  4. Ban dynamic method names when used with anything that would trigger an executable class body. Users can opt in to the restrictions of 2. by rewriting the method as an explicit prototype assignment.

I think 4. sounds best. I reckon this combination of things should be fairly rare, and option 4. allows the pattern to be used if truly desired, whilst making the trade-off clearer than option 2.

Option 3. might be a good option, but I'm not sure what we'd need to check for. That the expression for a dynamic name contains no variables from the executable body scope? That should be possible with Node::contains.

@GeoffreyBooth GeoffreyBooth modified the milestone: 2.0.0 Jan 23, 2017
@GeoffreyBooth GeoffreyBooth changed the title [CS2] Assignment in dynamic method names [CS2] Nodes, Scope: Assignment in dynamic method names May 6, 2017
@GeoffreyBooth
Copy link
Collaborator

Fixed by #4426.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants