[Bug]: Class Private Fields/Accessors/Methods must not be re-initialized on instance

[jridgewell]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

(See "Prevent private class members from being added more than once" section in https://github.com/evanw/esbuild/releases/tag/v0.11.20#:~:text=Prevent%20private%20class%20members%20from%20being%20added%20more%20than%20once)

class Base {
  constructor(obj) {
    return obj;
  }
}

let counter = 0;
class Derived extends Base {
  #foo = ++counter;
  static get(obj) {
    return obj.#foo;
  }
}

const foo = {};
new Derived(foo);
assert.equals(Derived.get(foo), 1);

// This should throw an error, private fields must not be re-initialized
// on an object that already contains the field.
assert.throws(() => {
  new Derived(foo);
});
// ^ That should have thrown, but it doesn't.

// Must not re-initialize the field
assert.equals(Derived.get(foo), 1);
// But the counter is incremented (the initializer runs, but the field
// should not re-add)
assert.equals(counter, 2);

REPL

Configuration file name

babel.config.json

Configuration

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "shippedProposals": true,
        "targets": {
          "chrome": "75"
        }
      }
    ]
  ]
}

Current and expected behavior

Currently, this will re-initialize the field #foo, but this is incorrect. It should throw an error when trying to initialize the field a second time.

Environment

• Babel version: v7.14.1

Possible solution

We should create new initializePrivateFieldSpec, initializePrivateAccessorSpec, and initializePrivateMethodSpec (and static versions) helpers to initialize the field to a descriptor. Inside the helpers, we should check if the WeakMap/WeakSet that represents the transformed field already contains the instance key. If so, throw. Else, initialize.

Or, we could just have a simple checkPrivateFieldInitSpec which does the check, but doesn't initialize. We'd then call the check before initializing a field.

After creating the helpers, we should call them in the appropriate place in https://github.com/babel/babel/blob/main/packages/babel-helper-create-class-features-plugin/src/fields.js (look for any function that ends in "InitSpec").

Additional context

This is a medium difficulty issue, so could be tackled by someone with a little experience making changes to Babel.

[nicolo-ribaudo]

I prefer the checkPrivateFieldInitSpec idea because:

1. We need a single function both for private fields and private methods, since it would use the .has method defined both on WeakMap and WeakSet
2. It's self-contained, so it would be easier to potentially introduce an noPrivateReinitialization assumption to omit it (since 99.99% of the apps won't probably need this check).

[jridgewell]

One specific item with checkPrivateFieldInitSpec. We still have to run the initializer, but it has to fail when trying to reset the field:

// From OP, the very last line
assert.equals(counter, 2);

If we go with checkPrivateFieldInitSpec, we'll have code that looks a bit like:

class Derived extends Base {
  constructor() {
    const _this = super();
-    _foo.set(_this, { value: ++counter }); 
+    _foo.set(_this, (desc = { value: ++counter }, checkPrivateFieldInitSpec(_this, _foo), desc)); 
  }
}

const _foo = new WeakMap();

[nicolo-ribaudo]

It could also be something like this:

