fix(compiler-cli): allow custom/duplicate decorators for @Injectable classes in local compilation mode (#54139)

pmvald · thePunderWoman · commit a592904c6918 · 2024-02-05T15:07:40.000Z
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
diff --git 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
@@ -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,20 +260,20 @@ 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 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<Decorator>() : null;
 
     for (const handler of this.handlers) {
       const result = handler.detect(clazz, decorators);
       if (result === undefined) {
         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<Decorator>, 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
@@ -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
@@ -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);
+              });
             });
           });
     });
