Sitelet https://github.com/angular/angular/pull/61421/files
Skip to content

refactor: ensure tsurge migrations have clear ownership of files #61421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
devversion wants to merge 3 commits into from
Closed

refactor: ensure tsurge migrations have clear ownership of files #61421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6aa4bca
refactor: ensure tsurge migrations have clear ownership of files
devversion May 15, 2025
afe3d48
refactor: clean-up deduplication workaround from migrations
devversion May 16, 2025
e540db5
Revert "fix(migrations): avoid applying the same replacements twice w…
devversion May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 3 additions & 23 deletions ...ges/core/schematics/migrations/self-closing-tags-migration/self-closing-tags-migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,9 @@ export class SelfClosingTagsMigration extends TsurgeFunnelMigration<
unitA: SelfClosingTagsCompilationUnitData,
unitB: SelfClosingTagsCompilationUnitData,
): Promise<Serializable<SelfClosingTagsCompilationUnitData>> {
const uniqueReplacements = removeDuplicateReplacements([
...unitA.tagReplacements,
...unitB.tagReplacements,
]);

return confirmAsSerializable({tagReplacements: uniqueReplacements});
return confirmAsSerializable({
tagReplacements: [...unitA.tagReplacements, ...unitB.tagReplacements],
});
}

override async globalMeta(
Expand Down Expand Up @@ -160,20 +157,3 @@ function prepareTextReplacement(
}),
);
}

function removeDuplicateReplacements(
replacements: SelfClosingTagsMigrationData[],
): SelfClosingTagsMigrationData[] {
const uniqueFiles = new Set<string>();
const result: SelfClosingTagsMigrationData[] = [];

for (const replacement of replacements) {
const fileId = replacement.file.id;
if (!uniqueFiles.has(fileId)) {
uniqueFiles.add(fileId);
result.push(replacement);
}
}

return result;
}
4 changes: 2 additions & 2 deletions packages/core/schematics/migrations/signal-migration/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import path from 'path';
import assert from 'assert';
import {SignalInputMigration} from './migration';
import {writeMigrationReplacements} from './write_replacements';
import {NodeJSFileSystem} from '../../../../../compiler-cli';

main(
path.resolve(process.argv[2]),
Expand All @@ -34,8 +35,7 @@ export async function main(
insertTodosForSkippedFields,
upgradeAnalysisPhaseToAvoidBatch: true,
});
const baseInfo = migration.createProgram(absoluteTsconfigPath);
const info = migration.prepareProgram(baseInfo);
const info = migration.createProgram(absoluteTsconfigPath, new NodeJSFileSystem());

await migration.analyze(info);

Expand Down
22 changes: 13 additions & 9 deletions packages/core/schematics/migrations/signal-migration/src/migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,12 @@ import {Replacement} from '../../../utils/tsurge/replacement';
import {populateKnownInputsFromGlobalData} from './batch/populate_global_data';
import {executeMigrationPhase} from './phase_migrate';
import {filterIncompatibilitiesForBestEffortMode} from './best_effort_mode';
import assert from 'assert';
import {
ClassIncompatibilityReason,
FieldIncompatibilityReason,
} from './passes/problematic_patterns/incompatibility';
import {MigrationConfig} from './migration_config';
import {ClassFieldUniqueKey} from './passes/reference_resolution/known_fields';
import {createBaseProgramInfo} from '../../../utils/tsurge/helpers/create_program';

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

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

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

