Fix nested classes reference private fields

[jridgewell]

Q	A
Fixed Issues?
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

We failed to properly handle nested private field, when only a single private field was redeclared. Example

When we hit a nested class that redeclares a field, we skip the nested class. And when we finally traverse that nested class, we'll only have the privates that are declared by it. We've lost the ancestor's private fields!

[nicolo-ribaudo]

Since nodes can be re-queued and skipped, it's not guaranteed that they are visited in-order.

Instead of a stack, I think we could use a WeakMap from class nodes to privateNamesMap, and get the parent class using path.findParent(p => p.isClass()).

[nicolo-ribaudo]

When using a WeakSet, please put this in the createClassFeaturePluign function so that when the plugin finishes running it can be garbage-collected, even if the AST was still retained.

[nicolo-ribaudo]

When we hit a nested class that redeclares a field, we skip the nested class.

I was wondering if it would be easier to fix it there. The Class visitor in privateNameVisitor could become something like this:

  Class(path) {
    const { privateNamesMap } = this;
    const body = path.get("body.body");

    // First, evaluate the things in the outer scope.
    path.traverse(privateNameInnerVisitor, this);

    const visiblePrivateNames = new Map(privateNamesMap);
    for (const prop of body) {
      if (prop.isPrivate()) visiblePrivateNames.delete(prop.node.key.id.name);
    }

    path.traverse(privateNameVisitor, {
      ...this,
      privateNamesMap: visiblePrivateNames,
      ignoreUndeclared: true,
    });
    path.skip();
  },

[jridgewell]

I was wondering if it would be easier to fix it there. The Class visitor in privateNameVisitor could become something like this:

I was trying to avoid processing the class multiple times. But this is easier to do, given the issues with out-of-order requeueing. Updated.

[JLHwung]

Can you add a case on redeclared private names with computed key reference? The following case should throw since #foo is shadowed.

class Foo {
  #foo = 1;

  test() {
    class Nested {
      #foo = 2;
      [this.#foo] = 3;
    }

    new Nested;
  }
}
(new Foo).test();

Given that it is not directly related to this PR, we can fix it in a separate one.

[jridgewell]

Shouldn't we wait till the other PR to add the test case? It's going to be much harder to fix.

[jridgewell]

Actually, I have a fix.

[JLHwung]

Oh I meant we can add the new case and the fixes in a separate PR, my bad. As long as you come up with a fix, that's awesome! 😄

[nicolo-ribaudo]

@JLHwung Maybe it would be enough to remove this line to fix it?

[JLHwung]

I don't think so but I haven't tried. The nested private name is still allowed as long as it does not access outer class private names. I think we shall whitelist computed element names for proper resolving behaviour.

class Foo {
  #foo = 1;

  test() {
    class Nested {
      #foo = 2;
      bar = this.#foo;
    }

    new Nested;
  }
}
(new Foo).test();

[JLHwung]

Awesome!

[nicolo-ribaudo]

👍