Skip to content

Commit 973033a

Browse files
JoostKthePunderWoman
JoostK
authored and
thePunderWoman
committed
fix(compiler-cli): avoid crash in isolated transform operations (#59869)
The CLI uses the `ts.transform` API to apply the Angular compiler's transformers on the source files when `isolatedModules` is true (and various other constraints) instead of going through a full `ts.Program.emit` operation. This results in the transformers to operate in an environment where no emit resolver is available, which was not previously accounted for. This commit reflects the possibility for the emit resolver to be missing and handles this scenario accordingly. Fixes #59837 PR Close #59869
1 parent fb39fe9 commit 973033a

File tree

7 files changed

+162
-37
lines changed

7 files changed

+162
-37
lines changed

‎packages/compiler-cli/src/ngtsc/imports/src/default.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ export class DefaultImportTracker {
109109
if (clausesToPreserve === null) {
110110
clausesToPreserve = loadIsReferencedAliasDeclarationPatch(context);
111111
}
112-
clausesToPreserve.add(clause);
112+
clausesToPreserve?.add(clause);
113113
}
114114
}
115115

‎packages/compiler-cli/src/ngtsc/imports/src/patch_alias_reference_resolution.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ export type AliasImportDeclaration = ts.ImportSpecifier | ts.NamespaceImport | t
1717
* that as public API: https://github.com/microsoft/TypeScript/issues/17516.
1818
*/
1919
interface TransformationContextWithResolver extends ts.TransformationContext {
20-
getEmitResolver: () => EmitResolver;
20+
getEmitResolver: () => EmitResolver | undefined;
2121
}
2222

2323
const patchedReferencedAliasesSymbol = Symbol('patchedReferencedAliases');
@@ -70,19 +70,29 @@ interface EmitResolver {
7070
* that have been referenced in a value-position by the transform, such the installed patch can
7171
* ensure that those import declarations are not elided.
7272
*
73+
* If `null` is returned then the transform operates in an isolated context, i.e. using the
74+
* `ts.transform` API. In such scenario there is no information whether an alias declaration
75+
* is referenced, so all alias declarations are naturally preserved and explicitly registering
76+
* an alias declaration as used isn't necessary.
77+
*
7378
* See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on
7479
* Github.
7580
* https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257
7681
*/
7782
export function loadIsReferencedAliasDeclarationPatch(
7883
context: ts.TransformationContext,
79-
): Set<ts.Declaration> {
84+
): Set<ts.Declaration> | null {
8085
// If the `getEmitResolver` method is not available, TS most likely changed the
8186
// internal structure of the transformation context. We will abort gracefully.
8287
if (!isTransformationContextWithEmitResolver(context)) {
8388
throwIncompatibleTransformationContextError();
8489
}
8590
const emitResolver = context.getEmitResolver();
91+
if (emitResolver === undefined) {
92+
// In isolated `ts.transform` operations no emit resolver is present, return null as `isReferencedAliasDeclaration`
93+
// will never be invoked.
94+
return null;
95+
}
8696

8797
// The emit resolver may have been patched already, in which case we return the set of referenced
8898
// aliases that was created when the patch was first applied.

‎packages/compiler-cli/src/ngtsc/imports/test/default_spec.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,50 @@ runInEachFileSystem(() => {
8282
expect(testContents).toContain(`var dep_1 = require("./dep");`);
8383
expect(testContents).toContain(`var ref = dep_1.default;`);
8484
});
85+
86+
it('should prevent a default import from being elided if used in an isolated transform', () => {
87+
const {program} = makeProgram(
88+
[
89+
{name: _('/dep.ts'), contents: `export default class Foo {}`},
90+
{
91+
name: _('/test.ts'),
92+
contents: `import Foo from './dep'; export function test(f: Foo) {}`,
93+
},
94+
95+
// This control file is identical to the test file, but will not have its import marked
96+
// for preservation. It exists to capture the behavior without the DefaultImportTracker's
97+
// emit modifications.
98+
{
99+
name: _('/ctrl.ts'),
100+
contents: `import Foo from './dep'; export function test(f: Foo) {}`,
101+
},
102+
],
103+
{
104+
module: ts.ModuleKind.ES2015,
105+
},
106+
);
107+
const fooClause = getDeclaration(program, _('/test.ts'), 'Foo', ts.isImportClause);
108+
const fooDecl = fooClause.parent as ts.ImportDeclaration;
109+
110+
const tracker = new DefaultImportTracker();
111+
tracker.recordUsedImport(fooDecl);
112+
113+
const result = ts.transform(
114+
[program.getSourceFile(_('/test.ts'))!, program.getSourceFile(_('/ctrl.ts'))!],
115+
[tracker.importPreservingTransformer()],
116+
);
117+
expect(result.diagnostics?.length ?? 0).toBe(0);
118+
expect(result.transformed.length).toBe(2);
119+
120+
const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
121+
122+
const testOutput = printer.printFile(result.transformed[0]);
123+
expect(testOutput).toContain(`import Foo from './dep';`);
124+
125+
// In an isolated transform, TypeScript also retains the default import.
126+
const ctrlOutput = printer.printFile(result.transformed[1]);
127+
expect(ctrlOutput).toContain(`import Foo from './dep';`);
128+
});
85129
});
86130

