fix(transformer/class-properties): correctly resolve private fields p…

…ointing to private methods

crates/oxc_transformer/src/es2022/class_properties/class.rs

@@ -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,14 +564,14 @@ 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 {
                 self.ctx.var_declarations.insert_var(binding, 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);
         }
 

crates/oxc_transformer/src/es2022/class_properties/class_details.rs

@@ -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,
 }

crates/oxc_transformer/src/es2022/class_properties/private_field.rs

@@ -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(

tasks/transform_conformance/snapshots/oxc.snap.md

@@ -1,6 +1,6 @@
 commit: 54a8389f
 
-Passed: 117/135
+Passed: 118/137
 
 # All Passed:
 * babel-plugin-transform-class-static-block
@@ -16,7 +16,7 @@ Passed: 117/135
 * regexp
 
 
-# babel-plugin-transform-class-properties (18/24)
+# babel-plugin-transform-class-properties (19/26)
 * interaction-with-other-transforms/input.js
 Bindings mismatch:
 after transform: ScopeId(0): ["C", "C2", "_ref", "_ref2"]
@@ -34,6 +34,9 @@ Symbol scope ID mismatch for "_g":
 after transform: SymbolId(6): ScopeId(1)
 rebuilt        : SymbolId(2): ScopeId(0)
 
+* 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)

tasks/transform_conformance/snapshots/oxc_exec.snap.md

@@ -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[email protected]/node_modules/@vitest/expect/dist/index.js:1438:21)
+    at Proxy.<anonymous> (./node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/expect/dist/index.js:923:17)
+    at Proxy.methodWrapper (./node_modules/.pnpm/[email protected]/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