From a592904c691844d2c1aed00bd914edabef49f9b1 Mon Sep 17 00:00:00 2001 From: Payam Valadkhan Date: Tue, 30 Jan 2024 11:01:18 -0500 Subject: [PATCH] fix(compiler-cli): allow custom/duplicate decorators for @Injectable classes in local compilation mode (#54139) Custom/duplicate decorators break the deps tracker in local mode. But deps tracker only deals with non-injectable classes. So applying custom/duplicate decorators to `@Injectable` only classes does not disturb deps tracker and local compilation in general. There are also ~ 100 such cases in g3 which cannot be cleaned up. PR Close #54139 --- .../src/ngtsc/core/src/compiler.ts | 2 +- .../src/ngtsc/transform/src/compilation.ts | 81 ++++++++++++++----- .../ngtsc/transform/test/compilation_spec.ts | 3 +- .../test/ngtsc/local_compilation_spec.ts | 16 ++++ 4 files changed, 79 insertions(+), 23 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index af569bbc9d726..8206fef07f851 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -1181,7 +1181,7 @@ export class NgCompiler { const traitCompiler = new TraitCompiler( handlers, reflector, this.delegatingPerfRecorder, this.incrementalCompilation, this.options.compileNonExportedClasses !== false, compilationMode, dtsTransforms, - semanticDepGraphUpdater, this.adapter); + semanticDepGraphUpdater, this.adapter, isCore); // Template type-checking may use the `ProgramDriver` to produce new `ts.Program`(s). If this // happens, they need to be tracked by the `NgCompiler`. diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 531d1efa3df5d..74d216632e3fb 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -105,6 +105,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { private dtsTransforms: DtsTransformRegistry, private semanticDepGraphUpdater: SemanticDepGraphUpdater|null, private sourceFileTypeIdentifier: SourceFileTypeIdentifier, + private readonly isCore: boolean, ) { for (const handler of handlers) { this.handlersByName.set(handler.name, handler); @@ -259,11 +260,11 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { let record: ClassRecord|null = this.recordFor(clazz); let foundTraits: PendingTrait[] = []; - // A set to track the undetected decorators (= either non-Angular decorators or Angular - // duplicate decorators) in local compilation mode. An error will be issued if such decorators - // are found. - const undetectedDecorators = - this.compilationMode === CompilationMode.LOCAL ? new Set(decorators) : null; + // A set to track the detected decorators in local compilation mode. An error will be issued if + // undetected decorators (= either non-Angular decorators or Angular duplicate decorators) are + // found. + const detectedDecorators = + this.compilationMode === CompilationMode.LOCAL ? new Set() : null; for (const handler of this.handlers) { const result = handler.detect(clazz, decorators); @@ -271,8 +272,8 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { continue; } - if (undetectedDecorators !== null && result.decorator !== null) { - undetectedDecorators.delete(result.decorator); + if (detectedDecorators !== null && result.decorator !== null) { + detectedDecorators.add(result.decorator); } const isPrimaryHandler = handler.precedence === HandlerPrecedence.PRIMARY; @@ -342,20 +343,30 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { } } - if (undetectedDecorators !== null && undetectedDecorators.size > 0 && record !== null && - record.metaDiagnostics === null) { - // Custom decorators found in local compilation mode! In this mode we don't support custom - // decorators yet. But will eventually do (b/320536434). For now a temporary error is thrown. - record.metaDiagnostics = [...undetectedDecorators].map( - decorator => ({ - category: ts.DiagnosticCategory.Error, - code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED), - file: getSourceFile(clazz), - start: decorator.node.getStart(), - length: decorator.node.getWidth(), - messageText: - 'In local compilation mode, Angular does not support custom decorators or duplicate Angular decorators. Ensure all class decorators are from Angular and each decorator is used at most once for each class.', - })); + // Local compilation uses the `DepsTracker` utility, which at the moment cannot track classes + // mutated by custom/duplicate decorators. So we forbid custom/duplicate decorators on classes + // used by deps tracker, i.e., Component, Directive, etc (basically everything except + // Injectable) + // TODO(b/320536434) Support custom/duplicate decorators for the DepsTracker utility. + if (decorators !== null && detectedDecorators !== null && + detectedDecorators.size < decorators.length && record !== null && + record.metaDiagnostics === null && + hasDepsTrackerAffectingScopeDecorator(detectedDecorators, this.isCore)) { + // Custom or duplicate decorators found in local compilation mode for a class which has + // Angular decorators other than `@Injectable`! This is not supported yet. But will eventually + // do (b/320536434). For now a temporary error is thrown. + record.metaDiagnostics = + decorators.filter(decorator => !detectedDecorators.has(decorator)) + .map( + decorator => ({ + category: ts.DiagnosticCategory.Error, + code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED), + file: getSourceFile(clazz), + start: decorator.node.getStart(), + length: decorator.node.getWidth(), + messageText: + 'In local compilation mode, Angular does not support custom decorators or duplicate Angular decorators (except for `@Injectable` classes). Ensure all class decorators are from Angular and each decorator is used at most once for each class.', + })); record.traits = foundTraits = []; } @@ -715,3 +726,31 @@ function containsErrors(diagnostics: ts.Diagnostic[]|null): boolean { return diagnostics !== null && diagnostics.some(diag => diag.category === ts.DiagnosticCategory.Error); } + +/** + * Duplicating the logic of `isAngularDecorator` in the package `ngtsc/annotations` which cannot be + * imported here due to circular deps. This helper is needed temporary though until custom + * decorators are supported by the runtime `DepsTracker`. + */ +function isInjectableDecorator(decorator: Decorator, isCore: boolean): boolean { + if (isCore) { + return decorator.name === 'Injectable'; + } else if (decorator.import !== null && decorator.import.from === '@angular/core') { + return decorator.import.name === 'Injectable'; + } + return false; +} + +/** + * Scope decorators are those who participate in determining the scope of an NgModule or a + * standalone component. They are: `@Component`, `@Directive`, `@Pipe` and `@NgModule`. + */ +function hasDepsTrackerAffectingScopeDecorator(decoratorSet: Set, isCore: boolean) { + for (const decorator of decoratorSet) { + if (!isInjectableDecorator(decorator, isCore)) { + return true; + } + } + + return false; +} diff --git a/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts b/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts index ea67dc8eae061..fd1038f3b5146 100644 --- a/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts +++ b/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts @@ -40,7 +40,8 @@ runInEachFileSystem(() => { const reflectionHost = new TypeScriptReflectionHost(checker); const compiler = new TraitCompiler( handlers, reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true, - compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier); + compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier, + /** isCore */ false); const sourceFile = program.getSourceFile(filename)!; return {compiler, sourceFile, program, filename: _('/' + filename)}; diff --git a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts index f7a588cc9a9a5..680ed28c9a075 100644 --- a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts +++ b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts @@ -2040,6 +2040,22 @@ runInEachFileSystem( expect(text1).toContain( 'In local compilation mode, Angular does not support custom decorators or duplicate Angular decorators'); }); + + it('should not produce diagnostic for duplicate Injectable decorator', () => { + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + import {SomeServiceImpl} from './some-where'; + + @Injectable({providedIn: 'root', useExisting: SomeServiceImpl}) + @Injectable({providedIn: 'root'}) + export class SomeService { + } + `); + + const errors = env.driveDiagnostics(); + + expect(errors.length).toBe(0); + }); }); }); });