fix(compiler-cli): use correct module resolution context for absolute…

… imports in .d.ts files (angular#42879)

The compiler keeps track of how a declaration has been referenced
using absolute module imports and from which path the absolute module
should be resolved from. There was a bug in how the .d.ts metadata
extraction would incorrectly use the .d.ts file itself as resolution
context for symbols that had been imported using a relative module
specifier. This could result in module resolution failures.

For example, when extracting NgModule metadata from
`/node_modules/lib/index.d.ts` that looks like

```
import {LibDirective} from './dir';

@NgModule({
  declarations: [LibDirective],
  exports: [LibDirective],
})
export class LibModule {}
```

and `/app.module.ts` that contains

```
import {LibModule} from 'lib';

@NgModule({
  imports: [LibModule],
})
export class AppModule {}
```

then `AppModule` would have recorded a reference to `LibModule` using
the `'lib'` module specifier. When extracting the NgModule metadata from
the `/node_modules/lib/index.d.ts` file the relative import into `./dir`
should also be assumed to be importable from `'lib'` (according to APF
where symbols need to be exported from a single entry-point)
so the reference to `LibDirective` should have `'lib'` as absolute
module specifier, but it would incorrectly have
`/node_modules/lib/index.d.ts` as resolution context path. The latter is
incorrect as `'lib'` needs to be resolved from `/app.module.ts` and not
from within the library itself.

Fixes angular#42810

PR Close angular#42879

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

@@ -30,7 +30,7 @@ export class DtsMetadataReader implements MetadataReader {
    */
   getNgModuleMetadata(ref: Reference<ClassDeclaration>): NgModuleMeta|null {
     const clazz = ref.node;
-    const resolutionContext = clazz.getSourceFile().fileName;
+
     // This operation is explicitly not memoized, as it depends on `ref.ownedByModuleGuess`.
     // TODO(alxhub): investigate caching of .d.ts module metadata.
     const ngModuleDef = this.reflector.getMembersOfClass(clazz).find(
@@ -49,12 +49,10 @@ export class DtsMetadataReader implements MetadataReader {
     const [_, declarationMetadata, importMetadata, exportMetadata] = ngModuleDef.type.typeArguments;
     return {
       ref,
-      declarations: extractReferencesFromType(
-          this.checker, declarationMetadata, ref.ownedByModuleGuess, resolutionContext),
-      exports: extractReferencesFromType(
-          this.checker, exportMetadata, ref.ownedByModuleGuess, resolutionContext),
-      imports: extractReferencesFromType(
-          this.checker, importMetadata, ref.ownedByModuleGuess, resolutionContext),
+      declarations:
+          extractReferencesFromType(this.checker, declarationMetadata, ref.bestGuessOwningModule),
+      exports: extractReferencesFromType(this.checker, exportMetadata, ref.bestGuessOwningModule),
+      imports: extractReferencesFromType(this.checker, importMetadata, ref.bestGuessOwningModule),
       schemas: [],
       rawDeclarations: null,
     };

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

@@ -8,16 +8,16 @@
 
 import * as ts from 'typescript';
 
-import {Reference} from '../../imports';
+import {OwningModule, Reference} from '../../imports';
 import {ClassDeclaration, ClassMember, ClassMemberKind, isNamedClassDeclaration, ReflectionHost, reflectTypeEntityToDeclaration} from '../../reflection';
 import {nodeDebugInfo} from '../../util/src/typescript';
 
 import {DirectiveMeta, DirectiveTypeCheckMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api';
 import {ClassPropertyMapping, ClassPropertyName} from './property_mapping';
 
 export function extractReferencesFromType(
-    checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string|null,
-    resolutionContext: string): Reference<ClassDeclaration>[] {
+    checker: ts.TypeChecker, def: ts.TypeNode,
+    bestGuessOwningModule: OwningModule|null): Reference<ClassDeclaration>[] {
   if (!ts.isTupleTypeNode(def)) {
     return [];
   }
@@ -31,11 +31,15 @@ export function extractReferencesFromType(
     if (!isNamedClassDeclaration(node)) {
       throw new Error(`Expected named ClassDeclaration: ${nodeDebugInfo(node)}`);
     }
-    const specifier = (from !== null && !from.startsWith('.') ? from : ngModuleImportedFrom);
-    if (specifier !== null) {
-      return new Reference(node, {specifier, resolutionContext});
+    if (from !== null && !from.startsWith('.')) {
+      // The symbol was imported using an absolute module specifier so return a reference that
+      // uses that absolute module specifier as its best guess owning module.
+      return new Reference(
+          node, {specifier: from, resolutionContext: def.getSourceFile().fileName});
     } else {
-      return new Reference(node);
+      // For local symbols or symbols that were imported using a relative module import it is
+      // assumed that the symbol is exported from the provided best guess owning module.
+      return new Reference(node, bestGuessOwningModule);
     }
   });
 }

packages/compiler-cli/src/ngtsc/metadata/test/dts_spec.ts

@@ -10,7 +10,7 @@ import * as ts from 'typescript';
 
 import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../file_system';
 import {runInEachFileSystem} from '../../file_system/testing';
-import {Reference} from '../../imports';
+import {OwningModule, Reference} from '../../imports';
 import {isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection';
 import {loadFakeCore, makeProgram} from '../../testing';
 
@@ -89,5 +89,93 @@ runInEachFileSystem(() => {
       const meta = dtsReader.getDirectiveMetadata(new Reference(clazz))!;
       expect(meta.isStructural).toBeTrue();
     });
+
+    it('should retain an absolute owning module for relative imports', () => {
+      const externalPath = absoluteFrom('/external.d.ts');
+      const {program} = makeProgram(
+          [
+            {
+              name: externalPath,
+              contents: `
+          import * as i0 from '@angular/core';
+          import * as i1 from 'absolute';
+          import * as i2 from './relative';
+
+          export declare class ExternalModule {
+            static ɵmod: i0.ɵɵNgModuleDeclaration<RelativeModule, [typeof i2.RelativeDir], never, [typeof i1.AbsoluteDir, typeof i2.RelativeDir]>;
+          }
+        `
+            },
+            {
+              name: absoluteFrom('/relative.d.ts'),
+              contents: `
+          import * as i0 from '@angular/core';
+
+          export declare class RelativeDir {
+            static ɵdir: i0.ɵɵDirectiveDeclaration<RelativeDir, '[dir]', never, never, never, never>;
+          }
+        `
+            },
+            {
+              name: absoluteFrom('/node_modules/absolute.d.ts'),
+              contents: `
+          import * as i0 from '@angular/core';
+
+          export declare class AbsoluteDir {
+            static ɵdir: i0.ɵɵDirectiveDeclaration<ExternalDir, '[dir]', never, never, never, never>;
+          }
+        `
+            }
+          ],
+          {
+            skipLibCheck: true,
+            lib: ['es6', 'dom'],
+          });
+
+      const externalSf = getSourceFileOrError(program, externalPath);
+      const clazz = externalSf.statements[3];
+      if (!isNamedClassDeclaration(clazz)) {
+        return fail('Expected class declaration');
+      }
+
+      const typeChecker = program.getTypeChecker();
+      const dtsReader =
+          new DtsMetadataReader(typeChecker, new TypeScriptReflectionHost(typeChecker));
+
+      const withoutOwningModule = dtsReader.getNgModuleMetadata(new Reference(clazz))!;
+      expect(withoutOwningModule.exports.length).toBe(2);
+
+      // `AbsoluteDir` was imported from an absolute module so the export Reference should have
+      // a corresponding best guess owning module.
+      expect(withoutOwningModule.exports[0].bestGuessOwningModule).toEqual({
+        specifier: 'absolute',
+        resolutionContext: externalSf.fileName,
+      });
+
+      // `RelativeDir` was imported from a relative module specifier so the original reference's
+      // best guess owning module should have been retained, which was null.
+      expect(withoutOwningModule.exports[1].bestGuessOwningModule).toBeNull();
+
+      const owningModule: OwningModule = {
+        specifier: 'module',
+        resolutionContext: absoluteFrom('/context.ts'),
+      };
+      const withOwningModule = dtsReader.getNgModuleMetadata(new Reference(clazz, owningModule))!;
+      expect(withOwningModule.exports.length).toBe(2);
+
+      // Again, `AbsoluteDir` was imported from an absolute module so the export Reference should
+      // have a corresponding best guess owning module; the owning module of the incoming reference
+      // is irrelevant here.
+      expect(withOwningModule.exports[0].bestGuessOwningModule).toEqual({
+        specifier: 'absolute',
+        resolutionContext: externalSf.fileName,
+      });
+
+      // As `RelativeDir` was imported from a relative module specifier, the export Reference should
+      // continue to have the owning module of the incoming reference as the relatively imported
+      // symbol is assumed to also be exported from the absolute module specifier as captured in the
+      // best guess owning module.
+      expect(withOwningModule.exports[1].bestGuessOwningModule).toEqual(owningModule);
+    });
   });
 });

packages/compiler-cli/src/ngtsc/scope/test/dependency_spec.ts

@@ -68,8 +68,7 @@ function makeTestEnv(
       const exportedSymbol = symbol.exports!.get(name as ts.__String);
       if (exportedSymbol !== undefined) {
         const decl = exportedSymbol.valueDeclaration as ts.ClassDeclaration;
-        const specifier = MODULE_FROM_NODE_MODULES_PATH.exec(sf.fileName)![1];
-        return new Reference(decl, {specifier, resolutionContext: sf.fileName});
+        return new Reference(decl);
       }
     }
     throw new Error('Class not found: ' + name);
@@ -143,10 +142,13 @@ runInEachFileSystem(() => {
       });
       const {Dir, ModuleB} = refs;
       const scope = resolver.resolve(ModuleB)!;
-      expect(scopeToRefs(scope)).toEqual([Dir]);
+      expect(scopeToRefs(scope).map(ref => ref.node)).toEqual([Dir.node]);
 
       // Explicitly verify that the directive has the correct owning module.
-      expect(scope.exported.directives[0].ref.ownedByModuleGuess).toBe('declaration');
+      expect(scope.exported.directives[0].ref.bestGuessOwningModule).toEqual({
+        specifier: 'declaration',
+        resolutionContext: ModuleB.node.getSourceFile().fileName,
+      });
     });
 
     it('should write correct aliases for deep dependencies', () => {