[Bug]: Reference to class expression in private methods is not correctly compiled

[nicolo-ribaudo]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

fn(class WorkerClient {
  static #worker_threads() {
    return WorkerClient;
  }
});

REPL

Configuration file name

No response

Configuration

{
  "plugins": [
    "@babel/plugin-proposal-private-methods",
    "@babel/plugin-proposal-class-properties"
  ]
}

Current and expected behavior

It is compiled to this:

var _class;

fn(_class = class WorkerClient {});

function _worker_threads() {
  return WorkerClient;
}

As you can see, WorkerClient is not reachable. It should be compiled to something like this:

var _class;

fn(_class = class WorkerClient {});

function _worker_threads() {
  return _class;
}

Environment

• Babel 7.14.4

Possible solution

In the replaceThisContext function that we use to replace this usage in class fields and private methods:

babel/packages/babel-helper-create-class-features-plugin/src/fields.ts

Line 672 in 813f2a3

function replaceThisContext(

we should add a new path.traverse call to replace references to the inner binding, with a visitor similar to this one:

const innerReferencesVisitor = {
  ReferencedIdentifier(path, state) {
    const { name } = path.node;
    if (path.scope.bindingIdentifierEquals(name, state.innerBinding)) {
      state.needsClassRef = true;
      path.replaceWith(t.cloneNode(state.classRef));
    }
  },
};

where state is the one used by thisContextVisitor with the addition of state.innerBinding, which is the node representing the id of the class expression.

Note that we don't need to merge this visitor with environmentVisitor: environmentVisitor is only needed to avoid replacing nested thises that don't refer to the class, while for normal bindings we use path.scope.bindingIdentifierEquals.

We can get innerBinding from right before extracting the class reference at

babel/packages/babel-helper-create-class-features-plugin/src/index.ts

Lines 173 to 178 in 813f2a3

	if (path.isClassExpression() || !path.node.id) {
	nameFunction(path);
	ref = path.scope.generateUidIdentifier("class");
	} else {
	ref = t.cloneNode(path.node.id);
	}

We only need to get it if we have a ClassExpression and path.node.id is set, otherwise we can leave innerBinding as undefined and use it as a "signal" to skip the new path.traverse call.

After working on the fix itself, we'll need a bunch of tests:

• A simple input.js/output.js test, for example my original example
• An exec.js test, to check that the binding is the correct one. For example:

  let x = class Test {
    static #getTest() { return Test }
    static getTest() { return this.getTest() }
  };
  
  expect(x.getTest()).toBe(x);

• An input/output test to verify that it works when the binding is used in nested scopes, for example:

  let x = class Test {
    static #method() {
      return function inner() {
        return Test;
      };
    }
  }

• An input/output test to verify that it doesn't rewrite shadowed bindings:

  let x = class Test {
    static #method() {
      new Test();
  
      return function inner() {
        let Test = 2;
        return Test;
      };
    }
  }

Additional context

The same bug happens with static class fields (both public and private) and instance private methods:

fn(class WorkerClient {
  static #x = WorkerClient;
  static y = WorkerClient;
});

we should duplicate the tests for those cases.

[lala7573]

Can I give it a try? I'm new to this project but the spec looks quite clear. ... :)

[fedeci]

@lala7573 sure!

[nicolo-ribaudo]

@lala7573 In case you need some help setting up this repository locally:

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. In a separate terminal, run make watch (or make build whenever you change a file)
6. Add a test (exec.js and input.js; output.js will be automatically generated)
7. Update the code!
8. yarn jest class-properties 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 class-properties private-methods and they will be automatically updated.
9. If it is working, run yarn jest (or make test) to run all the tests
10. Run git push and open a PR!

[lala7573]

This is the draft #13429
I'm not a native English speaker .. so any help with naming would be appreciated.