refactor(migrations): check inheritance in signal queries migration (a…

…ngular#57854)

Notably the inheritance checking is less complete as the one in the
input migration. That is because we can't efficiently determine query
fields in the analyze phase of compilation units. Unless we aggresively
consider every field of decorated classes as queries and complexify
the merged metadata significantly, we can't reliably check for cases
where a class is incompatible for migration because it overrides a
member from a class that is in a different compilation unit.

This is an acceptable limitation though (maybe for now), as worst case,
we would migrate the class and the other compilation unit would simply
break. Not ideal, but migrations are impossible to be 100% correct in
general— so not a surprise.

In the future, we may find ways to identify queries more reliably in
analyze phase already. e.g. if the compiler were to include this
metadata in the `.d.ts`, or if we decide to simply add this to the
metadata, accepting potential significant HDD increase.

PR Close angular#57854

packages/core/schematics/migrations/signal-migration/src/passes/4_check_inheritance.ts

@@ -9,9 +9,8 @@
 import {MetadataReader} from '@angular/compiler-cli/src/ngtsc/metadata';
 import assert from 'assert';
 import {KnownInputs} from '../input_detection/known_inputs';
-import {MigrationHost} from '../migration_host';
 import {InheritanceGraph} from '../utils/inheritance_graph';
-import {checkInheritanceOfInputs} from './problematic_patterns/check_inheritance';
+import {checkInheritanceOfKnownFields} from './problematic_patterns/check_inheritance';
 
 /**
  * Phase that propagates incompatibilities to derived classes or
@@ -38,7 +37,7 @@ export function pass4__checkInheritanceOfInputs(
   metaRegistry: MetadataReader,
   knownInputs: KnownInputs,
 ) {
-  checkInheritanceOfInputs(inheritanceGraph, metaRegistry, knownInputs, {
+  checkInheritanceOfKnownFields(inheritanceGraph, metaRegistry, knownInputs, {
     isClassWithKnownFields: (clazz) => knownInputs.isInputContainingClass(clazz),
     getFieldsForClass: (clazz) => {
       const directiveInfo = knownInputs.getDirectiveInfoForClass(clazz);

packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/check_inheritance.ts

@@ -38,7 +38,7 @@ import {ProblematicFieldRegistry} from './problematic_field_registry';
  * The logic here detects such cases and marks `bla` as incompatible. If `Derived`
  * would then have other derived classes as well, it would propagate the status.
  */
-export function checkInheritanceOfInputs<D extends ClassFieldDescriptor>(
+export function checkInheritanceOfKnownFields<D extends ClassFieldDescriptor>(
   inheritanceGraph: InheritanceGraph,
   metaRegistry: MetadataReader,
   fields: KnownFields<D> & ProblematicFieldRegistry<D>,

packages/core/schematics/migrations/signal-queries-migration/known_queries.ts

@@ -8,6 +8,7 @@
 
 import ts from 'typescript';
 import {ProgramInfo} from '../../utils/tsurge';
+import {ProblematicFieldRegistry} from '../signal-migration/src/passes/problematic_patterns/problematic_field_registry';
 import {
   ClassFieldDescriptor,
   ClassFieldUniqueKey,
@@ -16,29 +17,46 @@ import {
 import {getClassFieldDescriptorForSymbol} from './field_tracking';
 import type {CompilationUnitData} from './migration';
 
-export class KnownQueries implements KnownFields<ClassFieldDescriptor> {
-  private readonly classToQueryFields = new WeakMap<
-    ts.ClassLikeDeclaration,
-    ClassFieldUniqueKey[]
-  >();
+export class KnownQueries
+  implements KnownFields<ClassFieldDescriptor>, ProblematicFieldRegistry<ClassFieldDescriptor>
+{
+  private readonly classToQueryFields = new Map<ts.ClassLikeDeclaration, ClassFieldDescriptor[]>();
   private readonly knownQueryIDs = new Set<ClassFieldUniqueKey>();
 
   fieldNamesToConsiderForReferenceLookup: Set<string>;
 
   constructor(
     private readonly info: ProgramInfo,
-    globalMetadata: CompilationUnitData,
+    private globalMetadata: CompilationUnitData,
   ) {
     this.fieldNamesToConsiderForReferenceLookup = new Set(
       Object.values(globalMetadata.knownQueryFields).map((f) => f.fieldName),
     );
   }
 
+  isFieldIncompatible(descriptor: ClassFieldDescriptor): boolean {
+    return this.globalMetadata.problematicQueries[descriptor.key] !== undefined;
+  }
+
+  markFieldIncompatible(field: ClassFieldDescriptor): void {
+    this.globalMetadata.problematicQueries[field.key] = true;
+  }
+
+  markClassIncompatible(node: ts.ClassDeclaration): void {
+    this.classToQueryFields.get(node)?.forEach((f) => {
+      this.globalMetadata.problematicQueries[f.key] = true;
+    });
+  }
+
   registerQueryField(queryField: ts.PropertyDeclaration, id: ClassFieldUniqueKey) {
     if (!this.classToQueryFields.has(queryField.parent)) {
       this.classToQueryFields.set(queryField.parent, []);
     }
-    this.classToQueryFields.get(queryField.parent)!.push(id);
+
+    this.classToQueryFields.get(queryField.parent)!.push({
+      key: id,
+      node: queryField,
+    });
     this.knownQueryIDs.add(id);
   }
 
@@ -54,7 +72,11 @@ export class KnownQueries implements KnownFields<ClassFieldDescriptor> {
     return this.classToQueryFields.has(clazz);
   }
 
-  getQueryFieldsOfClass(clazz: ts.ClassDeclaration): ClassFieldUniqueKey[] | undefined {
+  getQueryFieldsOfClass(clazz: ts.ClassDeclaration): ClassFieldDescriptor[] | undefined {
     return this.classToQueryFields.get(clazz);
   }
+
+  getAllClassesWithQueries(): ts.ClassDeclaration[] {
+    return Array.from(this.classToQueryFields.keys()).filter((c) => ts.isClassDeclaration(c));
+  }
 }

packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts

@@ -470,6 +470,81 @@ describe('signal queries migration', () => {
     expect(actual).toContain(`label = viewChild<ElementRef>('labelRef')`);
     expect(actual).toContain(`@ViewChild('labelRef2') label2?: ElementRef;`);
   });
+
+  it('should not migrate if query class is manually instantiated', async () => {
+    const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
+      {
+        name: absoluteFrom('/app.component.ts'),
+        isProgramRootFile: true,
+        contents: `
+          import {ViewChild, ElementRef, Directive} from '@angular/core';
+
+          @Directive()
+          class MyComp implements CompInterface {
+            @ViewChild('labelRef') label?: ElementRef;
+          }
+
+          new MyComp();
+        `,
+      },
+    ]);
+
+    const actual = fs.readFile(absoluteFrom('/app.component.ts'));
+    expect(actual).not.toContain(`viewChild`);
+    expect(actual).toContain(`@ViewChild`);
+  });
+
+  describe('inheritance', () => {
+    it('should not migrate if member is inherited from interface', async () => {
+      const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
+        {
+          name: absoluteFrom('/app.component.ts'),
+          isProgramRootFile: true,
+          contents: `
+            import {ViewChild, ElementRef, Directive} from '@angular/core';
+
+            interface CompInterface {
+              label: ElementRef;
+            }
+
+            @Directive()
+            class MyComp implements CompInterface {
+              @ViewChild('labelRef') label?: ElementRef;
+            }
+          `,
+        },
+      ]);
+
+      const actual = fs.readFile(absoluteFrom('/app.component.ts'));
+      expect(actual).not.toContain(`viewChild`);
+      expect(actual).toContain(`@ViewChild`);
+    });
+
+    it('should not migrate if member is overridden via derived class', async () => {
+      const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
+        {
+          name: absoluteFrom('/app.component.ts'),
+          isProgramRootFile: true,
+          contents: `
+            import {ViewChild, ElementRef, Directive} from '@angular/core';
+
+            @Directive()
+            class MyComp implements CompInterface {
+              @ViewChild('labelRef') label?: ElementRef;
+            }
+
+            class Derived extends MyComp {
+              override label: ElementRef;
+            }
+          `,
+        },
+      ]);
+
+      const actual = fs.readFile(absoluteFrom('/app.component.ts'));
+      expect(actual).not.toContain(`viewChild`);
+      expect(actual).toContain(`@ViewChild`);
+    });
+  });
 });
 
 function populateDeclarationTestCaseComponent(declaration: string): string {

packages/core/schematics/migrations/signal-queries-migration/migration.ts

@@ -22,7 +22,7 @@ import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import
 import {ClassFieldDescriptor} from '../signal-migration/src';
 import {GroupedTsAstVisitor} from '../signal-migration/src/utils/grouped_ts_ast_visitor';
 import {computeReplacementsToMigrateQuery} from './convert_query_property';
-import {extractSourceQueryDefinition} from './identify_queries';
+import {ExtractedQuery, extractSourceQueryDefinition} from './identify_queries';
 import {queryFunctionNameToDecorator} from './query_api_names';
 import {ClassFieldUniqueKey} from '../signal-migration/src/passes/reference_resolution/known_fields';
 import {KnownQueries} from './known_queries';
@@ -39,6 +39,9 @@ import {migrateHostBindings} from '../signal-migration/src/passes/reference_migr
 import {migrateTypeScriptTypeReferences} from '../signal-migration/src/passes/reference_migration/migrate_ts_type_references';
 import {ReferenceResult} from '../signal-migration/src/passes/reference_resolution/reference_result';
 import {getClassFieldDescriptorForSymbol, getUniqueIDForClassProperty} from './field_tracking';
+import {checkIncompatiblePatterns} from '../signal-migration/src/passes/problematic_patterns/common_incompatible_patterns';
+import {InheritanceGraph} from '../signal-migration/src/utils/inheritance_graph';
+import {checkInheritanceOfKnownFields} from '../signal-migration/src/passes/problematic_patterns/check_inheritance';
 
 // TODO: Consider re-using inheritance logic from input migration
 // TODO: Consider re-using problematic pattern recognition logic from input migration
@@ -144,7 +147,7 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
     assert(info.ngCompiler !== null, 'Expected queries migration to have an Angular program.');
 
     // Pre-Analyze the program and get access to the template type checker.
-    const {templateTypeChecker} = await info.ngCompiler['ensureAnalyzed']();
+    const {templateTypeChecker, metaReader} = await info.ngCompiler['ensureAnalyzed']();
     const {program, sourceFiles} = info;
     const checker = program.getTypeChecker();
     const reflector = new TypeScriptReflectionHost(checker);
@@ -158,32 +161,19 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
 
     const knownQueries = new KnownQueries(info, globalMetadata);
     const referenceResult: ReferenceResult<ClassFieldDescriptor> = {references: []};
+    const sourceQueries: ExtractedQuery[] = [];
 
     const isMigratedQuery = (id: ClassFieldUniqueKey) =>
       globalMetadata.knownQueryFields[id] !== undefined &&
       globalMetadata.problematicQueries[id] === undefined;
 
+    // Detect all queries in this unit.
     const queryWholeProgramVisitor = (node: ts.Node) => {
       // Detect all SOURCE queries and migrate them, if possible.
       const extractedQuery = extractSourceQueryDefinition(node, reflector, evaluator, info);
       if (extractedQuery !== null) {
         knownQueries.registerQueryField(extractedQuery.node, extractedQuery.id);
-
-        if (!isMigratedQuery(extractedQuery.id)) {
-          updateFileState(filesWithIncompleteMigration, node.getSourceFile(), extractedQuery.kind);
-          return;
-        }
-        updateFileState(filesWithMigratedQueries, node.getSourceFile(), extractedQuery.kind);
-
-        replacements.push(
-          ...computeReplacementsToMigrateQuery(
-            node as ts.PropertyDeclaration,
-            extractedQuery,
-            importManager,
-            info,
-            printer,
-          ),
-        );
+        sourceQueries.push(extractedQuery);
         return;
       }
 
@@ -203,6 +193,7 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
       ts.forEachChild(sf, queryWholeProgramVisitor);
     }
 
+    // Find all references.
     const groupedAstVisitor = new GroupedTsAstVisitor(sourceFiles);
     groupedAstVisitor.register(
       createFindAllSourceFileReferencesVisitor(
@@ -216,14 +207,48 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
         referenceResult,
       ).visitor,
     );
+
+    const inheritanceGraph = new InheritanceGraph(checker).expensivePopulate(info.sourceFiles);
+    checkIncompatiblePatterns(inheritanceGraph, checker, groupedAstVisitor, knownQueries, () =>
+      knownQueries.getAllClassesWithQueries(),
+    );
     groupedAstVisitor.execute();
 
+    // Check inheritance.
+    checkInheritanceOfKnownFields(inheritanceGraph, metaReader, knownQueries, {
+      getFieldsForClass: (n) => knownQueries.getQueryFieldsOfClass(n) ?? [],
+      isClassWithKnownFields: (clazz) => knownQueries.getQueryFieldsOfClass(clazz) !== undefined,
+    });
+
+    // Migrate declarations.
+    for (const extractedQuery of sourceQueries) {
+      const node = extractedQuery.node;
+      const sf = node.getSourceFile();
+
+      if (!isMigratedQuery(extractedQuery.id)) {
+        updateFileState(filesWithIncompleteMigration, sf, extractedQuery.kind);
+        continue;
+      }
+      updateFileState(filesWithMigratedQueries, sf, extractedQuery.kind);
+
+      replacements.push(
+        ...computeReplacementsToMigrateQuery(
+          node as ts.PropertyDeclaration,
+          extractedQuery,
+          importManager,
+          info,
+          printer,
+        ),
+      );
+    }
+
+    // Migrate references.
     const referenceMigrationHost: ReferenceMigrationHost<ClassFieldDescriptor> = {
       printer,
       replacements,
       shouldMigrateReferencesToField: (field) => isMigratedQuery(field.key),
       shouldMigrateReferencesToClass: (clazz) =>
-        !!knownQueries.getQueryFieldsOfClass(clazz)?.some((q) => isMigratedQuery(q)),
+        !!knownQueries.getQueryFieldsOfClass(clazz)?.some((q) => isMigratedQuery(q.key)),
     };
     migrateTypeScriptReferences(referenceMigrationHost, referenceResult.references, checker, info);
     migrateTemplateReferences(referenceMigrationHost, referenceResult.references);