return {
Expand All @@ -87,12 +87,14 @@ export class SignalInputMigration extends TsurgeComplexMigration<
prepareAnalysisDeps(info: ProgramInfo): AnalysisProgramInfo {
const analysisInfo = {
...info,
...prepareAnalysisInfo(info.program, info.ngCompiler, info.programAbsoluteRootFileNames),
...prepareAnalysisInfo(info.program, info.ngCompiler, info.__programAbsoluteRootFileNames),
};
return analysisInfo;
}

override async analyze(info: ProgramInfo) {
info = this._prepareProgram(info);

const analysisDeps = this.prepareAnalysisDeps(info);
const knownInputs = new KnownInputs(info, this.config);
const result = new MigrationResult();
Expand Down Expand Up @@ -163,6 +165,8 @@ export class SignalInputMigration extends TsurgeComplexMigration<
analysisDeps: AnalysisProgramInfo;
},
) {
info = this._prepareProgram(info);

const knownInputs = nonBatchData?.knownInputs ?? new KnownInputs(info, this.config);
const result = nonBatchData?.result ?? new MigrationResult();
const host = nonBatchData?.host ?? createMigrationHost(info, this.config);
Expand Down
90 changes: 27 additions & 63 deletions packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,16 @@

import ts from 'typescript';
import {
BaseProgramInfo,
confirmAsSerializable,
ProgramInfo,
projectFile,
ProjectFileID,
Replacement,
Serializable,
TextUpdate,
TsurgeFunnelMigration,
} from '../../utils/tsurge';
import {ErrorCode, FileSystem, ngErrorCode} from '@angular/compiler-cli';
import {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
import {DiagnosticCategoryLabel, NgCompilerOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
import {ImportManager} from '@angular/compiler-cli/private/migrations';
import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import_manager';

Expand All @@ -28,8 +26,8 @@ export interface CompilationUnitData {
/** Text changes that should be performed. */
replacements: Replacement[];

/** Identifiers that have been removed from each file. */
removedIdentifiers: NodeID[];
/** Total number of imports that were removed. */
removedImports: number;

/** Total number of files that were changed. */
changedFiles: number;
Expand All @@ -44,7 +42,7 @@ interface RemovalLocations {
partialRemovals: Map<ts.ArrayLiteralExpression, Set<ts.Expression>>;

/** Text of all identifiers that have been removed. */
allRemovedIdentifiers: Set<ts.Identifier>;
allRemovedIdentifiers: Set<string>;
}

/** Tracks how identifiers are used across a single file. */
Expand All @@ -60,17 +58,14 @@ interface UsageAnalysis {
identifierCounts: Map<string, number>;
}

/** ID of a node based on its location. */
type NodeID = string & {__nodeID: true};

/** Migration that cleans up unused imports from a project. */
export class UnusedImportsMigration extends TsurgeFunnelMigration<
CompilationUnitData,
CompilationUnitData
> {
private printer = ts.createPrinter();

override createProgram(tsconfigAbsPath: string, fs?: FileSystem): BaseProgramInfo {
override createProgram(tsconfigAbsPath: string, fs: FileSystem): ProgramInfo {
return super.createProgram(tsconfigAbsPath, fs, {
extendedDiagnostics: {
checks: {
Expand All @@ -84,7 +79,7 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
override async analyze(info: ProgramInfo): Promise<Serializable<CompilationUnitData>> {
const nodePositions = new Map<ts.SourceFile, Set<string>>();
const replacements: Replacement[] = [];
const removedIdentifiers: NodeID[] = [];
let removedImports = 0;
let changedFiles = 0;

info.ngCompiler?.getDiagnostics().forEach((diag) => {
Expand All @@ -94,10 +89,15 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
diag.length !== undefined &&
diag.code === ngErrorCode(ErrorCode.UNUSED_STANDALONE_IMPORTS)
) {
// Skip files that aren't owned by this compilation unit.
if (!info.sourceFiles.includes(diag.file)) {
return;
}

if (!nodePositions.has(diag.file)) {
nodePositions.set(diag.file, new Set());
}
nodePositions.get(diag.file)!.add(this.getNodeID(diag.start, diag.length));
nodePositions.get(diag.file)!.add(this.getNodeKey(diag.start, diag.length));
}
});

Expand All @@ -106,15 +106,14 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
const usageAnalysis = this.analyzeUsages(sourceFile, resolvedLocations);

if (resolvedLocations.allRemovedIdentifiers.size > 0) {
removedImports += resolvedLocations.allRemovedIdentifiers.size;
changedFiles++;
resolvedLocations.allRemovedIdentifiers.forEach((identifier) => {
removedIdentifiers.push(this.getNodeID(identifier.getStart(), identifier.getWidth()));
});
}

this.generateReplacements(sourceFile, resolvedLocations, usageAnalysis, info, replacements);
});

return confirmAsSerializable({replacements, removedIdentifiers, changedFiles});
return confirmAsSerializable({replacements, removedImports, changedFiles});
}

override async migrate(globalData: CompilationUnitData) {
Expand All @@ -125,34 +124,10 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
unitA: CompilationUnitData,
unitB: CompilationUnitData,
): Promise<Serializable<CompilationUnitData>> {
const combinedReplacements: Replacement[] = [];
const combinedRemovedIdentifiers: NodeID[] = [];
const seenReplacements = new Set<string>();
const seenIdentifiers = new Set<NodeID>();
const changedFileIds = new Set<ProjectFileID>();

[unitA, unitB].forEach((unit) => {
for (const replacement of unit.replacements) {
const key = this.getReplacementID(replacement);
changedFileIds.add(replacement.projectFile.id);
if (!seenReplacements.has(key)) {
seenReplacements.add(key);
combinedReplacements.push(replacement);
}
}

for (const identifier of unit.removedIdentifiers) {
if (!seenIdentifiers.has(identifier)) {
seenIdentifiers.add(identifier);
combinedRemovedIdentifiers.push(identifier);
}
}
});

return confirmAsSerializable({
replacements: combinedReplacements,
removedIdentifiers: combinedRemovedIdentifiers,
changedFiles: changedFileIds.size,
replacements: [...unitA.replacements, ...unitB.replacements],
removedImports: unitA.removedImports + unitB.removedImports,
changedFiles: unitA.changedFiles + unitB.changedFiles,
});
}

Expand All @@ -164,20 +139,14 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<

override async stats(globalMetadata: CompilationUnitData) {
return confirmAsSerializable({
removedImports: globalMetadata.removedIdentifiers.length,
removedImports: globalMetadata.removedImports,
changedFiles: globalMetadata.changedFiles,
});
}

/** Gets an ID that can be used to look up a node based on its location. */
private getNodeID(start: number, length: number): NodeID {
return `${start}/${length}` as NodeID;
}

/** Gets a unique ID for a replacement. */
private getReplacementID(replacement: Replacement): string {
const {position, end, toInsert} = replacement.update.data;
return replacement.projectFile.id + '/' + position + '/' + end + '/' + toInsert;
/** Gets a key that can be used to look up a node based on its location. */
private getNodeKey(start: number, length: number): string {
return `${start}/${length}`;
}

/**
Expand Down Expand Up @@ -208,7 +177,7 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
return;
}

if (locations.has(this.getNodeID(node.getStart(), node.getWidth()))) {
if (locations.has(this.getNodeKey(node.getStart(), node.getWidth()))) {
// When the entire array needs to be cleared, the diagnostic is
// reported on the property assignment, rather than an array element.
if (
Expand All @@ -219,15 +188,15 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
result.fullRemovals.add(parent.initializer);
parent.initializer.elements.forEach((element) => {
if (ts.isIdentifier(element)) {
result.allRemovedIdentifiers.add(element);
result.allRemovedIdentifiers.add(element.text);
}
});
} else if (ts.isArrayLiteralExpression(parent)) {
if (!result.partialRemovals.has(parent)) {
result.partialRemovals.set(parent, new Set());
}
result.partialRemovals.get(parent)!.add(node);
result.allRemovedIdentifiers.add(node);
result.allRemovedIdentifiers.add(node.text);
}
}
};
Expand Down Expand Up @@ -358,13 +327,8 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
names.forEach((symbolName, localName) => {
// Note that in the `identifierCounts` lookup both zero and undefined
// are valid and mean that the identifiers isn't being used anymore.
if (!identifierCounts.get(localName)) {
for (const identifier of allRemovedIdentifiers) {
if (identifier.text === localName) {
importManager.removeImport(sourceFile, symbolName, moduleName);
break;
}
}
if (allRemovedIdentifiers.has(localName) && !identifierCounts.get(localName)) {
importManager.removeImport(sourceFile, symbolName, moduleName);
}
});
});
Expand Down
Loading