From 62510a7b4d97e1ab811e40f2e027b883fcd45e2d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 11 Mar 2024 14:50:36 +0000 Subject: [PATCH] refactor(compiler-cli): update type check generation code to use new import manager (#54983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: https://github.com/angular/angular/pull/53521#pullrequestreview-1778130879. In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. https://github.com/microsoft/TypeScript/pull/56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons. PR Close #54983 --- package.json | 2 +- .../src/ngtsc/typecheck/BUILD.bazel | 1 + .../src/ngtsc/typecheck/src/context.ts | 97 ++++++++++++------- .../src/ngtsc/typecheck/src/environment.ts | 4 +- .../src/reference_emit_environment.ts | 4 +- .../src/ngtsc/typecheck/src/tcb_util.ts | 20 ++-- .../src/ngtsc/typecheck/src/ts_util.ts | 11 --- .../ngtsc/typecheck/src/type_check_file.ts | 32 ++++-- yarn.lock | 15 ++- 9 files changed, 118 insertions(+), 68 deletions(-) diff --git a/package.json b/package.json index 44cc30765e7c1..c85d821597d27 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "karma-jasmine": "^5.0.0", "karma-requirejs": "^1.1.0", "karma-sourcemap-loader": "^0.4.0", - "magic-string": "0.30.7", + "magic-string": "^0.30.8", "memo-decorator": "^2.0.1", "ngx-flamegraph": "0.0.12", "ngx-progressbar": "^11.1.0", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel index 47a64194b22dd..e070f16a35fc2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel @@ -26,6 +26,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/diagnostics", "//packages/compiler-cli/src/ngtsc/util", "@npm//@types/node", + "@npm//magic-string", "@npm//typescript", ], ) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index c216b3e520403..5f947c5843ac2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -7,16 +7,17 @@ */ import {BoundTarget, ParseError, ParseSourceFile, R3TargetBinder, SchemaMetadata, TmplAstNode} from '@angular/compiler'; +import MagicString from 'magic-string'; import ts from 'typescript'; import {ErrorCode, ngErrorCode} from '../../../../src/ngtsc/diagnostics'; import {absoluteFromSourceFile, AbsoluteFsPath} from '../../file_system'; -import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports'; +import {Reference, ReferenceEmitter} from '../../imports'; import {PipeMeta} from '../../metadata'; import {PerfEvent, PerfRecorder} from '../../perf'; import {FileUpdate} from '../../program_driver'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; -import {ImportManager} from '../../translator'; +import {ImportManagerV2} from '../../translator'; import {TemplateDiagnostic, TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckContext, TypeCheckingConfig, TypeCtorMetadata} from '../api'; import {makeTemplateDiagnostic} from '../diagnostics'; @@ -27,7 +28,6 @@ import {ReferenceEmitEnvironment} from './reference_emit_environment'; import {TypeCheckShimGenerator} from './shim'; import {TemplateSourceManager} from './source'; import {requiresInlineTypeCheckBlock, TcbInliningRequirement} from './tcb_util'; -import {getImportString} from './ts_util'; import {generateTypeCheckBlock, TcbGenericContextBehavior} from './type_check_block'; import {TypeCheckFile} from './type_check_file'; import {generateInlineTypeCtor, requiresInlineTypeCtor} from './type_constructor'; @@ -358,34 +358,61 @@ export class TypeCheckContextImpl implements TypeCheckContext { return null; } - // Imports may need to be added to the file to support type-checking of directives used in the - // template within it. - const importManager = new ImportManager(new NoopImportRewriter(), '_i'); - - // Each Op has a splitPoint index into the text where it needs to be inserted. Split the - // original source text into chunks at these split points, where code will be inserted between - // the chunks. - const ops = this.opMap.get(sf)!.sort(orderOps); - const textParts = splitStringAtPoints(sf.text, ops.map(op => op.splitPoint)); - // Use a `ts.Printer` to generate source code. const printer = ts.createPrinter({omitTrailingSemicolon: true}); - // Begin with the initial section of the code text. - let code = textParts[0]; - - // Process each operation and use the printer to generate source code for it, inserting it into - // the source code in between the original chunks. - ops.forEach((op, idx) => { - const text = op.execute(importManager, sf, this.refEmitter, printer); - code += '\n\n' + text + textParts[idx + 1]; + // Imports may need to be added to the file to support type-checking of directives + // used in the template within it. + const importManager = new ImportManagerV2({ + // This minimizes noticeable changes with older versions of `ImportManager`. + forceGenerateNamespacesForNewImports: true, + // Type check block code affects code completion and fix suggestions. + // We want to encourage single quotes for now, like we always did. + shouldUseSingleQuotes: () => true, }); - // Write out the imports that need to be added to the beginning of the file. - let imports = importManager.getAllImports(sf.fileName).map(getImportString).join('\n'); - code = imports + '\n' + code; + // Execute ops. + // Each Op has a splitPoint index into the text where it needs to be inserted. + const updates: {pos: number, deletePos?: number, text: string}[] = + this.opMap.get(sf)!.map(op => { + return { + pos: op.splitPoint, + text: op.execute(importManager, sf, this.refEmitter, printer), + }; + }); + + const {newImports, updatedImports} = importManager.finalize(); + + // Capture new imports + if (newImports.has(sf.fileName)) { + newImports.get(sf.fileName)!.forEach(newImport => { + updates.push({ + pos: 0, + text: printer.printNode(ts.EmitHint.Unspecified, newImport, sf), + }); + }); + } + + // Capture updated imports + for (const [oldBindings, newBindings] of updatedImports.entries()) { + if (oldBindings.getSourceFile() !== sf) { + throw new Error('Unexpected updates to unrelated source files.'); + } + updates.push({ + pos: oldBindings.getStart(), + deletePos: oldBindings.getEnd(), + text: printer.printNode(ts.EmitHint.Unspecified, newBindings, sf), + }); + } - return code; + const result = new MagicString(sf.text, {filename: sf.fileName}); + for (const update of updates) { + if (update.deletePos !== undefined) { + result.remove(update.pos, update.deletePos); + } + result.appendLeft(update.pos, update.text); + } + return result.toString(); } finalize(): Map { @@ -510,8 +537,9 @@ interface Op { /** * Execute the operation and return the generated code as text. */ - execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer): - string; + execute( + im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter, + printer: ts.Printer): string; } /** @@ -531,16 +559,18 @@ class InlineTcbOp implements Op { return this.ref.node.end + 1; } - execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer): - string { + execute( + im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter, + printer: ts.Printer): string { const env = new Environment(this.config, im, refEmitter, this.reflector, sf); const fnName = ts.factory.createIdentifier(`_tcb_${this.ref.node.pos}`); - // Inline TCBs should copy any generic type parameter nodes directly, as the TCB code is inlined - // into the class in a context where that will always be legal. + // Inline TCBs should copy any generic type parameter nodes directly, as the TCB code is + // inlined into the class in a context where that will always be legal. const fn = generateTypeCheckBlock( env, this.ref, fnName, this.meta, this.domSchemaChecker, this.oobRecorder, TcbGenericContextBehavior.CopyClassNodes); + return printer.printNode(ts.EmitHint.Unspecified, fn, sf); } } @@ -560,8 +590,9 @@ class TypeCtorOp implements Op { return this.ref.node.end - 1; } - execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer): - string { + execute( + im: ImportManagerV2, sf: ts.SourceFile, refEmitter: ReferenceEmitter, + printer: ts.Printer): string { const emitEnv = new ReferenceEmitEnvironment(im, refEmitter, this.reflector, sf); const tcb = generateInlineTypeCtor(emitEnv, this.ref.node, this.meta); return printer.printNode(ts.EmitHint.Unspecified, tcb, sf); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index d1b184d49bcb2..ce5acb42e1d09 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -10,7 +10,7 @@ import ts from 'typescript'; import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; -import {ImportManager, translateExpression} from '../../translator'; +import {ImportManagerV2, translateExpression} from '../../translator'; import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from '../api'; import {ReferenceEmitEnvironment} from './reference_emit_environment'; @@ -42,7 +42,7 @@ export class Environment extends ReferenceEmitEnvironment { protected pipeInstStatements: ts.Statement[] = []; constructor( - readonly config: TypeCheckingConfig, importManager: ImportManager, + readonly config: TypeCheckingConfig, importManager: ImportManagerV2, refEmitter: ReferenceEmitter, reflector: ReflectionHost, contextFile: ts.SourceFile) { super(importManager, refEmitter, reflector, contextFile); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts index 1a70753c2883c..45d984a39079c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts @@ -11,7 +11,7 @@ import ts from 'typescript'; import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitKind, ReferenceEmitter} from '../../imports'; import {ReflectionHost} from '../../reflection'; -import {ImportManager, translateExpression, translateType} from '../../translator'; +import {ImportManagerV2, translateExpression, translateType} from '../../translator'; /** * An environment for a given source file that can be used to emit references. @@ -21,7 +21,7 @@ import {ImportManager, translateExpression, translateType} from '../../translato */ export class ReferenceEmitEnvironment { constructor( - readonly importManager: ImportManager, protected refEmitter: ReferenceEmitter, + readonly importManager: ImportManagerV2, protected refEmitter: ReferenceEmitter, readonly reflector: ReflectionHost, public contextFile: ts.SourceFile) {} canReferenceType( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts index 07100b3ac3302..12814821dc28b 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts @@ -19,17 +19,21 @@ import {ReferenceEmitEnvironment} from './reference_emit_environment'; import {TypeParameterEmitter} from './type_parameter_emitter'; /** - * External modules that always should exist for type check blocks and - * file hosting inline type constructors. + * External modules/identifiers that always should exist for type check + * block files. * * Importing the modules in preparation helps ensuring a stable import graph * that would not degrade TypeScript's incremental program structure re-use. + * + * Note: For inline type check blocks, or type constructors, we cannot add preparation + * imports, but ideally the required modules are already imported and can be re-used + * to not incur a structural TypeScript program re-use discarding. */ -const TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES = [ +const TCB_FILE_IMPORT_GRAPH_PREPARE_IDENTIFIERS = [ // Imports may be added for signal input checking. We wouldn't want to change the // import graph for incremental compilations when suddenly a signal input is used, // or removed. - R3Identifiers.InputSignalBrandWriteType.moduleName, + R3Identifiers.InputSignalBrandWriteType, ]; /** @@ -194,8 +198,12 @@ function getTemplateId( * import graph changes whenever e.g. a signal input is introduced in user code. */ export function ensureTypeCheckFilePreparationImports(env: ReferenceEmitEnvironment): void { - for (const moduleName of TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES) { - env.importManager.generateNamespaceImport(moduleName); + for (const identifier of TCB_FILE_IMPORT_GRAPH_PREPARE_IDENTIFIERS) { + env.importManager.addImport({ + exportModuleSpecifier: identifier.moduleName, + exportSymbolName: identifier.name, + requestedFile: env.contextFile, + }); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts index a943050d8a16f..33aee30fb4112 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts @@ -8,10 +8,7 @@ import ts from 'typescript'; -import {Import} from '../../translator'; - import {addExpressionIdentifier, ExpressionIdentifier} from './comments'; -import {wrapForTypeChecker} from './diagnostics'; @@ -163,11 +160,3 @@ export function tsNumericExpression(value: number): ts.NumericLiteral|ts.PrefixU return ts.factory.createNumericLiteral(value); } - -export function getImportString(imp: Import): string { - if (imp.qualifier === null) { - return `import from '${imp.specifier}';`; - } else { - return `import * as ${imp.qualifier.text} from '${imp.specifier}';`; - } -} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts index 6f46600fb0b05..257100b91fd59 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts @@ -8,16 +8,15 @@ import ts from 'typescript'; import {AbsoluteFsPath, join} from '../../file_system'; -import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports'; +import {Reference, ReferenceEmitter} from '../../imports'; import {ClassDeclaration, ReflectionHost} from '../../reflection'; -import {ImportManager} from '../../translator'; +import {ImportManagerV2} from '../../translator'; import {TypeCheckBlockMetadata, TypeCheckingConfig} from '../api'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {OutOfBandDiagnosticRecorder} from './oob'; import {ensureTypeCheckFilePreparationImports} from './tcb_util'; -import {getImportString} from './ts_util'; import {generateTypeCheckBlock, TcbGenericContextBehavior} from './type_check_block'; @@ -38,7 +37,14 @@ export class TypeCheckFile extends Environment { readonly fileName: AbsoluteFsPath, config: TypeCheckingConfig, refEmitter: ReferenceEmitter, reflector: ReflectionHost, compilerHost: Pick) { super( - config, new ImportManager(new NoopImportRewriter(), 'i'), refEmitter, reflector, + config, new ImportManagerV2({ + // This minimizes noticeable changes with older versions of `ImportManager`. + forceGenerateNamespacesForNewImports: true, + // Type check block code affects code completion and fix suggestions. + // We want to encourage single quotes for now, like we always did. + shouldUseSingleQuotes: () => true + }), + refEmitter, reflector, ts.createSourceFile( compilerHost.getCanonicalFileName(fileName), '', ts.ScriptTarget.Latest, true)); } @@ -60,11 +66,21 @@ export class TypeCheckFile extends Environment { // import to e.g. `@angular/core` always exists to guarantee a stable graph. ensureTypeCheckFilePreparationImports(this); - let source: string = this.importManager.getAllImports(this.contextFile.fileName) - .map(getImportString) - .join('\n') + - '\n\n'; + const importChanges = this.importManager.finalize(); + if (importChanges.updatedImports.size > 0) { + throw new Error( + 'AssertionError: Expected no imports to be updated for a new type check file.'); + } + const printer = ts.createPrinter({removeComments}); + let source = ''; + + const newImports = importChanges.newImports.get(this.contextFile.fileName); + if (newImports !== undefined) { + source += newImports.map(i => printer.printNode(ts.EmitHint.Unspecified, i, this.contextFile)) + .join('\n'); + } + source += '\n'; for (const stmt of this.pipeInstStatements) { source += printer.printNode(ts.EmitHint.Unspecified, stmt, this.contextFile) + '\n'; diff --git a/yarn.lock b/yarn.lock index 75a944222f85c..10a3e9d640bd8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -386,7 +386,6 @@ "@angular/build-tooling@https://github.com/angular/dev-infra-private-build-tooling-builds.git#65d002a534a74daa3c9bd26bdea5a092cbd519df": version "0.0.0-5774b71c01a55c4c998f858ee37d3b77ae704c31" - uid "65d002a534a74daa3c9bd26bdea5a092cbd519df" resolved "https://github.com/angular/dev-infra-private-build-tooling-builds.git#65d002a534a74daa3c9bd26bdea5a092cbd519df" dependencies: "@angular-devkit/build-angular" "17.2.0-rc.0" @@ -495,7 +494,6 @@ "@angular/docs@https://github.com/angular/dev-infra-private-docs-builds.git#82c4573f5c9d4fb864271a1c74259fc251457e2f": version "0.0.0-5774b71c01a55c4c998f858ee37d3b77ae704c31" - uid "82c4573f5c9d4fb864271a1c74259fc251457e2f" resolved "https://github.com/angular/dev-infra-private-docs-builds.git#82c4573f5c9d4fb864271a1c74259fc251457e2f" dependencies: "@angular/cdk" "17.3.0-next.0" @@ -4876,8 +4874,7 @@ "@types/glob" "*" "@types/node" "*" -"@types/selenium-webdriver4@npm:@types/selenium-webdriver@4.1.21", "@types/selenium-webdriver@^4.1.21": - name "@types/selenium-webdriver4" +"@types/selenium-webdriver4@npm:@types/selenium-webdriver@4.1.21": version "4.1.21" resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-4.1.21.tgz#79fe31faf9953a4143c3e32944d98d5146bbe185" integrity sha512-QGURnImvxYlIQz5DVhvHdqpYNLBjhJ2Vm+cnQI2G9QZzkWlZm0LkLcvDcHp+qE6N2KBz4CeuvXgPO7W3XQ0Tyw== @@ -4894,6 +4891,14 @@ resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-3.0.25.tgz#7982bf5add9539b4e953512354055545e8508cff" integrity sha512-gkb8rx0kjHuaVUzIfKa1tVVjnEmjYLufDmoOHKPsaoRraTcNjV9W9ruFWi3Xv821c7M5gZVmrGOT8BJYsY/acQ== +"@types/selenium-webdriver@^4.1.21": + name "@types/selenium-webdriver4" + version "4.1.22" + resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-4.1.22.tgz#344519b90727eb713e1ce6d2e0198eb0b4f8f316" + integrity sha512-MCL4l7q8dwxejr2Q2NXLyNwHWMPdlWE0Kpn6fFwJtvkJF7PTkG5jkvbH/X1IAAQxgt/L1dA8u2GtDeekvSKvOA== + dependencies: + "@types/ws" "*" + "@types/semver@^7.3.4": version "7.5.4" resolved "https://registry.yarnpkg.com/@types/semver/-/semver-7.5.4.tgz#0a41252ad431c473158b22f9bfb9a63df7541cff" @@ -12480,7 +12485,7 @@ magic-string@0.30.7: dependencies: "@jridgewell/sourcemap-codec" "^1.4.15" -magic-string@0.30.8: +magic-string@0.30.8, magic-string@^0.30.8: version "0.30.8" resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.30.8.tgz#14e8624246d2bedba70d5462aa99ac9681844613" integrity sha512-ISQTe55T2ao7XtlAStud6qwYPZjE4GK1S/BeVPus4jrq6JuOnQ00YKQC581RWhR122W7msZV263KzVeLoqidyQ==