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

[parser] Disallow private properties access on super #10470

Closed
nicolo-ribaudo opened this issue Sep 20, 2019 · 2 comments · Fixed by #10472
Closed

[parser] Disallow private properties access on super #10470

nicolo-ribaudo opened this issue Sep 20, 2019 · 2 comments · Fixed by #10472
Assignees
Labels
claimed good first issue i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser Spec: Class Fields

Comments

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 20, 2019

Bug Report

Input Code

class A extends B {
  #x;

  method() {
    super.#x;
  }
}

repl

Expected behavior/code

It should throw a parser error.

Specification

In the class fields proposal, MemberExpression (which is the technical tern for "property access") is defined as follows:

MemberExpression:
  PrimaryExpression
  MemberExpression [ Expression ]           <-- foo[bar]
  MemberExpression . IdentifierName         <-- foo.bar
  MemberExpression TemplateLiteral          <-- foo`bar`
  SuperProperty
  MetaProperty
  new MemberExpression Arguments            <-- new foo.bar()
  MemberExpression . PrivateIdentifier      <-- foo.#x

As you can see, .PrivateIdentifier (where a PrivateIdentifier is like #x) is only allowed after another MemberExpression.

Let's see what SuperProperty is. It is not redefined by the proposal, so we must search it in the main specification.

SuperProperty:
  super [ Expression ]                      <-- super[foo]
  super . IdentifierName                    <-- super.#x

As you might see, there is nothing allowing super.#x

Babel Configuration (.babelrc, package.json, cli command)

In order to test this bug, you must enable the classPrivateProperties parser plugin

Environment

  • Babel version(s): v7.6

Possible Solution

When we parse the super keyword, we check if it is followed by (, [ or . and throw otherwise:

if (
!this.match(tt.parenL) &&
!this.match(tt.bracketL) &&
!this.match(tt.dot)
) {
this.unexpected();
}
return this.finishNode(node, "Super");

A naive solution would be to also check the next token, using this.lookahead(). However, in @babel/parser lookahead() is really expensive because it needs to clone the whole parser state, produce the next token and then reset the state to what it was before: it should be avoided when possible.

What we could do is to disallow it when we are parsing the MemberExpression rather than the super keyword:

} else if (this.eat(tt.dot)) {
const node = this.startNodeAt(startPos, startLoc);
node.object = base;
node.property = this.parseMaybePrivateName();
node.computed = false;
if (state.optionalChainMember) {
node.optional = false;
return this.finishNode(node, "OptionalMemberExpression");
}
return this.finishNode(node, "MemberExpression");

After parsing the node.property (which is the "thing" after the dot), we can check if

  1. the base object is a Super node, and
  2. the property is a PrivateName node.

If both those condition are met, we should raise an error explaining that it is disallowed.


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 inside packages/babel-parser/test/fixtures/experimental/class-private-properties. You can look in the folders to check how tests about errors are structured.
  8. Update the code!
  9. yarn jest parser to run the tests
  10. If it is working, run make test to run all the tests
  11. Run git push and open a PR!

If you need any other help, you can join our Slack and ask in the #developement channel. *

*

I'm going bed now, but I'm sure you'll find someone else online! 🙂

@VivekNayyar
Copy link

I will pick this up

@nicolo-ribaudo
Copy link
Member Author

It's yours!

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
claimed good first issue i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser Spec: Class Fields
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants