fix(compiler-cli): forbid custom/duplicate decorator when option `for…

…bidOrphanComponents` is set (#54139)

The deps tracker which is responsible to track orphan components does not work for classes mutated by custom decorator. Some work needed to make this happen (tracked in b/320536434). As a result, with option `forbidOrphanComponents` being true the deps tracker will falsely report any component as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them.

PR Close #54139

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, isCore);
+        semanticDepGraphUpdater, this.adapter, isCore, !!this.options.forbidOrphanComponents);
 
     // Template type-checking may use the `ProgramDriver` to produce new `ts.Program`(s). If this
     // happens, they need to be tracked by the `NgCompiler`.

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

@@ -97,16 +97,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
 
   constructor(
       private handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[],
-      private reflector: ReflectionHost,
-      private perf: PerfRecorder,
+      private reflector: ReflectionHost, private perf: PerfRecorder,
       private incrementalBuild: IncrementalBuild<ClassRecord, unknown>,
-      private compileNonExportedClasses: boolean,
-      private compilationMode: CompilationMode,
+      private compileNonExportedClasses: boolean, private compilationMode: CompilationMode,
       private dtsTransforms: DtsTransformRegistry,
       private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
-      private sourceFileTypeIdentifier: SourceFileTypeIdentifier,
-      private readonly isCore: boolean,
-  ) {
+      private sourceFileTypeIdentifier: SourceFileTypeIdentifier, private readonly isCore: boolean,
+      private readonly forbidOrphanComponents: boolean) {
     for (const handler of handlers) {
       this.handlersByName.set(handler.name, handler);
     }
@@ -260,11 +257,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
     let record: ClassRecord|null = this.recordFor(clazz);
     let foundTraits: PendingTrait<unknown, unknown, SemanticSymbol|null, unknown>[] = [];
 
-    // A set to track the detected decorators in local compilation mode. An error will be issued if
+    // A set to track the detected decorators in some cases. An error will be issued if
     // undetected decorators (= either non-Angular decorators or Angular duplicate decorators) are
-    // found.
+    // found in these cases.
     const detectedDecorators =
-        this.compilationMode === CompilationMode.LOCAL ? new Set<Decorator>() : null;
+        (this.compilationMode === CompilationMode.LOCAL || this.forbidOrphanComponents) ?
+        new Set<Decorator>() :
+        null;
 
     for (const handler of this.handlers) {
       const result = handler.detect(clazz, decorators);
@@ -343,30 +342,38 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
       }
     }
 
-    // Local compilation uses the `DepsTracker` utility, which at the moment cannot track classes
+    // Local compilation and the logic for detecting orphan components both use 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.metaDiagnostics === null && hasDepsTrackerAffectingScopeDecorator(detectedDecorators, this.isCore)) {
+      // Custom or duplicate decorators found for a class which has
+      // Angular decorators other than `@Injectable`! This is not supported yet in some cases. But
+      // will eventually do (b/320536434). For now a temporary error is thrown.
+
+      let messageText: string;
+      if (this.compilationMode === CompilationMode.LOCAL) {
+        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.';
+      } else if (this.forbidOrphanComponents) {
+        messageText =
+            'When the Angular compiler option "forbidOrphanComponents" is set, 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.';
+      } else {
+        throw new Error('Impossible state!');
+      }
+
+      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,
+                                        }));
       record.traits = foundTraits = [];
     }
 

packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts

@@ -41,7 +41,7 @@ runInEachFileSystem(() => {
       const compiler = new TraitCompiler(
           handlers, reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true,
           compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier,
-          /** isCore */ false);
+          /** isCore */ false, /** forbidOrphanComponents */ false);
       const sourceFile = program.getSourceFile(filename)!;
 
       return {compiler, sourceFile, program, filename: _('/' + filename)};

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -552,6 +552,78 @@ function allTests(os: string) {
       })]);
     });
 
+    it('should error for custom decorators when forbidOrphanComponents is true', () => {
+      env.tsconfig({
+        forbidOrphanComponents: true,
+      });
+      env.write('test.ts', `
+      import {Component} from '@angular/core';
+      
+      function customDecorator<T extends new (...args: any[]) => {}>(original: T) {
+        return class extends original {
+          someProp = 'default';
+        };
+      }
+
+      @Component({template: ''})
+      @customDecorator
+      class MyComp {}
+      `);
+
+      const diagnostics = env.driveDiagnostics();
+
+      expect(diagnostics).toEqual([jasmine.objectContaining({
+        messageText: jasmine.stringMatching(
+            /When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators or duplicate Angular decorators/),
+      })]);
+    });
+
+    it('should error for duplicate Component decorator when forbidOrphanComponents is true', () => {
+      env.tsconfig({
+        forbidOrphanComponents: true,
+      });
+      env.write('test.ts', `
+      import {Component} from '@angular/core';
+      
+      @Component({template: '1'})
+      @Component({template: '2'})
+      class MyComp {}
+      `);
+
+      const diagnostics = env.driveDiagnostics();
+
+      expect(diagnostics).toEqual([jasmine.objectContaining({
+        messageText: jasmine.stringMatching(
+            /When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators or duplicate Angular decorators/),
+      })]);
+    });
+
+    it('should not error for duplicate @Injectable decorators when forbidOrphanComponents is true',
+       () => {
+         env.tsconfig({
+           forbidOrphanComponents: true,
+         });
+         env.write('test.ts', `
+      import {Injectable} from '@angular/core';     
+
+      class SomeServiceImpl {
+        getMessage() {
+          return 'Hi!';
+        }
+      }
+      
+      @Injectable({providedIn: 'root', useExisting: SomeServiceImpl})
+      @Injectable({providedIn: 'root'})
+      export abstract class SomeService {
+        abstract getMessage(): string;
+      }
+      `);
+
+         const diagnostics = env.driveDiagnostics();
+
+         expect(diagnostics).toEqual([]);
+       });
+
     // This test triggers the Tsickle compiler which asserts that the file-paths
     // are valid for the real OS. When on non-Windows systems it doesn't like paths
     // that start with `C:`.