fix(migrations): avoid migrating the same class multiple times in standalone migration (#49245)

crisbeto · AndrewKushnir · commit 44d095a61cb3 · 2023-02-28T10:04:55.000-08:00
If a class is declared in multiple modules, the standalone migration may end up generating invalid code. While declaring a class in multiple modules is an error, it can happen with modules in tests. These changes avoid the issue by using a `Set` to track the classes being migrated. PR Close #49245
diff --git a/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts b/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts
@@ -29,7 +29,7 @@ interface BootstrapCallAnalysis {
   /** Component that the module is bootstrapping. */
   component: NamedClassDeclaration;
   /** Classes declared by the bootstrapped module. */
-  declarations: Reference<ts.ClassDeclaration>[];
+  declarations: ts.ClassDeclaration[];
 }
 
 export function toStandaloneBootstrap(
@@ -42,8 +42,8 @@ export function toStandaloneBootstrap(
   const referenceResolver =
       new ReferenceResolver(program, host, rootFileNames, basePath, referenceLookupExcludedFiles);
   const bootstrapCalls: BootstrapCallAnalysis[] = [];
-  const testObjects: ts.ObjectLiteralExpression[] = [];
-  const allDeclarations: Reference<ts.ClassDeclaration>[] = [];
+  const testObjects = new Set<ts.ObjectLiteralExpression>();
+  const allDeclarations = new Set<ts.ClassDeclaration>();
 
   for (const sourceFile of sourceFiles) {
     sourceFile.forEachChild(function walk(node) {
@@ -59,11 +59,11 @@ export function toStandaloneBootstrap(
       node.forEachChild(walk);
     });
 
-    testObjects.push(...findTestObjectsToMigrate(sourceFile, typeChecker));
+    findTestObjectsToMigrate(sourceFile, typeChecker).forEach(obj => testObjects.add(obj));
   }
 
   for (const call of bootstrapCalls) {
-    allDeclarations.push(...call.declarations);
+    call.declarations.forEach(decl => allDeclarations.add(decl));
     migrateBootstrapCall(call, tracker, referenceResolver, typeChecker, printer);
   }
 
diff --git a/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts b/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts
@@ -22,7 +22,7 @@ import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty,
  * are going to be added to the imports of a component.
  */
 export type ComponentImportsRemapper =
-    (imports: PotentialImport[], component: Reference<ts.ClassDeclaration>) => PotentialImport[];
+    (imports: PotentialImport[], component: ts.ClassDeclaration) => PotentialImport[];
 
 /**
  * Converts all declarations in the specified files to standalone.
@@ -39,9 +39,9 @@ export function toStandalone(
     componentImportRemapper?: ComponentImportsRemapper): ChangesByFile {
   const templateTypeChecker = program.compiler.getTemplateTypeChecker();
   const typeChecker = program.getTsProgram().getTypeChecker();
-  const modulesToMigrate: ts.ClassDeclaration[] = [];
-  const testObjectsToMigrate: ts.ObjectLiteralExpression[] = [];
-  const declarations: Reference<ts.ClassDeclaration>[] = [];
+  const modulesToMigrate = new Set<ts.ClassDeclaration>();
+  const testObjectsToMigrate = new Set<ts.ObjectLiteralExpression>();
+  const declarations = new Set<ts.ClassDeclaration>();
   const tracker = new ChangeTracker(printer, fileImportRemapper);
 
   for (const sourceFile of sourceFiles) {
@@ -54,12 +54,12 @@ export function toStandalone(
           allModuleDeclarations, module, templateTypeChecker, typeChecker);
 
       if (unbootstrappedDeclarations.length > 0) {
-        modulesToMigrate.push(module);
-        declarations.push(...unbootstrappedDeclarations);
+        modulesToMigrate.add(module);
+        unbootstrappedDeclarations.forEach(decl => declarations.add(decl));
       }
     }
 
-    testObjectsToMigrate.push(...testObjects);
+    testObjects.forEach(obj => testObjectsToMigrate.add(obj));
   }
 
   for (const declaration of declarations) {
@@ -78,24 +78,23 @@ export function toStandalone(
 
 /**
  * Converts a single declaration defined through an NgModule to standalone.
- * @param ref References to the declaration being converted.
+ * @param decl Declaration being converted.
  * @param tracker Tracker used to track the file changes.
  * @param allDeclarations All the declarations that are being converted as a part of this migration.
  * @param typeChecker
  * @param importRemapper
  */
 export function convertNgModuleDeclarationToStandalone(
-    ref: Reference<ts.ClassDeclaration>, allDeclarations: Reference<ts.ClassDeclaration>[],
-    tracker: ChangeTracker, typeChecker: TemplateTypeChecker,
-    importRemapper?: ComponentImportsRemapper): void {
-  const directiveMeta = typeChecker.getDirectiveMetadata(ref.node);
+    decl: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
+    typeChecker: TemplateTypeChecker, importRemapper?: ComponentImportsRemapper): void {
+  const directiveMeta = typeChecker.getDirectiveMetadata(decl);
 
   if (directiveMeta && directiveMeta.decorator && !directiveMeta.isStandalone) {
     let decorator = addStandaloneToDecorator(directiveMeta.decorator);
 
     if (directiveMeta.isComponent) {
-      const importsToAdd =
-          getComponentImportExpressions(ref, allDeclarations, tracker, typeChecker, importRemapper);
+      const importsToAdd = getComponentImportExpressions(
+          decl, allDeclarations, tracker, typeChecker, importRemapper);
 
       if (importsToAdd.length > 0) {
         decorator = addPropertyToAngularDecorator(
@@ -107,7 +106,7 @@ export function convertNgModuleDeclarationToStandalone(
 
     tracker.replaceNode(directiveMeta.decorator, decorator);
   } else {
-    const pipeMeta = typeChecker.getPipeMetadata(ref.node);
+    const pipeMeta = typeChecker.getPipeMetadata(decl);
 
     if (pipeMeta && pipeMeta.decorator && !pipeMeta.isStandalone) {
       tracker.replaceNode(pipeMeta.decorator, addStandaloneToDecorator(pipeMeta.decorator));
@@ -118,26 +117,25 @@ export function convertNgModuleDeclarationToStandalone(
 /**
  * Gets the expressions that should be added to a component's
  * `imports` array based on its template dependencies.
- * @param ref Reference to the component class.
+ * @param decl Component class declaration.
  * @param allDeclarations All the declarations that are being converted as a part of this migration.
  * @param tracker
  * @param typeChecker
  * @param importRemapper
  */
 function getComponentImportExpressions(
-    ref: Reference<ts.ClassDeclaration>, allDeclarations: Reference<ts.ClassDeclaration>[],
-    tracker: ChangeTracker, typeChecker: TemplateTypeChecker,
-    importRemapper?: ComponentImportsRemapper): ts.Expression[] {
-  const templateDependencies = findTemplateDependencies(ref, typeChecker);
-  const usedDependenciesInMigration = new Set(templateDependencies.filter(
-      dep => allDeclarations.find(current => current.node === dep.node)));
+    decl: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
+    typeChecker: TemplateTypeChecker, importRemapper?: ComponentImportsRemapper): ts.Expression[] {
+  const templateDependencies = findTemplateDependencies(decl, typeChecker);
+  const usedDependenciesInMigration =
+      new Set(templateDependencies.filter(dep => allDeclarations.has(dep.node)));
   const imports: ts.Expression[] = [];
   const seenImports = new Set<string>();
   const resolvedDependencies: PotentialImport[] = [];
 
   for (const dep of templateDependencies) {
     const importLocation = findImportLocation(
-        dep as Reference<NamedClassDeclaration>, ref,
+        dep as Reference<NamedClassDeclaration>, decl,
         usedDependenciesInMigration.has(dep) ? PotentialImportMode.ForceDirect :
                                                PotentialImportMode.Normal,
         typeChecker);
@@ -149,19 +147,19 @@ function getComponentImportExpressions(
   }
 
   const processedDependencies =
-      importRemapper ? importRemapper(resolvedDependencies, ref) : resolvedDependencies;
+      importRemapper ? importRemapper(resolvedDependencies, decl) : resolvedDependencies;
 
   for (const importLocation of processedDependencies) {
     if (importLocation.moduleSpecifier) {
       const identifier = tracker.addImport(
-          ref.node.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
+          decl.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
       imports.push(identifier);
     } else {
       const identifier = ts.factory.createIdentifier(importLocation.symbolName);
 
       if (importLocation.isForwardReference) {
         const forwardRefExpression =
-            tracker.addImport(ref.node.getSourceFile(), 'forwardRef', '@angular/core');
+            tracker.addImport(decl.getSourceFile(), 'forwardRef', '@angular/core');
         const arrowFunction = ts.factory.createArrowFunction(
             undefined, undefined, [], undefined, undefined, identifier);
         imports.push(
@@ -184,15 +182,13 @@ function getComponentImportExpressions(
  * @param templateTypeChecker
  */
 function migrateNgModuleClass(
-    node: ts.ClassDeclaration, allDeclarations: Reference<ts.ClassDeclaration>[],
-    tracker: ChangeTracker, typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
+    node: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
+    typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
   const decorator = templateTypeChecker.getNgModuleMetadata(node)?.decorator;
   const metadata = decorator ? extractMetadataLiteral(decorator) : null;
 
   if (metadata) {
-    moveDeclarationsToImports(
-        metadata, allDeclarations.map(decl => decl.node), typeChecker, templateTypeChecker,
-        tracker);
+    moveDeclarationsToImports(metadata, allDeclarations, typeChecker, templateTypeChecker, tracker);
   }
 }
 
@@ -205,7 +201,7 @@ function migrateNgModuleClass(
  * @param tracker
  */
 function moveDeclarationsToImports(
-    literal: ts.ObjectLiteralExpression, allDeclarations: ts.ClassDeclaration[],
+    literal: ts.ObjectLiteralExpression, allDeclarations: Set<ts.ClassDeclaration>,
     typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker,
     tracker: ChangeTracker): void {
   const declarationsProp = findLiteralProperty(literal, 'declarations');
@@ -347,9 +343,9 @@ function isNamedPropertyAssignment(node: ts.Node): node is ts.PropertyAssignment
  * @param typeChecker
  */
 function findImportLocation(
-    target: Reference<NamedClassDeclaration>, inComponent: Reference<ts.ClassDeclaration>,
+    target: Reference<NamedClassDeclaration>, inComponent: ts.ClassDeclaration,
     importMode: PotentialImportMode, typeChecker: TemplateTypeChecker): PotentialImport|null {
-  const importLocations = typeChecker.getPotentialImportsFor(target, inComponent.node, importMode);
+  const importLocations = typeChecker.getPotentialImportsFor(target, inComponent, importMode);
   let firstSameFileImport: PotentialImport|null = null;
   let firstModuleImport: PotentialImport|null = null;
 
@@ -439,15 +435,14 @@ export function findTestObjectsToMigrate(sourceFile: ts.SourceFile, typeChecker:
 
 /**
  * Finds the classes corresponding to dependencies used in a component's template.
- * @param ref Component in whose template we're looking for dependencies.
+ * @param decl Component in whose template we're looking for dependencies.
  * @param typeChecker
  */
-function findTemplateDependencies(
-    ref: Reference<ts.ClassDeclaration>,
-    typeChecker: TemplateTypeChecker): Reference<NamedClassDeclaration>[] {
+function findTemplateDependencies(decl: ts.ClassDeclaration, typeChecker: TemplateTypeChecker):
+    Reference<NamedClassDeclaration>[] {
   const results: Reference<NamedClassDeclaration>[] = [];
-  const usedDirectives = typeChecker.getUsedDirectives(ref.node);
-  const usedPipes = typeChecker.getUsedPipes(ref.node);
+  const usedDirectives = typeChecker.getUsedDirectives(decl);
+  const usedPipes = typeChecker.getUsedPipes(decl);
 
   if (usedDirectives !== null) {
     for (const dir of usedDirectives) {
@@ -458,7 +453,7 @@ function findTemplateDependencies(
   }
 
   if (usedPipes !== null) {
-    const potentialPipes = typeChecker.getPotentialPipes(ref.node);
+    const potentialPipes = typeChecker.getPotentialPipes(decl);
 
     for (const pipe of potentialPipes) {
       if (ts.isClassDeclaration(pipe.ref.node) &&
@@ -480,7 +475,7 @@ function findTemplateDependencies(
  * @param typeChecker
  */
 function filterNonBootstrappedDeclarations(
-    declarations: Reference<ts.ClassDeclaration>[], ngModule: ts.ClassDeclaration,
+    declarations: ts.ClassDeclaration[], ngModule: ts.ClassDeclaration,
     templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
   const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
   const metaLiteral =
@@ -513,7 +508,7 @@ function filterNonBootstrappedDeclarations(
     }
   }
 
-  return declarations.filter(ref => !bootstrappedClasses.has(ref.node));
+  return declarations.filter(ref => !bootstrappedClasses.has(ref));
 }
 
 /**
@@ -523,10 +518,10 @@ function filterNonBootstrappedDeclarations(
  */
 export function extractDeclarationsFromModule(
     ngModule: ts.ClassDeclaration,
-    templateTypeChecker: TemplateTypeChecker): Reference<ts.ClassDeclaration>[] {
+    templateTypeChecker: TemplateTypeChecker): ts.ClassDeclaration[] {
   const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
-  return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node)) as
-          Reference<ts.ClassDeclaration>[] :
+  return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node))
+                        .map(decl => decl.node) as ts.ClassDeclaration[] :
                     [];
 }
 
@@ -539,12 +534,11 @@ export function extractDeclarationsFromModule(
  * @param typeChecker
  */
 export function migrateTestDeclarations(
-    testObjects: ts.ObjectLiteralExpression[],
-    declarationsOutsideOfTestFiles: Reference<ts.ClassDeclaration>[], tracker: ChangeTracker,
+    testObjects: Set<ts.ObjectLiteralExpression>,
+    declarationsOutsideOfTestFiles: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
     templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
   const {decorators, componentImports} = analyzeTestingModules(testObjects, typeChecker);
-  const allDeclarations: ts.ClassDeclaration[] =
-      declarationsOutsideOfTestFiles.map(ref => ref.node);
+  const allDeclarations = new Set(declarationsOutsideOfTestFiles);
 
   for (const decorator of decorators) {
     const closestClass = closestNode(decorator.node, ts.isClassDeclaration);
@@ -553,14 +547,14 @@ export function migrateTestDeclarations(
       tracker.replaceNode(decorator.node, addStandaloneToDecorator(decorator.node));
 
       if (closestClass) {
-        allDeclarations.push(closestClass);
+        allDeclarations.add(closestClass);
       }
     } else if (decorator.name === 'Component') {
       const newDecorator = addStandaloneToDecorator(decorator.node);
       const importsToAdd = componentImports.get(decorator.node);
 
       if (closestClass) {
-        allDeclarations.push(closestClass);
+        allDeclarations.add(closestClass);
       }
 
       if (importsToAdd && importsToAdd.size > 0) {
@@ -588,7 +582,7 @@ export function migrateTestDeclarations(
  * @param testObjects Object literals that should be analyzed.
  */
 function analyzeTestingModules(
-    testObjects: ts.ObjectLiteralExpression[], typeChecker: ts.TypeChecker) {
+    testObjects: Set<ts.ObjectLiteralExpression>, typeChecker: ts.TypeChecker) {
   const seenDeclarations = new Set<ts.Declaration>();
   const decorators: NgDecorator[] = [];
   const componentImports = new Map<ts.Decorator, Set<ts.Expression>>();
@@ -685,9 +679,9 @@ function extractMetadataLiteral(decorator: ts.Decorator): ts.ObjectLiteralExpres
  * @param templateTypeChecker
  */
 function isStandaloneDeclaration(
-    node: ts.ClassDeclaration, declarationsInMigration: ts.ClassDeclaration[],
+    node: ts.ClassDeclaration, declarationsInMigration: Set<ts.ClassDeclaration>,
     templateTypeChecker: TemplateTypeChecker): boolean {
-  if (declarationsInMigration.includes(node)) {
+  if (declarationsInMigration.has(node)) {
     return true;
   }
 
