Skip to content

Commit 1a811c9

Browse files
devversionatscott
devversion
authored and
atscott
committed
refactor: ensure tsurge migrations have clear ownership of files (#61421) (#61612)
Currently there can be cases, exlusively in 3P, where multiple tsconfig projects have overlap of source files. This is the default setup of new CLI applications as well. When this is the case, Tsurge will treat each tsconfig as an isolated compilation unit (given the concepts and mental model to support scalable batching). This is wrong though, and the same `.ts` source file can appear in two migration invocations; resulting in duplicate replacements or analysis (depending on the migration). We've worked around this problem in the past by deduplicating replacements, or migrating to an ID-based approach with natural deduplication. This worked, but it's just working around the root cause. This commit attempts to fix the root cause by adjusting Tsurge to ensure that no source file ever appears in two compilation units. This is naively achieved by not adding a source file to a migration unit, if it was part of a previous one. This is expected to be fine given the nature of Tsurge migrations that are built to operate on isolated pieces anyway— so it shouldn't be problematic if e.g. `app.component.ts` ends up being part of the test tsconfig compilation unit (we avoid this order though by visiting build targets first). PR Close #61421 PR Close #61612
1 parent 0a51f36 commit 1a811c9

19 files changed

Lines changed: 156 additions & 89 deletions

File tree

‎packages/core/schematics/migrations/signal-migration/src/cli.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import path from 'path';
1111
import assert from 'assert';
1212
import {SignalInputMigration} from './migration';
1313
import {writeMigrationReplacements} from './write_replacements';
14+
import {NodeJSFileSystem} from '../../../../../compiler-cli';
1415

1516
main(
1617
path.resolve(process.argv[2]),
@@ -34,8 +35,7 @@ export async function main(
3435
insertTodosForSkippedFields,
3536
upgradeAnalysisPhaseToAvoidBatch: true,
3637
});
37-
const baseInfo = migration.createProgram(absoluteTsconfigPath);
38-
const info = migration.prepareProgram(baseInfo);
38+
const info = migration.createProgram(absoluteTsconfigPath, new NodeJSFileSystem());
3939

4040
await migration.analyze(info);
4141

‎packages/core/schematics/migrations/signal-migration/src/migration.ts‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@ import {Replacement} from '../../../utils/tsurge/replacement';
2323
import {populateKnownInputsFromGlobalData} from './batch/populate_global_data';
2424
import {executeMigrationPhase} from './phase_migrate';
2525
import {filterIncompatibilitiesForBestEffortMode} from './best_effort_mode';
26-
import assert from 'assert';
2726
import {
2827
ClassIncompatibilityReason,
2928
FieldIncompatibilityReason,
3029
} from './passes/problematic_patterns/incompatibility';
3130
import {MigrationConfig} from './migration_config';
3231
import {ClassFieldUniqueKey} from './passes/reference_resolution/known_fields';
33-
import {createBaseProgramInfo} from '../../../utils/tsurge/helpers/create_program';
3432

3533
/**
3634
* Tsurge migration for migrating Angular `@Input()` declarations to
@@ -50,9 +48,8 @@ export class SignalInputMigration extends TsurgeComplexMigration<
5048
super();
5149
}
5250

53-
// Override the default program creation, to add extra flags.
54-
override createProgram(tsconfigAbsPath: string, fs?: FileSystem): BaseProgramInfo {
55-
return createBaseProgramInfo(tsconfigAbsPath, fs, {
51+
override createProgram(tsconfigAbsPath: string, fs: FileSystem): ProgramInfo {
52+
return super.createProgram(tsconfigAbsPath, fs, {
5653
_compilePoisonedComponents: true,
5754
// We want to migrate non-exported classes too.
5855
compileNonExportedClasses: true,
@@ -63,8 +60,11 @@ export class SignalInputMigration extends TsurgeComplexMigration<
6360
});
6461
}
6562

66-
override prepareProgram(baseInfo: BaseProgramInfo): ProgramInfo {
67-
const info = super.prepareProgram(baseInfo);
63+
/**
64+
* Prepares the program for this migration with additional custom
65+
* fields to allow for batch-mode testing.
66+
*/
67+
private _prepareProgram(info: ProgramInfo): ProgramInfo {
6868
// Optional filter for testing. Allows for simulation of parallel execution
6969
// even if some tsconfig's have overlap due to sharing of TS sources.
7070
// (this is commonly not the case in g3 where deps are `.d.ts` files).
@@ -74,7 +74,7 @@ export class SignalInputMigration extends TsurgeComplexMigration<
7474
// Optional replacement filter. Allows parallel execution in case
7575
// some tsconfig's have overlap due to sharing of TS sources.
7676
// (this is commonly not the case in g3 where deps are `.d.ts` files).
77-
!limitToRootNamesOnly || info.programAbsoluteRootFileNames!.includes(f.fileName),
77+
!limitToRootNamesOnly || info.__programAbsoluteRootFileNames!.includes(f.fileName),
7878
);
7979

8080
return {
@@ -87,12 +87,14 @@ export class SignalInputMigration extends TsurgeComplexMigration<
8787
prepareAnalysisDeps(info: ProgramInfo): AnalysisProgramInfo {
8888
const analysisInfo = {
8989
...info,
90-
...prepareAnalysisInfo(info.program, info.ngCompiler, info.programAbsoluteRootFileNames),
90+
...prepareAnalysisInfo(info.program, info.ngCompiler, info.__programAbsoluteRootFileNames),
9191
};
9292
return analysisInfo;
9393
}
9494

9595
override async analyze(info: ProgramInfo) {
96+
info = this._prepareProgram(info);
97+
9698
const analysisDeps = this.prepareAnalysisDeps(info);
9799
const knownInputs = new KnownInputs(info, this.config);
98100
const result = new MigrationResult();
@@ -163,6 +165,8 @@ export class SignalInputMigration extends TsurgeComplexMigration<
163165
analysisDeps: AnalysisProgramInfo;
164166
},
165167
) {
168+
info = this._prepareProgram(info);
169+
166170
const knownInputs = nonBatchData?.knownInputs ?? new KnownInputs(info, this.config);
167171
const result = nonBatchData?.result ?? new MigrationResult();
168172
const host = nonBatchData?.host ?? createMigrationHost(info, this.config);

‎packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import ts from 'typescript';
1010
import {
11-
BaseProgramInfo,
1211
confirmAsSerializable,
1312
MigrationStats,
1413
ProgramInfo,
@@ -20,7 +19,7 @@ import {
2019
TsurgeFunnelMigration,
2120
} from '../../utils/tsurge';
2221
import {ErrorCode, FileSystem, ngErrorCode} from '@angular/compiler-cli';
23-
import {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
22+
import {DiagnosticCategoryLabel, NgCompilerOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
2423
import {ImportManager} from '@angular/compiler-cli/private/migrations';
2524
import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import_manager';
2625

@@ -71,7 +70,7 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
7170
> {
7271
private printer = ts.createPrinter();
7372

74-
override createProgram(tsconfigAbsPath: string, fs?: FileSystem): BaseProgramInfo {
73+
override createProgram(tsconfigAbsPath: string, fs: FileSystem): ProgramInfo {
7574
return super.createProgram(tsconfigAbsPath, fs, {
7675
extendedDiagnostics: {
7776
checks: {

‎packages/core/schematics/utils/tsurge/base_migration.ts‎

Lines changed: 7 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import {absoluteFrom, FileSystem} from '@angular/compiler-cli/src/ngtsc/file_sys
1010
import {NgCompilerOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
1111
import {getRootDirs} from '@angular/compiler-cli/src/ngtsc/util/src/typescript';
1212
import {isShim} from '@angular/compiler-cli/src/ngtsc/shims';
13-
import {BaseProgramInfo, ProgramInfo} from './program_info';
13+
import {ProgramInfo} from './program_info';
1414
import {Serializable} from './helpers/serializable';
15-
import {createBaseProgramInfo} from './helpers/create_program';
15+
import {createBaseProgramInfo, getProgramInfoFromBaseInfo} from './helpers/create_program';
1616

1717
/**
1818
* Type describing statistics that could be tracked
@@ -34,52 +34,18 @@ export interface MigrationStats {
3434
*/
3535
export abstract class TsurgeBaseMigration<UnitAnalysisMetadata, CombinedGlobalMetadata> {
3636
/**
37-
* Advanced Tsurge users can override this method, but most of the time,
38-
* overriding {@link prepareProgram} is more desirable.
37+
* Creates the TypeScript program for a given compilation unit.
3938
*
4039
* By default:
4140
* - In 3P: Ngtsc programs are being created.
4241
* - In 1P: Ngtsc or TS programs are created based on the Blaze target.
4342
*/
4443
createProgram(
4544
tsconfigAbsPath: string,
46-
fs?: FileSystem,
47-
optionOverrides?: NgCompilerOptions,
48-
): BaseProgramInfo {
49-
return createBaseProgramInfo(tsconfigAbsPath, fs, optionOverrides);
50-
}
51-
52-
// Optional function to prepare the base `ProgramInfo` even further,
53-
// for the analyze and migrate phases. E.g. determining source files.
54-
prepareProgram(info: BaseProgramInfo): ProgramInfo {
55-
const fullProgramSourceFiles = [...info.program.getSourceFiles()];
56-
const sourceFiles = fullProgramSourceFiles.filter(
57-
(f) =>
58-
!f.isDeclarationFile &&
59-
// Note `isShim` will work for the initial program, but for TCB programs, the shims are no longer annotated.
60-
!isShim(f) &&
61-
!f.fileName.endsWith('.ngtypecheck.ts'),
62-
);
63-
64-
// Sort it by length in reverse order (longest first). This speeds up lookups,
65-
// since there's no need to keep going through the array once a match is found.
66-
const sortedRootDirs = getRootDirs(info.host, info.userOptions).sort(
67-
(a, b) => b.length - a.length,
68-
);
69-
70-
// TODO: Consider also following TS's logic here, finding the common source root.
71-
// See: Program#getCommonSourceDirectory.
72-
const primaryRoot = absoluteFrom(
73-
info.userOptions.rootDir ?? sortedRootDirs.at(-1) ?? info.program.getCurrentDirectory(),
74-
);
75-
76-
return {
77-
...info,
78-
sourceFiles,
79-
fullProgramSourceFiles,
80-
sortedRootDirs,
81-
projectRoot: primaryRoot,
82-
};
45+
fs: FileSystem,
46+
optionsOverride?: NgCompilerOptions,
47+
): ProgramInfo {
48+
return getProgramInfoFromBaseInfo(createBaseProgramInfo(tsconfigAbsPath, fs, optionsOverride));
8349
}
8450

8551
/** Analyzes the given TypeScript project and returns serializable compilation unit data. */

‎packages/core/schematics/utils/tsurge/executors/analyze_exec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {NodeJSFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
910
import {TsurgeMigration} from '../migration';
1011
import {Serializable} from '../helpers/serializable';
1112

1213
/**
13-
* Executes the analyze phase of the given migration against
14+
* 1P Logic: Executes the analyze phase of the given migration against
1415
* the specified TypeScript project.
1516
*
1617
* @returns the serializable migration unit data.
@@ -19,8 +20,7 @@ export async function executeAnalyzePhase<UnitData, GlobalData>(
1920
migration: TsurgeMigration<UnitData, GlobalData>,
2021
tsconfigAbsolutePath: string,
2122
): Promise<Serializable<UnitData>> {
22-
const baseInfo = migration.createProgram(tsconfigAbsolutePath);
23-
const info = migration.prepareProgram(baseInfo);
23+
const info = migration.createProgram(tsconfigAbsolutePath, new NodeJSFileSystem());
2424

2525
return await migration.analyze(info);
2626
}

‎packages/core/schematics/utils/tsurge/executors/combine_exec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {Serializable} from '../helpers/serializable';
1010
import {TsurgeMigration} from '../migration';
1111

1212
/**
13-
* Executes the combine phase for the given migration against
13+
* 1P Logic: Executes the combine phase for the given migration against
1414
* two unit analyses.
1515
*
1616
* @returns the serializable combined unit data.

‎packages/core/schematics/utils/tsurge/executors/global_meta_exec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {Serializable} from '../helpers/serializable';
1010
import {TsurgeMigration} from '../migration';
1111

1212
/**
13-
* Executes the `globalMeta` stage for the given migration
13+
* 1P Logic: Executes the `globalMeta` stage for the given migration
1414
* to convert the combined unit data into global meta.
1515
*
1616
* @returns the serializable global meta.

‎packages/core/schematics/utils/tsurge/executors/migrate_exec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,12 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
9+
import {AbsoluteFsPath, NodeJSFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
1010
import {TsurgeMigration} from '../migration';
1111
import {Replacement} from '../replacement';
1212

1313
/**
14-
* Executes the migrate phase of the given migration against
14+
* 1P Logic: Executes the migrate phase of the given migration against
1515
* the specified TypeScript project.
1616
*
1717
* This requires the global migration data, computed by the
@@ -25,8 +25,8 @@ export async function executeMigratePhase<UnitData, GlobalData>(
2525
globalMetadata: GlobalData,
2626
tsconfigAbsolutePath: string,
2727
): Promise<{replacements: Replacement[]; projectRoot: AbsoluteFsPath}> {
28-
const baseInfo = migration.createProgram(tsconfigAbsolutePath);
29-
const info = migration.prepareProgram(baseInfo);
28+
// In 1P, we never need to use a virtual file system.
29+
const info = migration.createProgram(tsconfigAbsolutePath, new NodeJSFileSystem());
3030

3131
return {
3232
...(await migration.migrate(globalMetadata, info)),

‎packages/core/schematics/utils/tsurge/helpers/angular_devkit/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,6 @@ ts_project(
1313
deps = [
1414
"//:node_modules/@angular-devkit/core",
1515
"//:node_modules/@angular-devkit/schematics",
16+
"//:node_modules/typescript",
1617
],
1718
)

‎packages/core/schematics/utils/tsurge/helpers/angular_devkit/run_in_devkit.ts‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ import {DevkitMigrationFilesystem} from './devkit_filesystem';
1212
import {groupReplacementsByFile} from '../group_replacements';
1313
import {synchronouslyCombineUnitData} from '../combine_units';
1414
import {TsurgeFunnelMigration, TsurgeMigration} from '../../migration';
15-
import {MigrationStats} from '../../base_migration';
1615
import {Replacement, TextUpdate} from '../../replacement';
1716
import {ProjectRootRelativePath} from '../../project_paths';
1817
import {ProgramInfo} from '../../program_info';
1918
import {getProjectTsConfigPaths} from '../../../../utils/project_tsconfig_paths';
19+
import ts from 'typescript';
2020

2121
export enum MigrationStage {
2222
/** The migration is analyzing an entrypoint */
@@ -72,13 +72,17 @@ export async function runMigrationInDevkit(config: TsurgeDevkitMigration): Promi
7272
const unitResults: unknown[] = [];
7373

7474
const isFunnelMigration = migration instanceof TsurgeFunnelMigration;
75+
const compilationUnitAssignments = new Map<string, string>();
76+
7577
for (const tsconfigPath of tsconfigPaths) {
7678
config.beforeProgramCreation?.(tsconfigPath, MigrationStage.Analysis);
77-
const baseInfo = migration.createProgram(tsconfigPath, fs);
78-
const info = migration.prepareProgram(baseInfo);
79-
config.afterProgramCreation?.(info, fs, MigrationStage.Analysis);
79+
const info = migration.createProgram(tsconfigPath, fs);
8080

81+
modifyProgramInfoToEnsureNonOverlappingFiles(tsconfigPath, info, compilationUnitAssignments);
82+
83+
config.afterProgramCreation?.(info, fs, MigrationStage.Analysis);
8184
config.beforeUnitAnalysis?.(tsconfigPath);
85+
8286
unitResults.push(await migration.analyze(info));
8387
}
8488

@@ -100,8 +104,9 @@ export async function runMigrationInDevkit(config: TsurgeDevkitMigration): Promi
100104

101105
for (const tsconfigPath of tsconfigPaths) {
102106
config.beforeProgramCreation?.(tsconfigPath, MigrationStage.Migrate);
103-
const baseInfo = migration.createProgram(tsconfigPath, fs);
104-
const info = migration.prepareProgram(baseInfo);
107+
const info = migration.createProgram(tsconfigPath, fs);
108+
modifyProgramInfoToEnsureNonOverlappingFiles(tsconfigPath, info, compilationUnitAssignments);
109+
105110
config.afterProgramCreation?.(info, fs, MigrationStage.Migrate);
106111

107112
const result = await migration.migrate(globalMeta, info);
@@ -130,3 +135,37 @@ export async function runMigrationInDevkit(config: TsurgeDevkitMigration): Promi
130135

131136
config.whenDone?.(await migration.stats(globalMeta));
132137
}
138+
139+
/**
140+
* Special logic for devkit migrations. In the Angular CLI, or in 3P precisely,
141+
* projects can have tsconfigs with overlapping source files. i.e. two tsconfigs
142+
* like e.g. build or test include the same `ts.SourceFile` (`.ts`). Migrations
143+
* should never have 2+ compilation units with overlapping source files as this
144+
* can result in duplicated replacements or analysis— hence we only ever assign a
145+
* source file to a compilation unit *once*.
146+
*
147+
* Note that this is fine as we expect Tsurge migrations to work together as
148+
* isolated compilation units— so it shouldn't matter if worst case a `.ts`
149+
* file ends up in the e.g. test program.
150+
*/
151+
function modifyProgramInfoToEnsureNonOverlappingFiles(
152+
tsconfigPath: string,
153+
info: ProgramInfo,
154+
compilationUnitAssignments: Map<string, string>,
155+
) {
156+
const sourceFiles: ts.SourceFile[] = [];
157+
158+
for (const sf of info.sourceFiles) {
159+
const assignment = compilationUnitAssignments.get(sf.fileName);
160+
161+
// File is already assigned to a different compilation unit.
162+
if (assignment !== undefined && assignment !== tsconfigPath) {
163+
continue;
164+
}
165+
166+
compilationUnitAssignments.set(sf.fileName, tsconfigPath);
167+
sourceFiles.push(sf);
168+
}
169+
170+
info.sourceFiles = sourceFiles;
171+
}

0 commit comments

Comments
 (0)