_foo.set(_this, _checkPrivateFieldInitSpec(_this, _foo, { value: ++counter });

where _checkPrivateFieldInitSpec is

function _checkPrivateFieldInitSpec(obj, collection, value) {
  if (collection.has(obj)) throw new TypeError();
  return value;
}

And for methods:

_checkPrivateFieldInitSpec(_this, _foo), _foo.add(_this);

EDIT: Now that I see it written down, I'm not sure that I prefer this option anymore. The alternative would be

_initializePrivateFieldSpec(_this, _foo, ++counter);

_initializePrivateMethodSpec(_this, _foo);

[mdaj06]

could i help here?

[nicolo-ribaudo]

Yes!

If it is the first time that you contribute to Babel, you can follow these steps: (you need to have make and yarn available on your machine)

1. Fork the repo
2. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
3. Run yarn && make bootstrap
4. Wait ⏳
5. Run make watch (or make build/yarn gulp build whenever you change a file). When updating @babel/helpers, I'm not 100% sure that make watch works.
6. Add a test (only exec.js and/or input.js; output.js will be automatically generated)
7. Update the code!
8. yarn jest [name-of-the-package-to-test] 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 [name-of-the-package-to-test] and they will be automatically updated.
9. If it is working, run make test to run all the tests
10. Run git push and open a PR!

[nicolo-ribaudo]

Could you implement the initializePrivateFieldSpec/initializePrivateMethodSpec version? The helpers are defined at https://github.com/babel/babel/blob/main/packages/babel-helpers/src/helpers.js and used at https://github.com/babel/babel/blob/main/packages/babel-helper-create-class-features-plugin/src/fields.js

[mdaj06]

sure i can try it out

[jridgewell]

@mdaj06: Hope the implementation is going well, please ping if you have any questions!

[mdaj06]

Yes sure will ping you if I'm blocked with anything!thanks!

[nicolo-ribaudo]

Hi @mdaj06! Are you still working on this?

[mdaj06]

Yes @nicolo-ribaudo! Got a bit side-tracked in the past few weeks.

[komyg]

Hi,

I would like to help out. Is there anything I can do?

Thanks,
Komyg

[mdaj06]

sure @komyg you can take it up , i havent gotten a chance to really work on it in the past few weeks.

[komyg]

Thanks, I will start working right away

[komyg]

Hi @nicolo-ribaudo, I saw that the link in the readme file is broken. I tried to search for it in the documentation, but I haven't found it:

[@babel/helper-create-class-features-plugin](https://babeljs.io/docs/en/babel-helper-create-class-features-plugin)

If you pass me the right link, I can update it as a part of this PR.

[nicolo-ribaudo]

Uh, that link is auto-generated and unfortunately I think we don't have docs for that internal package 😅

[komyg]

Hi @nicolo-ribaudo,

I think that the generated code should look somewhat like this:

'use strict';

function _classPrivateFieldGet(receiver, privateMap) {
  var descriptor = _classExtractFieldDescriptor(receiver, privateMap, 'get');
  return _classApplyDescriptorGet(receiver, descriptor);
}

function _classExtractFieldDescriptor(receiver, privateMap, action) {
  if (!privateMap.has(receiver)) {
    throw new TypeError(
      'attempted to ' + action + ' private field on non-instance'
    );
  }
  return privateMap.get(receiver);
}

function _classApplyDescriptorGet(receiver, descriptor) {
  if (descriptor.get) {
    return descriptor.get.call(receiver);
  }
  return descriptor.value;
}

/** CHANGES HERE */
function _checkPrivateFieldInitSpec(obj, privateMap) {
  if (privateMap.has(obj)) {
    throw new TypeError();
  }
}

class Base {
  constructor(obj) {
    return obj;
  }
}

let counter = 0;

var _foo = /*#__PURE__*/ new WeakMap();

class Derived extends Base {
  constructor(...args) {
    super(...args);

    /** CHANGES HERE */
    _checkPrivateFieldInitSpec(this, _foo);
    _foo.set(this, {
      writable: true,
      value: ++counter,
    });
  }

  static get(obj) {
    return _classPrivateFieldGet(obj, _foo);
  }
}

const foo = {};
new Derived(foo);
console.log(Derived.get(foo));

new Derived(foo);
console.log(Derived.get(foo));

I've created this function: _checkPrivateFieldInitSpec and called it inside the constructor: _checkPrivateFieldInitSpec(this, _foo);.

The rationale being that if the private map already has a reference to the object, then we know that we've passed through the constructor, and we can consider the field initialized.

I've tested it in node, and it does throw the expected exception.

What do you think? Can I go forward with this?

[komyg]

Hi guys, just created a draft PR for this issue, so you can follow my progress if you want.

I'll let you know once it is ready for review.

[nicolo-ribaudo]

Your example has a problem: the error is thrown before evaluating ++counter, so it's only incremented once and not twice.

We could introduce a new helper, _privateFieldInitSpec, called like this:

    _privateFieldInitSpec(this, _foo, {
      writable: true,
      value: ++counter,
    });

Internally this helper would throw the error if the field is already initialized, but at least ++counter would be evaluated before throwing.

[komyg]

I see. So based on your suggestion, I think I will create a new function:

function _privateFieldInitSpec(obj, privateMap, value) {
  if (privateMap.has(obj)) {
    throw new TypeError();
  }
  privateMap.set(obj, value);
}

// [...]
constructor(...args) {
  super(...args);

  _privateFieldInitSpec(this, _foo, {
    writable: true,
    value: ++counter,
  });
}

Here is the full code:

'use strict';

function _classPrivateFieldGet(receiver, privateMap) {
  var descriptor = _classExtractFieldDescriptor(receiver, privateMap, 'get');
  return _classApplyDescriptorGet(receiver, descriptor);
}

function _classExtractFieldDescriptor(receiver, privateMap, action) {
  if (!privateMap.has(receiver)) {
    throw new TypeError(
      'attempted to ' + action + ' private field on non-instance'
    );
  }
  return privateMap.get(receiver);
}

function _classApplyDescriptorGet(receiver, descriptor) {
  if (descriptor.get) {
    return descriptor.get.call(receiver);
  }
  return descriptor.value;
}

function _privateFieldInitSpec(obj, privateMap, value) {
  if (privateMap.has(obj)) {
    throw new TypeError();
  }
  privateMap.set(obj, value);
}

class Base {
  constructor(obj) {
    return obj;
  }
}

let counter = 0;

var _foo = /*#__PURE__*/ new WeakMap();

class Derived extends Base {
  constructor(...args) {
    super(...args);

    _privateFieldInitSpec(this, _foo, {
      writable: true,
      value: ++counter,
    });
  }

  static get(obj) {
    return _classPrivateFieldGet(obj, _foo);
  }
}

const foo = {};
new Derived(foo);
console.log(Derived.get(foo));  // 1
console.log('count', counter); // 1

try {
  new Derived(foo);
  console.log(Derived.get(foo));
} catch (err) {
  console.error(err);
}

console.log(Derived.get(foo));  // 1
console.log('count', counter); // 2

What do you think?

[nicolo-ribaudo]

It looks good! The new function can be created in packages/babel-helpers/src/helpers.js, and it can be used using the addHelper internal method (you can Ctrl+F for it to see how it works).

[komyg]

Hey @nicolo-ribaudo,

I've been trying to reproduce this error on Node with private static fields, but I wasn't able to. I think it is because the static fields are a part of the class itself, therefore there is no way to re-declare them.

This is an example that I came up to see if I could reproduce the error:

class Base {
  constructor(obj) {
    return obj;
  }
}

let counter = 0;
class Derived extends Base {
  static #foo = ++counter;

  static getFoo() {
    return this.#foo;
  }

  static #bar() {
    return 'hello world';
  }

  static getBar() {
    return this.#bar();
  }
}

const foo = {};
new Derived(foo);
console.log(Derived.getBar());

new Derived(foo);
console.log(Derived.getBar());

I've ran it on Node and it works fine.

Could you help me find an example where we should throw an error for the static field re-declaration, and we are not doing so?

[nicolo-ribaudo]

I believe it's impossible to initialize static fields twice, so we only need to fix instance fields.

[komyg]

Ok, in this case, I think I am done. I've marked the PR as ready for review.

My PR covers instance properties, methods and accessor methods. Is there anything else that we should cover?

[nicolo-ribaudo]

No, it's ok 👍

[komyg]

Great, thanks! 🙂

I await your review then.

[jridgewell]

I believe it's impossible to initialize static fields twice, so we only need to fix instance fields.

Correct. I was wrong in my issue description, this only applies to instance fields.