From d0a71a33b1c95975492cee6794264647dabe9861 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 17 Sep 2024 14:26:25 +0000 Subject: [PATCH] refactor(migrations): check inheritance in signal queries migration (#57854) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #57854 --- .../src/passes/4_check_inheritance.ts | 5 +- .../problematic_patterns/check_inheritance.ts | 2 +- .../signal-queries-migration/known_queries.ts | 38 ++++++++-- .../migration.spec.ts | 75 +++++++++++++++++++ .../signal-queries-migration/migration.ts | 63 +++++++++++----- 5 files changed, 152 insertions(+), 31 deletions(-) diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/4_check_inheritance.ts b/packages/core/schematics/migrations/signal-migration/src/passes/4_check_inheritance.ts index f29b6a657625f..02b03f005c978 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/4_check_inheritance.ts +++ b/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); diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/check_inheritance.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/check_inheritance.ts index 77b7cbec35c5e..ba5fa4d5c7be6 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/check_inheritance.ts +++ b/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( +export function checkInheritanceOfKnownFields( inheritanceGraph: InheritanceGraph, metaRegistry: MetadataReader, fields: KnownFields & ProblematicFieldRegistry, diff --git a/packages/core/schematics/migrations/signal-queries-migration/known_queries.ts b/packages/core/schematics/migrations/signal-queries-migration/known_queries.ts index e535e8cd1d398..2b89f53b4b636 100644 --- a/packages/core/schematics/migrations/signal-queries-migration/known_queries.ts +++ b/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 { - private readonly classToQueryFields = new WeakMap< - ts.ClassLikeDeclaration, - ClassFieldUniqueKey[] - >(); +export class KnownQueries + implements KnownFields, ProblematicFieldRegistry +{ + private readonly classToQueryFields = new Map(); private readonly knownQueryIDs = new Set(); fieldNamesToConsiderForReferenceLookup: Set; 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 { 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)); + } } diff --git a/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts b/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts index 9d651414c6130..db2ecb5f1e471 100644 --- a/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts +++ b/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts @@ -470,6 +470,81 @@ describe('signal queries migration', () => { expect(actual).toContain(`label = viewChild('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 { diff --git a/packages/core/schematics/migrations/signal-queries-migration/migration.ts b/packages/core/schematics/migrations/signal-queries-migration/migration.ts index c0ccfa9a7e36d..49057ff70fe43 100644 --- a/packages/core/schematics/migrations/signal-queries-migration/migration.ts +++ b/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 = {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 = { 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);