87131
function addReferenceTransformer(id: ts.Identifier): ts.TransformerFactory<ts.SourceFile> {

‎packages/compiler-cli/src/ngtsc/transform/jit/src/downlevel_decorators_transform.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@ export function getDownlevelDecoratorsTransform(
378378
// ensure that the alias declaration is not elided by TypeScript, and use its
379379
// name identifier to reference it at runtime.
380380
if (isAliasImportDeclaration(decl)) {
381-
referencedParameterTypes.add(decl);
381+
referencedParameterTypes?.add(decl);
382382
// If the entity name resolves to an alias import declaration, we reference the
383383
// entity based on the alias import name. This ensures that TypeScript properly
384384
// resolves the link to the import. Cloning the original entity name identifier

‎packages/compiler-cli/src/ngtsc/transform/jit/test/downlevel_decorators_transform_spec.ts‎

Lines changed: 73 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -872,38 +872,81 @@ describe('downlevel decorator transform', () => {
872872
);
873873
expect(written).toBe(numberOfTestFiles);
874874
});
875+
});
875876

876-
function createProgramWithTransform(files: string[]) {
877-
const program = ts.createProgram(
878-
files,
879-
{
880-
moduleResolution: ts.ModuleResolutionKind.Node10,
881-
importHelpers: true,
882-
lib: [],
883-
module: ts.ModuleKind.ESNext,
884-
target: ts.ScriptTarget.Latest,
885-
declaration: false,
886-
experimentalDecorators: true,
887-
emitDecoratorMetadata: false,
888-
},
889-
host,
890-
);
891-
const typeChecker = program.getTypeChecker();
892-
const reflectionHost = new TypeScriptReflectionHost(typeChecker);
893-
const transformers: ts.CustomTransformers = {
894-
before: [
895-
getDownlevelDecoratorsTransform(
896-
program.getTypeChecker(),
897-
reflectionHost,
898-
diagnostics,
899-
/* isCore */ false,
900-
isClosureEnabled,
901-
),
902-
],
903-
};
904-
return {program, transformers};
905-
}
877+
it('should work using an isolated transform operation', () => {
878+
context.writeFile(
879+
'foo_service.d.ts',
880+
`
881+
export declare class Foo {};
882+
`,
883+
);
884+
context.writeFile(
885+
'foo.ts',
886+
`
887+
import {Injectable} from '@angular/core';
888+
import {Foo} from './foo_service';
889+
890+
@Injectable()
891+
export class MyService {
892+
constructor(foo: Foo) {}
893+
}
894+
`,
895+
);
896+
897+
const {program, transformers} = createProgramWithTransform(['/foo.ts', '/foo_service.d.ts']);
898+
const result = ts.transform(program.getSourceFile('/foo.ts')!, [
899+
...(transformers.before ?? []),
900+
...(transformers.after ?? []),
901+
] as ts.TransformerFactory<ts.SourceFile>[]);
902+
expect(result.diagnostics?.length ?? 0).toBe(0);
903+
expect(result.transformed.length).toBe(1);
904+
905+
const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
906+
const output = printer.printFile(result.transformed[0]);
907+
expect(omitLeadingWhitespace(output)).toEqual(dedent`
908+
import { Injectable } from '@angular/core';
909+
import { Foo } from './foo_service';
910+
@Injectable()
911+
export class MyService {
912+
constructor(foo: Foo) { }
913+
static ctorParameters = () => [
914+
{ type: Foo }
915+
];
916+
}
917+
`);
906918
});
919+
920+
function createProgramWithTransform(files: string[]) {
921+
const program = ts.createProgram(
922+
files,
923+
{
924+
moduleResolution: ts.ModuleResolutionKind.Node10,
925+
importHelpers: true,
926+
lib: [],
927+
module: ts.ModuleKind.ESNext,
928+
target: ts.ScriptTarget.Latest,
929+
declaration: false,
930+
experimentalDecorators: true,
931+
emitDecoratorMetadata: false,
932+
},
933+
host,
934+
);
935+
const typeChecker = program.getTypeChecker();
936+
const reflectionHost = new TypeScriptReflectionHost(typeChecker);
937+
const transformers: ts.CustomTransformers = {
938+
before: [
939+
getDownlevelDecoratorsTransform(
940+
program.getTypeChecker(),
941+
reflectionHost,
942+
diagnostics,
943+
/* isCore */ false,
944+
isClosureEnabled,
945+
),
946+
],
947+
};
948+
return {program, transformers};
949+
}
907950
});
908951

