Skip to content

Commit 8b632db

Browse files
devversionpkozlowski-opensource
devversion
authored and
pkozlowski-opensource
committed
refactor(migrations): simplify and improve inheritance checking in input migration (angular#57883)
Instead of running inheritance checking for analyze and migration phases (in batch mode), we can run once and create a mini-graph in the compilation unit data. This can then improve lookups and propagation of incompatibilities. This commit fixes an issue where a class is chained between three isolated units and members are overriden. Currently this pattern would not be checked properly and e.g. an incompatibility of the superclass would not propagate to derived class, or deeper. PR Close angular#57883
1 parent 4ea8ad6 commit 8b632db

File tree

14 files changed

+262
-80
lines changed

14 files changed

+262
-80
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ export function getCompilationUnitMetadata(knownInputs: KnownInputs) {
3232
[inputClassFieldIdStr as string & ClassFieldUniqueKey]: {
3333
isIncompatible: incompatibility,
3434
seenAsSourceInput: info.metadata.inSourceFile,
35+
extendsFrom: info.extendsFrom?.key ?? null,
3536
},
3637
};
3738
},

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,11 @@
77
*/
88

99
import {InputIncompatibilityReason} from '../input_detection/incompatibility';
10+
import {GraphNode, topologicalSort} from '../utils/inheritance_sort';
1011
import {CompilationUnitData, IncompatibilityType} from './unit_data';
1112

13+
type InputData = {key: string; info: CompilationUnitData['knownInputs'][string]};
14+
1215
/** Merges a list of compilation units into a combined unit. */
1316
export function mergeCompilationUnitData(
1417
metadataFiles: CompilationUnitData[],
@@ -17,11 +20,21 @@ export function mergeCompilationUnitData(
1720
knownInputs: {},
1821
};
1922

23+
const idToGraphNode = new Map<string, GraphNode<InputData>>();
24+
const inheritanceGraph: GraphNode<InputData>[] = [];
25+
2026
for (const file of metadataFiles) {
2127
for (const [key, info] of Object.entries(file.knownInputs)) {
2228
const existing = result.knownInputs[key];
2329
if (existing === undefined) {
2430
result.knownInputs[key] = info;
31+
const node: GraphNode<InputData> = {
32+
incoming: new Set(),
33+
outgoing: new Set(),
34+
data: {info, key},
35+
};
36+
inheritanceGraph.push(node);
37+
idToGraphNode.set(key, node);
2538
continue;
2639
}
2740

@@ -30,12 +43,42 @@ export function mergeCompilationUnitData(
3043
// merge the incompatibility state.
3144
existing.isIncompatible = info.isIncompatible;
3245
}
46+
if (existing.extendsFrom === null && info.extendsFrom !== null) {
47+
existing.extendsFrom = info.extendsFrom;
48+
}
3349
if (!existing.seenAsSourceInput && info.seenAsSourceInput) {
3450
existing.seenAsSourceInput = true;
3551
}
3652
}
3753
}
3854

55+
for (const [key, info] of Object.entries(result.knownInputs)) {
56+
if (info.extendsFrom !== null) {
57+
const from = idToGraphNode.get(key)!;
58+
const target = idToGraphNode.get(info.extendsFrom)!;
59+
from.outgoing.add(target);
60+
target.incoming.add(from);
61+
}
62+
}
63+
64+
// Sort topologically and iterate super classes first, so that we can trivially
65+
// propagate incompatibility statuses (and other checks) without having to check
66+
// in both directions (derived classes, or base classes). This simplifies the
67+
// propagation.
68+
for (const node of topologicalSort(inheritanceGraph).reverse()) {
69+
for (const parent of node.outgoing) {
70+
// If parent is incompatible and not migrated, then this input
71+
// cannot be migrated either.
72+
if (parent.data.info.isIncompatible !== null) {
73+
node.data.info.isIncompatible = {
74+
kind: IncompatibilityType.VIA_INPUT,
75+
reason: InputIncompatibilityReason.ParentIsIncompatible,
76+
};
77+
break;
78+
}
79+
}
80+
}
81+
3982
for (const info of Object.values(result.knownInputs)) {
4083
// We never saw a source file for this input, globally. Mark it as incompatible,
4184
// so that all references and inheritance checks can propagate accordingly.
@@ -47,5 +90,7 @@ export function mergeCompilationUnitData(
4790
}
4891
}
4992

93+
console.error(result);
94+
5095
return result;
5196
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export interface CompilationUnitData {
3434
| {kind: IncompatibilityType.VIA_INPUT; reason: InputIncompatibilityReason}
3535
| null;
3636
seenAsSourceInput: boolean;
37+
extendsFrom: ClassFieldUniqueKey | null;
3738
};
3839
};
3940
}

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

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,17 @@ import {InputDescriptor} from '../utils/input_id';
1111
import {ExtractedInput} from './input_decorator';
1212
import {InputNode} from './input_node';
1313
import {DirectiveInfo} from './directive_info';
14-
import {ClassIncompatibilityReason, InputMemberIncompatibility} from './incompatibility';
14+
import {
15+
ClassIncompatibilityReason,
16+
InputIncompatibilityReason,
17+
InputMemberIncompatibility,
18+
} from './incompatibility';
1519
import {ClassFieldUniqueKey, KnownFields} from '../passes/reference_resolution/known_fields';
1620
import {attemptRetrieveInputFromSymbol} from './nodes_to_input';
1721
import {ProgramInfo, projectFile, ProjectFile} from '../../../../utils/tsurge';
1822
import {MigrationConfig} from '../migration_config';
1923
import {ProblematicFieldRegistry} from '../passes/problematic_patterns/problematic_field_registry';
24+
import {InheritanceTracker} from '../passes/problematic_patterns/check_inheritance';
2025

