fix(compiler-cli): prefer non-aliased exports in reference emitters (#41866)

This commit changes the reference emitters in the Ivy compiler to prefer
non-aliased exports if they exist. This avoids selecting "private
exports" that may not be stable, e.g. the reexports that have been added
by the View Engine compiler. Such reexports are not stable and are
therefore not suitable to be emitted into partial compilations, as the
output of partial compilations should only reference stable symbols
from upstream libraries.

An alternative solution has been considered where ViewEngine-generated
exports would gain a certain prefix, such that the Ivy compiler could
just exclude those exports (see #41443). However, that solution would
be insufficient in case a library is built using partial compilation and
while depending itself on a VE-compiled library from earlier versions of
Angular, where the magic prefix would be missing. For such libraries,
ngcc would have generated reexports using the declared name if not already
present so this change does result in choosing the correct export.

Because ngcc always generates reexports using the declared name even if
an aliased export is present, this change causes those ngcc-generated
exports to be chosen in downstream libraries using partial compilation.
This is unfortunate as it means that the declared names become
effectively public even if the library author was intentionally
exporting it using an alias. This commit does not address this problem;
it is expected that this should not result in widespread issues across
the library ecosystem.

Fixes #41277

PR Close #41866

Author: JoostK

packages/compiler-cli/src/ngtsc/imports/src/emitter.ts

@@ -12,7 +12,7 @@ import {UnifiedModulesHost} from '../../core/api';
 import {absoluteFromSourceFile, dirname, LogicalFileSystem, LogicalProjectPath, relative, toRelativeImport} from '../../file_system';
 import {stripExtension} from '../../file_system/src/util';
 import {DeclarationNode, ReflectionHost} from '../../reflection';
-import {getSourceFile, isDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript';
+import {getSourceFile, isDeclaration, isNamedDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript';
 
 import {findExportedNameOfNode} from './find_export';
 import {Reference} from './references';
@@ -255,9 +255,20 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy {
       return null;
     }
     const exportMap = new Map<DeclarationNode, string>();
-    exports.forEach((declaration, name) => {
+    for (const [name, declaration] of exports) {
+      if (exportMap.has(declaration.node)) {
+        // An export for this declaration has already been registered. We prefer an export that
+        // has the same name as the declared name, i.e. is not an aliased export. This is relevant
+        // for partial compilations where emitted references should import symbols using a stable
+        // name. This is particularly relevant for declarations inside VE-generated libraries, as
+        // such libraries contain private, unstable reexports of symbols.
+        const existingExport = exportMap.get(declaration.node)!;
+        if (isNamedDeclaration(declaration.node) && declaration.node.name.text === existingExport) {
+          continue;
+        }
+      }
       exportMap.set(declaration.node, name);
-    });
+    }
     return {module: entryPointFile, exportMap};
   }
 }

packages/compiler-cli/src/ngtsc/imports/src/find_export.ts

@@ -8,6 +8,7 @@
 
 import * as ts from 'typescript';
 import {ReflectionHost} from '../../reflection';
+import {isNamedDeclaration} from '../../util/src/typescript';
 
 /**
  * Find the name, if any, by which a node is exported from a given file.
@@ -18,18 +19,29 @@ export function findExportedNameOfNode(
   if (exports === null) {
     return null;
   }
+
+  const declaredName = isNamedDeclaration(target) ? target.name.text : null;
+
   // Look for the export which declares the node.
-  const keys = Array.from(exports.keys());
-  const name = keys.find(key => {
-    const decl = exports.get(key);
-    return decl !== undefined && decl.node === target;
-  });
+  let foundExportName: string|null = null;
+  for (const [exportName, declaration] of exports) {
+    if (declaration.node !== target) {
+      continue;
+    }
+
+    if (exportName === declaredName) {
+      // A non-alias export exists which is always preferred, so use that one.
+      return exportName;
+    }
+
+    foundExportName = exportName;
+  }
 
-  if (name === undefined) {
+  if (foundExportName === null) {
     throw new Error(
         `Failed to find exported name of node (${target.getText()}) in '${file.fileName}'.`);
   }
-  return name;
+  return foundExportName;
 }
 
 /**

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

@@ -11,6 +11,7 @@ ts_library(
     deps = [
         "//packages:types",
         "//packages/compiler",
+        "//packages/compiler-cli/src/ngtsc/core:api",
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/file_system/testing",
         "//packages/compiler-cli/src/ngtsc/imports",

packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts

@@ -7,12 +7,13 @@
  */
 import {ExternalExpr} from '@angular/compiler';
 import * as ts from 'typescript';
+import {UnifiedModulesHost} from '../../core/api';
 
-import {absoluteFrom as _, LogicalFileSystem} from '../../file_system';
+import {absoluteFrom as _, basename, LogicalFileSystem} from '../../file_system';
 import {runInEachFileSystem, TestFile} from '../../file_system/testing';
 import {Declaration, TypeScriptReflectionHost} from '../../reflection';
 import {getDeclaration, makeProgram} from '../../testing';
-import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy} from '../src/emitter';
+import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy, RelativePathStrategy, UnifiedModulesStrategy} from '../src/emitter';
 import {Reference} from '../src/references';
 import {ModuleResolver} from '../src/resolver';
 
