Skip to content

Commit

Permalink
fix(compiler-cli): avoid crash in isolated transform operations (#59869)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JoostK authored and thePunderWoman committed Feb 13, 2025
1 parent 04851c7 commit 16fc074
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 37 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/imports/src/default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export class DefaultImportTracker {
if (clausesToPreserve === null) {
clausesToPreserve = loadIsReferencedAliasDeclarationPatch(context);
}
clausesToPreserve.add(clause);
clausesToPreserve?.add(clause);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export type AliasImportDeclaration = ts.ImportSpecifier | ts.NamespaceImport | t
* that as public API: https://github.com/microsoft/TypeScript/issues/17516.
*/
interface TransformationContextWithResolver extends ts.TransformationContext {
getEmitResolver: () => EmitResolver;
getEmitResolver: () => EmitResolver | undefined;
}

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

// The emit resolver may have been patched already, in which case we return the set of referenced
// aliases that was created when the patch was first applied.
Expand Down
44 changes: 44 additions & 0 deletions packages/compiler-cli/src/ngtsc/imports/test/default_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,50 @@ runInEachFileSystem(() => {
expect(testContents).toContain(`var dep_1 = require("./dep");`);
expect(testContents).toContain(`var ref = dep_1.default;`);
});

it('should prevent a default import from being elided if used in an isolated transform', () => {
const {program} = makeProgram(
[
{name: _('/dep.ts'), contents: `export default class Foo {}`},
{
name: _('/test.ts'),
contents: `import Foo from './dep'; export function test(f: Foo) {}`,
},

// This control file is identical to the test file, but will not have its import marked
// for preservation. It exists to capture the behavior without the DefaultImportTracker's
// emit modifications.
{
name: _('/ctrl.ts'),
contents: `import Foo from './dep'; export function test(f: Foo) {}`,
},
],
{
module: ts.ModuleKind.ES2015,
},
);
const fooClause = getDeclaration(program, _('/test.ts'), 'Foo', ts.isImportClause);
const fooDecl = fooClause.parent as ts.ImportDeclaration;

const tracker = new DefaultImportTracker();
tracker.recordUsedImport(fooDecl);

const result = ts.transform(
[program.getSourceFile(_('/test.ts'))!, program.getSourceFile(_('/ctrl.ts'))!],
[tracker.importPreservingTransformer()],
);
expect(result.diagnostics?.length ?? 0).toBe(0);
expect(result.transformed.length).toBe(2);

const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});

const testOutput = printer.printFile(result.transformed[0]);
expect(testOutput).toContain(`import Foo from './dep';`);

// In an isolated transform, TypeScript also retains the default import.
const ctrlOutput = printer.printFile(result.transformed[1]);
expect(ctrlOutput).toContain(`import Foo from './dep';`);
});
});

function addReferenceTransformer(id: ts.Identifier): ts.TransformerFactory<ts.SourceFile> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ export function getDownlevelDecoratorsTransform(
// ensure that the alias declaration is not elided by TypeScript, and use its
// name identifier to reference it at runtime.
if (isAliasImportDeclaration(decl)) {
referencedParameterTypes.add(decl);
referencedParameterTypes?.add(decl);
// If the entity name resolves to an alias import declaration, we reference the
// entity based on the alias import name. This ensures that TypeScript properly
// resolves the link to the import. Cloning the original entity name identifier
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -872,38 +872,81 @@ describe('downlevel decorator transform', () => {
);
expect(written).toBe(numberOfTestFiles);
});
});

