From ebf9b4c5589145350a64e132fe57d4c5c32cb889 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Wed, 28 Oct 2026 15:13:21 +0800 Subject: [PATCH] =?UTF-8?q?fix(partial):=20keep=20pipe=20ctor=20deps=20and?= =?UTF-8?q?=20inherited=20factories=20in=20=C9=B5=C9=B5ngDeclareFactory?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Partial-mode factories lost constructor information, which broke code silently after linking: - A @Pipe always got `deps: []`, so its constructor deps were dropped. `PipeMetadata::to_r3_metadata` now passes the extracted deps through (with the type-only flag), like ngtsc's `PipeDecoratorHandler`, which calls `compileDeclareFactory(toFactoryMetadata(analysis.meta, FactoryTarget.Pipe))` with the analyzed ctor deps (compiler-cli/src/ngtsc/annotations/src/pipe.ts). - A @Pipe, @Injectable or @NgModule with no constructor that extends a base class got `deps: []` instead of `deps: null`. `null` is what `getConstructorDependencies` returns for such a class (compiler-cli/src/ngtsc/annotations/common/src/di.ts), and `compileDependencies` (compiler/src/render3/partial/util.ts) emits it as-is so the linker uses `ɵɵgetInheritedFactory`. Directives and components already did this. Co-Authored-By: Claude Opus 5.5 --- .../src/partial/injectable.rs | 16 +-- .../src/partial/ng_module.rs | 15 +-- .../oxc_angular_compiler/src/partial/pipe.rs | 20 +-- .../src/pipe/decorator.rs | 29 ++++- .../oxc_angular_compiler/src/pipe/metadata.rs | 10 +- .../tests/partial_e2e_test.rs | 121 ++++++++++++++++++ 6 files changed, 172 insertions(+), 39 deletions(-) diff --git a/crates/oxc_angular_compiler/src/partial/injectable.rs b/crates/oxc_angular_compiler/src/partial/injectable.rs index e976a3083..5c9855ebd 100644 --- a/crates/oxc_angular_compiler/src/partial/injectable.rs +++ b/crates/oxc_angular_compiler/src/partial/injectable.rs @@ -184,18 +184,10 @@ fn clone_constructor_deps<'a>( deps: Option<&[R3DependencyMetadata<'a>]>, ) -> R3FactoryDeps<'a> { match deps { - // No constructor in source. Match upstream's behavior for - // non-inheriting parameterless classes (`deps: []` → no-arg - // `new Class()` factory). This is the common case. - // - // For a service that extends another class (uncommon enough to - // ignore), the optimal emit would be `R3FactoryDeps::None` so - // the linker uses `ɵɵgetInheritedFactory`. R3InjectableMetadata - // doesn't carry an inheritance flag, so we default to the - // safe-and-correct empty form. Inheriting services still get a - // working factory (plain `new`), just without the - // inherited-factory optimization. - None => R3FactoryDeps::Valid(Vec::new_in(&allocator)), + // No constructor, but a superclass: `deps: null`, so the linker + // uses `ɵɵgetInheritedFactory` (upstream `compileDependencies` in + // `packages/compiler/src/render3/partial/util.ts`). + None => R3FactoryDeps::None, Some(deps) => { let mut out = Vec::with_capacity_in(deps.len(), &allocator); for dep in deps { diff --git a/crates/oxc_angular_compiler/src/partial/ng_module.rs b/crates/oxc_angular_compiler/src/partial/ng_module.rs index 4fdba1a37..632b94918 100644 --- a/crates/oxc_angular_compiler/src/partial/ng_module.rs +++ b/crates/oxc_angular_compiler/src/partial/ng_module.rs @@ -85,14 +85,11 @@ pub fn compile_declare_ng_module_from_metadata<'a>( /// Builds the partial ɵfac factory paired with this NgModule. /// -/// `deps` is the module's extracted constructor dependencies (`None` when it -/// has no constructor). A class without a constructor gets `deps: []` to -/// match upstream's golden behavior (see compiler-cli/test/compliance/ -/// test_cases/r3_view_compiler/hello_world/GOLDEN_PARTIAL.js — every ɵfac -/// for an NgModule carries `deps: []`). The linker then generates the -/// simple `new MyModule()` factory. OXC doesn't track base classes, so the -/// no-ctor `deps: null` upstream uses for the inherited-factory pattern -/// becomes `[]` here — a suboptimal-but-correct factory. +/// `deps` is the module's extracted constructor dependencies: empty for a +/// class with neither a constructor nor a superclass (`deps: []`, a plain +/// `new MyModule()` factory), and `None` for a class with no constructor +/// that extends another class (`deps: null`, so the linker uses +/// `ɵɵgetInheritedFactory`). pub fn compile_declare_factory_for_ng_module<'a>( allocator: &'a Allocator, core_namespace: &'a str, @@ -119,7 +116,7 @@ pub fn compile_declare_factory_for_ng_module<'a>( } R3FactoryDeps::Valid(factory_deps) } - None => R3FactoryDeps::Valid(Vec::new_in(&allocator)), + None => R3FactoryDeps::None, }; let factory_meta = R3FactoryMetadata::Constructor(R3ConstructorFactoryMetadata { name: Ident::from("NgModuleFactory"), diff --git a/crates/oxc_angular_compiler/src/partial/pipe.rs b/crates/oxc_angular_compiler/src/partial/pipe.rs index 1856642de..9511e4794 100644 --- a/crates/oxc_angular_compiler/src/partial/pipe.rs +++ b/crates/oxc_angular_compiler/src/partial/pipe.rs @@ -139,17 +139,10 @@ fn clone_constructor_deps<'a>( deps: Option<&Vec<'a, crate::pipe::R3DependencyMetadata<'a>>>, ) -> R3FactoryDeps<'a> { match deps { - // No constructor in source. Match upstream's behavior for - // non-inheriting parameterless classes (`deps: []` → no-arg - // `new Class()` factory). This is the common case. - // - // For a Pipe that DOES extend another class (rare), the optimal - // emit would be `R3FactoryDeps::None` so the linker calls - // `ɵɵgetInheritedFactory`. OXC's Pipe metadata doesn't track - // inheritance today, so we default to the safe-and-correct empty - // form. An inheriting pipe still gets a working factory (plain - // `new`), just without the inherited-factory optimization. - None => R3FactoryDeps::Valid(Vec::new_in(&allocator)), + // No constructor, but a superclass: `deps: null`, so the linker + // uses `ɵɵgetInheritedFactory` (upstream `compileDependencies` in + // `packages/compiler/src/render3/partial/util.ts`). + None => R3FactoryDeps::None, Some(deps) => { let mut out = Vec::with_capacity_in(deps.len(), &allocator); for dep in deps { @@ -165,10 +158,7 @@ fn clone_constructor_deps<'a>( optional: dep.optional, self_: dep.self_, skip_self: dep.skip_self, - // Pipe's R3DependencyMetadata predates the type-only-invalid - // tracking (#288); the field doesn't exist on this struct, - // so we default to false. - type_only_invalid: false, + type_only_invalid: dep.type_only_invalid, }); } R3FactoryDeps::Valid(out) diff --git a/crates/oxc_angular_compiler/src/pipe/decorator.rs b/crates/oxc_angular_compiler/src/pipe/decorator.rs index 440430d78..9e7c3077e 100644 --- a/crates/oxc_angular_compiler/src/pipe/decorator.rs +++ b/crates/oxc_angular_compiler/src/pipe/decorator.rs @@ -8,7 +8,7 @@ use oxc_ast::ast::{Argument, Class, Decorator, Expression, ObjectPropertyKind, P use oxc_span::Span; use oxc_str::Ident; -use super::metadata::R3PipeMetadata; +use super::metadata::{R3DependencyMetadata as PipeDependencyMetadata, R3PipeMetadata}; use crate::directive::StringConsts; use crate::factory::R3DependencyMetadata; use crate::output::ast::{OutputExpression, ReadVarExpr}; @@ -71,12 +71,37 @@ impl<'a> PipeMetadata<'a> { &allocator, )); + // Constructor dependencies feed the pipe's factory. `None` (no + // constructor, but a superclass) stays `None` so the factory is + // inherited, like ngtsc's `ctorDeps` in `PipeDecoratorHandler.analyze`. + let deps = self.deps.as_ref().map(|deps| { + let mut out = Vec::with_capacity_in(deps.len(), &allocator); + for dep in deps { + out.push(PipeDependencyMetadata { + token: dep + .token + .as_ref() + .map(|t| Box::new_in(t.clone_in(allocator), &allocator)), + attribute_name_type: dep + .attribute_name_type + .as_ref() + .map(|a| Box::new_in(a.clone_in(allocator), &allocator)), + host: dep.host, + optional: dep.optional, + self_: dep.self_, + skip_self: dep.skip_self, + type_only_invalid: dep.type_only_invalid, + }); + } + out + }); + Some(R3PipeMetadata { name: self.class_name.clone(), pipe_name: self.pipe_name.clone(), r#type: type_expr, type_argument_count: 0, - deps: None, + deps, pure: self.pure, is_standalone: self.standalone, }) diff --git a/crates/oxc_angular_compiler/src/pipe/metadata.rs b/crates/oxc_angular_compiler/src/pipe/metadata.rs index 4c57fd32c..72056dac3 100644 --- a/crates/oxc_angular_compiler/src/pipe/metadata.rs +++ b/crates/oxc_angular_compiler/src/pipe/metadata.rs @@ -26,7 +26,8 @@ pub struct R3PipeMetadata<'a> { pub type_argument_count: u32, /// Constructor dependencies for dependency injection. - /// None means use default factory (no-arg constructor). + /// `None` means the class has no constructor but extends another class, + /// so its factory is inherited (`ɵɵgetInheritedFactory`). pub deps: Option>>, /// Whether the pipe is pure (default: true). @@ -61,6 +62,11 @@ pub struct R3DependencyMetadata<'a> { /// Whether `@SkipSelf()` decorator is present. pub skip_self: bool, + + /// Whether the token came from a type-only import, which is erased at + /// runtime. Any such dependency makes the factory `ɵɵinvalidFactory()`. + /// See issue #288. + pub type_only_invalid: bool, } impl<'a> R3DependencyMetadata<'a> { @@ -73,6 +79,7 @@ impl<'a> R3DependencyMetadata<'a> { optional: false, self_: false, skip_self: false, + type_only_invalid: false, } } @@ -85,6 +92,7 @@ impl<'a> R3DependencyMetadata<'a> { optional: false, self_: false, skip_self: false, + type_only_invalid: false, } } } diff --git a/crates/oxc_angular_compiler/tests/partial_e2e_test.rs b/crates/oxc_angular_compiler/tests/partial_e2e_test.rs index 49d01e120..df5d3f258 100644 --- a/crates/oxc_angular_compiler/tests/partial_e2e_test.rs +++ b/crates/oxc_angular_compiler/tests/partial_e2e_test.rs @@ -120,3 +120,124 @@ export class AbstractService {} result.code ); } + +/// Transforms `source` in partial mode, then links the result. Returns the +/// partial output and the linked output, both with whitespace removed. +fn partial_and_linked(source: &str) -> (String, String) { + let allocator = Allocator::default(); + let options = + TransformOptions { compilation_mode: CompilationMode::Partial, ..Default::default() }; + let result = transform_angular_file(&allocator, "case.ts", source, Some(&options), None); + assert!(!result.has_errors(), "should not have errors, got: {:?}", result.diagnostics); + let linked = link(&allocator, &result.code, "case.ts"); + assert!(linked.linked, "linker should accept the partial output:\n{}", result.code); + let strip = |s: &str| s.chars().filter(|c| !c.is_whitespace()).collect::(); + (strip(&result.code), strip(&linked.code)) +} + +#[test] +fn partial_pipe_factory_keeps_constructor_deps() { + // ngtsc: ɵɵngDeclareFactory({..., type: P, deps: [{ token: TOK }, + // { token: Ok, optional: true }], target: i0.ɵɵFactoryTarget.Pipe }) + let (partial, linked) = partial_and_linked( + "import { Pipe, Inject, Optional, InjectionToken } from '@angular/core'; +const TOK = new InjectionToken('t'); +class Ok {} +@Pipe({ name: 'p' }) +export class P { + constructor(@Inject(TOK) t: string, @Optional() o: Ok) {} + transform(v: any) { return v; } +} +", + ); + assert!( + partial.contains("type:P,deps:[{token:TOK") + && partial.contains("},{token:Ok,optional:true}],target:i0.ɵɵFactoryTarget.Pipe"), + "pipe factory should keep its constructor deps, got:\n{partial}" + ); + assert!( + linked.contains( + "new(__ngFactoryType__||P)(i0.ɵɵdirectiveInject(TOK,16),i0.ɵɵdirectiveInject(Ok,24))" + ), + "linked pipe factory should inject TOK and Ok, got:\n{linked}" + ); +} + +#[test] +fn partial_pipe_factory_without_constructor() { + // ngtsc: a pipe with no constructor and no superclass gets `deps: []`; + // one that extends a base class gets `deps: null` (inherited factory). + let (partial, _) = partial_and_linked( + "import { Pipe } from '@angular/core'; +@Pipe({ name: 'p' }) +export class P { transform(v: any) { return v; } } +", + ); + assert!( + partial.contains("type:P,deps:[],target:i0.ɵɵFactoryTarget.Pipe"), + "pipe without a constructor should get deps:[], got:\n{partial}" + ); + + let (partial, linked) = partial_and_linked( + "import { Pipe } from '@angular/core'; +class Ok {} +class Base { constructor(o: Ok) {} } +@Pipe({ name: 'p' }) +export class P extends Base { transform(v: any) { return v; } } +", + ); + assert!( + partial.contains("type:P,deps:null,target:i0.ɵɵFactoryTarget.Pipe"), + "pipe inheriting its constructor should get deps:null, got:\n{partial}" + ); + assert!( + linked.contains("i0.ɵɵgetInheritedFactory(P)"), + "linked pipe factory should be inherited, got:\n{linked}" + ); +} + +#[test] +fn partial_injectable_factory_inherits_constructor() { + // ngtsc: ɵɵngDeclareFactory({..., type: S, deps: null, + // target: i0.ɵɵFactoryTarget.Injectable }) + let (partial, linked) = partial_and_linked( + "import { Injectable } from '@angular/core'; +class Ok {} +@Injectable() export class B { constructor(o: Ok) {} } +@Injectable() export class S extends B {} +", + ); + assert!( + partial.contains("type:S,deps:null,target:i0.ɵɵFactoryTarget.Injectable"), + "injectable inheriting its constructor should get deps:null, got:\n{partial}" + ); + assert!( + linked.contains("new(__ngFactoryType__||B)(i0.ɵɵinject(Ok))"), + "linked base injectable factory should inject Ok, got:\n{linked}" + ); + assert!( + linked.contains("i0.ɵɵgetInheritedFactory(S)"), + "linked injectable factory should be inherited, got:\n{linked}" + ); +} + +#[test] +fn partial_ng_module_factory_inherits_constructor() { + // ngtsc: ɵɵngDeclareFactory({..., type: M, deps: null, + // target: i0.ɵɵFactoryTarget.NgModule }) + let (partial, linked) = partial_and_linked( + "import { NgModule } from '@angular/core'; +class Ok {} +export class B { constructor(o: Ok) {} } +@NgModule({}) export class M extends B {} +", + ); + assert!( + partial.contains("type:M,deps:null,target:i0.ɵɵFactoryTarget.NgModule"), + "NgModule inheriting its constructor should get deps:null, got:\n{partial}" + ); + assert!( + linked.contains("i0.ɵɵgetInheritedFactory(M)"), + "linked NgModule factory should be inherited, got:\n{linked}" + ); +}