fix(transformer/class-properties): correctly resolve private fields pointing to private methods (#8042)

We don't transform private methods yet, but in class properties transform, still need to record private methods that classes have, so can correctly resolve private fields.

```js
class Outer {
  #foo = 123;
  method() {
    class Inner {
      #foo() {}
      // Refers to `Inner`'s `#foo` method, not `Outer`'s `#foo` property
      prop = this.#foo;
    }
  }
}
```
This commit is contained in:
overlookmotel 2024-12-23 03:44:08 +00:00
parent 1932f1e0a0
commit 6b08c6e6c4
10 changed files with 547 additions and 52 deletions

View file

@ -92,7 +92,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
let binding = ctx.generate_uid_in_current_hoist_scope(&ident.name);
private_props.insert(
ident.name.clone(),
PrivateProp { binding, is_static: prop.r#static },
PrivateProp::new(binding, prop.r#static, false),
);
}
@ -112,10 +112,16 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
}
}
ClassElement::MethodDefinition(method) => {
if method.kind == MethodDefinitionKind::Constructor
&& method.value.body.is_some()
{
constructor = Some(method);
if method.kind == MethodDefinitionKind::Constructor {
if method.value.body.is_some() {
constructor = Some(method);
}
} else if let PropertyKey::PrivateIdentifier(ident) = &method.key {
let dummy_binding = BoundIdentifier::new(Atom::empty(), SymbolId::new(0));
private_props.insert(
ident.name.clone(),
PrivateProp::new(dummy_binding, method.r#static, true),
);
}
}
ClassElement::AccessorProperty(_) | ClassElement::TSIndexSignature(_) => {
@ -126,7 +132,12 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// Exit if nothing to transform
if instance_prop_count == 0 && !has_static_prop && !has_static_block {
self.classes_stack.push(ClassDetails::empty(is_declaration));
self.classes_stack.push(ClassDetails {
is_declaration,
is_transform_required: false,
private_props: if private_props.is_empty() { None } else { Some(private_props) },
bindings: ClassBindings::dummy(),
});
return;
}
@ -381,12 +392,17 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
if let Some(private_props) = &class_details.private_props {
if self.private_fields_as_properties {
// TODO: Only call `insert_many_before` if some private *props*
self.ctx.statement_injector.insert_many_before(
&stmt_address,
private_props.iter().map(|(name, prop)| {
private_props.iter().filter_map(|(name, prop)| {
if prop.is_method {
return None;
}
// `var _prop = _classPrivateFieldLooseKey("prop");`
let value = Self::create_private_prop_key_loose(name, self.ctx, ctx);
create_variable_declaration(&prop.binding, value, ctx)
Some(create_variable_declaration(&prop.binding, value, ctx))
}),
);
} else {
@ -395,7 +411,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
self.ctx.statement_injector.insert_many_before(
&stmt_address,
private_props.values().filter_map(|prop| {
if prop.is_static {
if prop.is_static || prop.is_method {
return None;
}
@ -482,8 +498,8 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// which can be very far above the class in AST, when it's a `var`.
// Maybe for now only add class name if it doesn't shadow a var used within class?
// TODO: Deduct static private props from `expr_count`.
// Or maybe should store count and increment it when create private static props?
// TODO: Deduct static private props and private methods from `expr_count`.
// Or maybe should store count and increment it when create private static props or private methods?
// They're probably pretty rare, so it'll be rarely used.
let class_details = self.classes_stack.last();
@ -511,17 +527,25 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// `c = class C { #x = 1; static y = 2; }` -> `var _C, _x;`
// TODO(improve-on-babel): Simplify this.
if self.private_fields_as_properties {
exprs.extend(private_props.iter().map(|(name, prop)| {
exprs.extend(private_props.iter().filter_map(|(name, prop)| {
if prop.is_method {
return None;
}
// Insert `var _prop;` declaration
self.ctx.var_declarations.insert_var(&prop.binding, ctx);
// `_prop = _classPrivateFieldLooseKey("prop")`
let value = Self::create_private_prop_key_loose(name, self.ctx, ctx);
create_assignment(&prop.binding, value, ctx)
Some(create_assignment(&prop.binding, value, ctx))
}));
} else {
let mut weakmap_symbol_id = None;
exprs.extend(private_props.values().filter_map(|prop| {
if prop.is_method {
return None;
}
// Insert `var _prop;` declaration
self.ctx.var_declarations.insert_var(&prop.binding, ctx);
@ -540,7 +564,6 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
exprs.extend(self.insert_before.drain(..));
// Insert class + static property assignments + static blocks
let class_expr = ctx.ast.move_expression(expr);
if let Some(binding) = &class_details.bindings.temp {
// Insert `var _Class` statement, if it wasn't already in entry phase
if !class_details.bindings.temp_var_is_created {
@ -548,6 +571,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
}
// `_Class = class {}`
let class_expr = ctx.ast.move_expression(expr);
let assignment = create_assignment(binding, class_expr, ctx);
exprs.push(assignment);
// Add static property assignments + static blocks
@ -564,6 +588,11 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// -> `let C = ((x = 1), class C extends Bound {});`
exprs.extend(self.insert_after_exprs.drain(..));
if exprs.is_empty() {
return;
}
let class_expr = ctx.ast.move_expression(expr);
exprs.push(class_expr);
}

View file

@ -23,10 +23,10 @@ pub(super) struct ClassDetails<'a> {
}
impl<'a> ClassDetails<'a> {
/// Create empty `ClassDetails`.
/// Create dummy `ClassDetails`.
///
/// Used when class needs no transform, and for dummy entry at top of `ClassesStack`.
pub fn empty(is_declaration: bool) -> Self {
/// Used for dummy entry at top of `ClassesStack`.
pub fn dummy(is_declaration: bool) -> Self {
Self {
is_declaration,
is_transform_required: false,
@ -40,6 +40,13 @@ impl<'a> ClassDetails<'a> {
pub(super) struct PrivateProp<'a> {
pub binding: BoundIdentifier<'a>,
pub is_static: bool,
pub is_method: bool,
}
impl<'a> PrivateProp<'a> {
pub fn new(binding: BoundIdentifier<'a>, is_static: bool, is_method: bool) -> Self {
Self { binding, is_static, is_method }
}
}
/// Stack of `ClassDetails`.
@ -61,7 +68,7 @@ impl<'a> ClassesStack<'a> {
/// Create new `ClassesStack`.
pub fn new() -> Self {
// Default stack capacity is 4. That's is probably good. More than 4 nested classes is rare.
Self { stack: NonEmptyStack::new(ClassDetails::empty(false)) }
Self { stack: NonEmptyStack::new(ClassDetails::dummy(false)) }
}
/// Push an entry to stack.
@ -92,24 +99,25 @@ impl<'a> ClassesStack<'a> {
pub fn find_private_prop<'b>(
&'b mut self,
ident: &PrivateIdentifier<'a>,
) -> Option<ResolvedPrivateProp<'a, 'b>> {
) -> ResolvedPrivateProp<'a, 'b> {
// Check for binding in closest class first, then enclosing classes.
// We skip the first, because this is a `NonEmptyStack` with dummy first entry.
// TODO: Check there are tests for bindings in enclosing classes.
for class in self.stack[1..].iter_mut().rev() {
if let Some(private_props) = &mut class.private_props {
if let Some(prop) = private_props.get(&ident.name) {
return Some(ResolvedPrivateProp {
return ResolvedPrivateProp {
prop_binding: &prop.binding,
class_bindings: &mut class.bindings,
is_static: prop.is_static,
is_method: prop.is_method,
is_declaration: class.is_declaration,
});
};
}
}
}
// TODO: This should be unreachable. Only returning `None` because implementation is incomplete.
None
unreachable!();
}
}
@ -123,6 +131,8 @@ pub(super) struct ResolvedPrivateProp<'a, 'b> {
pub class_bindings: &'b mut ClassBindings<'a>,
/// `true` if is a static property
pub is_static: bool,
/// `true` if is a private method
pub is_method: bool,
/// `true` if class which defines this property is a class declaration
pub is_declaration: bool,
}

View file

@ -48,13 +48,19 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
is_assignment: bool,
ctx: &mut TraverseCtx<'a>,
) -> Expression<'a> {
let prop_details = self.classes_stack.find_private_prop(&field_expr.field);
// TODO: Should never be `None` - only because implementation is incomplete.
let Some(prop_details) = prop_details else {
let ResolvedPrivateProp {
prop_binding,
class_bindings,
is_static,
is_method,
is_declaration,
} = self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
// TODO: Should not consume existing `PrivateFieldExpression` and then create a new one
// which is identical to the original
return Expression::PrivateFieldExpression(field_expr);
};
let ResolvedPrivateProp { prop_binding, class_bindings, is_static, is_declaration } =
prop_details;
// TODO: Move this to top of function once `lookup_private_property` does not return `Option`
let PrivateFieldExpression { span, object, .. } = field_expr.unbox();
@ -178,9 +184,12 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
if self.private_fields_as_properties {
// `object.#prop(arg)` -> `_classPrivateFieldLooseBase(object, _prop)[_prop](arg)`
let prop_details = self.classes_stack.find_private_prop(&field_expr.field);
// TODO: Should never be `None` - only because implementation is incomplete.
let Some(ResolvedPrivateProp { prop_binding, .. }) = prop_details else { return };
let ResolvedPrivateProp { prop_binding, is_method, .. } =
self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
return;
}
let object = ctx.ast.move_expression(&mut field_expr.object);
call_expr.callee = Expression::from(Self::create_private_field_member_expr_loose(
@ -193,7 +202,6 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
return;
}
// TODO: Should never be `None` - only because implementation is incomplete.
let Some((callee, object)) = self.transform_private_field_callee(field_expr, ctx) else {
return;
};
@ -249,9 +257,18 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
field_expr: &mut PrivateFieldExpression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Option<(Expression<'a>, Expression<'a>)> {
// TODO: Should never be `None` - only because implementation is incomplete.
let ResolvedPrivateProp { prop_binding, class_bindings, is_static, is_declaration } =
self.classes_stack.find_private_prop(&field_expr.field)?;
let ResolvedPrivateProp {
prop_binding,
class_bindings,
is_static,
is_method,
is_declaration,
} = self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
return None;
};
let prop_ident = prop_binding.create_read_expression(ctx);
// `(object.#method)()`
@ -345,11 +362,17 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
unreachable!()
};
let prop_details = self.classes_stack.find_private_prop(&field_expr.field);
// TODO: Should never be `None` - only because implementation is incomplete.
let Some(prop_details) = prop_details else { return };
let ResolvedPrivateProp { prop_binding, class_bindings, is_static, is_declaration } =
prop_details;
let ResolvedPrivateProp {
prop_binding,
class_bindings,
is_static,
is_method,
is_declaration,
} = self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
return;
};
if self.private_fields_as_properties {
// `object.#prop = value` -> `_classPrivateFieldLooseBase(object, _prop)[_prop] = value`
@ -724,11 +747,17 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
_ => unreachable!(),
};
let prop_details = self.classes_stack.find_private_prop(&field_expr.field);
// TODO: Should never be `None` - only because implementation is incomplete.
let Some(prop_details) = prop_details else { return };
let ResolvedPrivateProp { prop_binding, class_bindings, is_static, is_declaration } =
prop_details;
let ResolvedPrivateProp {
prop_binding,
class_bindings,
is_static,
is_method,
is_declaration,
} = self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
return;
};
if self.private_fields_as_properties {
// `object.#prop++` -> `_classPrivateFieldLooseBase(object, _prop)[_prop]++`
@ -1518,10 +1547,12 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// // ^^^^^^^^^^^^^
// ```
// But this is not needed, so we omit it.
//
// TODO: Should never be `None` - only because implementation is incomplete.
let ResolvedPrivateProp { prop_binding, .. } =
self.classes_stack.find_private_prop(&field_expr.field)?;
let ResolvedPrivateProp { prop_binding, is_method, .. } =
self.classes_stack.find_private_prop(&field_expr.field);
if is_method {
return None;
}
let object = ctx.ast.move_expression(&mut field_expr.object);
let replacement = Self::create_private_field_member_expr_loose(

View file

@ -1,6 +1,6 @@
commit: 54a8389f
Passed: 119/136
Passed: 120/138
# All Passed:
* babel-plugin-transform-class-static-block
@ -16,7 +16,10 @@ Passed: 119/136
* regexp
# babel-plugin-transform-class-properties (19/24)
# babel-plugin-transform-class-properties (20/26)
* private-field-resolve-to-method-in-computed-key/input.js
x Output mismatch
* static-block-this-and-class-name/input.js
Symbol flags mismatch for "inner":
after transform: SymbolId(8): SymbolFlags(BlockScopedVariable | Function)

View file

@ -2,10 +2,17 @@ commit: 54a8389f
node: v22.12.0
Passed: 3 of 4 (75.00%)
Passed: 3 of 5 (60.00%)
Failures:
./fixtures/oxc/babel-plugin-transform-class-properties-test-fixtures-private-field-resolve-to-method-in-computed-key-exec.test.js
AssertionError: expected [Function] to throw an error
at Proxy.<anonymous> (./node_modules/.pnpm/@vitest+expect@2.1.2/node_modules/@vitest/expect/dist/index.js:1438:21)
at Proxy.<anonymous> (./node_modules/.pnpm/@vitest+expect@2.1.2/node_modules/@vitest/expect/dist/index.js:923:17)
at Proxy.methodWrapper (./node_modules/.pnpm/chai@5.1.2/node_modules/chai/chai.js:1610:25)
at ./tasks/transform_conformance/fixtures/oxc/babel-plugin-transform-class-properties-test-fixtures-private-field-resolve-to-method-in-computed-key-exec.test.js:84:33
./fixtures/oxc/babel-plugin-transform-class-properties-test-fixtures-static-super-tagged-template-exec.test.js
AssertionError: expected undefined to be [Function C] // Object.is equality
at ./tasks/transform_conformance/fixtures/oxc/babel-plugin-transform-class-properties-test-fixtures-static-super-tagged-template-exec.test.js:15:17

View file

@ -0,0 +1,86 @@
class Outer {
#prop = 1;
#shadowed() {
return 2;
}
getInner() {
class Inner {
#shadowed() {
return 3;
}
// Refers to `Outer`s private property - transform
[this.#prop] = 4;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 5;
}
return Inner;
}
getInnerWithAdditionalProp() {
class InnerWithAdditionalProp {
#shadowed() {
return 6;
}
#innerProp = 7;
// Refers to `Outer`s private property - transform
[this.#prop] = 8;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 9;
}
return InnerWithAdditionalProp;
}
}
let OuterExpr = class {
#prop = 1;
#shadowed() {
return 2;
}
getInner() {
return class InnerExpr {
#shadowed() {
return 3;
}
// Refers to `Outer`s private property - transform
[this.#prop] = 4;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 5;
};
}
getInnerWithAdditionalProp() {
return class InnerExprWithAdditionalProp {
#shadowed() {
return 6;
}
#innerProp = 7;
// Refers to `Outer`s private property - transform
[this.#prop] = 8;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 9;
};
}
};
const outer = new Outer();
expect(() => outer.getInner()).toThrowError("Receiver must be an instance of class Inner");
expect(() => outer.getInnerWithAdditionalProp()).toThrowError(
"Receiver must be an instance of class InnerWithAdditionalProp"
);
const outerExpr = new OuterExpr();
expect(() => outerExpr.getInner()).toThrowError("Receiver must be an instance of class InnerExpr");
expect(() => outerExpr.getInnerWithAdditionalProp()).toThrowError(
"Receiver must be an instance of class InnerExprWithAdditionalProp"
);

View file

@ -0,0 +1,74 @@
class Outer {
#prop = 1;
#shadowed() {
return 2;
}
getInner() {
class Inner {
#shadowed() {
return 3;
}
// Refers to `Outer`s private property - transform
[this.#prop] = 4;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 5;
}
return Inner;
}
getInnerWithAdditionalProp() {
class InnerWithAdditionalProp {
#shadowed() {
return 6;
}
#innerProp = 7;
// Refers to `Outer`s private property - transform
[this.#prop] = 8;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 9;
}
return InnerWithAdditionalProp;
}
}
let OuterExpr = class {
#prop = 1;
#shadowed() {
return 2;
}
getInner() {
return class InnerExpr {
#shadowed() {
return 3;
}
// Refers to `Outer`s private property - transform
[this.#prop] = 4;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 5;
};
}
getInnerWithAdditionalProp() {
return class InnerExprWithAdditionalProp {
#shadowed() {
return 6;
}
#innerProp = 7;
// Refers to `Outer`s private property - transform
[this.#prop] = 8;
// Refers to `Inner`s private method - don't transform
[this.#shadowed] = 9;
};
}
};

View file

@ -0,0 +1,114 @@
var _prop2;
var _prop = new WeakMap();
class Outer {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _prop, 1);
}
#shadowed() {
return 2;
}
getInner() {
let _this$prop, _this$shadowed;
_this$prop = babelHelpers.classPrivateFieldGet2(_prop, this);
// This isn't an existing helper. We need to add a helper that throws error.
// Problem only arises when class properties transform enabled and private methods transform isn't.
_this$shadowed = babelHelpers.illegalPrivateField("shadowed");
class Inner {
constructor() {
babelHelpers.defineProperty(this, _this$prop, 4);
babelHelpers.defineProperty(this, _this$shadowed, 5);
}
#shadowed() {
return 3;
}
}
return Inner;
}
getInnerWithAdditionalProp() {
let _this$prop2, _this$shadowed2;
var _innerProp = new WeakMap();
_this$prop2 = babelHelpers.classPrivateFieldGet2(_prop, this);
// This isn't an existing helper. We need to add a helper that throws error.
// Problem only arises when class properties transform enabled and private methods transform isn't.
_this$shadowed2 = babelHelpers.illegalPrivateField("shadowed");
class InnerWithAdditionalProp {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _innerProp, 7);
babelHelpers.defineProperty(this, _this$prop2, 8);
babelHelpers.defineProperty(this, _this$shadowed2, 9);
}
#shadowed() {
return 6;
}
}
return InnerWithAdditionalProp;
}
}
let OuterExpr = (
_prop2 = new WeakMap(),
class {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _prop2, 1);
}
#shadowed() {
return 2;
}
getInner() {
let _this$prop3, _this$shadowed3;
return (
_this$prop3 = babelHelpers.classPrivateFieldGet2(_prop2, this),
// This isn't an existing helper. We need to add a helper that throws error.
// Problem only arises when class properties transform enabled and private methods transform isn't.
_this$shadowed3 = babelHelpers.illegalPrivateField("shadowed"),
class InnerExpr {
constructor() {
babelHelpers.defineProperty(this, _this$prop3, 4);
babelHelpers.defineProperty(this, _this$shadowed3, 5);
}
#shadowed() {
return 3;
}
}
);
}
getInnerWithAdditionalProp() {
var _innerProp2;
let _this$prop4, _this$shadowed4;
return (
_innerProp2 = new WeakMap(),
_this$prop4 = babelHelpers.classPrivateFieldGet2(_prop2, this),
// This isn't an existing helper. We need to add a helper that throws error.
// Problem only arises when class properties transform enabled and private methods transform isn't.
_this$shadowed4 = babelHelpers.illegalPrivateField("shadowed"),
class InnerExprWithAdditionalProp {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _innerProp2, 7);
babelHelpers.defineProperty(this, _this$prop4, 8);
babelHelpers.defineProperty(this, _this$shadowed4, 9);
}
#shadowed() {
return 6;
}
}
);
}
}
);

