wip: explicit reference tracking

This implements a potential way of identifying the impact of export
shenanigans. The implementation here is up for discussion; it's mostly a
quick first attempt to get a feeling of how it could work.

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

@@ -16,8 +16,8 @@ import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from
 import {DependencyTracker} from '../../incremental/api';
 import {IndexingContext} from '../../indexer';
 import {ClassPropertyMapping, ComponentResources, DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, Resource, ResourceRegistry} from '../../metadata';
-import {SemanticDepGraphUpdater, SemanticSymbol} from '../../ngmodule_semantics';
-import {isArrayEqual, isSymbolEqual} from '../../ngmodule_semantics/src/util';
+import {SemanticDepGraphUpdater, SemanticReference, SemanticSymbol} from '../../ngmodule_semantics';
+import {isArrayEqual, isReferenceEqual} from '../../ngmodule_semantics/src/util';
 import {EnumValue, PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
 import {ClassDeclaration, DeclarationNode, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
 import {ComponentScopeReader, LocalModuleScopeRegistry, TypeCheckScopeRegistry} from '../../scope';
@@ -118,8 +118,8 @@ export const enum ResourceTypeForDiagnostics {
  * Represents an Angular component.
  */
 export class ComponentSymbol extends DirectiveSymbol {
-  usedDirectives: SemanticSymbol[] = [];
-  usedPipes: SemanticSymbol[] = [];
+  usedDirectives: SemanticReference[] = [];
+  usedPipes: SemanticReference[] = [];
   isRemotelyScoped = false;
 
   isEmitAffected(previousSymbol: SemanticSymbol, publicApiAffected: Set<SemanticSymbol>): boolean {
@@ -130,8 +130,8 @@ export class ComponentSymbol extends DirectiveSymbol {
     // Create an equality function that considers symbols equal if they represent the same
     // declaration, but only if the symbol in the current compilation does not have its public API
     // affected.
-    const isSymbolAffected = (current: SemanticSymbol, previous: SemanticSymbol) =>
-        isSymbolEqual(current, previous) && !publicApiAffected.has(current);
+    const isSymbolAffected = (current: SemanticReference, previous: SemanticReference) =>
+        isReferenceEqual(current, previous) && !publicApiAffected.has(current.symbol);
 
     // The emit of a component is affected if either of the following is true:
     //  1. The component used to be remotely scoped but no longer is, or vice versa.
@@ -607,10 +607,11 @@ export class ComponentDecoratorHandler implements
         });
       }
       if (this.semanticDepGraphUpdater !== null) {
-        symbol.usedDirectives =
-            usedDirectives.map(dir => this.semanticDepGraphUpdater!.getSymbol(dir.ref.node));
-        symbol.usedPipes =
-            usedPipes.map(pipe => this.semanticDepGraphUpdater!.getSymbol(pipe.ref.node));
+        symbol.usedDirectives = usedDirectives.map(
+            dir => this.semanticDepGraphUpdater!.getSemanticReference(dir.ref.node, dir.type));
+        symbol.usedPipes = usedPipes.map(
+            pipe =>
+                this.semanticDepGraphUpdater!.getSemanticReference(pipe.ref.node, pipe.expression));
       }
 
       // Scan through the directives/pipes actually used in the template and check whether any

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

@@ -12,8 +12,7 @@ import * as ts from 'typescript';
 import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics';
 import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports';
 import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
-import {SemanticSymbol} from '../../ngmodule_semantics/src/api';
-import {isArrayEqual, isSymbolEqual} from '../../ngmodule_semantics/src/util';
+import {isArrayEqual, isReferenceEqual, isSymbolEqual, SemanticReference, SemanticSymbol} from '../../ngmodule_semantics';
 import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
 import {ClassDeclaration, Decorator, isNamedClassDeclaration, ReflectionHost, reflectObjectLiteral, typeNodeToValueExpr} from '../../reflection';
 import {NgModuleRouteAnalyzer} from '../../routing';
@@ -50,9 +49,11 @@ export interface NgModuleResolution {
  * Represents an Angular NgModule.
  */
 export class NgModuleSymbol extends SemanticSymbol {
-  private remotelyScopedComponents:
-      {component: SemanticSymbol, usedDirectives: SemanticSymbol[], usedPipes: SemanticSymbol[]}[] =
-          [];
+  private remotelyScopedComponents: {
+    component: SemanticSymbol,
+    usedDirectives: SemanticReference[],
+    usedPipes: SemanticReference[]
+  }[] = [];
 
   isPublicApiAffected(previousSymbol: SemanticSymbol): boolean {
     if (!(previousSymbol instanceof NgModuleSymbol)) {
@@ -84,7 +85,7 @@ export class NgModuleSymbol extends SemanticSymbol {
         return true;
       }
 
-      if (!isArrayEqual(currEntry.usedDirectives, prevEntry.usedDirectives, isSymbolEqual)) {
+      if (!isArrayEqual(currEntry.usedDirectives, prevEntry.usedDirectives, isReferenceEqual)) {
         // The list of used directives or their order has changed. Since this NgModule emits
         // references to the list of used directives, it should be re-emitted to update this list.
         // Note: the NgModule does not have to be re-emitted when any of the directives has had
@@ -93,16 +94,16 @@ export class NgModuleSymbol extends SemanticSymbol {
         return true;
       }
 
-      if (!isArrayEqual(currEntry.usedPipes, prevEntry.usedPipes, isSymbolEqual)) {
+      if (!isArrayEqual(currEntry.usedPipes, prevEntry.usedPipes, isReferenceEqual)) {
         return true;
       }
     }
     return false;
   }
 
   addRemotelyScopedComponent(
-      component: SemanticSymbol, usedDirectives: SemanticSymbol[],
-      usedPipes: SemanticSymbol[]): void {
+      component: SemanticSymbol, usedDirectives: SemanticReference[],
+      usedPipes: SemanticReference[]): void {
     this.remotelyScopedComponents.push({component, usedDirectives, usedPipes});
   }
 }

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

@@ -8,6 +8,7 @@ ts_library(
         "src/**/*.ts",
     ]),
     deps = [
+        "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/incremental:api",
         "//packages/compiler-cli/src/ngtsc/metadata",

packages/compiler-cli/src/ngtsc/ngmodule_semantics/index.ts

@@ -6,6 +6,6 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-export {SemanticSymbol} from './src/api';
+export {SemanticReference, SemanticSymbol} from './src/api';
 export {SemanticDepGraph, SemanticDepGraphUpdater} from './src/graph';
-export {isArrayEqual, isSymbolEqual} from './src/util';
+export {isArrayEqual, isReferenceEqual, isSymbolEqual} from './src/util';

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

@@ -72,38 +72,30 @@ export abstract class SemanticSymbol {
   isEmitAffected?(previousSymbol: SemanticSymbol, publicApiAffected: Set<SemanticSymbol>): boolean;
 }
 
+/**
+ * Represents a reference to a semantic symbol that has been emitted into a source file. The
+ * reference may refer to the symbol using a different name than the semantic symbol's declared
+ * name, e.g. in case a re-export under a different name was chosen by a reference emitter.
+ * Consequently, to know that an emitted reference is still valid not only requires that the
+ * semantic symbol is still valid, but also that the path by which the symbol is imported has not
+ * changed.
+ */
+export interface SemanticReference {
+  symbol: SemanticSymbol;
+
+  /**
+   * The path by which the symbol has been referenced.
+   */
+  importPath: string|null;
+}
+
 function getSymbolIdentifier(decl: ClassDeclaration): string|null {
   if (!ts.isSourceFile(decl.parent)) {
     return null;
   }
 
-  if (!hasExportModifier(decl)) {
-    // If the declaration is not itself exported, then it is still possible for the declaration
-    // to be exported elsewhere, possibly using a different exported name. Therefore, we cannot
-    // consider the declaration's own name as its unique identifier.
-    //
-    // For example, renaming the name by which this declaration is exported without renaming the
-    // class declaration itself requires that any references to the declarations must be re-emitted
-    // to use its new exported name. The semantic dependency graph would be unaware of this rename
-    // however, hence non-exported declarations are excluded from semantic tracking by not assigning
-    // them a unique identifier.
-    //
-    // This relies on the assumption that the reference emitter prefers the direct export of the
-    // declaration. This is currently not the case however; the reference emitter chooses the first
-    // export in the source file that corresponds with the reference. As such, if a class is itself
-    // exported _and_ a secondary export of the class appears above it, renaming that secondary
-    // export would not currently trigger re-emit of any symbols that refer to the declaration by
-    // its previous name.
-    return null;
-  }
-
   // If this is a top-level class declaration, the class name is used as unique identifier.
   // Other scenarios are currently not supported and causes the symbol not to be identified
   // across rebuilds, unless the declaration node has not changed.
   return decl.name.text;
 }
-
-function hasExportModifier(decl: ClassDeclaration): boolean {
-  return decl.modifiers !== undefined &&
-      decl.modifiers.some(mod => mod.kind === ts.SyntaxKind.ExportKeyword);
-}

packages/compiler-cli/src/ngtsc/ngmodule_semantics/src/graph.ts

@@ -6,10 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {Expression, ExternalExpr} from '@angular/compiler';
+
 import {AbsoluteFsPath} from '../../file_system';
 import {ClassDeclaration} from '../../reflection';
 
-import {SemanticSymbol} from './api';
+import {SemanticReference, SemanticSymbol} from './api';
 
 export interface SemanticDependencyResult {
   /**
@@ -189,6 +191,13 @@ export class SemanticDepGraphUpdater {
     return needsEmit;
   }
 
+  getSemanticReference(decl: ClassDeclaration, expr: Expression): SemanticReference {
+    return {
+      symbol: this.getSymbol(decl),
+      importPath: getImportPath(expr),
+    };
+  }
+
   getSymbol(decl: ClassDeclaration): SemanticSymbol {
     const symbol = this.newGraph.getSymbolByDecl(decl);
     if (symbol === null) {
@@ -213,3 +222,11 @@ export class SemanticDepGraphUpdater {
     return symbol;
   }
 }
+
+function getImportPath(expr: Expression): string|null {
+  if (expr instanceof ExternalExpr) {
+    return `${expr.value.moduleName}\$${expr.value.name}`;
+  } else {
+    return null;
+  }
+}

packages/compiler-cli/src/ngtsc/ngmodule_semantics/src/util.ts

@@ -5,7 +5,7 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {SemanticSymbol} from './api';
+import {SemanticReference, SemanticSymbol} from './api';
 
 /**
  * Determines whether the provided symbols represent the same declaration.
@@ -24,6 +24,24 @@ export function isSymbolEqual(a: SemanticSymbol, b: SemanticSymbol): boolean {
   return a.path === b.path && a.identifier === b.identifier;
 }
 
+/**
+ * Determines whether the provided references to a semantic symbol are still equal, i.e. represent
+ * the same symbol and are imported by the same path.
+ */
+export function isReferenceEqual(a: SemanticReference, b: SemanticReference): boolean {
+  if (!isSymbolEqual(a.symbol, b.symbol)) {
+    // If the reference's target symbols are different, the reference itself is different.
+    return false;
+  }
+
+  if (a.importPath === null || b.importPath === null) {
+    // If no import path is known for either of the references they are considered different.
+    return false;
+  }
+
+  return a.importPath === b.importPath;
+}
+
 export function referenceEquality<T>(a: T, b: T): boolean {
   return a === b;
 }

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

@@ -717,9 +717,8 @@ runInEachFileSystem(() => {
         ]);
       });
 
-      xit('should recompile components when the name by which they are exported changes (2)',
-          () => {
-            env.write('cmp-user.ts', `
+      it('should recompile components when a re-export is renamed', () => {
+        env.write('cmp-user.ts', `
           import {Component} from '@angular/core';
 
           @Component({
@@ -728,7 +727,7 @@ runInEachFileSystem(() => {
           })
           export class CmpUser {}
         `);
-            env.write('cmp-dep.ts', `
+        env.write('cmp-dep.ts', `
           import {Component} from '@angular/core';
 
           export {CmpDep as CmpDepExport};
@@ -739,7 +738,7 @@ runInEachFileSystem(() => {
           })
           export class CmpDep {}
         `);
-            env.write('module.ts', `
+        env.write('module.ts', `
           import {NgModule} from '@angular/core';
           import {CmpUser} from './cmp-user';
           import {CmpDepExport} from './cmp-dep';
@@ -750,14 +749,14 @@ runInEachFileSystem(() => {
           export class Module {}
         `);
 
-            env.driveMain();
+        env.driveMain();
 
-            // Verify that the reference emitter used the export of `CmpDep` that appeared first in
-            // the source, i.e. `CmpDepExport`.
-            const userCmpJs = env.getContents('cmp-user.js');
-            expect(userCmpJs).toContain('CmpDepExport');
+        // Verify that the reference emitter used the export of `CmpDep` that appeared first in
+        // the source, i.e. `CmpDepExport`.
+        const userCmpJs = env.getContents('cmp-user.js');
+        expect(userCmpJs).toContain('CmpDepExport');
 
-            env.write('cmp-dep.ts', `
+        env.write('cmp-dep.ts', `
           import {Component} from '@angular/core';
 
           export {CmpDep as CmpDepExport2};
@@ -768,7 +767,7 @@ runInEachFileSystem(() => {
           })
           export class CmpDep {}
         `);
-            env.write('module.ts', `
+        env.write('module.ts', `
           import {NgModule} from '@angular/core';
           import {CmpUser} from './cmp-user';
           import {CmpDepExport2} from './cmp-dep';
@@ -779,19 +778,24 @@ runInEachFileSystem(() => {
           export class Module {}
         `);
 
-            env.flushWrittenFileTracking();
-            env.driveMain();
+        env.flushWrittenFileTracking();
+        env.driveMain();
+
+        expectToHaveWritten([
+          // CmpDep and its module were directly updated.
+          '/cmp-dep.js',
+          '/module.js',
 
-            expectToHaveWritten([
-              // CmpDep and its module were directly updated.
-              '/cmp-dep.js',
-              '/module.js',
+          // CmpUser required a re-emit because it was previous emitted as `CmpDepExport`, but
+          // that export has since been renamed.
+          '/cmp-user.js',
+        ]);
 
-              // CmpUser required a re-emit because it was previous emitted as `CmpDepExport`, but
-              // that export has since been renamed.
-              '/cmp-user.js',
-            ]);
-          });
+        // Verify that `CmpUser` now correctly imports `CmpDep` using its renamed
+        // re-export `CmpDepExport2`.
+        const userCmp2Js = env.getContents('cmp-user.js');
+        expect(userCmp2Js).toContain('CmpDepExport2');
+      });
 
 
       it('should not recompile components when a directive is changed into a component', () => {