function createProgramWithTransform(files: string[]) {
const program = ts.createProgram(
files,
{
moduleResolution: ts.ModuleResolutionKind.Node10,
importHelpers: true,
lib: [],
module: ts.ModuleKind.ESNext,
target: ts.ScriptTarget.Latest,
declaration: false,
experimentalDecorators: true,
emitDecoratorMetadata: false,
},
host,
);
const typeChecker = program.getTypeChecker();
const reflectionHost = new TypeScriptReflectionHost(typeChecker);
const transformers: ts.CustomTransformers = {
before: [
getDownlevelDecoratorsTransform(
program.getTypeChecker(),
reflectionHost,
diagnostics,
/* isCore */ false,
isClosureEnabled,
),
],
};
return {program, transformers};
}
it('should work using an isolated transform operation', () => {
context.writeFile(
'foo_service.d.ts',
`
export declare class Foo {};
`,
);
context.writeFile(
'foo.ts',
`
import {Injectable} from '@angular/core';
import {Foo} from './foo_service';
@Injectable()
export class MyService {
constructor(foo: Foo) {}
}
`,
);

const {program, transformers} = createProgramWithTransform(['/foo.ts', '/foo_service.d.ts']);
const result = ts.transform(program.getSourceFile('/foo.ts')!, [
...(transformers.before ?? []),
...(transformers.after ?? []),
] as ts.TransformerFactory<ts.SourceFile>[]);
expect(result.diagnostics?.length ?? 0).toBe(0);
expect(result.transformed.length).toBe(1);

const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
const output = printer.printFile(result.transformed[0]);
expect(omitLeadingWhitespace(output)).toEqual(dedent`
import { Injectable } from '@angular/core';
import { Foo } from './foo_service';
@Injectable()
export class MyService {
constructor(foo: Foo) { }
static ctorParameters = () => [
{ type: Foo }
];
}
`);
});

function createProgramWithTransform(files: string[]) {
const program = ts.createProgram(
files,
{
moduleResolution: ts.ModuleResolutionKind.Node10,
importHelpers: true,
lib: [],
module: ts.ModuleKind.ESNext,
target: ts.ScriptTarget.Latest,
declaration: false,
experimentalDecorators: true,
emitDecoratorMetadata: false,
},
host,
);
const typeChecker = program.getTypeChecker();
const reflectionHost = new TypeScriptReflectionHost(typeChecker);
const transformers: ts.CustomTransformers = {
before: [
getDownlevelDecoratorsTransform(
program.getTypeChecker(),
reflectionHost,
diagnostics,
/* isCore */ false,
isClosureEnabled,
),
],
};
return {program, transformers};
}
});

/** Template string function that can be used to dedent a given string literal. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ export function createTsTransformForImportManager(
// doesn't drop these thinking they are unused.
if (reusedOriginalAliasDeclarations.size > 0) {
const referencedAliasDeclarations = loadIsReferencedAliasDeclarationPatch(ctx);
reusedOriginalAliasDeclarations.forEach((aliasDecl) =>
referencedAliasDeclarations.add(aliasDecl),
);
if (referencedAliasDeclarations !== null) {
reusedOriginalAliasDeclarations.forEach((aliasDecl) =>
referencedAliasDeclarations.add(aliasDecl),
);
}
}

// Update the set of affected files to include files that need extra statements to be inserted.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,32 @@ describe('import manager', () => {
`),
);
});

it('should work when using an isolated transform', () => {
const {testFile} = createTestProgram('import { input } from "@angular/core";');
const manager = new ImportManager();
const ref = manager.addImport({
exportModuleSpecifier: '@angular/core',
exportSymbolName: 'input',
requestedFile: testFile,
});

const extraStatements = [ts.factory.createExpressionStatement(ref)];
const transformer = manager.toTsTransform(new Map([[testFile.fileName, extraStatements]]));

const result = ts.transform(testFile, [transformer]);
expect(result.diagnostics?.length ?? 0).toBe(0);
expect(result.transformed.length).toBe(1);

const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed});
const output = printer.printFile(result.transformed[0]);
expect(output).toBe(
omitLeadingWhitespace(`
import { input } from "@angular/core";
input;
`),
);
});
});

function createTestProgram(text: string): {
Expand Down

0 comments on commit 16fc074

Please sign in to comment.