diff --git a/crates/oxc_angular_compiler/src/component/transform.rs b/crates/oxc_angular_compiler/src/component/transform.rs index bf7dddd31..f47734295 100644 --- a/crates/oxc_angular_compiler/src/component/transform.rs +++ b/crates/oxc_angular_compiler/src/component/transform.rs @@ -42,7 +42,7 @@ use crate::directive::collect_string_consts; use crate::directive::{ R3QueryMetadata, create_content_queries_function, create_view_queries_function, decorator_io_errors, extract_class_queries, extract_directive_metadata, - find_directive_decorator, generate_directive_definitions, + find_directive_decorator, generate_directive_definitions, param_decorator_errors, }; use crate::dts; use crate::injectable::{ @@ -2910,12 +2910,17 @@ pub fn transform_angular_file( let implicit_standalone = options.implicit_standalone(); // `inputs:`/`outputs:` forms ngtsc rejects, rather than dropping them silently. - result.diagnostics.extend(decorator_io_errors( - allocator, - class, - Some(source), - &string_consts, - )); + let io_errors = decorator_io_errors(allocator, class, Some(source), &string_consts); + result.diagnostics.extend(io_errors.iter().cloned()); + if io_errors.is_empty() { + // Constructor parameter decorators: ngtsc's + // `getConstructorDependencies` (arity, unexpected decorators) + // runs only once the io/query checks passed. Upstream also + // throws earlier — malformed decorator args, a missing + // selector — for shapes oxc doesn't diagnose yet, so this + // can be the first error where ngtsc would report another. + result.diagnostics.extend(param_decorator_errors(class, &string_consts)); + } if let Some(mut metadata) = extract_component_metadata( &allocator, diff --git a/crates/oxc_angular_compiler/src/directive/decorator.rs b/crates/oxc_angular_compiler/src/directive/decorator.rs index e184ee8ee..e96a73f35 100644 --- a/crates/oxc_angular_compiler/src/directive/decorator.rs +++ b/crates/oxc_angular_compiler/src/directive/decorator.rs @@ -1268,6 +1268,91 @@ pub fn decorator_io_errors<'a>( .collect() } +/// The name upstream sees for a `@angular/core` decorator (`dec.import?.name +/// ?? dec.name`): the imported name for an aliased import (`@Inj(...)` for +/// `import { Inject as Inj }` is `Inject`), the member name for a namespace +/// import (`@core.Inject()`), the local name otherwise. +fn angular_decorator_name<'a>(decorator: &'a Decorator<'a>, consts: &StringConsts<'a>) -> &'a str { + let callee = match &decorator.expression { + Expression::CallExpression(call) => &call.callee, + expr => expr, + }; + match callee { + Expression::Identifier(id) => consts + .scope() + .import(&id.name) + .and_then(|import| import.imported) + // A default import has no imported name upstream + // (`getExportedName` falls back to the local identifier). + .filter(|name| *name != "default") + .unwrap_or(id.name.as_str()), + Expression::StaticMemberExpression(m) => m.property.name.as_str(), + _ => "", + } +} + +/// The first error ngtsc's `getConstructorDependencies` (`di.ts`) raises for a +/// class's constructor parameters: `@Inject()` / `@Attribute()` with anything +/// but one argument (`DECORATOR_ARITY_WRONG`), or another `@angular/core` +/// decorator on a parameter (`Unexpected decorator X on parameter.`). +/// +/// Runs for every class a DI handler compiles — `@Component`, `@Directive`, +/// `@Pipe`, `@NgModule` and `@Injectable` all call it — after the io/query +/// checks, which is where it sits inside `extractDirectiveMetadata`. +/// `getConstructorDependencies` only sees the constructor's parameters, so +/// decorators on other parameters are not checked. +pub fn param_decorator_errors<'a>( + class: &'a Class<'a>, + consts: &StringConsts<'a>, +) -> std::vec::Vec { + // Not a class any DI handler compiles: per-class analysis never looks + // at its parameters. (Upstream's `InjectableClassRegistry` can still + // reach undecorated provider/base classes through `getConstructorDependencies`; + // oxc has no equivalent registry checks.) + if !class.decorators.iter().any(|d| super::angular_class_decorator(d, consts).is_some()) { + 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, + }) + .flat_map(|ctor| ctor.value.params.items.iter()) + .flat_map(|param| param.decorators.iter()) + .filter(|d| super::is_angular_core_decorator(d, Some(consts))) + .find_map(|decorator| { + let args: Option<&oxc_allocator::Vec<'_, Argument<'_>>> = match &decorator.expression { + Expression::CallExpression(call) => Some(&call.arguments), + _ => None, + }; + let error = match angular_decorator_name(decorator, consts) { + name @ ("Inject" | "Attribute") => { + if args.is_none_or(|a| a.len() != 1) { + Some(format!("Unexpected number of arguments to @{name}().")) + } else { + None + } + } + "Optional" | "Self" | "SkipSelf" | "Host" => None, + name => Some(format!("Unexpected decorator {name} on parameter.")), + }; + error.map(|message| OxcDiagnostic::error(message).with_label(decorator.span)) + }) + .into_iter() + .collect() +} + /// ngtsc's error for the `@Input(...)` decorator of the member `name` /// (`tryParseInputFieldMapping`): more than one argument, an argument that /// isn't `null`, a string or an object, or a `transform` it can't use. Options diff --git a/crates/oxc_angular_compiler/src/directive/mod.rs b/crates/oxc_angular_compiler/src/directive/mod.rs index 522180bd5..b2439851c 100644 --- a/crates/oxc_angular_compiler/src/directive/mod.rs +++ b/crates/oxc_angular_compiler/src/directive/mod.rs @@ -33,7 +33,7 @@ pub(crate) use decorator::{ }; pub use decorator::{ StringConsts, collect_string_consts, decorator_io_errors, extract_directive_metadata, - find_directive_decorator_span, + find_directive_decorator_span, param_decorator_errors, }; pub(crate) use decorator::{ angular_decorator_config, extract_string_value, resolve_template_literal, diff --git a/crates/oxc_angular_compiler/tests/integration_test.rs b/crates/oxc_angular_compiler/tests/integration_test.rs index b950fc97c..cde6209be 100644 --- a/crates/oxc_angular_compiler/tests/integration_test.rs +++ b/crates/oxc_angular_compiler/tests/integration_test.rs @@ -9386,8 +9386,9 @@ fn test_class_metadata_lists_any_angular_core_decorator() { // listed; a same-named decorator from another module is not. // // Note: ngtsc rejects a non-DI `@angular/core` decorator on a ctor param - // in DI analysis (DECORATOR_UNEXPECTED), so upstream never emits this - // metadata — oxc lists it, matching the metadata filter's own semantics. + // in DI analysis (`Unexpected decorator Injectable on parameter.`), + // which oxc now reports too (#550); the metadata emit assertion below + // still verifies the filter's own semantics on the emitted code. let allocator = Allocator::default(); let source = r" import { Component, Inject, Injectable } from '@angular/core'; @@ -9417,7 +9418,14 @@ export class TestComponent { let options = ComponentTransformOptions::default(); let result = transform_angular_file(&allocator, "test.component.ts", source, Some(&options), None); - assert!(!result.has_errors(), "Should not have errors: {:?}", result.diagnostics); + assert!( + result + .diagnostics + .iter() + .any(|d| format!("{d}").contains("Unexpected decorator Injectable on parameter.")), + "Should report DECORATOR_UNEXPECTED for @Injectable on a ctor param: {:?}", + result.diagnostics + ); let compact: String = result.code.chars().filter(|c| !c.is_whitespace()).collect(); @@ -16348,6 +16356,132 @@ fn test_static_member_checks_keep_ngtsc_error_order() { ); } +#[test] +fn test_param_decorator_arity_is_diagnostic() { + // ngtsc's `getConstructorDependencies` (di.ts): `@Inject`/`@Attribute` need + // exactly one argument; bare (`@Inject`) counts as zero. + for (param, expected) in [ + ("@Inject() x: any", "Unexpected number of arguments to @Inject()."), + ("@Inject(A, B) x: any", "Unexpected number of arguments to @Inject()."), + ("@Inject x: any", "Unexpected number of arguments to @Inject()."), + ("@Attribute() x: any", "Unexpected number of arguments to @Attribute()."), + ("@Attribute('a', 'b') x: any", "Unexpected number of arguments to @Attribute()."), + ("@Input() x: any", "Unexpected decorator Input on parameter."), + ] { + let source = format!( + "import {{ Directive, Inject, Attribute, Input }} from '@angular/core';\n\ + @Directive({{ selector: '[d]' }})\n\ + export class D {{\n constructor({param}) {{}}\n}}" + ); + let diagnostics = expect_diagnostics(&source); + assert!( + diagnostics.iter().any(|d| d.contains(expected)), + "`{param}` should report {expected:?}. Got: {diagnostics:?}" + ); + } +} + +#[test] +fn test_param_decorator_checks_respect_import_and_kind() { + // Only the constructor's parameters are checked (upstream only visits + // ctor params), only `@angular/core` decorators count, and aliases resolve + // to the imported name. + for (source, expected) in [ + // Foreign @Inject: no diagnostic. + ( + "import { Directive } from '@angular/core';\n\ + import { Inject } from 'not-angular';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@Inject() x: any) {}\n}", + None, + ), + // Aliased import reports the imported name upstream + // (`dec.import.name`). + ( + "import { Directive, Inject as Inj } from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@Inj() x: any) {}\n}", + Some("Unexpected number of arguments to @Inject()."), + ), + // Namespace import resolves the member name. + ( + "import { Directive } from '@angular/core';\n\ + import * as core from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@core.Inject() x: any) {}\n}", + Some("Unexpected number of arguments to @Inject()."), + ), + // A bad decorator on a non-ctor param is not checked. + ( + "import { Directive, Inject } from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n m(@Inject() x: any) {}\n}", + None, + ), + // An undecorated class is never looked at. + ( + "import { Inject } from '@angular/core';\n\ + export class D {\n constructor(@Inject() x: any) {}\n}", + None, + ), + // Valid decorators: arity ok, and flags take no arity check. + ( + "import { Directive, Inject, Attribute, Optional } from '@angular/core';\n\ + const TOKEN = 't';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@Inject(TOKEN) a: any, @Attribute('x') b: any, @Optional(1) c: any) {}\n}", + None, + ), + // A bodiless overload signature is not the checked constructor. + ( + "import { Directive, Inject } from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@Inject() x: any);\n constructor(x: any) {}\n}", + None, + ), + // Default import resolves to the local name upstream + // (`getExportedName` falls back for non-ImportSpecifier decls). + ( + "import { Directive } from '@angular/core';\n\ + import Inj from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n constructor(@Inj() x: any) {}\n}", + Some("Unexpected decorator Inj on parameter."), + ), + ] { + let diagnostics = expect_diagnostics(source); + match expected { + Some(expected) => assert!( + diagnostics.iter().any(|d| d.contains(expected)), + "should report {expected:?}. Got: {diagnostics:?}" + ), + None => assert!( + diagnostics.is_empty(), + "should not report a param-decorator error. Got: {diagnostics:?}" + ), + } + } +} + +#[test] +fn test_param_decorator_error_waits_for_io_checks() { + // Upstream throws the io error first: `getConstructorDependencies` runs + // last inside `extractDirectiveMetadata`, so a class with both problems + // reports only the input error. + let source = "import { Directive, Inject, Input } from '@angular/core';\n\ + @Directive({ selector: '[d]' })\n\ + export class D {\n @Input() static x = 0;\n constructor(@Inject() y: any) {}\n}"; + let diagnostics = expect_diagnostics(source); + assert!( + diagnostics.iter().any(|d| d.contains("is incorrectly declared as static")), + "io error should be reported. Got: {diagnostics:?}" + ); + assert!( + !diagnostics.iter().any(|d| d.contains("Unexpected number of arguments")), + "param error should be suppressed while an io error exists. Got: {diagnostics:?}" + ); +} + #[test] fn test_instance_input_output_query_members_have_no_static_diagnostic() { // The same declarations without `static` stay valid.