@@ -49,6 +50,41 @@ runInEachFileSystem(() => {
       expect(emitted).toBeNull();
     });
 
+    it('should prefer non-aliased exports', () => {
+      const {strategy, program} = makeStrategy([
+        {
+          name: _('/node_modules/external.d.ts'),
+          contents: `
+             declare class Foo {}
+             export {Foo as A};
+             export {Foo};
+             export {Foo as B};
+           `,
+        },
+        {
+          name: _('/context.ts'),
+          contents: 'export class Context {}',
+        },
+      ]);
+      const decl =
+          getDeclaration(program, _('/node_modules/external.d.ts'), 'Foo', ts.isClassDeclaration);
+      const context = program.getSourceFile(_('/context.ts'))!;
+
+      const reference = new Reference(decl, {
+        specifier: 'external',
+        resolutionContext: context.fileName,
+      });
+      const emitted = strategy.emit(reference, context, ImportFlags.None);
+      if (emitted === null) {
+        return fail('Reference should be emitted');
+      }
+      if (!(emitted.expression instanceof ExternalExpr)) {
+        return fail('Reference should be emitted as ExternalExpr');
+      }
+      expect(emitted.expression.value.name).toEqual('Foo');
+      expect(emitted.expression.value.moduleName).toEqual('external');
+    });
+
     it('should generate an import using the exported name of the declaration', () => {
       const {strategy, program} = makeStrategy([
         {
@@ -173,5 +209,108 @@ runInEachFileSystem(() => {
       // Expect the prefixed name from the TestHost.
       expect((ref!.expression as ExternalExpr).value.name).toEqual('testFoo');
     });
+
+    it('should prefer non-aliased exports', () => {
+      const {program, host} = makeProgram([
+        {
+          name: _('/index.ts'),
+          contents: `
+             declare class Foo {}
+             export {Foo as A};
+             export {Foo};
+             export {Foo as B};
+           `,
+        },
+        {
+          name: _('/context.ts'),
+          contents: 'export class Context {}',
+        }
+      ]);
+      const checker = program.getTypeChecker();
+      const logicalFs = new LogicalFileSystem([_('/')], host);
+      const strategy = new LogicalProjectStrategy(new TypeScriptReflectionHost(checker), logicalFs);
+      const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration);
+      const context = program.getSourceFile(_('/context.ts'))!;
+      const emitted = strategy.emit(new Reference(decl), context);
+      if (emitted === null) {
+        return fail('Reference should be emitted');
+      }
+      if (!(emitted.expression instanceof ExternalExpr)) {
+        return fail('Reference should be emitted as ExternalExpr');
+      }
+      expect(emitted.expression.value.name).toEqual('Foo');
+      expect(emitted.expression.value.moduleName).toEqual('./index');
+    });
+  });
+
+  describe('RelativePathStrategy', () => {
+    it('should prefer non-aliased exports', () => {
+      const {program} = makeProgram([
+        {
+          name: _('/index.ts'),
+          contents: `
+             declare class Foo {}
+             export {Foo as A};
+             export {Foo};
+             export {Foo as B};
+           `,
+        },
+        {
+          name: _('/context.ts'),
+          contents: 'export class Context {}',
+        }
+      ]);
+      const checker = program.getTypeChecker();
+      const strategy = new RelativePathStrategy(new TypeScriptReflectionHost(checker));
+      const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration);
+      const context = program.getSourceFile(_('/context.ts'))!;
+      const emitted = strategy.emit(new Reference(decl), context);
+      if (emitted === null) {
+        return fail('Reference should be emitted');
+      }
+      if (!(emitted.expression instanceof ExternalExpr)) {
+        return fail('Reference should be emitted as ExternalExpr');
+      }
+      expect(emitted.expression.value.name).toEqual('Foo');
+      expect(emitted.expression.value.moduleName).toEqual('./index');
+    });
+  });
+
+  describe('UnifiedModulesStrategy', () => {
+    it('should prefer non-aliased exports', () => {
+      const {program} = makeProgram([
+        {
+          name: _('/index.ts'),
+          contents: `
+             declare class Foo {}
+             export {Foo as A};
+             export {Foo};
+             export {Foo as B};
+           `,
+        },
+        {
+          name: _('/context.ts'),
+          contents: 'export class Context {}',
+        }
+      ]);
+      const checker = program.getTypeChecker();
+      const host: UnifiedModulesHost = {
+        fileNameToModuleName(importedFilePath): string {
+          return basename(importedFilePath, '.ts');
+        }
+      };
+      const strategy = new UnifiedModulesStrategy(new TypeScriptReflectionHost(checker), host);
+      const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration);
+      const context = program.getSourceFile(_('/context.ts'))!;
+      const emitted = strategy.emit(new Reference(decl), context);
+      if (emitted === null) {
+        return fail('Reference should be emitted');
+      }
+      if (!(emitted.expression instanceof ExternalExpr)) {
+        return fail('Reference should be emitted as ExternalExpr');
+      }
+      expect(emitted.expression.value.name).toEqual('Foo');
+      expect(emitted.expression.value.moduleName).toEqual('index');
+    });
   });
 });

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

