From 7052e27677b9213009e99d34c8cfc013fd0552de Mon Sep 17 00:00:00 2001 From: JoostK Date: Wed, 29 Dec 2021 23:11:18 +0100 Subject: [PATCH] refactor(compiler-cli): improve DX for reference emit failures (#44587) In certain scenarios, the compiler may have crashed with an `Unable to write a reference` error which would be particularly hard to diagnose. One of the primary reasons for this failure is when the `rootDir` option is configured---typically the case for libraries--- and a source file is imported using a relative import from an external entry-point. This would normally report TS6059 for the invalid relative import, but the crash prevents this error from being surfaced. This commit refactors the reference emit logic to result in an explicit `Failure` state with a reason as to why the failure occurred. This state is then used to report a `FatalDiagnosticException`, preventing a hard crash. Closes #44414 PR Close #44587 --- goldens/public-api/compiler-cli/error_code.md | 1 + .../src/ngtsc/annotations/src/component.ts | 5 +- .../src/ngtsc/annotations/src/ng_module.ts | 48 ++++-- .../src/ngtsc/annotations/src/util.ts | 17 ++- .../src/ngtsc/diagnostics/index.ts | 2 +- .../src/ngtsc/diagnostics/src/error.ts | 10 ++ .../src/ngtsc/diagnostics/src/error_code.ts | 5 + .../src/ngtsc/imports/BUILD.bazel | 1 + .../compiler-cli/src/ngtsc/imports/index.ts | 2 +- .../src/ngtsc/imports/src/alias.ts | 8 +- .../src/ngtsc/imports/src/emitter.ts | 143 +++++++++++++++--- .../src/ngtsc/imports/src/find_export.ts | 28 +--- .../src/ngtsc/imports/test/emitter_spec.ts | 18 ++- .../compiler-cli/src/ngtsc/scope/src/local.ts | 7 +- .../src/ngtsc/typecheck/src/environment.ts | 4 +- .../compiler-cli/test/ngtsc/imports_spec.ts | 119 +++++++++++++++ 16 files changed, 330 insertions(+), 88 deletions(-) create mode 100644 packages/compiler-cli/test/ngtsc/imports_spec.ts diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index 52aba50642c..0fba1f0e373 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -31,6 +31,7 @@ export enum ErrorCode { DUPLICATE_VARIABLE_DECLARATION = 8006, HOST_BINDING_PARSE_ERROR = 5001, IMPORT_CYCLE_DETECTED = 3003, + IMPORT_GENERATION_FAILURE = 3004, INJECTABLE_DUPLICATE_PROV = 9001, INLINE_TCB_REQUIRED = 8900, INLINE_TYPE_CTOR_REQUIRED = 8901, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 4e114696580..036a04da3e2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -12,7 +12,7 @@ import ts from 'typescript'; import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; import {absoluteFrom, relative} from '../../file_system'; -import {ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; +import {assertSuccessfulReferenceEmit, ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; import {extractSemanticTypeParameters, isArrayEqual, isReferenceEqual, SemanticDepGraphUpdater, SemanticReference, SemanticSymbol} from '../../incremental/semantic_graph'; import {IndexingContext} from '../../indexer'; @@ -690,6 +690,8 @@ export class ComponentDecoratorHandler implements R3UsedDirectiveMetadata&{ref: Reference, importedFile: ImportedFile}; const usedDirectives: UsedDirective[] = bound.getUsedDirectives().map(directive => { const type = this.refEmitter.emit(directive.ref, context); + assertSuccessfulReferenceEmit( + type, node.name, directive.isComponent ? 'component' : 'directive'); return { ref: directive.ref, type: type.expression, @@ -715,6 +717,7 @@ export class ComponentDecoratorHandler implements } const pipe = pipes.get(pipeName)!; const type = this.refEmitter.emit(pipe, context); + assertSuccessfulReferenceEmit(type, node.name, 'pipe'); usedPipes.push({ ref: pipe, pipeName, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index f6e0a46cc18..216b118b763 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -10,7 +10,7 @@ import {compileClassMetadata, compileDeclareClassMetadata, compileDeclareInjecto import ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; -import {Reference, ReferenceEmitter} from '../../imports'; +import {assertSuccessfulReferenceEmit, Reference, ReferenceEmitter} from '../../imports'; import {isArrayEqual, isReferenceEqual, isSymbolEqual, SemanticReference, SemanticSymbol} from '../../incremental/semantic_graph'; import {InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator'; @@ -280,12 +280,19 @@ export class NgModuleDecoratorHandler implements typeContext = typeNode.getSourceFile(); } - const bootstrap = - bootstrapRefs.map(bootstrap => this._toR3Reference(bootstrap, valueContext, typeContext)); - const declarations = - declarationRefs.map(decl => this._toR3Reference(decl, valueContext, typeContext)); - const imports = importRefs.map(imp => this._toR3Reference(imp, valueContext, typeContext)); - const exports = exportRefs.map(exp => this._toR3Reference(exp, valueContext, typeContext)); + const bootstrap = bootstrapRefs.map( + bootstrap => this._toR3Reference( + bootstrap.getOriginForDiagnostics(meta, node.name), bootstrap, valueContext, + typeContext)); + const declarations = declarationRefs.map( + decl => this._toR3Reference( + decl.getOriginForDiagnostics(meta, node.name), decl, valueContext, typeContext)); + const imports = importRefs.map( + imp => this._toR3Reference( + imp.getOriginForDiagnostics(meta, node.name), imp, valueContext, typeContext)); + const exports = exportRefs.map( + exp => this._toR3Reference( + exp.getOriginForDiagnostics(meta, node.name), exp, valueContext, typeContext)); const isForwardReference = (ref: R3Reference) => isExpressionForwardReference(ref.value, node.name!, valueContext); @@ -419,7 +426,9 @@ export class NgModuleDecoratorHandler implements const context = getSourceFile(node); for (const exportRef of analysis.exports) { if (isNgModule(exportRef.node, scope.compilation)) { - data.injectorImports.push(this.refEmitter.emit(exportRef, context).expression); + const type = this.refEmitter.emit(exportRef, context); + assertSuccessfulReferenceEmit(type, node, 'NgModule'); + data.injectorImports.push(type.expression); } } @@ -506,12 +515,21 @@ export class NgModuleDecoratorHandler implements for (const decl of declarations) { const remoteScope = this.scopeRegistry.getRemoteScope(decl.node); if (remoteScope !== null) { - const directives = remoteScope.directives.map( - directive => this.refEmitter.emit(directive, context).expression); - const pipes = remoteScope.pipes.map(pipe => this.refEmitter.emit(pipe, context).expression); + const directives = remoteScope.directives.map(directive => { + const type = this.refEmitter.emit(directive, context); + assertSuccessfulReferenceEmit(type, node, 'directive'); + return type.expression; + }); + const pipes = remoteScope.pipes.map(pipe => { + const type = this.refEmitter.emit(pipe, context); + assertSuccessfulReferenceEmit(type, node, 'pipe'); + return type.expression; + }); const directiveArray = new LiteralArrayExpr(directives); const pipesArray = new LiteralArrayExpr(pipes); - const declExpr = this.refEmitter.emit(decl, context).expression; + const componentType = this.refEmitter.emit(decl, context); + assertSuccessfulReferenceEmit(componentType, node, 'component'); + const declExpr = componentType.expression; const setComponentScope = new ExternalExpr(R3Identifiers.setComponentScope); const callExpr = new InvokeFunctionExpr(setComponentScope, [declExpr, directiveArray, pipesArray]); @@ -543,17 +561,17 @@ export class NgModuleDecoratorHandler implements } private _toR3Reference( - valueRef: Reference, valueContext: ts.SourceFile, + origin: ts.Node, valueRef: Reference, valueContext: ts.SourceFile, typeContext: ts.SourceFile): R3Reference { if (valueRef.hasOwningModuleGuess) { - return toR3Reference(valueRef, valueRef, valueContext, valueContext, this.refEmitter); + return toR3Reference(origin, valueRef, valueRef, valueContext, valueContext, this.refEmitter); } else { let typeRef = valueRef; let typeNode = this.reflector.getDtsDeclaration(typeRef.node); if (typeNode !== null && isNamedClassDeclaration(typeNode)) { typeRef = new Reference(typeNode); } - return toR3Reference(valueRef, typeRef, valueContext, typeContext, this.refEmitter); + return toR3Reference(origin, valueRef, typeRef, valueContext, typeContext, this.refEmitter); } } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 2b4dca1afcf..56d38ccace2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -10,7 +10,7 @@ import {Expression, ExternalExpr, FactoryTarget, LiteralExpr, ParseLocation, Par import ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; -import {ImportFlags, Reference, ReferenceEmitter} from '../../imports'; +import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports'; import {attachDefaultImportDeclaration} from '../../imports/src/default'; import {ForeignFunctionResolver, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, CtorParameter, Decorator, Import, ImportedTypeValueReference, isNamedClassDeclaration, LocalTypeValueReference, ReflectionHost, TypeValueReference, TypeValueReferenceKind, UnavailableValue, ValueUnavailableKind} from '../../reflection'; @@ -257,13 +257,18 @@ function createUnsuitableInjectionTokenError( } export function toR3Reference( - valueRef: Reference, typeRef: Reference, valueContext: ts.SourceFile, + origin: ts.Node, valueRef: Reference, typeRef: Reference, valueContext: ts.SourceFile, typeContext: ts.SourceFile, refEmitter: ReferenceEmitter): R3Reference { + const emittedValueRef = refEmitter.emit(valueRef, valueContext); + assertSuccessfulReferenceEmit(emittedValueRef, origin, 'class'); + + const emittedTypeRef = refEmitter.emit( + typeRef, typeContext, ImportFlags.ForceNewImport | ImportFlags.AllowTypeImports); + assertSuccessfulReferenceEmit(emittedTypeRef, origin, 'class'); + return { - value: refEmitter.emit(valueRef, valueContext).expression, - type: refEmitter - .emit(typeRef, typeContext, ImportFlags.ForceNewImport | ImportFlags.AllowTypeImports) - .expression, + value: emittedValueRef.expression, + type: emittedTypeRef.expression, }; } diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/index.ts b/packages/compiler-cli/src/ngtsc/diagnostics/index.ts index 4ff58490a23..25a283f8781 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/index.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/index.ts @@ -7,7 +7,7 @@ */ export {COMPILER_ERRORS_WITH_GUIDES} from './src/docs'; -export {FatalDiagnosticError, isFatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from './src/error'; +export {FatalDiagnosticError, isFatalDiagnosticError, makeDiagnostic, makeDiagnosticChain, makeRelatedInformation} from './src/error'; export {ErrorCode} from './src/error_code'; export {ERROR_DETAILS_PAGE_BASE_URL} from './src/error_details_base_url'; export {ngErrorCode, replaceTsWithNgInErrors} from './src/util'; diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error.ts index 9ef6d8665c1..bd16c149591 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error.ts @@ -42,6 +42,16 @@ export function makeDiagnostic( }; } +export function makeDiagnosticChain( + messageText: string, next?: ts.DiagnosticMessageChain[]): ts.DiagnosticMessageChain { + return { + category: ts.DiagnosticCategory.Message, + code: 0, + messageText, + next, + }; +} + export function makeRelatedInformation( node: ts.Node, messageText: string): ts.DiagnosticRelatedInformation { node = ts.getOriginalNode(node); diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index f791145f786..b41686eee3a 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -64,6 +64,11 @@ export enum ErrorCode { */ IMPORT_CYCLE_DETECTED = 3003, + /** + * Raised when the compiler is unable to generate an import statement for a reference. + */ + IMPORT_GENERATION_FAILURE = 3004, + CONFIG_FLAT_MODULE_NO_INDEX = 4001, CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, diff --git a/packages/compiler-cli/src/ngtsc/imports/BUILD.bazel b/packages/compiler-cli/src/ngtsc/imports/BUILD.bazel index 026cfb7b0be..02288da57d2 100644 --- a/packages/compiler-cli/src/ngtsc/imports/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/imports/BUILD.bazel @@ -11,6 +11,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/core:api", + "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/util", diff --git a/packages/compiler-cli/src/ngtsc/imports/index.ts b/packages/compiler-cli/src/ngtsc/imports/index.ts index 523bdffe6ba..e042588d6eb 100644 --- a/packages/compiler-cli/src/ngtsc/imports/index.ts +++ b/packages/compiler-cli/src/ngtsc/imports/index.ts @@ -9,7 +9,7 @@ export {AliasingHost, AliasStrategy, PrivateExportAliasingHost, UnifiedModulesAliasingHost} from './src/alias'; export {ImportRewriter, NoopImportRewriter, R3SymbolsImportRewriter, validateAndRewriteCoreSymbol} from './src/core'; export {DefaultImportTracker} from './src/default'; -export {AbsoluteModuleStrategy, EmittedReference, ImportedFile, ImportFlags, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy, UnifiedModulesStrategy} from './src/emitter'; +export {AbsoluteModuleStrategy, assertSuccessfulReferenceEmit, EmittedReference, FailedEmitResult, ImportedFile, ImportFlags, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitKind, ReferenceEmitResult, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy, UnifiedModulesStrategy} from './src/emitter'; export {Reexport} from './src/reexport'; export {OwningModule, Reference} from './src/references'; export {ModuleResolver} from './src/resolver'; diff --git a/packages/compiler-cli/src/ngtsc/imports/src/alias.ts b/packages/compiler-cli/src/ngtsc/imports/src/alias.ts index 98b7ef703c6..4c41e3cc4a3 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/alias.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/alias.ts @@ -12,7 +12,7 @@ import ts from 'typescript'; import {UnifiedModulesHost} from '../../core/api'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; -import {EmittedReference, ImportFlags, ReferenceEmitStrategy} from './emitter'; +import {EmittedReference, ImportFlags, ReferenceEmitKind, ReferenceEmitStrategy} from './emitter'; import {Reference} from './references'; @@ -217,6 +217,10 @@ export class AliasStrategy implements ReferenceEmitStrategy { return null; } - return {expression: ref.alias, importedFile: 'unknown'}; + return { + kind: ReferenceEmitKind.Success, + expression: ref.alias, + importedFile: 'unknown', + }; } } diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index b1b86b19e34..d272a5113b2 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -9,6 +9,7 @@ import {Expression, ExternalExpr, ExternalReference, WrappedNodeExpr} from '@ang import ts from 'typescript'; import {UnifiedModulesHost} from '../../core/api'; +import {ErrorCode, FatalDiagnosticError, makeDiagnosticChain, makeRelatedInformation} from '../../diagnostics'; import {absoluteFromSourceFile, dirname, LogicalFileSystem, LogicalProjectPath, relative, toRelativeImport} from '../../file_system'; import {stripExtension} from '../../file_system/src/util'; import {DeclarationNode, ReflectionHost} from '../../reflection'; @@ -62,11 +63,18 @@ export enum ImportFlags { */ export type ImportedFile = ts.SourceFile|'unknown'|null; +export const enum ReferenceEmitKind { + Success, + Failed, +} + /** * Represents the emitted expression of a `Reference` that is valid in the source file it was * emitted from. */ export interface EmittedReference { + kind: ReferenceEmitKind.Success; + /** * The expression that refers to `Reference`. */ @@ -81,6 +89,52 @@ export interface EmittedReference { importedFile: ImportedFile; } +/** + * Represents a failure to emit a `Reference` into a different source file. + */ +export interface FailedEmitResult { + kind: ReferenceEmitKind.Failed; + + /** + * The reference that could not be emitted. + */ + ref: Reference; + + /** + * The source file into which the reference was requested to be emitted. + */ + context: ts.SourceFile; + + /** + * Describes why the reference could not be emitted. This may be shown in a diagnostic. + */ + reason: string; +} + +export type ReferenceEmitResult = EmittedReference|FailedEmitResult; + +/** + * Verifies that a reference was emitted successfully, or raises a `FatalDiagnosticError` otherwise. + * @param result The emit result that should have been successful. + * @param origin The node that is used to report the failure diagnostic. + * @param typeKind The kind of the symbol that the reference represents, e.g. 'component' or + * 'class'. + */ +export function assertSuccessfulReferenceEmit( + result: ReferenceEmitResult, origin: ts.Node, + typeKind: string): asserts result is EmittedReference { + if (result.kind === ReferenceEmitKind.Success) { + return; + } + + const message = makeDiagnosticChain( + `Unable to import ${typeKind} ${nodeNameForError(result.ref.node)}.`, + [makeDiagnosticChain(result.reason)]); + throw new FatalDiagnosticError( + ErrorCode.IMPORT_GENERATION_FAILURE, origin, message, + [makeRelatedInformation(result.ref.node, `The ${typeKind} is declared here.`)]); +} + /** * A particular strategy for generating an expression which refers to a `Reference`. * @@ -104,7 +158,7 @@ export interface ReferenceEmitStrategy { * @returns an `EmittedReference` which refers to the `Reference`, or `null` if none can be * generated */ - emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): EmittedReference|null; + emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): ReferenceEmitResult|null; } /** @@ -117,15 +171,20 @@ export class ReferenceEmitter { constructor(private strategies: ReferenceEmitStrategy[]) {} emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags = ImportFlags.None): - EmittedReference { + ReferenceEmitResult { for (const strategy of this.strategies) { const emitted = strategy.emit(ref, context, importFlags); if (emitted !== null) { return emitted; } } - throw new Error(`Unable to write a reference to ${nodeNameForError(ref.node)} in ${ - ref.node.getSourceFile().fileName} from ${context.fileName}`); + + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: `Unable to write a reference to ${nodeNameForError(ref.node)}.`, + }; } } @@ -150,6 +209,7 @@ export class LocalIdentifierStrategy implements ReferenceEmitStrategy { // invalid emission of a free-standing `foo` identifier, rather than `exports.foo`. if (!isDeclaration(ref.node) && refSf === context) { return { + kind: ReferenceEmitKind.Success, expression: new WrappedNodeExpr(ref.node), importedFile: null, }; @@ -160,6 +220,7 @@ export class LocalIdentifierStrategy implements ReferenceEmitStrategy { const identifier = ref.getIdentityIn(context); if (identifier !== null) { return { + kind: ReferenceEmitKind.Success, expression: new WrappedNodeExpr(identifier), importedFile: null, }; @@ -176,12 +237,12 @@ interface ModuleExports { /** * The source file of the module. */ - module: ts.SourceFile; + module: ts.SourceFile|null; /** * The map of declarations to their exported name. */ - exportMap: Map; + exportMap: Map|null; } /** @@ -198,13 +259,13 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { * A cache of the exports of specific modules, because resolving a module to its exports is a * costly operation. */ - private moduleExportsCache = new Map(); + private moduleExportsCache = new Map(); constructor( protected program: ts.Program, protected checker: ts.TypeChecker, protected moduleResolver: ModuleResolver, private reflectionHost: ReflectionHost) {} - emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): EmittedReference|null { + emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): ReferenceEmitResult|null { if (ref.bestGuessOwningModule === null) { // There is no module name available for this Reference, meaning it was arrived at via a // relative path. @@ -221,38 +282,48 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { // Try to find the exported name of the declaration, if one is available. const {specifier, resolutionContext} = ref.bestGuessOwningModule; const exports = this.getExportsOfModule(specifier, resolutionContext); - if (exports === null || !exports.exportMap.has(ref.node)) { - // TODO(alxhub): make this error a ts.Diagnostic pointing at whatever caused this import to be - // triggered. - throw new Error(`Symbol ${ref.debugName} declared in ${ - getSourceFile(ref.node).fileName} is not exported from ${specifier} (import into ${ - context.fileName})`); + if (exports.module === null) { + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: `The module '${specifier}' could not be found.`, + }; + } else if (exports.exportMap === null || !exports.exportMap.has(ref.node)) { + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: + `The symbol is not exported from ${exports.module.fileName} (module '${specifier}').`, + }; } const symbolName = exports.exportMap.get(ref.node)!; return { + kind: ReferenceEmitKind.Success, expression: new ExternalExpr(new ExternalReference(specifier, symbolName)), importedFile: exports.module, }; } - private getExportsOfModule(moduleName: string, fromFile: string): ModuleExports|null { + private getExportsOfModule(moduleName: string, fromFile: string): ModuleExports { if (!this.moduleExportsCache.has(moduleName)) { this.moduleExportsCache.set(moduleName, this.enumerateExportsOfModule(moduleName, fromFile)); } return this.moduleExportsCache.get(moduleName)!; } - protected enumerateExportsOfModule(specifier: string, fromFile: string): ModuleExports|null { + protected enumerateExportsOfModule(specifier: string, fromFile: string): ModuleExports { // First, resolve the module specifier to its entry point, and get the ts.Symbol for it. const entryPointFile = this.moduleResolver.resolveModule(specifier, fromFile); if (entryPointFile === null) { - return null; + return {module: null, exportMap: null}; } const exports = this.reflectionHost.getExportsOfModule(entryPointFile); if (exports === null) { - return null; + return {module: entryPointFile, exportMap: null}; } const exportMap = new Map(); for (const [name, declaration] of exports) { @@ -284,7 +355,7 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { export class LogicalProjectStrategy implements ReferenceEmitStrategy { constructor(private reflector: ReflectionHost, private logicalFs: LogicalFileSystem) {} - emit(ref: Reference, context: ts.SourceFile): EmittedReference|null { + emit(ref: Reference, context: ts.SourceFile): ReferenceEmitResult|null { const destSf = getSourceFile(ref.node); // Compute the relative path from the importing file to the file being imported. This is done @@ -292,7 +363,14 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { const destPath = this.logicalFs.logicalPathOfSf(destSf); if (destPath === null) { // The imported file is not within the logical project filesystem. - return null; + // Note: this error is analogous to `TS6059: File is not under 'rootDir'` that TypeScript + // reports. + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: `The file ${destSf.fileName} is outside of the configured 'rootDir'.`, + }; } const originPath = this.logicalFs.logicalPathOfSf(context); @@ -309,13 +387,19 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { const name = findExportedNameOfNode(ref.node, destSf, this.reflector); if (name === null) { // The target declaration isn't exported from the file it's declared in. This is an issue! - return null; + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: `The symbol is not exported from ${destSf.fileName}.`, + }; } // With both files expressed as LogicalProjectPaths, getting the module specifier as a relative // path is now straightforward. const moduleName = LogicalProjectPath.relativePathBetween(originPath, destPath); return { + kind: ReferenceEmitKind.Success, expression: new ExternalExpr({moduleName, name}), importedFile: destSf, }; @@ -331,14 +415,26 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { export class RelativePathStrategy implements ReferenceEmitStrategy { constructor(private reflector: ReflectionHost) {} - emit(ref: Reference, context: ts.SourceFile): EmittedReference|null { + emit(ref: Reference, context: ts.SourceFile): ReferenceEmitResult|null { const destSf = getSourceFile(ref.node); const relativePath = relative(dirname(absoluteFromSourceFile(context)), absoluteFromSourceFile(destSf)); const moduleName = toRelativeImport(stripExtension(relativePath)); const name = findExportedNameOfNode(ref.node, destSf, this.reflector); - return {expression: new ExternalExpr({moduleName, name}), importedFile: destSf}; + if (name === null) { + return { + kind: ReferenceEmitKind.Failed, + ref, + context, + reason: `The symbol is not exported from ${destSf.fileName}.`, + }; + } + return { + kind: ReferenceEmitKind.Success, + expression: new ExternalExpr({moduleName, name}), + importedFile: destSf, + }; } } @@ -360,6 +456,7 @@ export class UnifiedModulesStrategy implements ReferenceEmitStrategy { this.unifiedModulesHost.fileNameToModuleName(destSf.fileName, context.fileName); return { + kind: ReferenceEmitKind.Success, expression: new ExternalExpr({moduleName, name}), importedFile: destSf, }; diff --git a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts index 65e0f88a8bf..61214cee43e 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts @@ -7,6 +7,7 @@ */ import ts from 'typescript'; + import {ReflectionHost} from '../../reflection'; import {isNamedDeclaration} from '../../util/src/typescript'; @@ -36,32 +37,5 @@ export function findExportedNameOfNode( foundExportName = exportName; } - - if (foundExportName === null) { - throw new Error( - `Failed to find exported name of node (${target.getText()}) in '${file.fileName}'.`); - } return foundExportName; } - -/** - * Check whether a given `ts.Symbol` represents a declaration of a given node. - * - * This is not quite as trivial as just checking the declarations, as some nodes are - * `ts.ExportSpecifier`s and need to be unwrapped. - */ -function symbolDeclaresNode(sym: ts.Symbol, node: ts.Node, checker: ts.TypeChecker): boolean { - if (sym.declarations === undefined) { - return false; - } - - return sym.declarations.some(decl => { - if (ts.isExportSpecifier(decl)) { - const exportedSymbol = checker.getExportSpecifierLocalTargetSymbol(decl); - if (exportedSymbol !== undefined) { - return symbolDeclaresNode(exportedSymbol, node, checker); - } - } - return decl === node; - }); -} diff --git a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts index 04e1737302c..fc58e5a6ace 100644 --- a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts +++ b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts @@ -7,13 +7,13 @@ */ import {ExternalExpr} from '@angular/compiler'; import ts from 'typescript'; -import {UnifiedModulesHost} from '../../core/api'; +import {UnifiedModulesHost} from '../../core/api'; 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, RelativePathStrategy, UnifiedModulesStrategy} from '../src/emitter'; +import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy, ReferenceEmitKind, RelativePathStrategy, UnifiedModulesStrategy} from '../src/emitter'; import {Reference} from '../src/references'; import {ModuleResolver} from '../src/resolver'; @@ -75,7 +75,7 @@ runInEachFileSystem(() => { resolutionContext: context.fileName, }); const emitted = strategy.emit(reference, context, ImportFlags.None); - if (emitted === null) { + if (emitted === null || emitted.kind !== ReferenceEmitKind.Success) { return fail('Reference should be emitted'); } if (!(emitted.expression instanceof ExternalExpr)) { @@ -108,7 +108,7 @@ runInEachFileSystem(() => { resolutionContext: context.fileName, }); const emitted = strategy.emit(reference, context, ImportFlags.None); - if (emitted === null) { + if (emitted === null || emitted.kind !== ReferenceEmitKind.Success) { return fail('Reference should be emitted'); } if (!(emitted.expression instanceof ExternalExpr)) { @@ -160,7 +160,7 @@ runInEachFileSystem(() => { const reference = new Reference(decl, {specifier: 'external', resolutionContext: context.fileName}); const emitted = strategy.emit(reference, context, ImportFlags.AllowTypeImports); - if (emitted === null) { + if (emitted === null || emitted.kind !== ReferenceEmitKind.Success) { return fail('Reference should be emitted'); } if (!(emitted.expression instanceof ExternalExpr)) { @@ -204,7 +204,9 @@ runInEachFileSystem(() => { const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration); const context = program.getSourceFile(_('/context.ts'))!; const ref = strategy.emit(new Reference(decl), context); - expect(ref).not.toBeNull(); + if (ref === null || ref.kind !== ReferenceEmitKind.Success) { + return fail('Reference should be emitted'); + } // Expect the prefixed name from the TestHost. expect((ref!.expression as ExternalExpr).value.name).toEqual('testFoo'); @@ -232,7 +234,7 @@ runInEachFileSystem(() => { 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) { + if (emitted === null || emitted.kind !== ReferenceEmitKind.Success) { return fail('Reference should be emitted'); } if (!(emitted.expression instanceof ExternalExpr)) { @@ -265,7 +267,7 @@ runInEachFileSystem(() => { 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) { + if (emitted === null || emitted.kind !== ReferenceEmitKind.Success) { return fail('Reference should be emitted'); } if (!(emitted.expression instanceof ExternalExpr)) { diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index c1fd10c7784..3bc5cdda232 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -10,7 +10,7 @@ import {ExternalExpr, SchemaMetadata} from '@angular/compiler'; import ts from 'typescript'; import {ErrorCode, makeDiagnostic, makeRelatedInformation} from '../../diagnostics'; -import {AliasingHost, Reexport, Reference, ReferenceEmitter} from '../../imports'; +import {AliasingHost, assertSuccessfulReferenceEmit, Reexport, Reference, ReferenceEmitter} from '../../imports'; import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata'; import {ClassDeclaration, DeclarationNode} from '../../reflection'; import {identifierOfNode, nodeNameForError} from '../../util/src/typescript'; @@ -511,8 +511,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop asAlias: exportName, }); } else { - const expr = - this.refEmitter.emit(exportRef.cloneWithNoIdentifiers(), sourceFile).expression; + const emittedRef = this.refEmitter.emit(exportRef.cloneWithNoIdentifiers(), sourceFile); + assertSuccessfulReferenceEmit(emittedRef, ngModuleRef.node.name, 'class'); + const expr = emittedRef.expression; if (!(expr instanceof ExternalExpr) || expr.value.moduleName === null || expr.value.name === null) { throw new Error('Expected ExternalExpr'); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index c8975b22e27..0995cffe968 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -9,7 +9,7 @@ import {ExpressionType, ExternalExpr, Type, WrappedNodeExpr} from '@angular/compiler'; import ts from 'typescript'; -import {ImportFlags, Reference, ReferenceEmitter} from '../../imports'; +import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; import {ImportManager, translateExpression, translateType} from '../../translator'; import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from '../api'; @@ -121,6 +121,7 @@ export class Environment { // in these cases as there is no strict dependency checking during the template type-checking // pass. const ngExpr = this.refEmitter.emit(ref, this.contextFile, ImportFlags.NoAliasing); + assertSuccessfulReferenceEmit(ngExpr, this.contextFile, 'class'); // Use `translateExpression` to convert the `Expression` into a `ts.Expression`. return translateExpression(ngExpr.expression, this.importManager); @@ -134,6 +135,7 @@ export class Environment { referenceType(ref: Reference): ts.TypeNode { const ngExpr = this.refEmitter.emit( ref, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports); + assertSuccessfulReferenceEmit(ngExpr, this.contextFile, 'symbol'); // Create an `ExpressionType` from the `Expression` and translate it via `translateType`. // TODO(alxhub): support references to types with generic arguments in a clean way. diff --git a/packages/compiler-cli/test/ngtsc/imports_spec.ts b/packages/compiler-cli/test/ngtsc/imports_spec.ts new file mode 100644 index 00000000000..cc3d7cbc428 --- /dev/null +++ b/packages/compiler-cli/test/ngtsc/imports_spec.ts @@ -0,0 +1,119 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import ts from 'typescript'; + +import {absoluteFrom} from '../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; +import {loadStandardTestFiles} from '../../src/ngtsc/testing'; + +import {NgtscTestEnvironment} from './env'; + +const testFiles = loadStandardTestFiles(); + +runInEachFileSystem(() => { + describe('import generation', () => { + let env!: NgtscTestEnvironment; + + beforeEach(() => { + env = NgtscTestEnvironment.setup(testFiles, absoluteFrom('/app')); + const tsconfig: {[key: string]: any} = { + extends: '../tsconfig-base.json', + compilerOptions: { + baseUrl: '.', + rootDirs: ['/app'], + }, + angularCompilerOptions: {}, + }; + env.write('tsconfig.json', JSON.stringify(tsconfig, null, 2)); + }); + + it('should report an error when using a directive outside of rootDirs', () => { + env.write('/app/module.ts', ` + import {NgModule} from '@angular/core'; + import {ExternalDir} from '../lib/dir'; + import {MyComponent} from './comp'; + + @NgModule({ + declarations: [ExternalDir, MyComponent], + }) + export class MyModule {} + `); + env.write('/app/comp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class MyComponent {} + `); + env.write('/lib/dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[external]'}) + export class ExternalDir {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(ts.flattenDiagnosticMessageText(diags[0].messageText, '\n')) + .toEqual(`Unable to import class ExternalDir. + The file ${absoluteFrom('/lib/dir.ts')} is outside of the configured 'rootDir'.`); + expect(diags[0].file!.fileName).toEqual(absoluteFrom('/app/module.ts')); + expect(getDiagnosticSourceCode(diags[0])).toEqual('ExternalDir'); + }); + + it('should report an error when a library entry-point does not export the symbol', () => { + env.write('/app/module.ts', ` + import {NgModule} from '@angular/core'; + import {ExternalModule} from 'lib'; + import {MyComponent} from './comp'; + + @NgModule({ + imports: [ExternalModule], + declarations: [MyComponent], + }) + export class MyModule {} + `); + env.write('/app/comp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class MyComponent {} + `); + env.write('/node_modules/lib/index.d.ts', ` + import {ɵɵNgModuleDeclaration} from '@angular/core'; + import {ExternalDir} from './dir'; + + export class ExternalModule { + static ɵmod: ɵɵNgModuleDeclaration; + } + `); + env.write('/node_modules/lib/dir.d.ts', ` + import {ɵɵDirectiveDeclaration} from '@angular/core'; + + export class ExternalDir { + static ɵdir: ɵɵDirectiveDeclaration; + } + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(ts.flattenDiagnosticMessageText(diags[0].messageText, '\n')) + .toEqual(`Unable to import directive ExternalDir. + The symbol is not exported from ${absoluteFrom('/node_modules/lib/index.d.ts')} (module 'lib').`); + expect(diags[0].file!.fileName).toEqual(absoluteFrom('/app/comp.ts')); + expect(getDiagnosticSourceCode(diags[0])).toEqual('MyComponent'); + }); + }); +}); + +function getDiagnosticSourceCode(diag: ts.Diagnostic): string { + return diag.file!.text.substring(diag.start!, diag.start! + diag.length!); +}