909952
/** Template string function that can be used to dedent a given string literal. */

‎packages/compiler-cli/src/ngtsc/translator/src/import_manager/import_typescript_transform.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,11 @@ export function createTsTransformForImportManager(
3737
// doesn't drop these thinking they are unused.
3838
if (reusedOriginalAliasDeclarations.size > 0) {
3939
const referencedAliasDeclarations = loadIsReferencedAliasDeclarationPatch(ctx);
40-
reusedOriginalAliasDeclarations.forEach((aliasDecl) =>
41-
referencedAliasDeclarations.add(aliasDecl),
42-
);
40+
if (referencedAliasDeclarations !== null) {
41+
reusedOriginalAliasDeclarations.forEach((aliasDecl) =>
42+
referencedAliasDeclarations.add(aliasDecl),
43+
);
44+
}
4345
}
4446

4547
// Update the set of affected files to include files that need extra statements to be inserted.

‎packages/compiler-cli/src/ngtsc/translator/test/import_manager_spec.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,6 +1081,32 @@ describe('import manager', () => {
10811081
`),
10821082
);
10831083
});
1084+
1085+
it('should work when using an isolated transform', () => {
1086+
const {testFile} = createTestProgram('import { input } from "@angular/core";');
1087+
const manager = new ImportManager();
1088+
const ref = manager.addImport({
1089+
exportModuleSpecifier: '@angular/core',
1090+
exportSymbolName: 'input',
1091+
requestedFile: testFile,
1092+
});
1093+
1094+
const extraStatements = [ts.factory.createExpressionStatement(ref)];
1095+
const transformer = manager.toTsTransform(new Map([[testFile.fileName, extraStatements]]));
1096+
1097+
const result = ts.transform(testFile, [transformer]);
1098+
expect(result.diagnostics?.length ?? 0).toBe(0);
1099+
expect(result.transformed.length).toBe(1);
1100+
1101+
const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
1102+
const output = printer.printFile(result.transformed[0]);
1103+
expect(output).toBe(
1104+
omitLeadingWhitespace(`
1105+
import { input } from "@angular/core";
1106+
input;
1107+
`),
1108+
);
1109+
});
10841110
});
10851111

10861112
function createTestProgram(text: string): {

0 commit comments

Comments
 (0)