Skip to content

Commit 6338cb7

Browse files
devversionpkozlowski-opensource
authored andcommitted
fix(compiler-cli): do not fail fatal when references to non-existent module are discovered (angular#58515)
Currently when application source code references e.g. an NgModule that points to references that aren't available, the compiler will break at runtime without any actionable/reasonable error. This could happen for example when a library is referenced in code, but the library is simply not available in the `node_modules`. Clearly, TypeScript would issue import diagnostics here, but the Angular compiler shouldn't break fatally. This is useful for migrations which may run against projects which aren't fully compilable. The compiler handles this fine in all cases, except when processing `.d.ts` currently... and the runtime exception invalides all other information of the program etc. This commit fixes this by degrading such unexpected cases for `.d.ts` metadata reading to be handled gracefully. This matches existing logic where the `.d.ts` doesn't necessarily match the "expecation"/"expected format". The worst case is that the Angular compiler will not have type information for some directives of e.g. a library that just isn't installed in local `node_modules`; compared to magical errors and unexpected runtime behavior. PR Close angular#58515
1 parent 8d5d5f8 commit 6338cb7

File tree

9 files changed

+194
-26
lines changed

9 files changed

+194
-26
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -734,6 +734,7 @@ export class NgModuleDecoratorHandler
734734
rawExports: analysis.rawExports,
735735
decorator: analysis.decorator,
736736
mayDeclareProviders: analysis.providers !== null,
737+
isPoisoned: false,
737738
});
738739

