diff --git a/crates/oxc_angular_compiler/src/class_metadata/builders.rs b/crates/oxc_angular_compiler/src/class_metadata/builders.rs index f074a2c62..edeb2ece3 100644 --- a/crates/oxc_angular_compiler/src/class_metadata/builders.rs +++ b/crates/oxc_angular_compiler/src/class_metadata/builders.rs @@ -6,8 +6,8 @@ use oxc_allocator::{Allocator, Box, Vec as AllocVec}; use oxc_ast::ast::{ BindingPattern, ChainElement, Class, ClassElement, Declaration, Decorator, Expression, - FormalParameter, MethodDefinitionKind, ObjectPropertyKind, PropertyKey, Statement, - TSNamespaceDeclarationBody, TSType, TSTypeName, VariableDeclaration, + FormalParameter, ObjectPropertyKind, PropertyKey, Statement, TSNamespaceDeclarationBody, + TSType, TSTypeName, VariableDeclaration, }; use oxc_span::GetSpan; use oxc_str::Ident; @@ -440,14 +440,7 @@ pub fn build_ctor_params_metadata_in<'a>( // purely from the type annotation (`typeToValue`), never the DI token. let _ = constructor_deps; // Find constructor - let constructor = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element - && method.kind == MethodDefinitionKind::Constructor - { - return method.value.params.items.as_slice().into(); - } - None - })?; + let constructor = crate::util::find_constructor(class)?.value.params.items.as_slice(); // Names the type annotation may resolve to locally but not in value space: // interfaces, type aliases, `const enum`s, and the class's type parameters — diff --git a/crates/oxc_angular_compiler/src/component/decorator.rs b/crates/oxc_angular_compiler/src/component/decorator.rs index 525bb7eef..c82a173c7 100644 --- a/crates/oxc_angular_compiler/src/component/decorator.rs +++ b/crates/oxc_angular_compiler/src/component/decorator.rs @@ -664,14 +664,7 @@ fn extract_constructor_deps<'a>( consts: &StringConsts<'a>, ) -> Option>> { // Find the constructor method - let constructor = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }); + let constructor = crate::util::find_constructor(class); match constructor { Some(ctor) => { @@ -907,14 +900,7 @@ fn for_each_constructor_param_decorator<'a>( class: &'a Class<'a>, mut f: impl FnMut(&'a Decorator<'a>), ) { - let constructor = class.body.body.iter().find_map(|element| match element { - ClassElement::MethodDefinition(method) - if method.kind == MethodDefinitionKind::Constructor => - { - Some(method) - } - _ => None, - }); + let constructor = crate::util::find_constructor(class); for param in constructor.into_iter().flat_map(|ctor| &ctor.value.params.items) { param.decorators.iter().for_each(&mut f); } diff --git a/crates/oxc_angular_compiler/src/component/import_elision.rs b/crates/oxc_angular_compiler/src/component/import_elision.rs index 8d17e63c2..2418f01a4 100644 --- a/crates/oxc_angular_compiler/src/component/import_elision.rs +++ b/crates/oxc_angular_compiler/src/component/import_elision.rs @@ -42,8 +42,8 @@ use std::path::Path; use oxc_ast::ast::{ - BindingIdentifier, ClassElement, Expression, ImportDeclarationSpecifier, MethodDefinitionKind, - Program, Statement, StringLiteral, TSType, + BindingIdentifier, ClassElement, Expression, ImportDeclarationSpecifier, Program, Statement, + StringLiteral, TSType, }; use oxc_ast_visit::Visit; use oxc_semantic::{ReferenceId, Semantic, SemanticBuilder, SymbolFlags}; @@ -311,13 +311,7 @@ impl<'a> ImportElisionAnalyzer<'a> { return; } let type_params = crate::class_metadata::type_param_names_of(class); - for element in &class.body.body { - let ClassElement::MethodDefinition(method) = element else { - continue; - }; - if method.kind != MethodDefinitionKind::Constructor { - continue; - } + if let Some(method) = crate::util::find_constructor(class) { for param in &method.value.params.items { let Some(annotation) = ¶m.type_annotation else { continue; @@ -554,8 +548,9 @@ impl<'a> ImportElisionAnalyzer<'a> { /// them and compiles nothing that names them. Only on a class the transform compiles (one with an Angular /// class decorator, see [`crate::directive::angular_class_decorator`]): /// - /// - an Angular constructor parameter decorator of the first constructor (the one - /// `collect_constructor_decorator_spans` strips and the factory reads), with the root of `@Inject(X)` / + /// - an Angular constructor parameter decorator of the constructor implementation (the one + /// `collect_constructor_decorator_spans` strips and the factory reads, see + /// [`crate::util::find_constructor`]), with the root of `@Inject(X)` / /// `@Inject(X.sub)`: the factory names `X` through a namespace import (`i1.X.sub`), while any other /// argument (`TOKEN['sub']`, `forwardRef(() => TOKEN)`) is copied as written /// - the Angular member decorator itself on a `declare` property, not its arguments: a query predicate @@ -622,14 +617,7 @@ impl<'a> ImportElisionAnalyzer<'a> { }) { continue; } - let constructor = class.body.body.iter().find_map(|element| match element { - ClassElement::MethodDefinition(method) - if method.kind == MethodDefinitionKind::Constructor => - { - Some(method) - } - _ => None, - }); + let constructor = crate::util::find_constructor(class); for param in constructor.into_iter().flat_map(|ctor| &ctor.value.params.items) { for decorator in ¶m.decorators { // by what it resolves to: `@I(TOKEN)` for `import { Inject as I }` is removed too diff --git a/crates/oxc_angular_compiler/src/component/transform.rs b/crates/oxc_angular_compiler/src/component/transform.rs index 178013e33..1ae94a4d8 100644 --- a/crates/oxc_angular_compiler/src/component/transform.rs +++ b/crates/oxc_angular_compiler/src/component/transform.rs @@ -1038,15 +1038,7 @@ struct JitNonAngularMemberDecorator { /// also skips that walk, and our single-file transform is closer to LOCAL /// mode than to a full reflector. fn class_has_own_constructor_params(class: &oxc_ast::ast::Class<'_>) -> bool { - use oxc_ast::ast::{ClassElement, MethodDefinitionKind}; - class.body.body.iter().any(|element| { - if let ClassElement::MethodDefinition(method) = element { - method.kind == MethodDefinitionKind::Constructor - && !method.value.params.items.is_empty() - } else { - false - } - }) + crate::util::find_constructor(class).is_some_and(|ctor| !ctor.value.params.items.is_empty()) } /// Return the `@Service` decorator on a class iff the `Service` identifier @@ -1263,16 +1255,7 @@ fn extract_jit_ctor_params( class: &oxc_ast::ast::Class<'_>, consts: &crate::directive::StringConsts<'_>, ) -> (std::vec::Vec, std::vec::Vec) { - use oxc_ast::ast::{ClassElement, MethodDefinitionKind}; - - let constructor = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }); + let constructor = crate::util::find_constructor(class); let Some(ctor) = constructor else { return (std::vec::Vec::new(), std::vec::Vec::new()); @@ -3315,15 +3298,8 @@ pub fn transform_angular_file( // __param, so their imports must be kept — the same result // TypeScript's elision produces on ngtsc's rewritten AST. let mut param_idents = JitCtorParamIdentifiers::default(); - for element in &class.body.body { - if let oxc_ast::ast::ClassElement::MethodDefinition(method) = element - && method.kind == oxc_ast::ast::MethodDefinitionKind::Constructor - { - oxc_ast_visit::Visit::visit_formal_parameters( - &mut param_idents, - &method.value.params, - ); - } + if let Some(ctor) = crate::util::find_constructor(class) { + oxc_ast_visit::Visit::visit_formal_parameters(&mut param_idents, &ctor.value.params); } for name in param_idents.0 { import_elision_mut.preserve(name); @@ -4569,7 +4545,7 @@ pub fn transform_angular_file( // The analyzer counts an Angular constructor parameter decorator, and the Angular decorator of a `declare` // property, as removed on every class with an Angular class decorator. One the walk above left in place (a - // component whose template is missing, a constructor after the first) keeps what it references. + // component whose template is missing, a constructor other than the implementation) keeps what it references. { let stripped: rustc_hash::FxHashSet = decorator_spans_to_remove.iter().copied().collect(); diff --git a/crates/oxc_angular_compiler/src/directive/decorator.rs b/crates/oxc_angular_compiler/src/directive/decorator.rs index 490b7d69e..29334b675 100644 --- a/crates/oxc_angular_compiler/src/directive/decorator.rs +++ b/crates/oxc_angular_compiler/src/directive/decorator.rs @@ -300,14 +300,7 @@ fn extract_constructor_deps<'a>( consts: &StringConsts<'a>, ) -> Option>> { // Find the constructor method - let constructor = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }); + let constructor = crate::util::find_constructor(class); match constructor { Some(ctor) => { @@ -1670,21 +1663,8 @@ pub fn param_decorator_errors<'a>( return std::vec::Vec::new(); } - class - .body - .body - .iter() - .filter_map(|element| match element { - // Upstream picks the first constructor *with a body* - // (typescript.ts `findConstructorDeclaration`): an overload - // signature's parameters are not checked. - ClassElement::MethodDefinition(m) - if m.kind == MethodDefinitionKind::Constructor && m.value.body.is_some() => - { - Some(m) - } - _ => None, - }) + crate::util::find_constructor(class) + .into_iter() .flat_map(|ctor| ctor.value.params.items.iter()) .flat_map(|param| param.decorators.iter()) .filter(|d| super::is_angular_core_decorator(d, Some(consts))) diff --git a/crates/oxc_angular_compiler/src/injectable/decorator.rs b/crates/oxc_angular_compiler/src/injectable/decorator.rs index 78bc38309..433e3f50e 100644 --- a/crates/oxc_angular_compiler/src/injectable/decorator.rs +++ b/crates/oxc_angular_compiler/src/injectable/decorator.rs @@ -5,8 +5,7 @@ use oxc_allocator::{Allocator, Box, Vec}; use oxc_ast::ast::{ - Argument, ArrayExpressionElement, Class, ClassElement, Decorator, Expression, - MethodDefinitionKind, ObjectPropertyKind, PropertyKey, + Argument, ArrayExpressionElement, Class, Decorator, Expression, ObjectPropertyKind, PropertyKey, }; use oxc_span::Span; use oxc_str::Ident; @@ -570,14 +569,7 @@ pub fn extract_constructor_deps<'a>( consts: Option<&StringConsts<'_>>, ) -> Option>> { // Find the constructor method - let Some(constructor) = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }) else { + let Some(constructor) = crate::util::find_constructor(class) else { // No constructor: a class with a superclass inherits its factory // (`None`), one without gets an implicit no-arg constructor. // See: packages/compiler-cli/src/ngtsc/annotations/common/src/di.ts:47-52 diff --git a/crates/oxc_angular_compiler/src/ng_module/decorator.rs b/crates/oxc_angular_compiler/src/ng_module/decorator.rs index 326d470b1..dfa7d14da 100644 --- a/crates/oxc_angular_compiler/src/ng_module/decorator.rs +++ b/crates/oxc_angular_compiler/src/ng_module/decorator.rs @@ -5,8 +5,7 @@ use oxc_allocator::{Allocator, Box, Vec}; use oxc_ast::ast::{ - Argument, ArrayExpressionElement, Class, ClassElement, Decorator, Expression, - MethodDefinitionKind, ObjectPropertyKind, PropertyKey, + Argument, ArrayExpressionElement, Class, Decorator, Expression, ObjectPropertyKind, PropertyKey, }; use oxc_span::Span; use oxc_str::Ident; @@ -503,14 +502,7 @@ pub fn extract_constructor_deps<'a>( consts: Option<&StringConsts<'_>>, ) -> Option>> { // Find the constructor method - let Some(constructor) = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }) else { + let Some(constructor) = crate::util::find_constructor(class) else { // No constructor: a class with a superclass inherits its factory // (`None`), one without gets an implicit no-arg constructor. // See: packages/compiler-cli/src/ngtsc/annotations/common/src/di.ts:47-52 diff --git a/crates/oxc_angular_compiler/src/pipe/decorator.rs b/crates/oxc_angular_compiler/src/pipe/decorator.rs index d75ed95cb..440430d78 100644 --- a/crates/oxc_angular_compiler/src/pipe/decorator.rs +++ b/crates/oxc_angular_compiler/src/pipe/decorator.rs @@ -4,10 +4,7 @@ //! on TypeScript class declarations. use oxc_allocator::{Allocator, Box, Vec}; -use oxc_ast::ast::{ - Argument, Class, ClassElement, Decorator, Expression, MethodDefinitionKind, ObjectPropertyKind, - PropertyKey, -}; +use oxc_ast::ast::{Argument, Class, Decorator, Expression, ObjectPropertyKind, PropertyKey}; use oxc_span::Span; use oxc_str::Ident; @@ -269,14 +266,7 @@ fn extract_constructor_deps<'a>( consts: Option<&StringConsts<'_>>, ) -> Option>> { // Find the constructor method - let Some(constructor) = class.body.body.iter().find_map(|element| { - if let ClassElement::MethodDefinition(method) = element { - if method.kind == MethodDefinitionKind::Constructor { - return Some(method); - } - } - None - }) else { + let Some(constructor) = crate::util::find_constructor(class) else { // No constructor: a class with a superclass inherits its factory // (`None`), one without gets an implicit no-arg constructor. // See: packages/compiler-cli/src/ngtsc/annotations/common/src/di.ts:47-52 diff --git a/crates/oxc_angular_compiler/src/util/mod.rs b/crates/oxc_angular_compiler/src/util/mod.rs index c58741771..5d099dc53 100644 --- a/crates/oxc_angular_compiler/src/util/mod.rs +++ b/crates/oxc_angular_compiler/src/util/mod.rs @@ -15,3 +15,22 @@ pub use type_extract::*; pub fn is_metadata_property(prop: &oxc_ast::ast::ObjectProperty<'_>) -> bool { !prop.method && prop.kind == oxc_ast::ast::PropertyKind::Init } + +/// The class's constructor implementation: the one with a body. Overload +/// signatures (no body) are skipped, since the implementation is what runs and +/// what carries the parameter decorators. +/// +/// See: packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +/// (`TypeScriptReflectionHost.getConstructorParameters`) +pub fn find_constructor<'c, 'a>( + class: &'c oxc_ast::ast::Class<'a>, +) -> Option<&'c oxc_ast::ast::MethodDefinition<'a>> { + class.body.body.iter().find_map(|element| match element { + oxc_ast::ast::ClassElement::MethodDefinition(method) + if method.kind.is_constructor() && method.value.body.is_some() => + { + Some(&**method) + } + _ => None, + }) +} diff --git a/crates/oxc_angular_compiler/tests/ctor_overloads_test.rs b/crates/oxc_angular_compiler/tests/ctor_overloads_test.rs new file mode 100644 index 000000000..e9e1c1f9c --- /dev/null +++ b/crates/oxc_angular_compiler/tests/ctor_overloads_test.rs @@ -0,0 +1,89 @@ +//! With constructor overloads, the implementation (the declaration with a body) +//! is what runs and what carries the parameter decorators, so like ngtsc's +//! `TypeScriptReflectionHost.getConstructorParameters` the factory, +//! `setClassMetadata` and JIT `ctorParameters` read it, not the first overload. +//! +//! Ported from Angular's `ngtsc_spec.ts`: "should compile @Injectable with +//! constructor overloads". + +use oxc_allocator::Allocator; +use oxc_angular_compiler::{TransformOptions, transform_angular_file}; + +fn transform(source: &str, options: &TransformOptions) -> String { + let allocator = Allocator::default(); + let result = transform_angular_file(&allocator, "test.ts", source, Some(options), None); + assert!(result.diagnostics.is_empty(), "unexpected diagnostics: {:?}", result.diagnostics); + result.code.chars().filter(|c| !c.is_whitespace()).collect() +} + +fn source(decorator: &str, extra_members: &str) -> String { + format!( + r" +import {{Component, Directive, Injectable, NgModule, Optional, Pipe}} from '@angular/core'; + +@Injectable() +class Dep {{}} + +@Injectable() +class OptionalDep {{}} + +{decorator} +class Service {{ + constructor(dep: Dep); + + constructor(dep: Dep, @Optional() optionalDep?: OptionalDep) {{}} + {extra_members} +}} +" + ) +} + +#[test] +fn injectable_factory_reads_the_implementation() { + let code = transform(&source("@Injectable()", ""), &TransformOptions::default()); + assert!( + code.contains( + "returnnew(__ngFactoryType__||Service)(i0.ɵɵinject(Dep),i0.ɵɵinject(OptionalDep,8));" + ), + "{code}" + ); +} + +#[test] +fn every_decorator_kind_reads_the_implementation() { + let cases = [ + ("@Component({selector: 'c', template: ''})", "", "i0.ɵɵdirectiveInject(OptionalDep,8)"), + ("@Directive({selector: '[d]'})", "", "i0.ɵɵdirectiveInject(OptionalDep,8)"), + ("@Pipe({name: 'p'})", "transform() {}", "i0.ɵɵdirectiveInject(OptionalDep,24)"), + ("@NgModule({})", "", "i0.ɵɵinject(OptionalDep,8)"), + ("@Injectable()", "", "i0.ɵɵinject(OptionalDep,8)"), + ]; + for (decorator, extra, optional_dep) in cases { + let code = transform(&source(decorator, extra), &TransformOptions::default()); + assert!(code.contains(&format!(",{optional_dep});")), "{decorator}: {code}"); + // setClassMetadata's ctor params come from the implementation too. + assert!( + code.contains("()=>[{type:Dep},{type:OptionalDep,decorators:[{type:Optional}]}]"), + "{decorator}: {code}" + ); + // The implementation's parameter decorator is compiled, so it is removed. + assert!( + code.contains("constructor(dep:Dep,optionalDep?:OptionalDep){}"), + "{decorator}: {code}" + ); + } +} + +#[test] +fn jit_ctor_parameters_read_the_implementation() { + let options = TransformOptions { jit: true, ..TransformOptions::default() }; + let code = transform(&source("@Injectable()", ""), &options); + assert!( + code.contains( + "staticctorParameters=()=>[{type:Dep},{type:OptionalDep,decorators:[{type:Optional}]}];" + ), + "{code}" + ); + // ...and its Angular parameter decorator is removed, not lowered to `__param`. + assert!(!code.contains("Optional()"), "{code}"); +} diff --git a/crates/oxc_angular_compiler/tests/import_elision_output_references_test.rs b/crates/oxc_angular_compiler/tests/import_elision_output_references_test.rs index 3bb420158..e3ebe610b 100644 --- a/crates/oxc_angular_compiler/tests/import_elision_output_references_test.rs +++ b/crates/oxc_angular_compiler/tests/import_elision_output_references_test.rs @@ -176,9 +176,10 @@ export class AppComponent { constructor(@Inject(LOCALE_ID) public locale: string } #[test] -fn ctor_overload_keeps_implementation_param_decorators() { - // Angular compliance case r3_view_compiler_di/di/ctor_overload.ts - assert_kept( +fn ctor_overload_implementation_param_decorators_are_elided() { + // Angular compliance case r3_view_compiler_di/di/ctor_overload.ts: the factory reads the + // implementation, so its `@Optional()` is compiled into the flags and stripped. + let code = compile( r#" import {Injectable, Optional} from '@angular/core'; class MyDependency {} @@ -189,8 +190,11 @@ export class MyService { constructor(dep: MyDependency, @Optional() optionalDep?: MyOptionalDependency) {} } "#, - &["Optional"], ); + assert!(!imports(&code, "Optional"), "{code}"); + assert!(!uses(&code, "Optional"), "{code}"); + let flat: String = code.chars().filter(|c| !c.is_whitespace()).collect(); + assert!(flat.contains("ɵɵinject(MyOptionalDependency,8)"), "{code}"); } #[test]