View file

@ -0,0 +1,61 @@
class Outer {
#prop = 1;
#shadowed = 2;
method() {
class Inner {
#shadowed() {}
method() {
// Refers to `Outer`s private property - transform
this.#prop;
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
}
class InnerWithAdditionalProp {
#shadowed() {}
#innerProp = 3;
method() {
// Refers to `Outer`s private property - transform
this.#prop;
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
}
}
}
let OuterExpr = class {
#prop = 1;
#shadowed = 2;
method() {
let InnerExpr = class {
#shadowed() {}
method() {
// Refers to `Outer`s private property - transform
this.#prop;
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
};
let InnerExprWithAdditionalProp = class {
#shadowed() {}
#innerProp = 3;
method() {
// Refers to `Outer`s private property - transform
this.#prop;
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
};
}
};

View file

@ -0,0 +1,80 @@
var _prop2, _shadowed2;
var _prop = new WeakMap();
var _shadowed = new WeakMap();
class Outer {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _prop, 1);
babelHelpers.classPrivateFieldInitSpec(this, _shadowed, 2);
}
method() {
class Inner {
#shadowed() {}
method() {
// Refers to `Outer`s private property - transform
babelHelpers.classPrivateFieldGet2(_prop, this);
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
}
var _innerProp = new WeakMap();
class InnerWithAdditionalProp {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _innerProp, 3);
}
#shadowed() {}
method() {
// Refers to `Outer`s private property - transform
babelHelpers.classPrivateFieldGet2(_prop, this);
// Refers to `Inner`s private method - don't transform
this.#shadowed;
}
}
}
}
let OuterExpr = (
_prop2 = new WeakMap(),
_shadowed2 = new WeakMap(),
class {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _prop2, 1);
babelHelpers.classPrivateFieldInitSpec(this, _shadowed2, 2);
}
method() {
var _innerProp2;
let InnerExpr = class {
#shadowed() {}
method() {
babelHelpers.classPrivateFieldGet2(_prop2, this);
this.#shadowed;
}
};
let InnerExprWithAdditionalProp = (
_innerProp2 = new WeakMap(),
class {
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _innerProp2, 3);
}
#shadowed() {}
method() {
babelHelpers.classPrivateFieldGet2(_prop2, this);
this.#shadowed;
}
}
);
}
}
);