739740
this.injectableRegistry.registerInjectable(node, {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ export interface NgModuleMeta {
2525
exports: Reference<ClassDeclaration>[];
2626
schemas: SchemaMetadata[];
2727

28+
/**
29+
* Whether the module had some issue being analyzed.
30+
* This means it likely does not have complete and reliable metadata.
31+
*/
32+
isPoisoned: boolean;
33+
2834
/**
2935
* The raw `ts.Expression` which gave rise to `declarations`, if one exists.
3036
*

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

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -76,16 +76,36 @@ export class DtsMetadataReader implements MetadataReader {
7676

7777
// Read the ModuleData out of the type arguments.
7878
const [_, declarationMetadata, importMetadata, exportMetadata] = ngModuleDef.type.typeArguments;
79+
80+
const declarations = extractReferencesFromType(
81+
this.checker,
82+
declarationMetadata,
83+
ref.bestGuessOwningModule,
84+
);
85+
const exports = extractReferencesFromType(
86+
this.checker,
87+
exportMetadata,
88+
ref.bestGuessOwningModule,
89+
);
90+
const imports = extractReferencesFromType(
91+
this.checker,
92+
importMetadata,
93+
ref.bestGuessOwningModule,
94+
);
95+
96+
// The module is considered poisoned if it's exports couldn't be
97+
// resolved completely. This would make the module not necessarily
98+
// usable for scope computation relying on this module; so we propagate
99+
// this "incompleteness" information to the caller.
100+
const isPoisoned = exports.isIncomplete;
101+
79102
return {
80103
kind: MetaKind.NgModule,
81104
ref,
82-
declarations: extractReferencesFromType(
83-
this.checker,
84-
declarationMetadata,
85-
ref.bestGuessOwningModule,
86-
),
87-
exports: extractReferencesFromType(this.checker, exportMetadata, ref.bestGuessOwningModule),
88-
imports: extractReferencesFromType(this.checker, importMetadata, ref.bestGuessOwningModule),
105+
declarations: declarations.result,
106+
isPoisoned,
107+
exports: exports.result,
108+
imports: imports.result,
89109
schemas: [],
90110
rawDeclarations: null,
91111
rawImports: null,
@@ -156,6 +176,11 @@ export class DtsMetadataReader implements MetadataReader {
156176
const isSignal =
157177
def.type.typeArguments.length > 9 && (readBooleanType(def.type.typeArguments[9]) ?? false);
158178

179+
// At this point in time, the `.d.ts` may not be fully extractable when
180+
// trying to resolve host directive types to their declarations.
181+
// If this cannot be done completely, the metadata is incomplete and "poisoned".
182+
const isPoisoned = hostDirectives !== null && hostDirectives?.isIncomplete;
183+
159184
return {
160185
kind: MetaKind.Directive,
161186
matchSource: MatchSource.Selector,
@@ -166,11 +191,11 @@ export class DtsMetadataReader implements MetadataReader {
166191
exportAs: readStringArrayType(def.type.typeArguments[2]),
167192
inputs,
168193
outputs,
169-
hostDirectives,
194+
hostDirectives: hostDirectives?.result ?? null,
170195
queries: readStringArrayType(def.type.typeArguments[5]),
171196
...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
172197
baseClass: readBaseClass(clazz, this.checker, this.reflector),
173-
isPoisoned: false,
198+
isPoisoned,
174199
isStructural,
175200
animationTriggerNames: null,
176201
ngContentSelectors,
@@ -330,12 +355,13 @@ function readHostDirectivesType(
330355
checker: ts.TypeChecker,
331356
type: ts.TypeNode,
332357
bestGuessOwningModule: OwningModule | null,
333-
): HostDirectiveMeta[] | null {
358+
): {result: HostDirectiveMeta[]; isIncomplete: boolean} | null {
334359
if (!ts.isTupleTypeNode(type) || type.elements.length === 0) {
335360
return null;
336361
}
337362

338363
const result: HostDirectiveMeta[] = [];
364+
let isIncomplete = false;
339365

340366
for (const hostDirectiveType of type.elements) {
341367
const {directive, inputs, outputs} = readMapType(hostDirectiveType, (type) => type);
@@ -345,14 +371,20 @@ function readHostDirectivesType(
345371
throw new Error(`Expected TypeQueryNode: ${nodeDebugInfo(directive)}`);
346372
}
347373

374+
const ref = extraReferenceFromTypeQuery(checker, directive, type, bestGuessOwningModule);
375+
if (ref === null) {
376+
isIncomplete = true;
377+
continue;
378+
}
379+
348380
result.push({
349-
directive: extraReferenceFromTypeQuery(checker, directive, type, bestGuessOwningModule),
381+
directive: ref,
350382
isForwardReference: false,
351383
inputs: readMapType(inputs, readStringType),
352384
outputs: readMapType(outputs, readStringType),
353385
});
354386
}
355387
}
356388

357-
return result.length > 0 ? result : null;
389+
return result.length > 0 ? {result, isIncomplete} : null;
358390
}

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

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,36 +31,66 @@ import {
3131
TemplateGuardMeta,
3232
} from './api';
3333
import {ClassPropertyMapping, ClassPropertyName} from './property_mapping';
34+
import {TypeEntityToDeclarationError} from '../../reflection/src/typescript';
3435

3536
export function extractReferencesFromType(
3637
checker: ts.TypeChecker,
3738
def: ts.TypeNode,
3839
bestGuessOwningModule: OwningModule | null,
39-
): Reference<ClassDeclaration>[] {
40+
): {result: Reference<ClassDeclaration>[]; isIncomplete: boolean} {
4041
if (!ts.isTupleTypeNode(def)) {
41-
return [];
42+
return {result: [], isIncomplete: false};
4243
}
4344

44-
return def.elements.map((element) => {
45+
const result: Reference<ClassDeclaration>[] = [];
46+
let isIncomplete = false;
47+
48+
for (const element of def.elements) {
4549
if (!ts.isTypeQueryNode(element)) {
4650
throw new Error(`Expected TypeQueryNode: ${nodeDebugInfo(element)}`);
4751
}
4852

49-
return extraReferenceFromTypeQuery(checker, element, def, bestGuessOwningModule);
50-
});
53+
const ref = extraReferenceFromTypeQuery(checker, element, def, bestGuessOwningModule);
54+
55+
// Note: Sometimes a reference inside the type tuple/array
56+
// may not be resolvable/existent. We proceed with incomplete data.
57+
if (ref === null) {
58+
isIncomplete = true;
59+
} else {
60+
result.push(ref);
61+
}
62+
}
63+
64+
return {result, isIncomplete};
5165
}
5266

5367
export function extraReferenceFromTypeQuery(
5468
checker: ts.TypeChecker,
5569
typeNode: ts.TypeQueryNode,
5670
origin: ts.TypeNode,
5771
bestGuessOwningModule: OwningModule | null,
58-
) {
72+
): Reference<ClassDeclaration> | null {
5973
const type = typeNode.exprName;
60-
const {node, from} = reflectTypeEntityToDeclaration(type, checker);
74+
let node: ts.Declaration;
75+
let from: string | null;
76+
77+
// Gracefully handle when the type entity could not be converted or
78+
// resolved to its declaration node.
79+
try {
80+
const result = reflectTypeEntityToDeclaration(type, checker);
81+
node = result.node;
82+
from = result.from;
83+
} catch (e) {
84+
if (e instanceof TypeEntityToDeclarationError) {
85+
return null;
86+
}
87+
throw e;
88+
}
89+
6190
if (!isNamedClassDeclaration(node)) {
6291
throw new Error(`Expected named ClassDeclaration: ${nodeDebugInfo(node)}`);
6392
}
93+
6494
if (from !== null && !from.startsWith('.')) {
6595
// The symbol was imported using an absolute module specifier so return a reference that
6696
// uses that absolute module specifier as its best guess owning module.

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,4 +337,42 @@ runInEachFileSystem(() => {
337337
expect(meta.inputs.toDirectMappedObject()).toEqual({input: 'input', otherInput: 'alias'});
338338
expect(Array.from(meta.inputs).filter((i) => i.required)).toEqual([]);
339339
});
340+
341+
it('should not fail fatal at runtime when reference cannot be resolved', () => {
342+
const externalPath = absoluteFrom('/external.d.ts');
343+
const {program} = makeProgram(
344+
[
345+
{
346+
name: externalPath,
347+
contents: `
348+
import * as i0 from '@angular/core';
349+
import * as i2 from './relative';
350+
351+
export class ValidRef {}
352+
353+
export declare class ExternalModule {
354+
static ɵmod: i0.ɵɵNgModuleDeclaration<RelativeModule, [], never, [typeof i2.InvalidTypesForIt, typeof ValidRef]>;
355+
}
356+
`,
357+
},
358+
],
359+
{
360+
skipLibCheck: true,
361+
lib: ['es6', 'dom'],
362+
},
363+
);
364+
365+
const externalSf = getSourceFileOrError(program, externalPath);
366+
const clazz = externalSf.statements[3];
367+
if (!isNamedClassDeclaration(clazz)) {
368+
return fail('Expected class declaration');
369+
}
370+
371+
const typeChecker = program.getTypeChecker();
372+
const dtsReader = new DtsMetadataReader(typeChecker, new TypeScriptReflectionHost(typeChecker));
373+
374+
const withoutOwningModule = dtsReader.getNgModuleMetadata(new Reference(clazz))!;
375+
expect(withoutOwningModule.exports.length).toBe(1);
376+
expect(withoutOwningModule.isPoisoned).toBe(true);
377+
});
340378
});

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

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -533,13 +533,21 @@ export function reflectIdentifierOfDeclaration(decl: ts.Declaration): ts.Identif
533533
return null;
534534
}
535535

536+
export class TypeEntityToDeclarationError extends Error {}
537+
538+
/**
539+
* @throws {TypeEntityToDeclarationError} if the type cannot be converted
540+
* to a declaration.
541+
*/
536542
export function reflectTypeEntityToDeclaration(
537543
type: ts.EntityName,
538544
checker: ts.TypeChecker,
539545
): {node: ts.Declaration; from: string | null} {
540546
let realSymbol = checker.getSymbolAtLocation(type);
541547
if (realSymbol === undefined) {
542-
throw new Error(`Cannot resolve type entity ${type.getText()} to symbol`);
548+
throw new TypeEntityToDeclarationError(
549+
`Cannot resolve type entity ${type.getText()} to symbol`,
550+
);
543551
}
544552
while (realSymbol.flags & ts.SymbolFlags.Alias) {
545553
realSymbol = checker.getAliasedSymbol(realSymbol);
@@ -551,33 +559,35 @@ export function reflectTypeEntityToDeclaration(
551559
} else if (realSymbol.declarations !== undefined && realSymbol.declarations.length === 1) {
552560
node = realSymbol.declarations[0];
553561
} else {
554-
throw new Error(`Cannot resolve type entity symbol to declaration`);
562+
throw new TypeEntityToDeclarationError(`Cannot resolve type entity symbol to declaration`);
555563
}
556564

557565
if (ts.isQualifiedName(type)) {
558566
if (!ts.isIdentifier(type.left)) {
559-
throw new Error(`Cannot handle qualified name with non-identifier lhs`);
567+
throw new TypeEntityToDeclarationError(
568+
`Cannot handle qualified name with non-identifier lhs`,
569+
);
560570
}
561571
const symbol = checker.getSymbolAtLocation(type.left);
562572
if (
563573
symbol === undefined ||
564574
symbol.declarations === undefined ||
565575
symbol.declarations.length !== 1
566576
) {
567-
throw new Error(`Cannot resolve qualified type entity lhs to symbol`);
577+
throw new TypeEntityToDeclarationError(`Cannot resolve qualified type entity lhs to symbol`);
568578
}
569579
const decl = symbol.declarations[0];
570580
if (ts.isNamespaceImport(decl)) {
571581
const clause = decl.parent!;
572582
const importDecl = clause.parent!;
573583
if (!ts.isStringLiteral(importDecl.moduleSpecifier)) {
574-
throw new Error(`Module specifier is not a string`);
584+
throw new TypeEntityToDeclarationError(`Module specifier is not a string`);
575585
}
576586
return {node, from: importDecl.moduleSpecifier.text};
577587
} else if (ts.isModuleDeclaration(decl)) {
578588
return {node, from: null};
579589
} else {
580-
throw new Error(`Unknown import type?`);
590+
throw new TypeEntityToDeclarationError(`Unknown import type?`);
581591
}
582592
} else {
583593
return {node, from: null};

packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
124124
const exportScope: ExportScope = {
125125
exported: {
126126
dependencies,
127-
isPoisoned: false,
127+
isPoisoned: meta.isPoisoned,
128128
},
129129
};
130130
this.cache.set(clazz, exportScope);

0 commit comments

Comments
 (0)