2126
/**
2227
* Public interface describing a single known `@Input()` in the
@@ -30,6 +35,7 @@ export type KnownInputInfo = {
3035
metadata: ExtractedInput;
3136
descriptor: InputDescriptor;
3237
container: DirectiveInfo;
38+
extendsFrom: InputDescriptor | null;
3339
isIncompatible: () => boolean;
3440
};
3541

@@ -40,7 +46,10 @@ export type KnownInputInfo = {
4046
* loaded into the program.
4147
*/
4248
export class KnownInputs
43-
implements KnownFields<InputDescriptor>, ProblematicFieldRegistry<InputDescriptor>
49+
implements
50+
KnownFields<InputDescriptor>,
51+
ProblematicFieldRegistry<InputDescriptor>,
52+
InheritanceTracker<InputDescriptor>
4453
{
4554
/**
4655
* Known inputs from the whole program.
@@ -93,6 +102,7 @@ export class KnownInputs
93102
metadata: data.metadata,
94103
descriptor: data.descriptor,
95104
container: directiveInfo,
105+
extendsFrom: null,
96106
isIncompatible: () => directiveInfo.isInputMemberIncompatible(data.descriptor),
97107
};
98108

@@ -134,4 +144,28 @@ export class KnownInputs
134144
shouldTrackClassReference(clazz: ts.ClassDeclaration): boolean {
135145
return this.isInputContainingClass(clazz);
136146
}
147+
148+
captureKnownFieldInheritanceRelationship(
149+
derived: InputDescriptor,
150+
parent: InputDescriptor,
151+
): void {
152+
if (!this.has(derived)) {
153+
throw new Error(`Expected input to exist in registry: ${derived.key}`);
154+
}
155+
this.get(derived)!.extendsFrom = parent;
156+
}
157+
158+
captureUnknownDerivedField(field: InputDescriptor): void {
159+
this.markFieldIncompatible(field, {
160+
context: null,
161+
reason: InputIncompatibilityReason.OverriddenByDerivedClass,
162+
});
163+
}
164+
165+
captureUnknownParentField(field: InputDescriptor): void {
166+
this.markFieldIncompatible(field, {
167+
context: null,
168+
reason: InputIncompatibilityReason.TypeConflictWithBaseClass,
169+
});
170+
}
137171
}

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

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -95,26 +95,31 @@ export class SignalInputMigration extends TsurgeComplexMigration<
9595
this.config.reportProgressFn?.(10, 'Analyzing project (input usages)..');
9696
const {inheritanceGraph} = executeAnalysisPhase(host, knownInputs, result, analysisDeps);
9797

98+
// Mark filtered inputs before checking inheritance. This ensures filtered
99+
// inputs properly influence e.g. inherited or derived inputs that now wouldn't
100+
// be safe either (BUT can still be skipped via best effort mode later).
98101
filterInputsViaConfig(result, knownInputs, this.config);
99102

103+
// Analyze inheritance, track edges etc. and later propagate incompatibilities in
104+
// the merge stage.
100105
this.config.reportProgressFn?.(40, 'Checking inheritance..');
101-
pass4__checkInheritanceOfInputs(inheritanceGraph, metaRegistry, knownInputs);
106+
pass4__checkInheritanceOfInputs(inheritanceGraph, analysisDeps.metaRegistry, knownInputs);
107+
108+
// Filter best effort incompatibilities, so that the new filtered ones can
109+
// be accordingly respected in the merge phase.
102110
if (this.config.bestEffortMode) {
103111
filterIncompatibilitiesForBestEffortMode(knownInputs);
104112
}
105113

106-
const unitData = getCompilationUnitMetadata(knownInputs, result);
114+
const unitData = getCompilationUnitMetadata(knownInputs);
107115

108116
// Non-batch mode!
109117
if (this.config.upgradeAnalysisPhaseToAvoidBatch) {
110118
const merged = await this.merge([unitData]);
111-
112-
this.config.reportProgressFn?.(60, 'Collecting migration changes..');
113119
const replacements = await this.migrate(merged, info, {
114120
knownInputs,
115121
result,
116122
host,
117-
inheritanceGraph,
118123
analysisDeps,
119124
});
120125
this.config.reportProgressFn?.(100, 'Completed migration.');
@@ -126,7 +131,6 @@ export class SignalInputMigration extends TsurgeComplexMigration<
126131
knownInputs,
127132
};
128133
}
129-
130134
return confirmAsSerializable(unitData);
131135
}
132136

@@ -141,29 +145,27 @@ export class SignalInputMigration extends TsurgeComplexMigration<
141145
knownInputs: KnownInputs;
142146
result: MigrationResult;
143147
host: MigrationHost;
144-
inheritanceGraph: InheritanceGraph;
145148
analysisDeps: AnalysisProgramInfo;
146149
},
147150
): Promise<Replacement[]> {
148151
const knownInputs = nonBatchData?.knownInputs ?? new KnownInputs(info, this.config);
149152
const result = nonBatchData?.result ?? new MigrationResult();
150153
const host = nonBatchData?.host ?? createMigrationHost(info, this.config);
151154
const analysisDeps = nonBatchData?.analysisDeps ?? this.prepareAnalysisDeps(info);
152-
let inheritanceGraph: InheritanceGraph;
153155

154156
// Can't re-use analysis structures, so re-build them.
155157
if (nonBatchData === undefined) {
156-
const analysisRes = executeAnalysisPhase(host, knownInputs, result, analysisDeps);
157-
inheritanceGraph = analysisRes.inheritanceGraph;
158-
populateKnownInputsFromGlobalData(knownInputs, globalMetadata);
159-
160-
filterInputsViaConfig(result, knownInputs, this.config);
161-
pass4__checkInheritanceOfInputs(inheritanceGraph, analysisDeps.metaRegistry, knownInputs);
162-
if (this.config.bestEffortMode) {
163-
filterIncompatibilitiesForBestEffortMode(knownInputs);
164-
}
158+
executeAnalysisPhase(host, knownInputs, result, analysisDeps);
159+
}
160+
161+
// Incorporate global metadata into known inputs.
162+
populateKnownInputsFromGlobalData(knownInputs, globalMetadata);
163+
164+
if (this.config.bestEffortMode) {
165+
filterIncompatibilitiesForBestEffortMode(knownInputs);
165166
}
166167

168+
this.config.reportProgressFn?.(60, 'Collecting migration changes..');
167169
executeMigrationPhase(host, knownInputs, result, analysisDeps);
168170

169171
return result.replacements;

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

Lines changed: 17 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,16 @@ import {MetadataReader} from '@angular/compiler-cli/src/ngtsc/metadata';
1111
import {ClassDeclaration} from '@angular/compiler-cli/src/ngtsc/reflection';
1212
import assert from 'assert';
1313
import ts from 'typescript';
14-
import {InputIncompatibilityReason} from '../../input_detection/incompatibility';
1514
import {getMemberName} from '../../utils/class_member_names';
1615
import {InheritanceGraph} from '../../utils/inheritance_graph';
1716
import {topologicalSort} from '../../utils/inheritance_sort';
1817
import {ClassFieldDescriptor, KnownFields} from '../reference_resolution/known_fields';
19-
import {ProblematicFieldRegistry} from './problematic_field_registry';
18+
19+
export interface InheritanceTracker<D extends ClassFieldDescriptor> {
20+
captureKnownFieldInheritanceRelationship(derived: D, parent: D): void;
21+
captureUnknownDerivedField(field: D): void;
22+
captureUnknownParentField(field: D): void;
23+
}
2024

2125
/**
2226
* Phase that propagates incompatibilities to derived classes or
@@ -41,21 +45,17 @@ import {ProblematicFieldRegistry} from './problematic_field_registry';
4145
export function checkInheritanceOfKnownFields<D extends ClassFieldDescriptor>(
4246
inheritanceGraph: InheritanceGraph,
4347
metaRegistry: MetadataReader,
44-
fields: KnownFields<D> & ProblematicFieldRegistry<D>,
48+
fields: KnownFields<D> & InheritanceTracker<D>,
4549
opts: {
4650
getFieldsForClass: (node: ts.ClassDeclaration) => D[];
4751
isClassWithKnownFields: (node: ts.ClassDeclaration) => boolean;
4852
},
4953
) {
50-
// Sort topologically and iterate super classes first, so that we can trivially
51-
// propagate incompatibility statuses (and other checks) without having to check
52-
// in both directions (derived classes, or base classes). This simplifies the logic
53-
// further down in this function significantly.
54-
const topologicalSortedClasses = topologicalSort(inheritanceGraph)
55-
.filter((t) => ts.isClassDeclaration(t) && opts.isClassWithKnownFields(t))
56-
.reverse();
54+
const allInputClasses = Array.from(inheritanceGraph.allClassesInInheritance).filter(
55+
(t) => ts.isClassDeclaration(t) && opts.isClassWithKnownFields(t),
56+
);
5757

58-
for (const inputClass of topologicalSortedClasses) {
58+
for (const inputClass of allInputClasses) {
5959
// Note: Class parents of `inputClass` were already checked by
6060
// the previous iterations (given the reverse topological sort)—
6161
// hence it's safe to assume that incompatibility of parent classes will
@@ -97,25 +97,20 @@ export function checkInheritanceOfKnownFields<D extends ClassFieldDescriptor>(
9797
(ts.isIdentifier(fieldDescr.node.name) || ts.isStringLiteralLike(fieldDescr.node.name)) &&
9898
inputFieldNamesFromMetadataArray.has(fieldDescr.node.name.text)
9999
) {
100-
fields.markFieldIncompatible(fieldDescr, {
101-
context: null,
102-
reason: InputIncompatibilityReason.RedeclaredViaDerivedClassInputsArray,
103-
});
100+
fields.captureUnknownDerivedField(fieldDescr);
104101
}
105102

106103
for (const derived of derivedMembers) {
107104
const derivedInput = fields.attemptRetrieveDescriptorFromSymbol(derived);
108105
if (derivedInput !== null) {
106+
// Note: We always track dependencies from the child to the parent,
107+
// so skip here for now.
109108
continue;
110109
}
111110

112111
// If we discover a derived, non-input member, then it will cause
113112
// conflicts, and we mark the current input as incompatible.
114-
fields.markFieldIncompatible(fieldDescr, {
115-
context: derived.valueDeclaration ?? inputNode,
116-
reason: InputIncompatibilityReason.OverriddenByDerivedClass,
117-
});
118-
113+
fields.captureUnknownDerivedField(fieldDescr);
119114
continue inputCheck;
120115
}
121116

@@ -127,21 +122,10 @@ export function checkInheritanceOfKnownFields<D extends ClassFieldDescriptor>(
127122
const inheritedMemberInput = fields.attemptRetrieveDescriptorFromSymbol(inherited);
128123
// Parent is not an input, and hence will conflict..
129124
if (inheritedMemberInput === null) {
130-
fields.markFieldIncompatible(fieldDescr, {
131-
context: inherited.valueDeclaration ?? inputNode,
132-
reason: InputIncompatibilityReason.TypeConflictWithBaseClass,
133-
});
134-
continue;
135-
}
136-
// Parent is incompatible, so this input also needs to be.
137-
// It cannot be migrated.
138-
if (fields.isFieldIncompatible(inheritedMemberInput)) {
139-
fields.markFieldIncompatible(fieldDescr, {
140-
context: inheritedMemberInput.node,
141-
reason: InputIncompatibilityReason.ParentIsIncompatible,
142-
});
125+
fields.captureUnknownParentField(fieldDescr);
143126
continue;
144127
}
128+
fields.captureKnownFieldInheritanceRelationship(fieldDescr, inheritedMemberInput);
145129
}
146130
}
147131
}

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,22 @@ export type GraphNode = ts.ClassDeclaration | ts.ClassExpression | ts.InterfaceD
2424
*/
2525
export class InheritanceGraph {
2626
/** Maps nodes to their parent nodes. */
27-
classParents = new Map<GraphNode, GraphNode[]>();
27+
classToParents = new Map<GraphNode, GraphNode[]>();
2828
/** Maps nodes to their derived nodes. */
2929
parentToChildren = new Map<GraphNode, GraphNode[]>();
30+
/** All classes seen participating in inheritance chains. */
31+
allClassesInInheritance = new Set<GraphNode>();
3032

3133
constructor(private checker: ts.TypeChecker) {}
3234

3335
/** Registers a given class in the graph. */
3436
registerClass(clazz: GraphNode, parents: GraphNode[]) {
35-
this.classParents.set(clazz, parents);
37+
this.classToParents.set(clazz, parents);
38+
this.allClassesInInheritance.add(clazz);
3639

3740
for (const parent of parents) {
41+
this.allClassesInInheritance.add(parent);
42+
3843
if (!this.parentToChildren.has(parent)) {
3944
this.parentToChildren.set(parent, []);
4045
}
@@ -56,7 +61,7 @@ export class InheritanceGraph {
5661
inherited: ts.Symbol | undefined;
5762
derivedMembers: ts.Symbol[];
5863
} {
59-
const inheritedTypes = (this.classParents.get(clazz) ?? []).map((c) =>
64+
const inheritedTypes = (this.classToParents.get(clazz) ?? []).map((c) =>
6065
this.checker.getTypeAtLocation(c),
6166
);
6267
const derivedLeafs = this._traceDerivedChainToLeafs(clazz).map((c) =>

0 commit comments

Comments
 (0)