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

Get operation on private accessor without getter should throw #12673

Closed
1 task
mkubilayk opened this issue Jan 22, 2021 · 5 comments · Fixed by #12689
Closed
1 task

Get operation on private accessor without getter should throw #12673

mkubilayk opened this issue Jan 22, 2021 · 5 comments · Fixed by #12689
Labels
good first issue i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue Spec: Class Fields

Comments

@mkubilayk
Copy link

Bug Report

  • I would like to work on a fix!

Current behavior

Transpiled code doesn't throw an error in foo() and returns undefined.

Input Code

class C {
  foo() { return this.#bar; }
  set #bar(a) {}
}

new C().foo();

Expected behavior

new C().foo(); should throw a TypeError.

  • Example from Google Chrome Version 90.0.4393.0 Canary:
    VM490:2 Uncaught TypeError: '#bar' was defined without a getter
        at C.foo (<anonymous>:2:11)
        at <anonymous>:6:9
    
  • Related test262 test: get-access-of-missing-private-getter.js.

Babel Configuration: See the REPL link

Environment

  • Babel version(s): v7.12.12
  • Node/npm version: 10.13
  • OS: Using REPL
  • Monorepo: Using REPL
  • How you are using Babel: Using REPL

Possible Solution

_classPrivateFieldGet helper should not fallback to returning descriptor.value if descriptor.get is false-y. If get property exists but is undefined, it can throw this particular error.

Additional context

See PrivateFieldGet-6.b from the spec:

PrivateFieldGet ( P, O )
    1. Assert: P is a Private Name.
    2. If O is not an object, throw a TypeError exception.
    3. If P.[[Kind]] is "field",
        a. Let entry be PrivateFieldFind(P, O).
        b. If entry is empty, throw a TypeError exception.
        c. Return entry.[[PrivateFieldValue]].
    4. Perform ? PrivateBrandCheck(O, P).
    5. If P.[[Kind]] is "method",
        a. Return P.[[Value]].
    6. Else,
        a. Assert: P.[[Kind]] is "accessor".
        b. If P does not have a [[Get]] field, throw a TypeError exception.
        c. Let getter be P.[[Get]].
        d. Return ? Call(getter, O).
@babel-bot
Copy link
Collaborator

Hey @mkubilayk! 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."

@jridgewell
Copy link
Member

Sigh, another difference with normal properties.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 24, 2021

We can inject the error in a new helper (we already have readOnlyError, and we can create writeOnlyError): it's easy to detect this case at build time, and by doing so we don't unnecessarly increase the helper size for 99.9% of our users.

if (isAccessor) {
return t.callExpression(file.addHelper("classPrivateFieldGet"), [
this.receiver(member),
t.cloneNode(id),
]);
}

Instead of checking if isAccessor, we can:

  1. If it's a getter (if (getId)), we inject _classPrivateFieldGet(...)
  2. Otherwise, if it's a setter, we inject _writeOnlyError("#bar").

We need to be careful when introducing a new helper, because it will throw an error if we try to inject it with older @babel/core versions. Instead of just doing file.addHelper, we should check if file.availableHelper("writeOnlyError")

I'll mark this as a good first issue: it's not "easy", but it should be self-contained enough.
@mkubilayk If you want to work on this it's yours (two days to claim it!), otherwise it's up for grab by whoever is interested 🙂.


If it is the first time that you contribute to 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. Update the test at packages/babel-plugin-proposal-private-methods/test/fixtures/accessors/get-only-setter (delete output.js and it will be automatically generated)
  8. Update the code!
  9. yarn jest private-methods 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
    • If you prefer, you can run OVERWRITE=true yarn jest private-methods and they will be automatically updated.
  10. If it is working, run make test to run all the tests
  11. Run git push and open a PR!

@mkubilayk
Copy link
Author

@nicolo-ribaudo writeOnlyError sounds good. I don't have the bandwidth to pick this up right now so happy for someone else to take it. 🙂

@fedeci
Copy link
Member

fedeci commented Jan 25, 2021

I'll take this, seems to be a quick fix.

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

Successfully merging a pull request may close this issue.

6 participants