@@ -83,6 +83,11 @@ export function isTypeDeclaration(node: ts.Node): node is ts.EnumDeclaration|
       ts.isInterfaceDeclaration(node);
 }
 
+export function isNamedDeclaration(node: ts.Node): node is ts.Declaration&{name: ts.Identifier} {
+  const namedNode = node as {name?: ts.Identifier};
+  return namedNode.name !== undefined && ts.isIdentifier(namedNode.name);
+}
+
 export function isExported(node: DeclarationNode): boolean {
   let topLevel: ts.Node = node;
   if (ts.isVariableDeclaration(node) && ts.isVariableDeclarationList(node.parent)) {

packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js

@@ -567,3 +567,55 @@ export declare class TestCmp {
     static ɵcmp: i0.ɵɵComponentDeclaration<TestCmp, "test-cmp", never, {}, {}, never, never>;
 }
 
+/****************************************************************************************************
+ * PARTIAL FILE: library_exports.js
+ ****************************************************************************************************/
+// This test verifies that a directive from an external library is emitted using its declared name,
+// even in the presence of alias exports that could have been chosen.
+// See https://github.com/angular/angular/issues/41277.
+import { Component, NgModule } from '@angular/core';
+import { LibModule } from 'external_library';
+import * as i0 from "@angular/core";
+import * as i1 from "external_library";
+export class TestComponent {
+}
+TestComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestComponent, deps: [], target: i0.ɵɵFactoryTarget.Component });
+TestComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: TestComponent, selector: "ng-component", ngImport: i0, template: `
+    <lib-dir></lib-dir>
+  `, isInline: true, directives: [{ type: i1.LibDirective, selector: "lib-dir" }] });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestComponent, decorators: [{
+            type: Component,
+            args: [{
+                    template: `
+    <lib-dir></lib-dir>
+  `,
+                }]
+        }] });
+export class TestModule {
+}
+TestModule.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, deps: [], target: i0.ɵɵFactoryTarget.NgModule });
+TestModule.ɵmod = i0.ɵɵngDeclareNgModule({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, declarations: [TestComponent], imports: [LibModule] });
+TestModule.ɵinj = i0.ɵɵngDeclareInjector({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, imports: [[LibModule]] });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, decorators: [{
+            type: NgModule,
+            args: [{
+                    declarations: [TestComponent],
+                    imports: [LibModule],
+                }]
+        }] });
+
+/****************************************************************************************************
+ * PARTIAL FILE: library_exports.d.ts
+ ****************************************************************************************************/
+import * as i0 from "@angular/core";
+import * as i1 from "external_library";
+export declare class TestComponent {
+    static ɵfac: i0.ɵɵFactoryDeclaration<TestComponent, never>;
+    static ɵcmp: i0.ɵɵComponentDeclaration<TestComponent, "ng-component", never, {}, {}, never, never>;
+}
+export declare class TestModule {
+    static ɵfac: i0.ɵɵFactoryDeclaration<TestModule, never>;
+    static ɵmod: i0.ɵɵNgModuleDeclaration<TestModule, [typeof TestComponent], [typeof i1.LibModule], never>;
+    static ɵinj: i0.ɵɵInjectorDeclaration<TestModule>;
+}
+

packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json

@@ -178,6 +178,27 @@
       "inputFiles": [
         "non_literal_template_with_concatenation.ts"
       ]
+    },
+    {
+      "description": "should not use reexport names inside declarations when a direct export is available",
+      "inputFiles": [
+        "external_library.d.ts",
+        "library_exports.ts"
+      ],
+      "expectations": [
+        {
+          "failureMessage": "Invalid list of directives",
+          "files": [
+            "library_exports.js"
+          ]
+        }
+      ],
+      "compilerOptions": {
+        "baseUrl": ".",
+        "paths": {
+          "external_library": ["./external_library"]
+        }
+      }
     }
   ]
 }

packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/external_library.d.ts

@@ -0,0 +1,16 @@
+import * as ɵngcc0 from '@angular/core';
+
+declare class LibDirective {
+  static ɵfac: ɵngcc0.ɵɵFactoryDeclaration<LibDirective, never>;
+  static ɵdir: ɵngcc0.ɵɵDirectiveDeclaration<LibDirective, 'lib-dir', never, {}, {}, never>;
+}
+
+export declare class LibModule {
+  static ɵfac: ɵngcc0.ɵɵFactoryDeclaration<LibModule, never>;
+  static ɵmod: ɵngcc0.ɵɵNgModuleDeclaration<LibModule, [typeof LibDirective], never, [typeof LibDirective]>;
+  static ɵinj: ɵngcc0.ɵɵInjectorDeclaration<LibModule>;
+}
+
+export {LibDirective as ɵangular_packages_forms_forms_a}
+export {LibDirective}
+export {LibDirective as ɵangular_packages_forms_forms_b}

packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.js

@@ -0,0 +1,3 @@
+// NOTE: The declared name must have been used to refer to `LibDirective`, as the aliased exports
+// NOTE: are not stable and therefore not suitable for partial compilation outputs.
+directives: [$i0$.LibDirective]

packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.ts

@@ -0,0 +1,20 @@
+// This test verifies that a directive from an external library is emitted using its declared name,
+// even in the presence of alias exports that could have been chosen.
+// See https://github.com/angular/angular/issues/41277.
+import {Component, NgModule} from '@angular/core';
+import {LibModule} from 'external_library';
+
+@Component({
+  template: `
+    <lib-dir></lib-dir>
+  `,
+})
+export class TestComponent {
+}
+
+@NgModule({
+  declarations: [TestComponent],
+  imports: [LibModule],
+})
+export class TestModule {
+}