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

Private Static Fields Features: Stage 3 #8205

Merged
merged 4 commits into from Sep 1, 2018

Conversation

@rricard
Copy link
Contributor

rricard commented Jun 20, 2018

Q A
Fixed Issues? #8052 (except accessors)
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Add private static fields support
Tests Added + Pass? Tests Updated
Documentation PR
Any Dependency Changes?
License MIT
Sponsor @bloomberg
  • No changes in parser
  • Added a new helper to check private static field access provenance: classStaticPrivateFieldBase
  • While visiting a class:
    1. Check for duplicate private static declarations
    2. For every private static declaration add it to a container object in the classe's scope and remove it from the class itself
      class C {
        static #foo = "bar";
        /*...*/
      }
      // will become (in loose mode):
      class C {
        /*...*/
      }
      var _CStatics = {};
      _CStatics._foo = "bar";
    3. For every private static declaration, start visiting the class to explore each member method in order to wrap the access in the helper:
      myMethod() {
        C.#foo = this.#foo + "baz";
      }
      // Becomes
      myMethod() {
        babelHelper.classStaticPrivateFieldBase(C, C, _CStatics)._foo = 
          babelHelper.classStaticPrivateFieldBase(this, C, _CStatics)._foo + "baz";
      }
@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch 4 times, most recently from 2824e86 to 1c43798 Jun 21, 2018
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jun 21, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8935/

@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch from 1c43798 to b00fa81 Jun 21, 2018
@rricard rricard changed the title [WIP] Static Class Features: Stage 3 Private Static Fields: Stage 3 Jun 22, 2018
@rricard rricard changed the title Private Static Fields: Stage 3 [wip] Private Static Features: Stage 3 Jun 22, 2018
@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Jun 22, 2018

We only handled private static fields for now, @tim-mc is going to help bring private static methods now. From there we'll be able to think about accessors...

@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch from 181e814 to ff1203e Jun 26, 2018
@rricard rricard changed the title [wip] Private Static Features: Stage 3 Private Static Fields Features: Stage 3 Jun 26, 2018
@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch from ff1203e to 69e5e2e Jun 26, 2018
@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Jun 26, 2018

It feels like this PR is ready for review.

I also would like some help for making that Travis build pass...

@hzoo hzoo requested a review from jridgewell Jun 27, 2018

expect("bar" in Foo).toBe(false)
expect(Foo.test()).toBe(undefined)
expect(Foo.test()).toBe(undefined)

This comment has been minimized.

Copy link
@hzoo

hzoo Jun 27, 2018

Member

Some of these duplicated tests are weird to me expect(Foo.test()).toBe(undefined) - I know they were just copied over but is it supposed to be testing the static and instance methods or just remove it?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jun 27, 2018

Member

Maybe it should be expect(() => Foo.test()).not.toThrow()?

This comment has been minimized.

Copy link
@rricard

rricard Jun 28, 2018

Author Contributor

@hzoo, you're right, I'll go ahead and do a second review pass on those tests (was only checking for the output I was seeing). I'm also trying to run test-262 right now against it (having npm link issues) and I might add some relevant tests then

This comment has been minimized.

Copy link
@hzoo

hzoo Jun 28, 2018

Member

Yeah a general thing we should do moving forward is run against test262, I know @leobalter/@rwaldron have done some work there. (@xtuc had https://github.com/xtuc/babel-test262 but we haven't used)


helpers.classStaticPrivateFieldBase = () => template.program.ast`
export default function _classStaticPrivateFieldBase(receiver, classConstructor, privateClass) {
if (receiver !== classConstructor && receiver.constructor !== classConstructor) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

The receiver.constructor !== classConstructor shouldn't be necessary.

This comment has been minimized.

Copy link
@rricard

rricard Jul 20, 2018

Author Contributor

Unfortunately, this seems necessary to be spec compliant. We should discuss that on slack.

throw path.buildCodeFrameError(
"Static class fields are not spec'ed yet.",
);
if (privateStaticNames.has(name)) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

Private names are shared between instance and static, ie, the following is an error:

class Example {
  static #x;
  #x;
}

This comment has been minimized.

Copy link
@rricard

rricard Jul 10, 2018

Author Contributor

Wasn't sure about this one, I'll make the change.

var Foo =
/*#__PURE__*/
function () {
"use strict";

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

This is my mistake. This test case was meant to output class expressions, not compile classes to functions.

This comment has been minimized.

Copy link
@rricard

rricard Jul 10, 2018

Author Contributor

That makes more sense indeed, I'll redo the whole test dir with that then!

This comment has been minimized.

Copy link
@rricard

rricard Jul 20, 2018

Author Contributor

@jridgewell what should I do to convert tests to output class expressions?

@@ -83,6 +83,18 @@ export default declare((api, options) => {
},
};

// Traverses the class scope, handling private static name references.
const staticPrivatePropertyVisitor = {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

We should be able to reuse privateNameVisitor. This is necessary because we have to use its inner traverser, too.

This comment has been minimized.

Copy link
@rricard

rricard Jul 10, 2018

Author Contributor

Ok, I kept them separate to make sure I don't mess with your logic but indeed, it's better to change just a few things in your logic than to branch out an entirely new one!

@@ -1011,3 +1011,12 @@ helpers.classPrivateFieldSet = () => template.program.ast`
return value;
}
`;

helpers.classStaticPrivateFieldBase = () => template.program.ast`
export default function _classStaticPrivateFieldBase(receiver, classConstructor, privateClass) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

