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

Use of `arguments` should be permitted even in a field initializer, if the field is initialized with a class expression. #10771

Closed
Elberet opened this issue Nov 27, 2019 · 4 comments · Fixed by #10801

Comments

@Elberet
Copy link

@Elberet Elberet commented Nov 27, 2019

Bug Report

Current Behavior
Any use of arguments within a class field initializer leads to 'arguments' is not allowed in class field initializer, even when such a limitation appears unwarranted - e.g. if the class field is initialized with a class expression and arguments appears inside one of the declared class' methods.

Input Code

class Voodoo {
  static magic = {
    wizardry: class extends Witchcraft {
      cast() {
        super.cast(...arguments)
      }
    }
  }
}

See also babeljs.io/repl

Expected behavior/code
The example code should transpile without error; the class field initializer scope should not prevent use of arguments in a field initializer once it has been bound to some scope declared within the initializer and can no longer refer to the original class' constructor arguments.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Nov 27, 2019

Hey @Elberet! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Nov 27, 2019

Here is a minimal reproducible example.

Babel will throw "arguments not allowed" error when checking if an identifier name is a reserved word.

if (this.state.inClassProperty && word === "arguments") {
this.raise(
startLoc,
"'arguments' is not allowed in class field initializer",
);
return;
}

As one can see it depends on the parser state this.state.inClassProperty. To solve this issue, we may pay attention to the class declaration parser

const oldStrict = this.state.strict;
this.state.strict = true;
this.parseClassId(node, isStatement, optionalId);
this.parseClassSuper(node);
node.body = this.parseClassBody(!!node.superClass);
this.state.strict = oldStrict;

You may notice that the parser state this.state.strict is pushed by a new value true and then popped after the class declaration has been parsed, so that during the class declaration is being parsed, we are always enforcing a strict mode. We may consider to do some similar maneuvers for inClassProperty.

Note that it is upon your discretion to determine where the inClassProperty should be pushed a new value, you can place it before parseClassId or before parseClassBody, which will have subtle behaviour difference.

If you don't know how to clone Babel, follow these steps: (you need to have make and yarn available on your machine).

  1. Write a comment there to let other possible contributors know that you are working on this bug.
  2. Fork the repo
  3. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
  4. Run yarn && make bootstrap
  5. Wait
  6. Run make watch (or make build whenever you change a file)
  7. Add a test (only input.js; output.js will be automatically generated) to packages/babel-parser/test/fixtures/experimental/class-properties/arguments-in-class.
  8. Update the code!
  9. yarn jest babel-parser to run the tests
    • If some test outputs don't match but the new results are correct, you can delete the bad output.js files and run the tests again
  10. If it is working, run make test to run all the tests
  11. Run git push and open a PR!

@Elberet Let me know if you want to give it a try or not.

@Elberet

This comment has been minimized.

Copy link
Author

@Elberet Elberet commented Nov 27, 2019

@JLHwung Sure, I can prepare a PR, but I think there are still some questions to discuss and I don't feel sufficiently proficient with the TC39 spec to decide all the answers myself - especially, for which nodes exactly the inClassProperty state flag should be reset.

For example, resetting the flag before parsing the class body may permit access to arguments within decorator call expressions:

class C {
  p = class {
    @d(arguments)
    m() { }
  }
}

This is, IMO quite obviously, wrong. The decorator's value is evaluated at the same time the class expression is evaluated, which happens when the class field is initialized, and in that scope arguments should not be accessible.

Gut instinct tells me that the right place to reset the inClassProperty flag would be here:

// Parse function body and check parameters.
parseFunctionBody(
node: N.Function,
allowExpression: ?boolean,
isMethod?: boolean = false,
): void {
const isExpression = allowExpression && !this.match(tt.braceL);
const oldStrict = this.state.strict;
let useStrict = false;

but then if I were to review that code I'd loudly complain that inClassProperty is being abused to track whether we have a proper function scope for arguments et al - so maybe we should refactor the whole thing and add a new inFunctionScope state?

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Nov 27, 2019

@Elberet Good point! Note that if we reset inClassProperty in parseFunctionBody, the following case would be a false negative

class C { P = () => arguments }

But I believe you have pointed to the correct direction. We can check if the scope where arguments resides is the same scope as we have toggled on inClassProperty, i.e. we can record another state property that points to the inClassProperty scope.

Another approach is: Can we simply replace this.state.inClassProperties by this.scope.inClass in the checkReservedWord condition?

if (this.state.inClassProperty && word === "arguments") {

There are two cases when this.scope.inClass is true. 1) When arguments is within the same scope of class properties, and apparently it should throw. 2) When arguments is in a nested class scope, it must be in one of class property or class private property, which should be thrown.

this.scope.inClass is not implemented but you can add one, which will be similar to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.