fix(ivy): force new imports for .d.ts files (#25080)

When ngtsc encounters a reference to a type (for example, a Component
type listed in an NgModule declarations array), it traces the import
of that type and attempts to determine the best way to refer to it.

In the event the type is defined in the same file where a reference
is being generated, the identifier of the type is used. If the type
was imported, ngtsc has a choice. It can use the identifier from the
original import, or it can write a new import to the module where the
type came from.

ngtsc has a bug currently when it elects to rely on the user's import.
When writing a .d.ts file, the user's import may have been elided as
the type was not referred to from the type side of the program. Thus,
in .d.ts files ngtsc must always assume the import may not exist, and
generate a new one.

In .js output the import is guaranteed to still exist, so it's
preferable for ngtsc to continue using the existing import if one is
available.

This commit changes how @angular/compiler writes type definitions, and
allows it to use a different expression to write a type definition than
is used to write the value. This allows ngtsc to specify that types in
type definitions should always be imported. A corresponding change to
the staticallyResolve() Reference system allows the choice of which
type of import to use when generating an Expression from a Reference.

PR Close #25080

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

@@ -14,7 +14,7 @@ import {Reference, ResolvedValue, reflectObjectLiteral, staticallyResolve} from
 import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform';
 
 import {SelectorScopeRegistry} from './selector_scope';
-import {getConstructorDependencies, isAngularCore, referenceToExpression, unwrapExpression} from './util';
+import {getConstructorDependencies, isAngularCore, toR3Reference, unwrapExpression} from './util';
 
 export interface NgModuleAnalysis {
   ngModuleDef: R3NgModuleMetadata;
@@ -87,9 +87,9 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
     const ngModuleDef: R3NgModuleMetadata = {
       type: new WrappedNodeExpr(node.name !),
       bootstrap: [],
-      declarations: declarations.map(decl => referenceToExpression(decl, context)),
-      exports: exports.map(exp => referenceToExpression(exp, context)),
-      imports: imports.map(imp => referenceToExpression(imp, context)),
+      declarations: declarations.map(decl => toR3Reference(decl, context)),
+      exports: exports.map(exp => toR3Reference(exp, context)),
+      imports: imports.map(imp => toR3Reference(imp, context)),
       emitInline: false,
     };
 

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

@@ -13,7 +13,7 @@ import {ReflectionHost} from '../../host';
 import {AbsoluteReference, Reference, reflectTypeEntityToDeclaration} from '../../metadata';
 import {reflectIdentifierOfDeclaration, reflectNameOfDeclaration} from '../../metadata/src/reflector';
 
-import {referenceToExpression} from './util';
+import {toR3Reference} from './util';
 
 
 
@@ -384,7 +384,7 @@ function convertReferenceMap(
     map: Map<string, Reference>, context: ts.SourceFile): Map<string, Expression> {
   return new Map<string, Expression>(Array.from(map.entries()).map(([selector, ref]): [
     string, Expression
-  ] => [selector, referenceToExpression(ref, context)]));
+  ] => [selector, toR3Reference(ref, context).value]));
 }
 
 function convertScopeToExpressions(

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

@@ -6,11 +6,11 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Expression, R3DependencyMetadata, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler';
+import {Expression, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import {Decorator, ReflectionHost} from '../../host';
-import {AbsoluteReference, Reference} from '../../metadata';
+import {AbsoluteReference, ImportMode, Reference} from '../../metadata';
 
 export function getConstructorDependencies(
     clazz: ts.ClassDeclaration, reflector: ReflectionHost,
@@ -79,12 +79,13 @@ export function getConstructorDependencies(
   return useType;
 }
 
-export function referenceToExpression(ref: Reference, context: ts.SourceFile): Expression {
-  const exp = ref.toExpression(context);
-  if (exp === null) {
+export function toR3Reference(ref: Reference, context: ts.SourceFile): R3Reference {
+  const value = ref.toExpression(context, ImportMode.UseExistingImport);
+  const type = ref.toExpression(context, ImportMode.ForceNewImport);
+  if (value === null || type === null) {
     throw new Error(`Could not refer to ${ts.SyntaxKind[ref.node.kind]}`);
   }
-  return exp;
+  return {value, type};
 }
 
 export function isAngularCore(decorator: Decorator): boolean {

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

@@ -7,4 +7,4 @@
  */
 
 export {TypeScriptReflectionHost, filterToMembersWithDecorator, findMember, reflectObjectLiteral, reflectTypeEntityToDeclaration} from './src/reflector';
-export {AbsoluteReference, Reference, ResolvedValue, isDynamicValue, staticallyResolve} from './src/resolver';
+export {AbsoluteReference, ImportMode, Reference, ResolvedValue, isDynamicValue, staticallyResolve} from './src/resolver';

packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts

@@ -78,6 +78,11 @@ export interface ResolvedValueArray extends Array<ResolvedValue> {}
  */
 type Scope = Map<ts.ParameterDeclaration, ResolvedValue>;
 
+export enum ImportMode {
+  UseExistingImport,
+  ForceNewImport,
+}
+
 /**
  * A reference to a `ts.Node`.
  *
@@ -99,7 +104,7 @@ export abstract class Reference<T extends ts.Node = ts.Node> {
    * This could be a local variable reference, if the symbol is imported, or it could be a new
    * import if needed.
    */
-  abstract toExpression(context: ts.SourceFile): Expression|null;
+  abstract toExpression(context: ts.SourceFile, importMode?: ImportMode): Expression|null;
 
   abstract withIdentifier(identifier: ts.Identifier): Reference;
 }
@@ -115,7 +120,7 @@ export class NodeReference<T extends ts.Node = ts.Node> extends Reference<T> {
 
   toExpression(context: ts.SourceFile): null { return null; }
 
-  withIdentifier(identifier: ts.Identifier): NodeReference { return this; }
+  withIdentifier(identifier: ts.Identifier): NodeReference<T> { return this; }
 }
 
 /**
@@ -128,8 +133,20 @@ export class ResolvedReference<T extends ts.Node = ts.Node> extends Reference<T>
 
   readonly expressable = true;
 
-  toExpression(context: ts.SourceFile): Expression {
-    if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) {
+  toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport):
+      Expression {
+    let compareCtx: ts.Node|null = null;
+    switch (importMode) {
+      case ImportMode.UseExistingImport:
+        compareCtx = this.identifier;
+        break;
+      case ImportMode.ForceNewImport:
+        compareCtx = this.node;
+        break;
+      default:
+        throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`);
+    }
+    if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) {
       return new WrappedNodeExpr(this.identifier);
     } else {
       // Relative import from context -> this.node.getSourceFile().
@@ -175,8 +192,20 @@ export class AbsoluteReference extends Reference {
 
   readonly expressable = true;
 
-  toExpression(context: ts.SourceFile): Expression {
-    if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) {
+  toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport):
+      Expression {
+    let compareCtx: ts.Node|null = null;
+    switch (importMode) {
+      case ImportMode.UseExistingImport:
+        compareCtx = this.identifier;
+        break;
+      case ImportMode.ForceNewImport:
+        compareCtx = this.node;
+        break;
+      default:
+        throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`);
+    }
+    if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) {
       return new WrappedNodeExpr(this.identifier);
     } else {
       return new ExternalExpr(new ExternalReference(this.moduleName, this.symbolName));

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

@@ -246,6 +246,65 @@ describe('ngtsc behavioral tests', () => {
     expect(dtsContents).toContain('static ngInjectorDef: i0.ɵInjectorDef');
   });
 
+  it('should compile NgModules with references to local components', () => {
+    writeConfig();
+    write('test.ts', `
+      import {NgModule} from '@angular/core';
+      import {Foo} from './foo';
+
+      @NgModule({
+        declarations: [Foo],
+      })
+      export class FooModule {}
+    `);
+    write('foo.ts', `
+      import {Component} from '@angular/core';
+      @Component({selector: 'foo', template: ''})
+      export class Foo {}
+    `);
+
+    const exitCode = main(['-p', basePath], errorSpy);
+    expect(errorSpy).not.toHaveBeenCalled();
+    expect(exitCode).toBe(0);
+
+    const jsContents = getContents('test.js');
+    const dtsContents = getContents('test.d.ts');
+
+    expect(jsContents).toContain('import { Foo } from \'./foo\';');
+    expect(jsContents).not.toMatch(/as i[0-9] from '.\/foo'/);
+    expect(dtsContents).toContain('as i1 from \'./foo\';');
+  });
+
+  it('should compile NgModules with references to absolute components', () => {
+    writeConfig();
+    write('test.ts', `
+      import {NgModule} from '@angular/core';
+      import {Foo} from 'foo';
+
+      @NgModule({
+        declarations: [Foo],
+      })
+      export class FooModule {}
+    `);
+    write('node_modules/foo/index.d.ts', `
+      import * as i0 from '@angular/core';
+      export class Foo {
+        static ngComponentDef: i0.ɵComponentDef<Foo, 'foo'>;
+      }
+    `);
+
+    const exitCode = main(['-p', basePath], errorSpy);
+    expect(errorSpy).not.toHaveBeenCalled();
+    expect(exitCode).toBe(0);
+
+    const jsContents = getContents('test.js');
+    const dtsContents = getContents('test.d.ts');
+
+    expect(jsContents).toContain('import { Foo } from \'foo\';');
+    expect(jsContents).not.toMatch(/as i[0-9] from 'foo'/);
+    expect(dtsContents).toContain('as i1 from \'foo\';');
+  });
+
   it('should compile Pipes without errors', () => {
     writeConfig();
     write('test.ts', `