From 4aea2b158cdaa60e8a1d94f1fb9633c1bd8e204f Mon Sep 17 00:00:00 2001 From: Dunqing <29533304+Dunqing@users.noreply.github.com> Date: Wed, 19 Jun 2024 14:06:01 +0000 Subject: [PATCH] feat(isolated-declarations): improve inferring the type of accessor (#3749) --- crates/oxc_isolated_declarations/src/class.rs | 226 +++++++++++------- .../tests/fixtures/set-get-accessor.ts | 26 ++ .../tests/snapshots/set-get-accessor.snap | 30 +++ 3 files changed, 200 insertions(+), 82 deletions(-) create mode 100644 crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts create mode 100644 crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap diff --git a/crates/oxc_isolated_declarations/src/class.rs b/crates/oxc_isolated_declarations/src/class.rs index 14214d40b42b5..7dc466828b5fb 100644 --- a/crates/oxc_isolated_declarations/src/class.rs +++ b/crates/oxc_isolated_declarations/src/class.rs @@ -31,8 +31,8 @@ impl<'a> IsolatedDeclarations<'a> { } pub fn report_property_key(&self, key: &PropertyKey<'a>, computed: bool) -> bool { - if computed && self.is_literal_key(key) { - computed_property_name(key.span()); + if computed && !self.is_literal_key(key) { + self.error(computed_property_name(key.span())); true } else { false @@ -50,7 +50,7 @@ impl<'a> IsolatedDeclarations<'a> { } } - pub fn transform_class_property_definition( + fn transform_class_property_definition( &self, property: &PropertyDefinition<'a>, ) -> ClassElement<'a> { @@ -94,7 +94,7 @@ impl<'a> IsolatedDeclarations<'a> { ) } - pub fn transform_class_method_definition( + fn transform_class_method_definition( &self, definition: &MethodDefinition<'a>, params: Box<'a, FormalParameters<'a>>, @@ -102,23 +102,6 @@ impl<'a> IsolatedDeclarations<'a> { ) -> ClassElement<'a> { let function = &definition.value; - if definition.accessibility.is_some_and(|a| a.is_private()) { - let r#type = match definition.r#type { - MethodDefinitionType::MethodDefinition => { - PropertyDefinitionType::PropertyDefinition - } - MethodDefinitionType::TSAbstractMethodDefinition => { - PropertyDefinitionType::TSAbstractPropertyDefinition - } - }; - return self.create_class_property( - r#type, - self.ast.copy(&definition.key), - definition.r#override, - self.transform_accessibility(definition.accessibility), - ); - } - let value = self.ast.function( FunctionType::TSEmptyBodyFunctionExpression, function.span, @@ -148,7 +131,7 @@ impl<'a> IsolatedDeclarations<'a> { ) } - pub fn create_class_property( + fn create_class_property( &self, r#type: PropertyDefinitionType, key: PropertyKey<'a>, @@ -173,7 +156,7 @@ impl<'a> IsolatedDeclarations<'a> { ) } - pub fn transform_formal_parameter_to_class_property( + fn transform_formal_parameter_to_class_property( &self, param: &FormalParameter<'a>, type_annotation: Option>>, @@ -201,6 +184,69 @@ impl<'a> IsolatedDeclarations<'a> { )) } + fn transform_private_modifier_method(&self, method: &MethodDefinition<'a>) -> ClassElement<'a> { + match method.kind { + MethodDefinitionKind::Method => { + let r#type = match method.r#type { + MethodDefinitionType::MethodDefinition => { + PropertyDefinitionType::PropertyDefinition + } + MethodDefinitionType::TSAbstractMethodDefinition => { + PropertyDefinitionType::TSAbstractPropertyDefinition + } + }; + self.create_class_property( + r#type, + self.ast.copy(&method.key), + method.r#override, + self.transform_accessibility(method.accessibility), + ) + } + MethodDefinitionKind::Get => { + let params = self.ast.formal_parameters( + SPAN, + FormalParameterKind::Signature, + self.ast.new_vec(), + None, + ); + self.transform_class_method_definition(method, params, None) + } + MethodDefinitionKind::Set => { + let params = self.create_formal_parameters( + self.ast.binding_pattern_identifier(BindingIdentifier::new( + SPAN, + self.ast.new_atom("value"), + )), + None, + ); + self.transform_class_method_definition(method, params, None) + } + MethodDefinitionKind::Constructor => { + unreachable!() + } + } + } + + fn transform_constructor_params_to_class_properties( + &self, + function: &Function<'a>, + params: &FormalParameters<'a>, + ) -> oxc_allocator::Vec<'a, ClassElement<'a>> { + let mut elements = self.ast.new_vec(); + for (index, param) in function.params.items.iter().enumerate() { + if param.accessibility.is_some() { + // transformed params will definitely have type annotation + let type_annotation = self.ast.copy(¶ms.items[index].pattern.type_annotation); + if let Some(new_element) = + self.transform_formal_parameter_to_class_property(param, type_annotation) + { + elements.push(new_element); + } + } + } + elements + } + pub fn transform_class(&self, decl: &Class<'a>) -> Option>> { if decl.is_declare() { return None; @@ -219,52 +265,61 @@ impl<'a> IsolatedDeclarations<'a> { } } - let mut elements = self.ast.new_vec(); - let mut has_private_key = false; - let mut accessor_return_types: FxHashMap<&Atom<'a>, Option>>> = + let mut inferred_accessor_type: FxHashMap, Box<'a, TSTypeAnnotation<'a>>> = FxHashMap::default(); - // Transform get accessor first, and collect return type. - // The return type will be used to infer the type of the set accessor. + // Infer get accessor return type from set accessor + // Infer set accessor parameter type from get accessor for element in &decl.body.body { if let ClassElement::MethodDefinition(method) = element { - if method.key.is_private_identifier() { - has_private_key = true; + if method.key.is_private_identifier() + || method.accessibility.is_some_and(|a| a.is_private()) + || (method.computed && !self.is_literal_key(&method.key)) + { continue; } - if self.report_property_key(&method.key, method.computed) { + let Some(name) = method.key.static_name() else { + continue; + }; + let name = self.ast.new_atom(&name); + if inferred_accessor_type.contains_key(&name) { + // We've inferred that accessor type already continue; } - - if method.kind.is_get() { - if let PropertyKey::StaticIdentifier(ident) = &method.key { - let function = &method.value; - let params = self.transform_formal_parameters(&function.params); + let function = &method.value; + match method.kind { + MethodDefinitionKind::Get => { let return_type = self.infer_function_return_type(function); - if return_type.is_none() { - self.error(accessor_must_have_explicit_return_type(method.key.span())); + if let Some(return_type) = return_type { + inferred_accessor_type.insert(name, self.ast.copy(&return_type)); } - accessor_return_types.insert(&ident.name, self.ast.copy(&return_type)); - let element = - self.transform_class_method_definition(method, params, return_type); - elements.push(element); - continue; } + MethodDefinitionKind::Set => { + if let Some(param) = function.params.items.first() { + let type_annotation = + param.pattern.type_annotation.as_ref().map_or_else( + || { + self.infer_type_from_formal_parameter(param) + .map(|x| self.ast.ts_type_annotation(SPAN, x)) + }, + |t| Some(self.ast.copy(t)), + ); + if let Some(type_annotation) = type_annotation { + inferred_accessor_type.insert(name, type_annotation); + } + } + } + _ => {} } } - elements.push(self.ast.copy(element)); } - let mut new_elements = self.ast.new_vec(); - for element in elements.drain(..) { + let mut has_private_key = false; + let mut elements = self.ast.new_vec(); + for element in &decl.body.body { match element { ClassElement::StaticBlock(_) => {} ClassElement::MethodDefinition(ref method) => { - // Transformed in the first loop - if method.kind.is_get() { - new_elements.push(element); - continue; - } if method.key.is_private_identifier() { has_private_key = true; continue; @@ -272,36 +327,33 @@ impl<'a> IsolatedDeclarations<'a> { if self.report_property_key(&method.key, method.computed) { continue; } + if method.accessibility.is_some_and(|a| a.is_private()) { + elements.push(self.transform_private_modifier_method(method)); + continue; + } let function = &method.value; let params = if method.kind.is_set() { - if let PropertyKey::StaticIdentifier(ident) = &method.key { - self.transform_set_accessor_params( - &function.params, - accessor_return_types.remove(&ident.name).unwrap_or_default(), - ) - } else { - self.transform_formal_parameters(&function.params) - } + method.key.static_name().map_or_else( + || self.transform_formal_parameters(&function.params), + |n| { + self.transform_set_accessor_params( + &function.params, + inferred_accessor_type + .get(&self.ast.new_atom(&n)) + .map(|t| self.ast.copy(t)), + ) + }, + ) } else { self.transform_formal_parameters(&function.params) }; if let MethodDefinitionKind::Constructor = method.kind { - for (index, param) in function.params.items.iter().enumerate() { - if param.accessibility.is_some() { - // transformed params will definitely have type annotation - let type_annotation = - self.ast.copy(¶ms.items[index].pattern.type_annotation); - if let Some(new_element) = self - .transform_formal_parameter_to_class_property( - param, - type_annotation, - ) - { - new_elements.push(new_element); - } - } - } + elements.extend( + self.transform_constructor_params_to_class_properties( + function, ¶ms, + ), + ); } let return_type = match method.kind { @@ -314,14 +366,24 @@ impl<'a> IsolatedDeclarations<'a> { } rt } - MethodDefinitionKind::Set | MethodDefinitionKind::Constructor => None, MethodDefinitionKind::Get => { - unreachable!("get accessor should be transformed in the first loop") + let rt = method.key.static_name().and_then(|name| { + inferred_accessor_type + .get(&self.ast.new_atom(&name)) + .map(|t| self.ast.copy(t)) + }); + if rt.is_none() { + self.error(accessor_must_have_explicit_return_type( + method.key.span(), + )); + } + rt } + MethodDefinitionKind::Set | MethodDefinitionKind::Constructor => None, }; let new_element = self.transform_class_method_definition(method, params, return_type); - new_elements.push(new_element); + elements.push(new_element); } ClassElement::PropertyDefinition(property) => { if self.report_property_key(&property.key, property.computed) { @@ -331,7 +393,7 @@ impl<'a> IsolatedDeclarations<'a> { if property.key.is_private_identifier() { has_private_key = true; } else { - new_elements.push(self.transform_class_property_definition(&property)); + elements.push(self.transform_class_property_definition(property)); } } ClassElement::AccessorProperty(property) => { @@ -354,9 +416,9 @@ impl<'a> IsolatedDeclarations<'a> { property.r#static, self.ast.new_vec(), ); - new_elements.push(new_element); + elements.push(new_element); } - ClassElement::TSIndexSignature(_) => new_elements.push(element), + ClassElement::TSIndexSignature(_) => elements.push(self.ast.copy(element)), } } @@ -374,10 +436,10 @@ impl<'a> IsolatedDeclarations<'a> { None, decorators, ); - new_elements.insert(0, element); + elements.insert(0, element); } - let body = self.ast.class_body(decl.body.span, new_elements); + let body = self.ast.class_body(decl.body.span, elements); let mut modifiers = self.modifiers_declare(); if decl.modifiers.is_contains_abstract() { diff --git a/crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts b/crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts new file mode 100644 index 0000000000000..5884d615ffae4 --- /dev/null +++ b/crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts @@ -0,0 +1,26 @@ +// Correct +class Cls { + get a() { + return 1; + } + set a() { + return; + } + + get b(): string { + } + set b(v) { + } + + private get c() {} + private set c() {} +} + +// Incorrect +class ClsBad { + get a() { + return; + } + set a(v) { + } +} \ No newline at end of file diff --git a/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap b/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap new file mode 100644 index 0000000000000..0ab6aebbd938b --- /dev/null +++ b/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap @@ -0,0 +1,30 @@ +--- +source: crates/oxc_isolated_declarations/tests/mod.rs +input_file: crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts +--- +==================== .D.TS ==================== + +declare class Cls { + get a(): number; + set a(value: number); + get b(): string; + set b(v: string); + private get c(); + private set c(value); +} +declare class ClsBad { + get a(); + set a(v); +} + + +==================== Errors ==================== + + x At least one accessor must have an explicit return type annotation with + | --isolatedDeclarations. + ,-[21:7] + 20 | class ClsBad { + 21 | get a() { + : ^ + 22 | return; + `----