From cb113805156c0332c47d9d71b5e8c6573f9e360c Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 19 Dec 2019 16:33:56 -0800 Subject: [PATCH] fix(ivy): disable use of aliasing in template type-checking (#34649) FileToModuleHost aliasing supports compilation within environments that have two properties: 1. A `FileToModuleHost` exists which defines canonical module names for any given TS file. 2. Dependency restrictions exist which prevent the import of arbitrary files even if such files are within the .d.ts transitive closure of a compilation ("strictdeps"). In such an environment, generated imports can only go through import paths which are already present in the user program. The aliasing system supports the generation and consumption of such imports at runtime. `FileToModuleHost` aliasing does not emit re-exports in .d.ts files. This means that it's safe to rely on alias re-exports in generated .js code (they are guaranteed to exist at runtime) but not in template type-checking code (since TS will not be able to follow such imports). Therefore, non-aliased imports should be used in template type-checking code. This commit adds a `NoAliasing` flag to `ImportFlags` and sets it when generating imports in template type-checking code. The testing environment is also patched to support resolution of FileToModuleHost canonical paths within the template type-checking program, enabling testing of this change. PR Close #34649 --- .../src/ngtsc/imports/src/alias.ts | 4 ++ .../src/ngtsc/imports/src/emitter.ts | 8 +++ .../src/ngtsc/typecheck/src/environment.ts | 10 ++- .../src/ngtsc/typecheck/src/host.ts | 6 ++ packages/compiler-cli/test/ngtsc/env.ts | 19 ++++++ .../test/ngtsc/template_typecheck_spec.ts | 65 +++++++++++++++++++ 6 files changed, 109 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/imports/src/alias.ts b/packages/compiler-cli/src/ngtsc/imports/src/alias.ts index d28c53d8a5..6d0cfe2230 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/alias.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/alias.ts @@ -209,6 +209,10 @@ export class PrivateExportAliasingHost implements AliasingHost { */ export class AliasStrategy implements ReferenceEmitStrategy { emit(ref: Reference, context: ts.SourceFile, importMode: ImportFlags): Expression|null { + if (importMode & ImportFlags.NoAliasing) { + return null; + } + return ref.alias; } } diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index 4f5c12f98b..11bb847312 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -42,6 +42,14 @@ export enum ImportFlags { * This is sometimes required if there's a risk TypeScript might remove imports during emit. */ ForceNewImport = 0x01, + + /** + * Don't make use of any aliasing information when emitting a reference. + * + * This is sometimes required if emitting into a context where generated references will be fed + * into TypeScript and type-checked (such as in template type-checking). + */ + NoAliasing = 0x02, } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index 4a5c6dcb41..676bfc3146 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 * as ts from 'typescript'; -import {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports'; +import {ImportFlags, NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; import {ImportManager, translateExpression, translateType} from '../../translator'; @@ -205,7 +205,11 @@ export class Environment { * This may involve importing the node into the file if it's not declared there already. */ reference(ref: Reference>): ts.Expression { - const ngExpr = this.refEmitter.emit(ref, this.contextFile); + // Disable aliasing for imports generated in a template type-checking context, as there is no + // guarantee that any alias re-exports exist in the .d.ts files. It's safe to use direct imports + // 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); // Use `translateExpression` to convert the `Expression` into a `ts.Expression`. return translateExpression( @@ -218,7 +222,7 @@ export class Environment { * This may involve importing the node into the file if it's not declared there already. */ referenceType(ref: Reference): ts.TypeNode { - const ngExpr = this.refEmitter.emit(ref, this.contextFile); + const ngExpr = this.refEmitter.emit(ref, this.contextFile, ImportFlags.NoAliasing); // 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/src/ngtsc/typecheck/src/host.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts index d27f587127..8bbc0edd4c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/host.ts @@ -19,12 +19,18 @@ export class TypeCheckProgramHost implements ts.CompilerHost { */ private sfMap: Map; + readonly resolveModuleNames?: ts.CompilerHost['resolveModuleNames']; + constructor(sfMap: Map, private delegate: ts.CompilerHost) { this.sfMap = sfMap; if (delegate.getDirectories !== undefined) { this.getDirectories = (path: string) => delegate.getDirectories !(path); } + + if (delegate.resolveModuleNames !== undefined) { + this.resolveModuleNames = delegate.resolveModuleNames; + } } getSourceFile( diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index 66b242fa7e..d8c63fb1fa 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -248,12 +248,31 @@ class AugmentedCompilerHost extends NgtscCompilerHost { delegate !: ts.CompilerHost; } +const ROOT_PREFIX = 'root/'; + class FileNameToModuleNameHost extends AugmentedCompilerHost { fileNameToModuleName(importedFilePath: string): string { const relativeFilePath = this.fs.relative(this.fs.pwd(), this.fs.resolve(importedFilePath)); const rootedPath = this.fs.join('root', relativeFilePath); return rootedPath.replace(/(\.d)?.ts$/, ''); } + + resolveModuleNames( + moduleNames: string[], containingFile: string, reusedNames: string[]|undefined, + redirectedReference: ts.ResolvedProjectReference|undefined, + options: ts.CompilerOptions): (ts.ResolvedModule|undefined)[] { + return moduleNames.map(moduleName => { + if (moduleName.startsWith(ROOT_PREFIX)) { + // Strip the artificially added root prefix. + moduleName = '/' + moduleName.substr(ROOT_PREFIX.length); + } + + return ts + .resolveModuleName( + moduleName, containingFile, options, this, /* cache */ undefined, redirectedReference) + .resolvedModule; + }); + } } class MultiCompileHostExt extends AugmentedCompilerHost implements Partial { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 97641b203d..74ce588220 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -1173,6 +1173,71 @@ export declare class AnimationEvent { expect(getSourceCodeForDiagnostic(diags[0])).toEqual('y = !y'); }); + it('should still type-check when fileToModuleName aliasing is enabled, but alias exports are not in the .d.ts file', + () => { + // The template type-checking file imports directives/pipes in order to type-check their + // usage. When `FileToModuleHost` aliasing is enabled, these imports would ordinarily use + // aliased values. However, such aliases are not guaranteed to exist in the .d.ts files, + // and so feeding such imports back into TypeScript does not work. + // + // Instead, direct imports should be used within template type-checking code. This test + // verifies that template type-checking is able to cope with such a scenario where + // aliasing is enabled and alias re-exports don't exist in .d.ts files. + env.tsconfig({ + // Setting this private flag turns on aliasing. + '_useHostForImportGeneration': true, + // Because the tsconfig is overridden, template type-checking needs to be turned back on + // explicitly as well. + 'fullTemplateTypeCheck': true, + }); + + // 'alpha' declares the directive which will ultimately be imported. + env.write('alpha.d.ts', ` + import {ɵɵDirectiveDefWithMeta, ɵɵNgModuleDefWithMeta} from '@angular/core'; + + export declare class ExternalDir { + input: string; + static ɵdir: ɵɵDirectiveDefWithMeta; + } + + export declare class AlphaModule { + static ɵmod: ɵɵNgModuleDefWithMeta; + } + `); + + // 'beta' re-exports AlphaModule from alpha. + env.write('beta.d.ts', ` + import {ɵɵNgModuleDefWithMeta} from '@angular/core'; + import {AlphaModule} from './alpha'; + + export declare class BetaModule { + static ɵmod: ɵɵNgModuleDefWithMeta; + } + `); + + // The application imports BetaModule from beta, gaining visibility of ExternalDir from + // alpha. + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {BetaModule} from './beta'; + + @Component({ + selector: 'cmp', + template: '
', + }) + export class Cmp {} + + @NgModule({ + declarations: [Cmp], + imports: [BetaModule], + }) + export class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + describe('input coercion', () => { beforeEach(() => { env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true});