From 6c5b22f728aaa3e35305041ce0decd1e32e6fa6c Mon Sep 17 00:00:00 2001 From: Dunqing Date: Fri, 5 Jan 2024 18:38:51 +0800 Subject: [PATCH] refactor(semantic): improve ClassTable implmention and merge properties and methods to elements (#1902) --- .../eslint/no_unused_private_class_members.rs | 35 +++----- crates/oxc_semantic/src/checker/javascript.rs | 3 +- crates/oxc_semantic/src/class/builder.rs | 47 ++++++---- crates/oxc_semantic/src/class/table.rs | 85 +++++-------------- crates/oxc_semantic/tests/classes.rs | 21 ++++- .../oxc_semantic/tests/util/class_tester.rs | 29 ++++--- crates/oxc_syntax/src/class.rs | 27 +++++- tasks/coverage/parser_typescript.snap | 52 +----------- 8 files changed, 129 insertions(+), 170 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_private_class_members.rs b/crates/oxc_linter/src/rules/eslint/no_unused_private_class_members.rs index 4f84a119a..408942e5d 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_private_class_members.rs +++ b/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 - ctx.semantic().classes().properties[class_id].iter_enumerated() + for (element_id, element) in + 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()) - && ident.property_id.is_some_and(|id| id == property_id) + // If the element is a property, it must be read. + (!element.kind.is_property() || is_read(ident.id, ctx.semantic().nodes())) + && ident.element_ids.contains(&element_id) }) { ctx.diagnostic(NoUnusedPrivateClassMembersDiagnostic( - property.name.clone(), - property.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, + element.name.clone(), + element.span, )); } } diff --git a/crates/oxc_semantic/src/checker/javascript.rs b/crates/oxc_semantic/src/checker/javascript.rs index ecf10feda..ae4e224ce 100644 --- a/crates/oxc_semantic/src/checker/javascript.rs +++ b/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() - && reference.method_ids.is_empty() + if reference.element_ids.is_empty() && !ctx.class_table_builder.classes.ancestors(class_id).skip(1).any(|class_id| { ctx.class_table_builder .classes diff --git a/crates/oxc_semantic/src/class/builder.rs b/crates/oxc_semantic/src/class/builder.rs index 5fa885d21..240478653 100644 --- a/crates/oxc_semantic/src/class/builder.rs +++ b/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 - .add_property(class_id, Property::new(name, property.key.span(), is_private)); + self.classes.add_element( + 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 method_ids = if property_id.is_some() { - Vec::default() - } else { - self.classes.get_method_ids(class_id, &ident.name) - }; + let element_ids = self.classes.get_element_ids(class_id, &ident.name); let reference = PrivateIdentifierReference::new( current_node_id, ident.name.clone(), ident.span, - property_id, - method_ids, + element_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), ); } } diff --git a/crates/oxc_semantic/src/class/table.rs b/crates/oxc_semantic/src/class/table.rs index 7df5ef1de..56078bef9 100644 --- a/crates/oxc_semantic/src/class/table.rs +++ b/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 { - pub fn new(name: Atom, span: Span, is_private: bool) -> 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 { +impl Element { + pub fn new(name: Atom, span: Span, is_private: bool, kind: ElementKind) -> 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, - /// If the private identifier is used in a get/set method, this will be has 2 method ids - pub method_ids: Vec, + pub element_ids: Vec, } impl PrivateIdentifierReference { - pub fn new( - id: AstNodeId, - name: Atom, - span: Span, - property_id: Option, - method_ids: Vec, - ) -> Self { - Self { id, name, span, property_id, method_ids } + pub fn new(id: AstNodeId, name: Atom, span: Span, element_ids: Vec) -> Self { + Self { id, name, span, element_ids } } } @@ -62,10 +40,7 @@ impl PrivateIdentifierReference { pub struct ClassTable { pub parent_ids: FxHashMap, pub declarations: IndexVec, - // PropertyDefinition - pub properties: IndexVec>, - // MethodDefinition - pub methods: IndexVec>, + pub elements: IndexVec>, // PrivateIdentifier reference pub private_identifiers: IndexVec>, } @@ -90,38 +65,29 @@ impl ClassTable { self.declarations[class_id] } - pub fn get_property_id(&self, class_id: ClassId, name: &Atom) -> Option { - self.properties[class_id].iter_enumerated().find_map(|(property_id, property)| { - if property.name == *name { - Some(property_id) - } else { - None - } - }) - } + pub fn get_element_ids(&self, class_id: ClassId, name: &Atom) -> Vec { + let mut element_ids = vec![]; + for (element_id, element) in self.elements[class_id].iter_enumerated() { + if element.name == *name { + element_ids.push(element_id); - pub fn get_method_ids(&self, class_id: ClassId, name: &Atom) -> Vec { - let mut method_ids = vec![]; - 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() { + // Property or Accessor only has 1 element + if element.kind.intersects(ElementKind::Accessor | ElementKind::Property) { 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.methods[class_id].iter().any(|m| m.is_private && m.name == *name) + self.elements[class_id].iter().any(|p| p.is_private && p.name == *name) } pub fn declare_class(&mut self, parent_id: Option, 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.methods.push(IndexVec::default()); + self.elements.push(IndexVec::default()); self.private_identifiers.push(Vec::new()); class_id } - pub fn add_property(&mut self, class_id: ClassId, property: Property) { - self.properties[class_id].push(property); - } - - pub fn add_method(&mut self, class_id: ClassId, method: Method) { - self.methods[class_id].push(method); + pub fn add_element(&mut self, class_id: ClassId, element: Element) { + self.elements[class_id].push(element); } pub fn add_private_identifier_reference( diff --git a/crates/oxc_semantic/tests/classes.rs b/crates/oxc_semantic/tests/classes.rs index 719a3562e..13792566c 100644 --- a/crates/oxc_semantic/tests/classes.rs +++ b/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_properties(2) + .has_number_of_elements(5) .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"); +} diff --git a/crates/oxc_semantic/tests/util/class_tester.rs b/crates/oxc_semantic/tests/util/class_tester.rs index 9f862a1ec..8501f9ee7 100644 --- a/crates/oxc_semantic/tests/util/class_tester.rs +++ b/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 { - let property_len = self.semantic.classes().properties[self.class_id].len(); - debug_assert!(property_len == len, "Expected {len} properties, found {property_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}"); + pub fn has_number_of_elements(&self, len: usize) -> &Self { + let element_len = self.semantic.classes().elements[self.class_id].len(); + debug_assert!(element_len == len, "Expected {len} elements, found {element_len}"); self } pub fn has_property(&self, name: &str) -> &Self { - let property = - self.semantic.classes().properties[self.class_id].iter().find(|p| p.name == name); + let property = self.semantic.classes().elements[self.class_id] + .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 + } } diff --git a/crates/oxc_syntax/src/class.rs b/crates/oxc_syntax/src/class.rs index a7e734def..31d0b13d6 100644 --- a/crates/oxc_syntax/src/class.rs +++ b/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) + } } diff --git a/tasks/coverage/parser_typescript.snap b/tasks/coverage/parser_typescript.snap index 847335d00..3c60c3ee1 100644 --- a/tasks/coverage/parser_typescript.snap +++ b/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]