From d4d76ead802837bc6cc7908bc9ebfefa73eb9969 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 5 Nov 2024 13:47:46 +0000 Subject: [PATCH] fix(compiler-cli): do not fail fatal when references to non-existent module are discovered (#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 #58515 --- .../annotations/ng_module/src/handler.ts | 1 + .../src/ngtsc/metadata/src/api.ts | 6 ++ .../src/ngtsc/metadata/src/dts.ts | 56 +++++++++++++++---- .../src/ngtsc/metadata/src/util.ts | 44 ++++++++++++--- .../src/ngtsc/metadata/test/dts_spec.ts | 38 +++++++++++++ .../src/ngtsc/reflection/src/typescript.ts | 22 ++++++-- .../src/ngtsc/scope/src/dependency.ts | 2 +- .../src/ngtsc/scope/test/local_spec.ts | 14 +++++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 37 ++++++++++++ 9 files changed, 194 insertions(+), 26 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts index a349166f0ea..1cfb85862b9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts @@ -734,6 +734,7 @@ export class NgModuleDecoratorHandler rawExports: analysis.rawExports, decorator: analysis.decorator, mayDeclareProviders: analysis.providers !== null, + isPoisoned: false, }); this.injectableRegistry.registerInjectable(node, { diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 511872bc27a..93d998978a7 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -25,6 +25,12 @@ export interface NgModuleMeta { exports: Reference[]; schemas: SchemaMetadata[]; + /** + * Whether the module had some issue being analyzed. + * This means it likely does not have complete and reliable metadata. + */ + isPoisoned: boolean; + /** * The raw `ts.Expression` which gave rise to `declarations`, if one exists. * diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index 28f0a502a07..33716418f64 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -76,16 +76,36 @@ export class DtsMetadataReader implements MetadataReader { // Read the ModuleData out of the type arguments. const [_, declarationMetadata, importMetadata, exportMetadata] = ngModuleDef.type.typeArguments; + + const declarations = extractReferencesFromType( + this.checker, + declarationMetadata, + ref.bestGuessOwningModule, + ); + const exports = extractReferencesFromType( + this.checker, + exportMetadata, + ref.bestGuessOwningModule, + ); + const imports = extractReferencesFromType( + this.checker, + importMetadata, + ref.bestGuessOwningModule, + ); + + // The module is considered poisoned if it's exports couldn't be + // resolved completely. This would make the module not necessarily + // usable for scope computation relying on this module; so we propagate + // this "incompleteness" information to the caller. + const isPoisoned = exports.isIncomplete; + return { kind: MetaKind.NgModule, ref, - declarations: extractReferencesFromType( - this.checker, - declarationMetadata, - ref.bestGuessOwningModule, - ), - exports: extractReferencesFromType(this.checker, exportMetadata, ref.bestGuessOwningModule), - imports: extractReferencesFromType(this.checker, importMetadata, ref.bestGuessOwningModule), + declarations: declarations.result, + isPoisoned, + exports: exports.result, + imports: imports.result, schemas: [], rawDeclarations: null, rawImports: null, @@ -156,6 +176,11 @@ export class DtsMetadataReader implements MetadataReader { const isSignal = def.type.typeArguments.length > 9 && (readBooleanType(def.type.typeArguments[9]) ?? false); + // At this point in time, the `.d.ts` may not be fully extractable when + // trying to resolve host directive types to their declarations. + // If this cannot be done completely, the metadata is incomplete and "poisoned". + const isPoisoned = hostDirectives !== null && hostDirectives?.isIncomplete; + return { kind: MetaKind.Directive, matchSource: MatchSource.Selector, @@ -166,11 +191,11 @@ export class DtsMetadataReader implements MetadataReader { exportAs: readStringArrayType(def.type.typeArguments[2]), inputs, outputs, - hostDirectives, + hostDirectives: hostDirectives?.result ?? null, queries: readStringArrayType(def.type.typeArguments[5]), ...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector), baseClass: readBaseClass(clazz, this.checker, this.reflector), - isPoisoned: false, + isPoisoned, isStructural, animationTriggerNames: null, ngContentSelectors, @@ -330,12 +355,13 @@ function readHostDirectivesType( checker: ts.TypeChecker, type: ts.TypeNode, bestGuessOwningModule: OwningModule | null, -): HostDirectiveMeta[] | null { +): {result: HostDirectiveMeta[]; isIncomplete: boolean} | null { if (!ts.isTupleTypeNode(type) || type.elements.length === 0) { return null; } const result: HostDirectiveMeta[] = []; + let isIncomplete = false; for (const hostDirectiveType of type.elements) { const {directive, inputs, outputs} = readMapType(hostDirectiveType, (type) => type); @@ -345,8 +371,14 @@ function readHostDirectivesType( throw new Error(`Expected TypeQueryNode: ${nodeDebugInfo(directive)}`); } + const ref = extraReferenceFromTypeQuery(checker, directive, type, bestGuessOwningModule); + if (ref === null) { + isIncomplete = true; + continue; + } + result.push({ - directive: extraReferenceFromTypeQuery(checker, directive, type, bestGuessOwningModule), + directive: ref, isForwardReference: false, inputs: readMapType(inputs, readStringType), outputs: readMapType(outputs, readStringType), @@ -354,5 +386,5 @@ function readHostDirectivesType( } } - return result.length > 0 ? result : null; + return result.length > 0 ? {result, isIncomplete} : null; } diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index ce8aa79342f..e3668894ffb 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -31,23 +31,37 @@ import { TemplateGuardMeta, } from './api'; import {ClassPropertyMapping, ClassPropertyName} from './property_mapping'; +import {TypeEntityToDeclarationError} from '../../reflection/src/typescript'; export function extractReferencesFromType( checker: ts.TypeChecker, def: ts.TypeNode, bestGuessOwningModule: OwningModule | null, -): Reference[] { +): {result: Reference[]; isIncomplete: boolean} { if (!ts.isTupleTypeNode(def)) { - return []; + return {result: [], isIncomplete: false}; } - return def.elements.map((element) => { + const result: Reference[] = []; + let isIncomplete = false; + + for (const element of def.elements) { if (!ts.isTypeQueryNode(element)) { throw new Error(`Expected TypeQueryNode: ${nodeDebugInfo(element)}`); } - return extraReferenceFromTypeQuery(checker, element, def, bestGuessOwningModule); - }); + const ref = extraReferenceFromTypeQuery(checker, element, def, bestGuessOwningModule); + + // Note: Sometimes a reference inside the type tuple/array + // may not be resolvable/existent. We proceed with incomplete data. + if (ref === null) { + isIncomplete = true; + } else { + result.push(ref); + } + } + + return {result, isIncomplete}; } export function extraReferenceFromTypeQuery( @@ -55,12 +69,28 @@ export function extraReferenceFromTypeQuery( typeNode: ts.TypeQueryNode, origin: ts.TypeNode, bestGuessOwningModule: OwningModule | null, -) { +): Reference | null { const type = typeNode.exprName; - const {node, from} = reflectTypeEntityToDeclaration(type, checker); + let node: ts.Declaration; + let from: string | null; + + // Gracefully handle when the type entity could not be converted or + // resolved to its declaration node. + try { + const result = reflectTypeEntityToDeclaration(type, checker); + node = result.node; + from = result.from; + } catch (e) { + if (e instanceof TypeEntityToDeclarationError) { + return null; + } + throw e; + } + if (!isNamedClassDeclaration(node)) { throw new Error(`Expected named ClassDeclaration: ${nodeDebugInfo(node)}`); } + 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. diff --git a/packages/compiler-cli/src/ngtsc/metadata/test/dts_spec.ts b/packages/compiler-cli/src/ngtsc/metadata/test/dts_spec.ts index 665015d5c17..4a168a13aaa 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/test/dts_spec.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/test/dts_spec.ts @@ -337,4 +337,42 @@ runInEachFileSystem(() => { expect(meta.inputs.toDirectMappedObject()).toEqual({input: 'input', otherInput: 'alias'}); expect(Array.from(meta.inputs).filter((i) => i.required)).toEqual([]); }); + + it('should not fail fatal at runtime when reference cannot be resolved', () => { + const externalPath = absoluteFrom('/external.d.ts'); + const {program} = makeProgram( + [ + { + name: externalPath, + contents: ` + import * as i0 from '@angular/core'; + import * as i2 from './relative'; + + export class ValidRef {} + + export declare class ExternalModule { + static ɵmod: i0.ɵɵNgModuleDeclaration; + } + `, + }, + ], + { + 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(1); + expect(withoutOwningModule.isPoisoned).toBe(true); + }); }); diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index c5e946e3c75..ce176ca50df 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -533,13 +533,21 @@ export function reflectIdentifierOfDeclaration(decl: ts.Declaration): ts.Identif return null; } +export class TypeEntityToDeclarationError extends Error {} + +/** + * @throws {TypeEntityToDeclarationError} if the type cannot be converted + * to a declaration. + */ export function reflectTypeEntityToDeclaration( type: ts.EntityName, checker: ts.TypeChecker, ): {node: ts.Declaration; from: string | null} { let realSymbol = checker.getSymbolAtLocation(type); if (realSymbol === undefined) { - throw new Error(`Cannot resolve type entity ${type.getText()} to symbol`); + throw new TypeEntityToDeclarationError( + `Cannot resolve type entity ${type.getText()} to symbol`, + ); } while (realSymbol.flags & ts.SymbolFlags.Alias) { realSymbol = checker.getAliasedSymbol(realSymbol); @@ -551,12 +559,14 @@ export function reflectTypeEntityToDeclaration( } else if (realSymbol.declarations !== undefined && realSymbol.declarations.length === 1) { node = realSymbol.declarations[0]; } else { - throw new Error(`Cannot resolve type entity symbol to declaration`); + throw new TypeEntityToDeclarationError(`Cannot resolve type entity symbol to declaration`); } if (ts.isQualifiedName(type)) { if (!ts.isIdentifier(type.left)) { - throw new Error(`Cannot handle qualified name with non-identifier lhs`); + throw new TypeEntityToDeclarationError( + `Cannot handle qualified name with non-identifier lhs`, + ); } const symbol = checker.getSymbolAtLocation(type.left); if ( @@ -564,20 +574,20 @@ export function reflectTypeEntityToDeclaration( symbol.declarations === undefined || symbol.declarations.length !== 1 ) { - throw new Error(`Cannot resolve qualified type entity lhs to symbol`); + throw new TypeEntityToDeclarationError(`Cannot resolve qualified type entity lhs to symbol`); } const decl = symbol.declarations[0]; if (ts.isNamespaceImport(decl)) { const clause = decl.parent!; const importDecl = clause.parent!; if (!ts.isStringLiteral(importDecl.moduleSpecifier)) { - throw new Error(`Module specifier is not a string`); + throw new TypeEntityToDeclarationError(`Module specifier is not a string`); } return {node, from: importDecl.moduleSpecifier.text}; } else if (ts.isModuleDeclaration(decl)) { return {node, from: null}; } else { - throw new Error(`Unknown import type?`); + throw new TypeEntityToDeclarationError(`Unknown import type?`); } } else { return {node, from: null}; diff --git a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts index f7d8da7d219..85069799f0e 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts @@ -124,7 +124,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver { const exportScope: ExportScope = { exported: { dependencies, - isPoisoned: false, + isPoisoned: meta.isPoisoned, }, }; this.cache.set(clazz, exportScope); diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index f77bce7f8f0..5a3a14e4b70 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -79,6 +79,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope; @@ -101,6 +102,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -114,6 +116,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -127,6 +130,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; @@ -149,6 +153,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -162,6 +167,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; @@ -184,6 +190,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -197,6 +204,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -210,6 +218,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scope = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; @@ -239,6 +248,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope; @@ -260,6 +270,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -273,6 +284,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope; @@ -294,6 +306,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); metaRegistry.registerNgModuleMetadata({ kind: MetaKind.NgModule, @@ -307,6 +320,7 @@ describe('LocalModuleScopeRegistry', () => { rawExports: null, decorator: null, mayDeclareProviders: false, + isPoisoned: false, }); expect(scopeRegistry.getScopeOfModule(ModuleA.node)!.compilation.isPoisoned).toBeTrue(); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 769757a0385..fca81c9bac5 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1215,6 +1215,43 @@ runInEachFileSystem((os: string) => { ); }); + it( + 'should not throw, but issue a diagnostic when an `NgModule` from `d.ts` references ' + + 'non-existent classes', + () => { + env.tsconfig(); + env.write( + 'test.ts', + ` + import {NgModule} from '@angular/core'; + import {MyModule} from './lib'; + + @NgModule({ + imports: [MyModule], + }) + export class Mod {} + `, + ); + env.write( + 'lib.d.ts', + ` + import * as i0 from '@angular/core'; + import {InvalidRef} from 'other-lib'; + + export declare class MyModule { + static ɵmod: i0.ɵɵNgModuleDeclaration + } + `, + ); + + const diagnostics = env.driveDiagnostics(); + expect(diagnostics.length).toBe(1); + expect(diagnostics[0].messageText).toBe( + 'This import contains errors, which may affect components that depend on this NgModule.', + ); + }, + ); + it('should respect imported module order while processing Directives and Components', () => { env.tsconfig({}); env.write(