So there are a few issues here, and we can choose either of two ways to handle it:

  • We can remove this privateClass holder object for loose mode.
  • We can handle get, set, and call for both loose and spec mode.

The first option would allow us to reuse the same code we already have for instance private fields.

If we continue to use a holder object, we have to handle get, set, and call operations on the holder, and use the receiver as the calling context for all those cases. This complicates the runtime for loose mode. But, we're gonna need to do that anyways for spec mode.

This comment has been minimized.

Copy link
@rricard

rricard Jul 10, 2018

Author Contributor

This is super helpful, I had a hard time understanding the line between loose and spec.

@@ -157,6 +169,23 @@ export default declare((api, options) => {
},
};

const staticPrivatePropertyHandler = {
handle(member) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 3, 2018

Member

So this doesn't handle get, set, and call operations. Given that all this is stored on a holder object, anything like ClassName.#x() will really have to be transformed to base(ClassName, ClassName, holder)['x'].call(ClassName). This currently just outputs base(ClassName, ClassName, holder)['x']().

This comment has been minimized.

Copy link
@rricard

rricard Jul 10, 2018

Author Contributor

I think I just missed something about how the handlers are working and that made it way clearer. Thanks!

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Jul 9, 2018

Thanks @jridgewell, I'm sorry I have a lot of work on the side of this but I'll try to get back to it this week.

@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch 2 times, most recently from e9c8e56 to 83fbdbc Jul 10, 2018
@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Jul 10, 2018

Todo:

  • get/set/call handler
  • Rework the helper(s)
  • Redo the tests
@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch 2 times, most recently from 2923f1b to 1c448ee Jul 20, 2018
@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 8, 2018

Hi @jridgewell, I could use some help changing the tests to not transform the classes. Otherwise, I think I'm almost done.

Copy link
Member

jridgewell left a comment

Sorry for being flakey. This is looking good!


helpers.classStaticPrivateFieldLooseBase = () => template.program.ast`
export default function _classStaticPrivateFieldLooseBase(receiver, classConstructor) {
if (receiver !== classConstructor && receiver.constructor !== classConstructor) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 18, 2018

Member

What's the receiver.constructor !== classConstructor for?

This comment has been minimized.

Copy link
@rricard

rricard Aug 20, 2018

Author Contributor

That's for when the static field gets called from this. In that case I'll probably have an instance of the class instead of the constructor itself. I do think that's what we expect from the spec but it's been a while since the last time I dived in the proposal.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 20, 2018

Member

It has to throw a type error if it's called on an instance of the class. This check should just be the receiver !== classConstructor.

This comment has been minimized.

Copy link
@rricard

rricard Aug 20, 2018

Author Contributor

I'm not sure, I think that if the instance is of the class type, access should work through this. I can check that again. However, in the meantime, we can restrict the access so it does not work on the instance while we figure that out.

// Create a private static "host" object if it does not exist
privateClassId = path.scope.generateUidIdentifier(ref.name + "Statics");
staticNodesToAdd.push(
template.statement`const PRIVATE_CLASS_ID = {};`({

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 18, 2018

Member

Nit: Should probably use a prototype-less object here.

if (privateNames.has(name)) {
throw path.buildCodeFrameError("Duplicate private field");
throw path.buildCodeFrameError("Duplicate static private field");

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 18, 2018

Member

Let's leave this as the old message.

state,
privateClassId,
);
staticNodesToAdd.forEach(node => staticNodes.push(node));

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 18, 2018

Member

Can just staticNodes.push(...staticNodesToAdd)

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 20, 2018

@jridgewell don't worry, we're in no rush. I'll just need some help setting up the tests correctly.

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 20, 2018

I was trying to prevent the class transform itself but I managed to get that right in the plane

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 20, 2018

It should be good for a second review

Copy link
Member

jridgewell left a comment

Looking good!

Need tests for loose, and some that do a call operation on the private static.

const { scope, parentPath } = path;
const { key, value } = path.node;
const { name } = key.id;
const privateId = scope.generateUidIdentifier(name);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 20, 2018

Member

Nit: So we actually don't need to generate a unique id here, we can just use name.

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 20, 2018

Loose tests have been removed while I changed my test methodology, they're back now. I also removed the privateId for spec mode but not loose mode (where the unique id prevents test breakage in loose mode)

@rricard rricard force-pushed the rricard:static-class-feature-stage-3 branch from eb66be9 to 6dc2a28 Aug 20, 2018
@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 20, 2018

Rebased the whole thing. Still need to test call operation

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 24, 2018

@jridgewell I think this covers all of the requested changes. Thanks!


}

Foo._foo = "foo";

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 26, 2018

Member

My last nit: This should be using Object.defineProperty with { enumerable: false, configurable: false }. We can do this in a follow PR if you'd like.

This comment has been minimized.

Copy link
@rricard

rricard Aug 26, 2018

Author Contributor

That is a good point, I'll try to put that here before we merge

@rricard

This comment has been minimized.

Copy link
Contributor Author

rricard commented Aug 26, 2018

@jridgewell I made the change, would you like to have it in a separate helper though?

Copy link
Member

jridgewell left a comment

This is all good. Need one more approval before we can merge.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Sep 1, 2018

Eh, let's just go for it.

@jridgewell jridgewell merged commit fb66fa6 into babel:master Sep 1, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.55% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Sep 3, 2018

I'm sorry, I forgot to review this PR 😅
I'll open a few PR with some small tweaks.

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