refactor(semantic): improve ClassTable implmention and merge properties and methods to elements (#1902)

crates/oxc_linter/src/rules/eslint/no_unused_private_class_members.rs

@@ -7,6 +7,7 @@ use oxc_diagnostics::{
 use oxc_macros::declare_oxc_lint;
 use oxc_semantic::{AstNode, AstNodeId, AstNodes};
 use oxc_span::{Atom, Span};
+use oxc_syntax::class::ElementKind;
 
 use crate::{context::LintContext, rule::Rule};
 
@@ -92,34 +93,22 @@ declare_oxc_lint!(
 impl Rule for NoUnusedPrivateClassMembers {
     fn run_once(&self, ctx: &LintContext) {
         ctx.semantic().classes().iter_enumerated().for_each(|(class_id, _)| {
-            for (property_id, property) in
+            for (element_id, element) in
-                ctx.semantic().classes().properties[class_id].iter_enumerated()
+                ctx.semantic().classes().elements[class_id].iter_enumerated()
             {
-                if property.is_private
+                if !element.kind.intersects(ElementKind::Property | ElementKind::Method) {
+                    continue;
+                }
+                if element.is_private
                     && !ctx.semantic().classes().iter_private_identifiers(class_id).any(|ident| {
-                        is_read(ident.id, ctx.semantic().nodes())
+                        // If the element is a property, it must be read.
-                            && ident.property_id.is_some_and(|id| id == property_id)
+                        (!element.kind.is_property() || is_read(ident.id, ctx.semantic().nodes()))
+                            && ident.element_ids.contains(&element_id)
                     })
                 {
                     ctx.diagnostic(NoUnusedPrivateClassMembersDiagnostic(
-                        property.name.clone(),
+                        element.name.clone(),
-                        property.span,
+                        element.span,
-                    ));
-                }
-            }
-
-            for (method_id, method) in ctx.semantic().classes().methods[class_id].iter_enumerated()
-            {
-                if method.is_private
-                    && !ctx
-                        .semantic()
-                        .classes()
-                        .iter_private_identifiers(class_id)
-                        .any(|ident| ident.method_ids.contains(&method_id))
-                {
-                    ctx.diagnostic(NoUnusedPrivateClassMembersDiagnostic(
-                        method.name.clone(),
-                        method.span,
                     ));
                 }
             }

crates/oxc_semantic/src/checker/javascript.rs

@@ -294,8 +294,7 @@ fn check_private_identifier_outside_class(ident: &PrivateIdentifier, ctx: &Seman
 fn check_private_identifier(ctx: &SemanticBuilder<'_>) {
     if let Some(class_id) = ctx.class_table_builder.current_class_id {
         ctx.class_table_builder.classes.iter_private_identifiers(class_id).for_each(|reference| {
-            if reference.property_id.is_none()
+            if reference.element_ids.is_empty()
-                && reference.method_ids.is_empty()
                 && !ctx.class_table_builder.classes.ancestors(class_id).skip(1).any(|class_id| {
                     ctx.class_table_builder
                         .classes

crates/oxc_semantic/src/class/builder.rs

@@ -1,14 +1,17 @@
 use oxc_ast::{
-    ast::{ClassBody, ClassElement, MethodDefinition, PrivateIdentifier, PropertyDefinition},
+    ast::{
+        AccessorProperty, ClassBody, ClassElement, MethodDefinition, PrivateIdentifier,
+        PropertyDefinition,
+    },
     AstKind,
 };
 use oxc_span::GetSpan;
-use oxc_syntax::class::ClassId;
+use oxc_syntax::class::{ClassId, ElementKind};
 
 use crate::{AstNodeId, AstNodes};
 
 use super::{
-    table::{Method, PrivateIdentifierReference, Property},
+    table::{Element, PrivateIdentifierReference},
     ClassTable,
 };
 
@@ -44,11 +47,29 @@ impl ClassTableBuilder {
                 ClassElement::MethodDefinition(definition) => {
                     self.declare_class_method(definition.0);
                 }
+                ClassElement::AccessorProperty(definition) => {
+                    self.declare_class_accessor(definition.0);
+                }
                 _ => {}
             }
         }
     }
 
+    pub fn declare_class_accessor(&mut self, property: &AccessorProperty) {
+        let is_private = property.key.is_private_identifier();
+        let name =
+            if is_private { property.key.private_name() } else { property.key.static_name() };
+
+        if let Some(name) = name {
+            if let Some(class_id) = self.current_class_id {
+                self.classes.add_element(
+                    class_id,
+                    Element::new(name, property.key.span(), is_private, ElementKind::Property),
+                );
+            }
+        }
+    }
+
     pub fn declare_class_property(&mut self, property: &PropertyDefinition) {
         let is_private = property.key.is_private_identifier();
         let name =
@@ -56,8 +77,10 @@ impl ClassTableBuilder {
 
         if let Some(name) = name {
             if let Some(class_id) = self.current_class_id {
-                self.classes
+                self.classes.add_element(
-                    .add_property(class_id, Property::new(name, property.key.span(), is_private));
+                    class_id,
+                    Element::new(name, property.key.span(), is_private, ElementKind::Property),
+                );
             }
         }
     }
@@ -73,19 +96,13 @@ impl ClassTableBuilder {
             if matches!(parent_kind, AstKind::PrivateInExpression(_) | AstKind::MemberExpression(_))
             {
                 if let Some(class_id) = self.current_class_id {
-                    let property_id = self.classes.get_property_id(class_id, &ident.name);
+                    let element_ids = self.classes.get_element_ids(class_id, &ident.name);
-                    let method_ids = if property_id.is_some() {
-                        Vec::default()
-                    } else {
-                        self.classes.get_method_ids(class_id, &ident.name)
-                    };
 
                     let reference = PrivateIdentifierReference::new(
                         current_node_id,
                         ident.name.clone(),
                         ident.span,
-                        property_id,
+                        element_ids,
-                        method_ids,
                     );
                     self.classes.add_private_identifier_reference(class_id, reference);
                 }
@@ -102,9 +119,9 @@ impl ClassTableBuilder {
 
         if let Some(name) = name {
             if let Some(class_id) = self.current_class_id {
-                self.classes.add_method(
+                self.classes.add_element(
                     class_id,
-                    Method::new(name, method.key.span(), is_private, method.kind),
+                    Element::new(name, method.key.span(), is_private, ElementKind::Method),
                 );
             }
         }

crates/oxc_semantic/src/class/table.rs

@@ -1,34 +1,20 @@
-use oxc_ast::ast::MethodDefinitionKind;
 use oxc_index::IndexVec;
 use oxc_span::{Atom, Span};
-use oxc_syntax::class::{ClassId, MethodId, PropertyId};
+use oxc_syntax::class::{ClassId, ElementId, ElementKind};
 use rustc_hash::FxHashMap;
 
 use crate::node::AstNodeId;
 
 #[derive(Debug)]
-pub struct Property {
+pub struct Element {
     pub name: Atom,
     pub span: Span,
     pub is_private: bool,
+    pub kind: ElementKind,
 }
 
-impl Property {
+impl Element {
-    pub fn new(name: Atom, span: Span, is_private: bool) -> Self {
+    pub fn new(name: Atom, span: Span, is_private: bool, kind: ElementKind) -> Self {
-        Self { name, span, is_private }
-    }
-}
-
-#[derive(Debug)]
-pub struct Method {
-    pub name: Atom,
-    pub span: Span,
-    pub is_private: bool,
-    pub kind: MethodDefinitionKind,
-}
-
-impl Method {
-    pub fn new(name: Atom, span: Span, is_private: bool, kind: MethodDefinitionKind) -> Self {
         Self { name, span, is_private, kind }
     }
 }
@@ -38,20 +24,12 @@ pub struct PrivateIdentifierReference {
     pub id: AstNodeId,
     pub name: Atom,
     pub span: Span,
-    pub property_id: Option<PropertyId>,
+    pub element_ids: Vec<ElementId>,
-    /// If the private identifier is used in a get/set method, this will be has 2 method ids
-    pub method_ids: Vec<MethodId>,
 }
 
 impl PrivateIdentifierReference {
-    pub fn new(
+    pub fn new(id: AstNodeId, name: Atom, span: Span, element_ids: Vec<ElementId>) -> Self {
-        id: AstNodeId,
+        Self { id, name, span, element_ids }
-        name: Atom,
-        span: Span,
-        property_id: Option<PropertyId>,
-        method_ids: Vec<MethodId>,
-    ) -> Self {
-        Self { id, name, span, property_id, method_ids }
     }
 }
 
@@ -62,10 +40,7 @@ impl PrivateIdentifierReference {
 pub struct ClassTable {
     pub parent_ids: FxHashMap<ClassId, ClassId>,
     pub declarations: IndexVec<ClassId, AstNodeId>,
-    // PropertyDefinition
+    pub elements: IndexVec<ClassId, IndexVec<ElementId, Element>>,
-    pub properties: IndexVec<ClassId, IndexVec<PropertyId, Property>>,
-    // MethodDefinition
-    pub methods: IndexVec<ClassId, IndexVec<MethodId, Method>>,
     // PrivateIdentifier reference
     pub private_identifiers: IndexVec<ClassId, Vec<PrivateIdentifierReference>>,
 }
@@ -90,38 +65,29 @@ impl ClassTable {
         self.declarations[class_id]
     }
 
-    pub fn get_property_id(&self, class_id: ClassId, name: &Atom) -> Option<PropertyId> {
+    pub fn get_element_ids(&self, class_id: ClassId, name: &Atom) -> Vec<ElementId> {
-        self.properties[class_id].iter_enumerated().find_map(|(property_id, property)| {
+        let mut element_ids = vec![];
-            if property.name == *name {
+        for (element_id, element) in self.elements[class_id].iter_enumerated() {
-                Some(property_id)
+            if element.name == *name {
-            } else {
+                element_ids.push(element_id);
-                None
-            }
-        })
-    }
 
-    pub fn get_method_ids(&self, class_id: ClassId, name: &Atom) -> Vec<MethodId> {
+                // Property or Accessor only has 1 element
-        let mut method_ids = vec![];
+                if element.kind.intersects(ElementKind::Accessor | ElementKind::Property) {
-        for (method_id, method) in self.methods[class_id].iter_enumerated() {
-            if method.name == *name {
-                method_ids.push(method_id);
-                // Only have 1 method id for MethodDefinition::Method
-                if method.kind.is_method() {
                     break;
                 }
+
                 // Maximum 2 method ids, for get/set
-                if method_ids.len() == 2 {
+                if element_ids.len() == 2 {
                     break;
                 }
             }
         }
 
-        method_ids
+        element_ids
     }
 
     pub fn has_private_definition(&self, class_id: ClassId, name: &Atom) -> bool {
-        self.properties[class_id].iter().any(|p| p.is_private && p.name == *name)
+        self.elements[class_id].iter().any(|p| p.is_private && p.name == *name)
-            || self.methods[class_id].iter().any(|m| m.is_private && m.name == *name)
     }
 
     pub fn declare_class(&mut self, parent_id: Option<ClassId>, ast_node_id: AstNodeId) -> ClassId {
@@ -129,18 +95,13 @@ impl ClassTable {
         if let Some(parent_id) = parent_id {
             self.parent_ids.insert(class_id, parent_id);
         };
-        self.properties.push(IndexVec::default());
+        self.elements.push(IndexVec::default());
-        self.methods.push(IndexVec::default());
         self.private_identifiers.push(Vec::new());
         class_id
     }
 
-    pub fn add_property(&mut self, class_id: ClassId, property: Property) {
+    pub fn add_element(&mut self, class_id: ClassId, element: Element) {
-        self.properties[class_id].push(property);
+        self.elements[class_id].push(element);
-    }
-
-    pub fn add_method(&mut self, class_id: ClassId, method: Method) {
-        self.methods[class_id].push(method);
     }
 
     pub fn add_private_identifier_reference(

crates/oxc_semantic/tests/classes.rs

@@ -19,8 +19,25 @@ fn test_class_simple() {
     ",
     )
     .has_class("Foo")
-    .has_number_of_methods(3)
+    .has_number_of_elements(5)
-    .has_number_of_properties(2)
     .has_method("a")
     .has_property("privateProperty");
 }
+
+#[test]
+fn test_class_with_ts() {
+    SemanticTester::ts(
+        "
+      class Foo {
+        accessor ap = 1;
+        accessor #pap = 1;
+        constructor() {} // this method is skip
+      }
+        
+    ",
+    )
+    .has_class("Foo")
+    .has_number_of_elements(2)
+    .has_accessor("ap")
+    .has_accessor("pap");
+}

crates/oxc_semantic/tests/util/class_tester.rs

@@ -28,28 +28,33 @@ impl<'a> ClassTester<'a> {
         }
     }
 
-    pub fn has_number_of_properties(&self, len: usize) -> &Self {
+    pub fn has_number_of_elements(&self, len: usize) -> &Self {
-        let property_len = self.semantic.classes().properties[self.class_id].len();
+        let element_len = self.semantic.classes().elements[self.class_id].len();
-        debug_assert!(property_len == len, "Expected {len} properties, found {property_len}");
+        debug_assert!(element_len == len, "Expected {len} elements, found {element_len}");
-        self
-    }
-
-    pub fn has_number_of_methods(&self, len: usize) -> &Self {
-        let method_len = self.semantic.classes().methods[self.class_id].len();
-        debug_assert!(method_len == len, "Expected `{len}` methods, found {method_len}");
         self
     }
 
     pub fn has_property(&self, name: &str) -> &Self {
-        let property =
+        let property = self.semantic.classes().elements[self.class_id]
-            self.semantic.classes().properties[self.class_id].iter().find(|p| p.name == name);
+            .iter()
+            .find(|p| p.kind.is_property() && p.name == name);
         debug_assert!(property.is_some(), "Expected property `{name}` not found");
         self
     }
 
     pub fn has_method(&self, name: &str) -> &Self {
-        let method = self.semantic.classes().methods[self.class_id].iter().find(|m| m.name == name);
+        let method = self.semantic.classes().elements[self.class_id]
+            .iter()
+            .find(|m| m.kind.is_method() && m.name == name);
         debug_assert!(method.is_some(), "Expected method `{name}` not found");
         self
     }
+
+    pub fn has_accessor(&self, name: &str) -> &Self {
+        let method = self.semantic.classes().elements[self.class_id]
+            .iter()
+            .find(|m| m.kind.is_accessor() && m.name == name);
+        debug_assert!(method.is_some(), "Expected accessor `{name}` not found");
+        self
+    }
 }

crates/oxc_syntax/src/class.rs

@@ -1,11 +1,32 @@
+use bitflags::bitflags;
 use oxc_index::define_index_type;
 
 define_index_type! {
     pub struct ClassId = u32;
 }
 define_index_type! {
-    pub struct PropertyId = u32;
+    pub struct ElementId = u32;
 }
-define_index_type! {
+
-    pub struct MethodId = u32;
+bitflags! {
+    #[derive(Debug, Clone, Copy)]
+    pub struct ElementKind: u8 {
+        const Accessor = 0;
+        const Method = 1;
+        const Property = 2;
+    }
+}
+
+impl ElementKind {
+    pub fn is_property(self) -> bool {
+        self.contains(Self::Property)
+    }
+
+    pub fn is_method(self) -> bool {
+        self.contains(Self::Method)
+    }
+
+    pub fn is_accessor(self) -> bool {
+        self.contains(Self::Accessor)
+    }
 }

tasks/coverage/parser_typescript.snap

@@ -1,6 +1,6 @@
 parser_typescript Summary:
 AST Parsed     : 5201/5205 (99.92%)
-Positive Passed: 5192/5205 (99.75%)
+Positive Passed: 5194/5205 (99.79%)
 Negative Passed: 1023/4859 (21.05%)
 Expect Syntax Error: "compiler/ClassDeclaration10.ts"
 Expect Syntax Error: "compiler/ClassDeclaration11.ts"
@@ -3880,39 +3880,6 @@ Expect to Parse: "compiler/withStatementInternalComments.ts"
    ·       ────
    ╰────
 
-Expect to Parse: "conformance/classes/propertyMemberDeclarations/autoAccessor2.ts"
-  × Private field 'a' must be declared in an enclosing class
-    ╭─[conformance/classes/propertyMemberDeclarations/autoAccessor2.ts:9:1]
-  9 │     constructor() {
- 10 │         this.#a = 3;
-    ·              ──
- 11 │         this.#b = 4;
-    ╰────
-
-  × Private field 'b' must be declared in an enclosing class
-    ╭─[conformance/classes/propertyMemberDeclarations/autoAccessor2.ts:10:1]
- 10 │         this.#a = 3;
- 11 │         this.#b = 4;
-    ·              ──
- 12 │     }
-    ╰────
-
-  × Private field 'c' must be declared in an enclosing class
-    ╭─[conformance/classes/propertyMemberDeclarations/autoAccessor2.ts:14:1]
- 14 │     static {
- 15 │         this.#c = 5;
-    ·              ──
- 16 │         this.#d = 6;
-    ╰────
-
-  × Private field 'd' must be declared in an enclosing class
-    ╭─[conformance/classes/propertyMemberDeclarations/autoAccessor2.ts:15:1]
- 15 │         this.#c = 5;
- 16 │         this.#d = 6;
-    ·              ──
- 17 │     }
-    ╰────
-
 Expect to Parse: "conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts"
   × Classes may not have a static property named prototype
     ╭─[conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts:55:1]
@@ -3971,23 +3938,6 @@ Expect to Parse: "conformance/es6/moduleExportsSystem/topLevelVarHoistingCommonJ
  69 │     var y = _;
     ╰────
 
-Expect to Parse: "conformance/esDecorators/classDeclaration/fields/esDecorators-classDeclaration-fields-staticPrivateAccessor.ts"
-  × Private field 'field1' must be declared in an enclosing class
-    ╭─[conformance/esDecorators/classDeclaration/fields/esDecorators-classDeclaration-fields-staticPrivateAccessor.ts:14:1]
- 14 │     static {
- 15 │         this.#field1;
-    ·              ───────
- 16 │         this.#field1 = 1;
-    ╰────
-
-  × Private field 'field1' must be declared in an enclosing class
-    ╭─[conformance/esDecorators/classDeclaration/fields/esDecorators-classDeclaration-fields-staticPrivateAccessor.ts:15:1]
- 15 │         this.#field1;
- 16 │         this.#field1 = 1;
-    ·              ───────
- 17 │     }
-    ╰────
-
 Expect to Parse: "conformance/esDecorators/esDecorators-preservesThis.ts"
   × Unexpected token
     ╭─[conformance/esDecorators/esDecorators-preservesThis.ts:26:1]