feat(ivy): detect cycles and use remote scoping of components if needed (angular#28169)

By its nature, Ivy alters the import graph of a TS program, adding imports
where template dependencies exist. For example, if ComponentA uses PipeB
in its template, Ivy will insert an import of PipeB into the file in which
ComponentA is declared.

Any insertion of an import into a program has the potential to introduce a
cycle into the import graph. If for some reason the file in which PipeB is
declared imports the file in which ComponentA is declared (maybe it makes
use of a service or utility function that happens to be in the same file as
ComponentA) then this could create an import cycle. This turns out to
happen quite regularly in larger Angular codebases.

TypeScript and the Ivy runtime have no issues with such cycles. However,
other tools are not so accepting. In particular the Closure Compiler is
very anti-cycle.

To mitigate this problem, it's necessary to detect when the insertion of
an import would create a cycle. ngtsc can then use a different strategy,
known as "remote scoping", instead of directly writing a reference from
one component to another. Under remote scoping, a function
'setComponentScope' is called after the declaration of the component's
module, which does not require the addition of new imports.

FW-647 #resolve

PR Close angular#28169

Author: alxhub

packages/compiler-cli/BUILD.bazel

@@ -24,6 +24,7 @@ ts_library(
     deps = [
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/annotations",
+        "//packages/compiler-cli/src/ngtsc/cycles",
         "//packages/compiler-cli/src/ngtsc/diagnostics",
         "//packages/compiler-cli/src/ngtsc/entry_point",
         "//packages/compiler-cli/src/ngtsc/imports",

packages/compiler-cli/src/ngcc/BUILD.bazel

@@ -12,6 +12,7 @@ ts_library(
         "//packages:types",
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/annotations",
+        "//packages/compiler-cli/src/ngtsc/cycles",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",
         "//packages/compiler-cli/src/ngtsc/reflection",

packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts

@@ -11,7 +11,8 @@ import * as fs from 'fs';
 import * as ts from 'typescript';
 
 import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader, SelectorScopeRegistry} from '../../../ngtsc/annotations';
-import {TsReferenceResolver} from '../../../ngtsc/imports';
+import {CycleAnalyzer, ImportGraph} from '../../../ngtsc/cycles';
+import {ModuleResolver, TsReferenceResolver} from '../../../ngtsc/imports';
 import {PartialEvaluator} from '../../../ngtsc/partial_evaluator';
 import {CompileResult, DecoratorHandler} from '../../../ngtsc/transform';
 import {DecoratedClass} from '../host/decorated_class';
@@ -65,12 +66,15 @@ export class DecorationAnalyzer {
   resolver = new TsReferenceResolver(this.program, this.typeChecker, this.options, this.host);
   scopeRegistry = new SelectorScopeRegistry(this.typeChecker, this.reflectionHost, this.resolver);
   evaluator = new PartialEvaluator(this.reflectionHost, this.typeChecker, this.resolver);
+  moduleResolver = new ModuleResolver(this.program, this.options, this.host);
+  importGraph = new ImportGraph(this.moduleResolver);
+  cycleAnalyzer = new CycleAnalyzer(this.importGraph);
   handlers: DecoratorHandler<any, any>[] = [
     new BaseDefDecoratorHandler(this.reflectionHost, this.evaluator),
     new ComponentDecoratorHandler(
         this.reflectionHost, this.evaluator, this.scopeRegistry, this.isCore, this.resourceManager,
-        this.rootDirs, /* defaultPreserveWhitespaces */ false,
-        /* i18nUseExternalIds */ true),
+        this.rootDirs, /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true,
+        this.moduleResolver, this.cycleAnalyzer),
     new DirectiveDecoratorHandler(
         this.reflectionHost, this.evaluator, this.scopeRegistry, this.isCore),
     new InjectableDecoratorHandler(this.reflectionHost, this.isCore),

packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel

@@ -10,6 +10,7 @@ ts_library(
     ]),
     deps = [
         "//packages/compiler",
+        "//packages/compiler-cli/src/ngtsc/cycles",
         "//packages/compiler-cli/src/ngtsc/diagnostics",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

@@ -6,12 +6,13 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, ElementSchemaRegistry, Expression, InterpolationConfig, R3ComponentMetadata, R3DirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
+import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, ElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, R3ComponentMetadata, R3DirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
 import * as path from 'path';
 import * as ts from 'typescript';
 
+import {CycleAnalyzer} from '../../cycles';
 import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
-import {ResolvedReference} from '../../imports';
+import {ModuleResolver, Reference, ResolvedReference} from '../../imports';
 import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
 import {Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection';
 import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform';
@@ -41,7 +42,8 @@ export class ComponentDecoratorHandler implements
       private reflector: ReflectionHost, private evaluator: PartialEvaluator,
       private scopeRegistry: SelectorScopeRegistry, private isCore: boolean,
       private resourceLoader: ResourceLoader, private rootDirs: string[],
-      private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean) {}
+      private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
+      private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer) {}
 
   private literalCache = new Map<Decorator, ts.ObjectLiteralExpression>();
   private elementSchemaRegistry = new DomElementSchemaRegistry();
@@ -289,28 +291,39 @@ export class ComponentDecoratorHandler implements
     }
   }
 
-  compile(node: ts.ClassDeclaration, analysis: ComponentHandlerData, pool: ConstantPool):
-      CompileResult {
+  resolve(node: ts.ClassDeclaration, analysis: ComponentHandlerData): void {
     // Check whether this component was registered with an NgModule. If so, it should be compiled
     // under that module's compilation scope.
     const scope = this.scopeRegistry.lookupCompilationScope(node);
     let metadata = analysis.meta;
     if (scope !== null) {
       // Replace the empty components and directives from the analyze() step with a fully expanded
-      // scope. This is possible now because during compile() the whole compilation unit has been
+      // scope. This is possible now because during resolve() the whole compilation unit has been
       // fully analyzed.
       const {pipes, containsForwardDecls} = scope;
-      const directives: {selector: string, expression: Expression}[] = [];
-
-      for (const meta of scope.directives) {
-        directives.push({selector: meta.selector, expression: meta.directive});
+      const directives =
+          scope.directives.map(dir => ({selector: dir.selector, expression: dir.directive}));
+
+      // Scan through the references of the `scope.directives` array and check whether
+      // any import which needs to be generated for the directive would create a cycle.
+      const origin = node.getSourceFile();
+      const cycleDetected =
+          scope.directives.some(meta => this._isCyclicImport(meta.directive, origin)) ||
+          Array.from(scope.pipes.values()).some(pipe => this._isCyclicImport(pipe, origin));
+      if (!cycleDetected) {
+        const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls;
+        metadata.directives = directives;
+        metadata.pipes = pipes;
+        metadata.wrapDirectivesAndPipesInClosure = wrapDirectivesAndPipesInClosure;
+      } else {
+        this.scopeRegistry.setComponentAsRequiringRemoteScoping(node);
       }
-      const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls;
-      metadata = {...metadata, directives, pipes, wrapDirectivesAndPipesInClosure};
     }
+  }
 
-    const res =
-        compileComponentFromMetadata(metadata, pool, makeBindingParser(metadata.interpolation));
+  compile(node: ts.ClassDeclaration, analysis: ComponentHandlerData, pool: ConstantPool):
+      CompileResult {
+    const res = compileComponentFromMetadata(analysis.meta, pool, makeBindingParser());
 
     const statements = res.statements;
     if (analysis.metadataStmt !== null) {
@@ -373,4 +386,19 @@ export class ComponentDecoratorHandler implements
     }
     return styleUrls as string[];
   }
+
+  private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean {
+    if (!(expr instanceof ExternalExpr)) {
+      return false;
+    }
+
+    // Figure out what file is being imported.
+    const imported = this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin);
+    if (imported === null) {
+      return false;
+    }
+
+    // Check whether the import is legal.
+    return this.cycleAnalyzer.wouldCreateCycle(origin, imported);
+  }
 }

packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Expression, LiteralArrayExpr, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler';
+import {Expression, ExternalExpr, InvokeFunctionExpr, LiteralArrayExpr, R3Identifiers, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
@@ -25,6 +25,7 @@ export interface NgModuleAnalysis {
   ngModuleDef: R3NgModuleMetadata;
   ngInjectorDef: R3InjectorMetadata;
   metadataStmt: Statement|null;
+  declarations: Reference<ts.Declaration>[];
 }
 
 /**
@@ -152,6 +153,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
       analysis: {
         ngModuleDef,
         ngInjectorDef,
+        declarations,
         metadataStmt: generateSetClassMetadataCall(node, this.reflector, this.isCore),
       },
       factorySymbolName: node.name !== undefined ? node.name.text : undefined,
@@ -165,6 +167,28 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
     if (analysis.metadataStmt !== null) {
       ngModuleStatements.push(analysis.metadataStmt);
     }
+    const context = node.getSourceFile();
+    for (const decl of analysis.declarations) {
+      if (this.scopeRegistry.requiresRemoteScope(decl.node)) {
+        const scope = this.scopeRegistry.lookupCompilationScopeAsRefs(decl.node);
+        if (scope === null) {
+          continue;
+        }
+        const directives: Expression[] = [];
+        const pipes: Expression[] = [];
+        scope.directives.forEach(
+            (directive, _) => { directives.push(directive.ref.toExpression(context) !); });
+        scope.pipes.forEach(pipe => pipes.push(pipe.toExpression(context) !));
+        const directiveArray = new LiteralArrayExpr(directives);
+        const pipesArray = new LiteralArrayExpr(pipes);
+        const declExpr = decl.toExpression(context) !;
+        const setComponentScope = new ExternalExpr(R3Identifiers.setComponentScope);
+        const callExpr =
+            new InvokeFunctionExpr(setComponentScope, [declExpr, directiveArray, pipesArray]);
+
+        ngModuleStatements.push(callExpr.toStmt());
+      }
+    }
     return [
       {
         name: 'ngModuleDef',

packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts

@@ -84,6 +84,11 @@ export class SelectorScopeRegistry {
    */
   private _pipeToName = new Map<ts.Declaration, string>();
 
+  /**
+   * Components that require remote scoping.
+   */
+  private _requiresRemoteScope = new Set<ts.Declaration>();
+
   /**
    * Map of components/directives/pipes to their module.
    */
@@ -132,6 +137,21 @@ export class SelectorScopeRegistry {
     this._pipeToName.set(node, name);
   }
 
+  /**
+   * Mark a component (identified by its `ts.Declaration`) as requiring its `directives` scope to be
+   * set remotely, from the file of the @NgModule which declares the component.
+   */
+  setComponentAsRequiringRemoteScoping(component: ts.Declaration): void {
+    this._requiresRemoteScope.add(component);
+  }
+
+  /**
+   * Check whether the given component requires its `directives` scope to be set remotely.
+   */
+  requiresRemoteScope(component: ts.Declaration): boolean {
+    return this._requiresRemoteScope.has(ts.getOriginalNode(component) as ts.Declaration);
+  }
+
   lookupCompilationScopeAsRefs(node: ts.Declaration): CompilationScope<Reference>|null {
     node = ts.getOriginalNode(node) as ts.Declaration;
 

packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel

@@ -12,6 +12,7 @@ ts_library(
         "//packages:types",
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/annotations",
+        "//packages/compiler-cli/src/ngtsc/cycles",
         "//packages/compiler-cli/src/ngtsc/diagnostics",
         "//packages/compiler-cli/src/ngtsc/imports",
         "//packages/compiler-cli/src/ngtsc/partial_evaluator",

packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts

@@ -8,8 +8,9 @@
 
 import * as ts from 'typescript';
 
+import {CycleAnalyzer, ImportGraph} from '../../cycles';
 import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
-import {TsReferenceResolver} from '../../imports';
+import {ModuleResolver, TsReferenceResolver} from '../../imports';
 import {PartialEvaluator} from '../../partial_evaluator';
 import {TypeScriptReflectionHost} from '../../reflection';
 import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript';
@@ -45,9 +46,13 @@ describe('ComponentDecoratorHandler', () => {
     const reflectionHost = new TypeScriptReflectionHost(checker);
     const resolver = new TsReferenceResolver(program, checker, options, host);
     const evaluator = new PartialEvaluator(reflectionHost, checker, resolver);
+    const moduleResolver = new ModuleResolver(program, options, host);
+    const importGraph = new ImportGraph(moduleResolver);
+    const cycleAnalyzer = new CycleAnalyzer(importGraph);
+
     const handler = new ComponentDecoratorHandler(
         reflectionHost, evaluator, new SelectorScopeRegistry(checker, reflectionHost, resolver),
-        false, new NoopResourceLoader(), [''], false, true);
+        false, new NoopResourceLoader(), [''], false, true, moduleResolver, cycleAnalyzer);
     const TestCmp = getDeclaration(program, 'entry.ts', 'TestCmp', ts.isClassDeclaration);
     const detected = handler.detect(TestCmp, reflectionHost.getDecoratorsOfDeclaration(TestCmp));
     if (detected === undefined) {

packages/compiler-cli/src/ngtsc/program.ts

@@ -14,6 +14,7 @@ import {nocollapseHack} from '../transformers/nocollapse_hack';
 
 import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry, SelectorScopeRegistry} from './annotations';
 import {BaseDefDecoratorHandler} from './annotations/src/base_def';
+import {CycleAnalyzer, ImportGraph} from './cycles';
 import {ErrorCode, ngErrorCode} from './diagnostics';
 import {FlatIndexGenerator, ReferenceGraph, checkForPrivateExports, findFlatIndexEntryPoint} from './entry_point';
 import {ImportRewriter, ModuleResolver, NoopImportRewriter, R3SymbolsImportRewriter, Reference, TsReferenceResolver} from './imports';
@@ -48,6 +49,7 @@ export class NgtscProgram implements api.Program {
 
   private constructionDiagnostics: ts.Diagnostic[] = [];
   private moduleResolver: ModuleResolver;
+  private cycleAnalyzer: CycleAnalyzer;
 
 
   constructor(
@@ -128,6 +130,7 @@ export class NgtscProgram implements api.Program {
 
     this.entryPoint = entryPoint !== null ? this.tsProgram.getSourceFile(entryPoint) || null : null;
     this.moduleResolver = new ModuleResolver(this.tsProgram, options, this.host);
+    this.cycleAnalyzer = new CycleAnalyzer(new ImportGraph(this.moduleResolver));
   }
 
   getTsProgram(): ts.Program { return this.tsProgram; }
@@ -184,6 +187,7 @@ export class NgtscProgram implements api.Program {
                           .filter(file => !file.fileName.endsWith('.d.ts'))
                           .map(file => this.compilation !.analyzeAsync(file))
                           .filter((result): result is Promise<void> => result !== undefined));
+    this.compilation.resolve();
   }
 
   listLazyRoutes(entryRoute?: string|undefined): api.LazyRoute[] {
@@ -213,6 +217,7 @@ export class NgtscProgram implements api.Program {
       this.tsProgram.getSourceFiles()
           .filter(file => !file.fileName.endsWith('.d.ts'))
           .forEach(file => this.compilation !.analyzeSync(file));
+      this.compilation.resolve();
     }
     return this.compilation;
   }
@@ -307,7 +312,7 @@ export class NgtscProgram implements api.Program {
       new ComponentDecoratorHandler(
           this.reflector, evaluator, scopeRegistry, this.isCore, this.resourceManager,
           this.rootDirs, this.options.preserveWhitespaces || false,
-          this.options.i18nUseExternalIds !== false),
+          this.options.i18nUseExternalIds !== false, this.moduleResolver, this.cycleAnalyzer),
       new DirectiveDecoratorHandler(this.reflector, evaluator, scopeRegistry, this.isCore),
       new InjectableDecoratorHandler(this.reflector, this.isCore),
       new NgModuleDecoratorHandler(

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

@@ -44,6 +44,15 @@ export interface DecoratorHandler<A, M> {
    */
   analyze(node: ts.Declaration, metadata: M): AnalysisOutput<A>;
 
+  /**
+   * Perform resolution on the given decorator along with the result of analysis.
+   *
+   * The resolution phase happens after the entire `ts.Program` has been analyzed, and gives the
+   * `DecoratorHandler` a chance to leverage information from the whole compilation unit to enhance
+   * the `analysis` before the emit phase.
+   */
+  resolve?(node: ts.Declaration, analysis: A): void;
+
   typeCheck?(ctx: TypeCheckContext, node: ts.Declaration, metadata: A): void;
 
   /**

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

@@ -163,6 +163,14 @@ export class IvyCompilation {
     }
   }
 
+  resolve(): void {
+    this.analysis.forEach((op, decl) => {
+      if (op.adapter.resolve !== undefined) {
+        op.adapter.resolve(decl, op.analysis);
+      }
+    });
+  }
+
   typeCheck(context: TypeCheckContext): void {
     this.typeCheckMap.forEach((handler, node) => {
       if (handler.typeCheck !== undefined) {