diff --git a/crates/oxc_angular_compiler/src/class_metadata/builders.rs b/crates/oxc_angular_compiler/src/class_metadata/builders.rs index cf04251d0..ff1401478 100644 --- a/crates/oxc_angular_compiler/src/class_metadata/builders.rs +++ b/crates/oxc_angular_compiler/src/class_metadata/builders.rs @@ -361,7 +361,9 @@ fn build_param_type_expression<'a>( /// Used to get the type name for namespace-prefixed references in metadata. fn extract_param_type_name<'a>(param: &FormalParameter<'a>) -> Option> { let type_annotation = param.type_annotation.as_ref()?; - match &type_annotation.type_annotation { + // Narrow `T | null` unions to `T` so optional-DI patterns expose the type. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; + match ts_type { TSType::TSTypeReference(type_ref) => match &type_ref.type_name { TSTypeName::IdentifierReference(id) => Some(id.name.into()), TSTypeName::QualifiedName(qualified) => Some(qualified.right.name.into()), @@ -381,8 +383,11 @@ fn extract_param_type_expression<'a>( // Get the type annotation from the formal parameter let type_annotation = param.type_annotation.as_ref()?; + // Narrow `T | null` unions to `T` so optional-DI patterns expose the type. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; + // Extract the type name from the annotation - match &type_annotation.type_annotation { + match ts_type { TSType::TSTypeReference(type_ref) => { // Handle simple type references like SomeService match &type_ref.type_name { diff --git a/crates/oxc_angular_compiler/src/component/decorator.rs b/crates/oxc_angular_compiler/src/component/decorator.rs index a5c271d43..934d47182 100644 --- a/crates/oxc_angular_compiler/src/component/decorator.rs +++ b/crates/oxc_angular_compiler/src/component/decorator.rs @@ -1029,7 +1029,8 @@ fn extract_inject_token<'a>(arg: &'a Argument<'a>) -> Option> { fn extract_param_token<'a>(param: &'a oxc_ast::ast::FormalParameter<'a>) -> Option> { // Get the type annotation (directly on FormalParameter) let type_annotation = param.type_annotation.as_ref()?; - let ts_type = &type_annotation.type_annotation; + // Narrow `T | null` unions to `T` to match the reference compiler. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; // Handle TSTypeReference: SomeClass, SomeModule, etc. if let oxc_ast::ast::TSType::TSTypeReference(type_ref) = ts_type { @@ -2635,6 +2636,136 @@ mod tests { }); } + #[test] + fn test_component_optional_with_nullable_type() { + // Regression test for issue #285: + // `@Optional() svc: MyService | null` is the canonical optional-DI pattern, + // but the union with `null` previously caused token extraction to fail and + // emit `ɵɵinvalidFactoryDep`, which throws at runtime. + // + // Angular's reference compiler filters out `null` literal type nodes from + // the union; when exactly one type remains it becomes the token. + let code = r#" + @Component({ + selector: 'app-test', + template: '' + }) + class TestComponent { + constructor( + @Optional() private svc: MyService | null, + ) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.constructor_deps.as_ref().unwrap(); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(dep.optional, "Should have optional flag"); + assert_eq!( + dep.token.as_ref().expect("token should resolve to MyService").as_str(), + "MyService", + "null should be filtered from the union, leaving MyService as the token", + ); + }); + } + + #[test] + fn test_component_nullable_type_without_decorator() { + // Bare `svc: MyService | null` (no `@Optional()`) should also have its + // token resolved to `MyService`. Token resolution is independent of + // optionality — `@Optional()` only controls the inject flag. + let code = r#" + @Component({ + selector: 'app-test', + template: '' + }) + class TestComponent { + constructor( + private svc: MyService | null, + ) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.constructor_deps.as_ref().unwrap(); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(!dep.optional, "Should not have optional flag"); + assert_eq!( + dep.token.as_ref().expect("token should resolve to MyService").as_str(), + "MyService", + ); + }); + } + + #[test] + fn test_component_nullable_type_null_first() { + // `null` can appear in either position of the union — both must be filtered. + let code = r#" + @Component({ + selector: 'app-test', + template: '' + }) + class TestComponent { + constructor( + @Optional() private svc: null | MyService, + ) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.constructor_deps.as_ref().unwrap(); + let dep = &deps[0]; + assert_eq!(dep.token.as_ref().unwrap().as_str(), "MyService"); + }); + } + + #[test] + fn test_component_parenthesized_nullable_type() { + // Parenthesized nullable union: `(MyService | null)`. + // oxc preserves `TSParenthesizedType` in the AST, so the resolver must + // unwrap it before checking for unions, or the dependency falls through + // to `ɵɵinvalidFactoryDep` (issue #285 follow-up). + let code = r#" + @Component({ + selector: 'app-test', + template: '' + }) + class TestComponent { + constructor( + @Optional() private svc: (MyService | null), + ) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.constructor_deps.as_ref().unwrap(); + let dep = &deps[0]; + assert_eq!( + dep.token.as_ref().expect("token should resolve to MyService").as_str(), + "MyService", + ); + }); + } + + #[test] + fn test_component_paren_around_inner_type_in_union() { + // Parens around just the non-null side: `(MyService) | null`. + let code = r#" + @Component({ + selector: 'app-test', + template: '' + }) + class TestComponent { + constructor( + @Optional() private svc: (MyService) | null, + ) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.constructor_deps.as_ref().unwrap(); + let dep = &deps[0]; + assert_eq!(dep.token.as_ref().unwrap().as_str(), "MyService"); + }); + } + #[test] fn test_component_with_skip_self_decorator() { let code = r#" diff --git a/crates/oxc_angular_compiler/src/directive/decorator.rs b/crates/oxc_angular_compiler/src/directive/decorator.rs index 5a0c5359c..778314c82 100644 --- a/crates/oxc_angular_compiler/src/directive/decorator.rs +++ b/crates/oxc_angular_compiler/src/directive/decorator.rs @@ -394,7 +394,8 @@ fn extract_param_token<'a>( ) -> Option> { // Get the type annotation (directly on FormalParameter) let type_annotation = param.type_annotation.as_ref()?; - let ts_type = &type_annotation.type_annotation; + // Narrow `T | null` unions to `T` to match the reference compiler. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; // Handle TSTypeReference: SomeClass, SomeModule, etc. if let oxc_ast::ast::TSType::TSTypeReference(type_ref) = ts_type { @@ -1536,6 +1537,30 @@ mod tests { }); } + #[test] + fn test_directive_optional_with_nullable_type() { + // Regression test for issue #285: + // `@Optional() svc: MyService | null` must resolve the token to `MyService`. + let code = r#" + @Directive({ selector: '[myDir]' }) + class MyDirective { + constructor(@Optional() private svc: MyService | null) {} + } + "#; + assert_directive_metadata(code, |meta| { + let deps = meta.deps.as_ref().expect("Directive should have deps"); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(dep.optional, "Should have optional flag"); + let token = dep.token.as_ref().expect("token should resolve to MyService"); + if let crate::output::ast::OutputExpression::ReadVar(var) = token { + assert_eq!(var.name.as_str(), "MyService"); + } else { + panic!("expected ReadVar token, got {:?}", token); + } + }); + } + #[test] fn test_extract_param_token_returns_read_var_not_read_prop() { // Regression test for bug where extract_param_token() returned diff --git a/crates/oxc_angular_compiler/src/injectable/decorator.rs b/crates/oxc_angular_compiler/src/injectable/decorator.rs index 6adf80162..0d0ab178e 100644 --- a/crates/oxc_angular_compiler/src/injectable/decorator.rs +++ b/crates/oxc_angular_compiler/src/injectable/decorator.rs @@ -640,7 +640,8 @@ fn extract_param_token<'a>( ) -> Option> { // First try to get the type annotation (directly on FormalParameter, not on pattern) let type_annotation = param.type_annotation.as_ref()?; - let ts_type = &type_annotation.type_annotation; + // Narrow `T | null` unions to `T` to match the reference compiler. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; // Handle TSTypeReference: SomeClass, SomeModule, etc. if let oxc_ast::ast::TSType::TSTypeReference(type_ref) = ts_type { @@ -917,6 +918,32 @@ mod tests { assert!(deps[2].token.is_some(), "Third dep should have token"); } + #[test] + fn test_injectable_optional_with_nullable_type() { + // Regression test for issue #285: + // `@Optional() svc: MyService | null` must resolve the token to `MyService`. + let allocator = Allocator::default(); + let source = r#" + @Injectable() + class MyService { + constructor(@Optional() private svc: OtherService | null) {} + } + "#; + + let metadata = parse_and_extract(&allocator, source).expect("Should have metadata"); + let deps = metadata.deps.as_ref().expect("Should have constructor deps"); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(dep.optional, "Should have optional flag"); + let token = dep.token.as_ref().expect("token should resolve to OtherService"); + match token { + crate::output::ast::OutputExpression::ReadVar(var) => { + assert_eq!(var.name.as_str(), "OtherService"); + } + other => panic!("expected ReadVar token, got {:?}", other), + } + } + #[test] fn test_injectable_with_optional_and_skip_self() { let allocator = Allocator::default(); diff --git a/crates/oxc_angular_compiler/src/ng_module/decorator.rs b/crates/oxc_angular_compiler/src/ng_module/decorator.rs index 193581e66..aae2f05b8 100644 --- a/crates/oxc_angular_compiler/src/ng_module/decorator.rs +++ b/crates/oxc_angular_compiler/src/ng_module/decorator.rs @@ -518,7 +518,8 @@ fn extract_param_token<'a>( ) -> Option> { // First try to get the type annotation (directly on FormalParameter, not on pattern) let type_annotation = param.type_annotation.as_ref()?; - let ts_type = &type_annotation.type_annotation; + // Narrow `T | null` unions to `T` to match the reference compiler. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; // Handle TSTypeReference: SomeClass, SomeModule, etc. if let oxc_ast::ast::TSType::TSTypeReference(type_ref) = ts_type { @@ -902,4 +903,29 @@ mod tests { assert!(dep.token.is_some(), "Token should be extracted from type annotation"); }); } + + #[test] + fn test_ng_module_optional_with_nullable_type() { + // Regression test for issue #285: + // `@Optional() svc: MyService | null` must resolve the token to `MyService`. + let code = r#" + @NgModule({}) + class AppModule { + constructor(@Optional() private parent: AppModule | null) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.deps.as_ref().expect("Should have constructor deps"); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(dep.optional); + let token = dep.token.as_ref().expect("token should resolve to AppModule"); + match token { + crate::output::ast::OutputExpression::ReadVar(var) => { + assert_eq!(var.name.as_str(), "AppModule"); + } + other => panic!("expected ReadVar token, got {:?}", other), + } + }); + } } diff --git a/crates/oxc_angular_compiler/src/pipe/decorator.rs b/crates/oxc_angular_compiler/src/pipe/decorator.rs index f6162844d..288533cb1 100644 --- a/crates/oxc_angular_compiler/src/pipe/decorator.rs +++ b/crates/oxc_angular_compiler/src/pipe/decorator.rs @@ -351,7 +351,8 @@ fn extract_param_token<'a>( ) -> Option> { // First try to get the type annotation (directly on FormalParameter, not on pattern) let type_annotation = param.type_annotation.as_ref()?; - let ts_type = &type_annotation.type_annotation; + // Narrow `T | null` unions to `T` to match the reference compiler. + let ts_type = crate::util::resolve_di_token_type(&type_annotation.type_annotation)?; // Handle TSTypeReference: SomeClass, SomeModule, etc. if let oxc_ast::ast::TSType::TSTypeReference(type_ref) = ts_type { @@ -569,6 +570,32 @@ mod tests { }); } + #[test] + fn test_pipe_optional_with_nullable_type() { + // Regression test for issue #285: + // `@Optional() svc: MyService | null` must resolve the token to `MyService`. + let code = r#" + @Pipe({ name: 'myPipe' }) + class MyPipe { + constructor(@Optional() private svc: MyService | null) {} + } + "#; + assert_metadata(code, |meta| { + let deps = meta.deps.as_ref().expect("Should have deps"); + assert_eq!(deps.len(), 1); + let dep = &deps[0]; + assert!(dep.optional); + let token = dep.token.as_ref().expect("token should resolve to MyService"); + let token: &crate::output::ast::OutputExpression<'_> = token; + match token { + crate::output::ast::OutputExpression::ReadVar(var) => { + assert_eq!(var.name.as_str(), "MyService"); + } + other => panic!("expected ReadVar token, got {:?}", other), + } + }); + } + // ========================================================================= // Edge cases // ========================================================================= diff --git a/crates/oxc_angular_compiler/src/util/mod.rs b/crates/oxc_angular_compiler/src/util/mod.rs index eb072c2e9..1ed0d18c4 100644 --- a/crates/oxc_angular_compiler/src/util/mod.rs +++ b/crates/oxc_angular_compiler/src/util/mod.rs @@ -3,6 +3,8 @@ pub mod chars; mod deferred_time; mod parse_util; +mod type_extract; pub use deferred_time::*; pub use parse_util::*; +pub use type_extract::*; diff --git a/crates/oxc_angular_compiler/src/util/type_extract.rs b/crates/oxc_angular_compiler/src/util/type_extract.rs new file mode 100644 index 000000000..aa5feb681 --- /dev/null +++ b/crates/oxc_angular_compiler/src/util/type_extract.rs @@ -0,0 +1,55 @@ +//! Type extraction helpers shared between DI token resolution and class metadata. +//! +//! Mirrors Angular's `typeReferenceToExpression` in +//! `packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts`, which filters +//! `null` literal type nodes out of a union before resolving the remaining type +//! as an injection token. + +use oxc_ast::ast::TSType; + +/// Resolves a TypeScript type annotation to the concrete type usable as a DI +/// injection token. +/// +/// When the annotation is a union, `null` keyword variants are filtered out; +/// if exactly one non-null type remains, that type is returned. Otherwise +/// (no remaining types, or two or more remaining types) the result is `None`, +/// matching the reference compiler's "unresolvable token" behavior. +/// +/// Parenthesized type nodes (`(T)`) are transparently unwrapped before union +/// handling — oxc preserves them in the AST and the reference compiler treats +/// them as the inner type for token resolution. +/// +/// `undefined` is intentionally **not** stripped — this matches Angular's +/// reference compiler, where `T | undefined` remains ambiguous for token +/// resolution while `T | null` is the canonical optional-DI pattern. +/// +/// Non-union, non-parenthesized types are returned as-is. +pub fn resolve_di_token_type<'a, 'b>(ts_type: &'b TSType<'a>) -> Option<&'b TSType<'a>> { + match ts_type { + TSType::TSParenthesizedType(paren) => resolve_di_token_type(&paren.type_annotation), + TSType::TSUnionType(union) => { + let mut non_null = union.types.iter().filter(|t| { + // Unwrap parens inside the union so `MyService | (null)` and + // `(MyService) | null` both reduce to `MyService`. + !matches!(unwrap_parens(t), TSType::TSNullKeyword(_)) + }); + let first = non_null.next()?; + if non_null.next().is_some() { + // More than one non-null type — ambiguous for token resolution. + return None; + } + // Recurse so nested cases like `(MyService | null) | null` collapse. + resolve_di_token_type(first) + } + _ => Some(ts_type), + } +} + +/// Strip nested `TSParenthesizedType` wrappers, returning the underlying type. +fn unwrap_parens<'a, 'b>(ts_type: &'b TSType<'a>) -> &'b TSType<'a> { + let mut current = ts_type; + while let TSType::TSParenthesizedType(paren) = current { + current = &paren.type_annotation; + } + current +}