Skip to content

Commit 274a4c0

Browse files
devversionatscott
devversion
authored and
atscott
committed
refactor(migrations): assign significance to input migration incompatibility reasons (angular#57957)
If an input is already skipped from the migration for e.g. being an accessor, then the fact that it may be incompatible due inheritance is not relevant. This commit assigns signficance/priority to input incompatibilities. This is also important for not accidentally overriding "explicit config filter" incompatibilities with e.g. accessor incompatibility (which may be recognized later or visible from another compilation unit). PR Close angular#57957
1 parent fc7e805 commit 274a4c0

File tree

8 files changed

+117
-66
lines changed

8 files changed

+117
-66
lines changed

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

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,24 @@
88

99
import {KnownInputs} from '../input_detection/known_inputs';
1010
import {ClassFieldUniqueKey} from '../passes/reference_resolution/known_fields';
11-
import {CompilationUnitData, IncompatibilityType} from './unit_data';
11+
import {CompilationUnitData} from './unit_data';
1212

1313
export function getCompilationUnitMetadata(knownInputs: KnownInputs) {
1414
const struct: CompilationUnitData = {
1515
knownInputs: Array.from(knownInputs.knownInputIds.entries()).reduce(
1616
(res, [inputClassFieldIdStr, info]) => {
1717
const classIncompatibility =
18-
info.container.incompatible !== null
19-
? ({kind: IncompatibilityType.VIA_CLASS, reason: info.container.incompatible!} as const)
20-
: null;
18+
info.container.incompatible !== null ? info.container.incompatible : null;
2119
const memberIncompatibility = info.container.memberIncompatibility.has(inputClassFieldIdStr)
22-
? ({
23-
kind: IncompatibilityType.VIA_INPUT,
24-
reason: info.container.memberIncompatibility.get(inputClassFieldIdStr)!.reason,
25-
} as const)
20+
? info.container.memberIncompatibility.get(inputClassFieldIdStr)!.reason
2621
: null;
27-
const incompatibility = classIncompatibility ?? memberIncompatibility ?? null;
2822

2923
// Note: Trim off the `context` as it cannot be serialized with e.g. TS nodes.
3024
return {
3125
...res,
3226
[inputClassFieldIdStr as string & ClassFieldUniqueKey]: {
33-
isIncompatible: incompatibility,
27+
owningClassIncompatibility: classIncompatibility,
28+
memberIncompatibility,
3429
seenAsSourceInput: info.metadata.inSourceFile,
3530
extendsFrom: info.extendsFrom?.key ?? null,
3631
},

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

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {InputIncompatibilityReason} from '../input_detection/incompatibility';
9+
import {
10+
InputIncompatibilityReason,
11+
pickInputIncompatibility,
12+
} from '../input_detection/incompatibility';
1013
import {GraphNode, topologicalSort} from '../utils/inheritance_sort';
11-
import {CompilationUnitData, IncompatibilityType} from './unit_data';
14+
import {CompilationUnitData} from './unit_data';
1215

1316
type InputData = {key: string; info: CompilationUnitData['knownInputs'][string]};
1417

@@ -22,6 +25,8 @@ export function mergeCompilationUnitData(
2225

2326
const idToGraphNode = new Map<string, GraphNode<InputData>>();
2427
const inheritanceGraph: GraphNode<InputData>[] = [];
28+
const isNodeIncompatible = (node: InputData) =>
29+
node.info.memberIncompatibility !== null || node.info.owningClassIncompatibility !== null;
2530

2631
for (const file of metadataFiles) {
2732
for (const [key, info] of Object.entries(file.knownInputs)) {
@@ -38,17 +43,37 @@ export function mergeCompilationUnitData(
3843
continue;
3944
}
4045

41-
if (existing.isIncompatible === null && info.isIncompatible) {
42-
// input might not be incompatible in one target, but others might invalidate it.
43-
// merge the incompatibility state.
44-
existing.isIncompatible = info.isIncompatible;
45-
}
46+
// Merge metadata.
4647
if (existing.extendsFrom === null && info.extendsFrom !== null) {
4748
existing.extendsFrom = info.extendsFrom;
4849
}
4950
if (!existing.seenAsSourceInput && info.seenAsSourceInput) {
5051
existing.seenAsSourceInput = true;
5152
}
53+
54+
// Merge member incompatibility.
55+
if (info.memberIncompatibility !== null) {
56+
if (existing.memberIncompatibility === null) {
57+
existing.memberIncompatibility = info.memberIncompatibility;
58+
} else {
59+
// Input might not be incompatible in one target, but others might invalidate it.
60+
// merge the incompatibility state.
61+
existing.memberIncompatibility = pickInputIncompatibility(
62+
{reason: info.memberIncompatibility, context: null},
63+
{reason: existing.memberIncompatibility, context: null},
64+
).reason;
65+
}
66+
}
67+
68+
// Merge incompatibility of the class owning the input.
69+
// Note: This metadata is stored per field for simplicity currently,
70+
// but in practice it could be a separate field in the compilation data.
71+
if (
72+
info.owningClassIncompatibility !== null &&
73+
existing.owningClassIncompatibility === null
74+
) {
75+
existing.owningClassIncompatibility = info.owningClassIncompatibility;
76+
}
5277
}
5378
}
5479

@@ -66,27 +91,37 @@ export function mergeCompilationUnitData(
6691
// in both directions (derived classes, or base classes). This simplifies the
6792
// propagation.
6893
for (const node of topologicalSort(inheritanceGraph).reverse()) {
94+
const existingMemberIncompatibility =
95+
node.data.info.memberIncompatibility !== null
96+
? {reason: node.data.info.memberIncompatibility, context: null}
97+
: null;
98+
6999
for (const parent of node.outgoing) {
70100
// 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-
};
101+
// cannot be migrated either. Try propagating parent incompatibility then.
102+
if (isNodeIncompatible(parent.data)) {
103+
node.data.info.memberIncompatibility = pickInputIncompatibility(
104+
{reason: InputIncompatibilityReason.ParentIsIncompatible, context: null},
105+
existingMemberIncompatibility,
106+
).reason;
77107
break;
78108
}
79109
}
80110
}
81111

82112
for (const info of Object.values(result.knownInputs)) {
83-
// We never saw a source file for this input, globally. Mark it as incompatible,
113+
// We never saw a source file for this input, globally. Try marking it as incompatible,
84114
// so that all references and inheritance checks can propagate accordingly.
85115
if (!info.seenAsSourceInput) {
86-
info.isIncompatible = {
87-
kind: IncompatibilityType.VIA_INPUT,
88-
reason: InputIncompatibilityReason.OutsideOfMigrationScope,
89-
};
116+
const existingMemberIncompatibility =
117+
info.memberIncompatibility !== null
118+
? {reason: info.memberIncompatibility, context: null}
119+
: null;
120+
121+
info.memberIncompatibility = pickInputIncompatibility(
122+
{reason: InputIncompatibilityReason.OutsideOfMigrationScope, context: null},
123+
existingMemberIncompatibility,
124+
).reason;
90125
}
91126
}
92127

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

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {KnownInputs} from '../input_detection/known_inputs';
1010
import {ClassFieldUniqueKey} from '../passes/reference_resolution/known_fields';
11-
import {CompilationUnitData, IncompatibilityType} from './unit_data';
11+
import {CompilationUnitData} from './unit_data';
1212

1313
export function populateKnownInputsFromGlobalData(
1414
knownInputs: KnownInputs,
@@ -24,18 +24,18 @@ export function populateKnownInputsFromGlobalData(
2424
}
2525

2626
const inputMetadata = knownInputs.get({key})!;
27-
if (!inputMetadata.isIncompatible() && info.isIncompatible) {
28-
if (info.isIncompatible.kind === IncompatibilityType.VIA_CLASS) {
29-
knownInputs.markClassIncompatible(
30-
inputMetadata.container.clazz,
31-
info.isIncompatible.reason,
32-
);
33-
} else {
34-
knownInputs.markFieldIncompatible(inputMetadata.descriptor, {
35-
context: null, // No context serializable.
36-
reason: info.isIncompatible.reason,
37-
});
38-
}
27+
if (info.memberIncompatibility) {
28+
knownInputs.markFieldIncompatible(inputMetadata.descriptor, {
29+
context: null, // No context serializable.
30+
reason: info.memberIncompatibility,
31+
});
32+
}
33+
34+
if (info.owningClassIncompatibility) {
35+
knownInputs.markClassIncompatible(
36+
inputMetadata.container.clazz,
37+
info.owningClassIncompatibility,
38+
);
3939
}
4040
}
4141
}

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,6 @@ import {
1212
} from '../input_detection/incompatibility';
1313
import {ClassFieldUniqueKey} from '../passes/reference_resolution/known_fields';
1414

15-
/** Type of incompatibility. */
16-
export enum IncompatibilityType {
17-
VIA_CLASS,
18-
VIA_INPUT,
19-
}
20-
2115
/**
2216
* Type describing a serializable compilation unit data.
2317
*
@@ -29,10 +23,8 @@ export interface CompilationUnitData {
2923
knownInputs: {
3024
// Use `string` here so that it's a usable index key.
3125
[inputIdKey: string]: {
32-
isIncompatible:
33-
| {kind: IncompatibilityType.VIA_CLASS; reason: ClassIncompatibilityReason}
34-
| {kind: IncompatibilityType.VIA_INPUT; reason: InputIncompatibilityReason}
35-
| null;
26+
owningClassIncompatibility: ClassIncompatibilityReason | null;
27+
memberIncompatibility: InputIncompatibilityReason | null;
3628
seenAsSourceInput: boolean;
3729
extendsFrom: ClassFieldUniqueKey | null;
3830
};

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,6 @@ export class DirectiveInfo {
6060
getInputMemberIncompatibility(
6161
input: InputDescriptor,
6262
): ClassIncompatibilityReason | InputMemberIncompatibility | null {
63-
return this.incompatible ?? this.memberIncompatibility.get(input.key) ?? null;
63+
return this.memberIncompatibility.get(input.key) ?? this.incompatible ?? null;
6464
}
6565
}

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

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,24 @@
88

99
import ts from 'typescript';
1010

11-
/** Reasons why an input cannot be migrated. */
11+
/**
12+
* Reasons why an input cannot be migrated.
13+
*
14+
* Higher values of incompatibility reasons indicate a more significant
15+
* incompatibility reason. Lower ones may be overridden by higher ones.
16+
* */
1217
export enum InputIncompatibilityReason {
13-
OutsideOfMigrationScope,
14-
SkippedViaConfigFilter,
15-
Accessor,
16-
WriteAssignment,
17-
OverriddenByDerivedClass,
18-
RedeclaredViaDerivedClassInputsArray,
19-
TypeConflictWithBaseClass,
20-
ParentIsIncompatible,
21-
SpyOnThatOverwritesField,
22-
PotentiallyNarrowedInTemplateButNoSupportYet,
23-
RequiredInputButNoGoodExplicitTypeExtractable,
18+
OverriddenByDerivedClass = 1,
19+
RedeclaredViaDerivedClassInputsArray = 2,
20+
TypeConflictWithBaseClass = 3,
21+
ParentIsIncompatible = 4,
22+
SpyOnThatOverwritesField = 5,
23+
PotentiallyNarrowedInTemplateButNoSupportYet = 6,
24+
RequiredInputButNoGoodExplicitTypeExtractable = 7,
25+
WriteAssignment = 8,
26+
Accessor = 9,
27+
OutsideOfMigrationScope = 10,
28+
SkippedViaConfigFilter = 11,
2429
}
2530

2631
/** Reasons why a whole class and its inputs cannot be migrated. */
@@ -45,3 +50,17 @@ export function isInputMemberIncompatibility(value: unknown): value is InputMemb
4550
)
4651
);
4752
}
53+
54+
/** Picks the more significant input compatibility. */
55+
export function pickInputIncompatibility(
56+
a: InputMemberIncompatibility,
57+
b: InputMemberIncompatibility | null,
58+
): InputMemberIncompatibility {
59+
if (b === null) {
60+
return a;
61+
}
62+
if (a.reason < b.reason) {
63+
return b;
64+
}
65+
return a;
66+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import {
1515
ClassIncompatibilityReason,
1616
InputIncompatibilityReason,
1717
InputMemberIncompatibility,
18+
isInputMemberIncompatibility,
19+
pickInputIncompatibility,
1820
} from './incompatibility';
1921
import {ClassFieldUniqueKey, KnownFields} from '../passes/reference_resolution/known_fields';
2022
import {attemptRetrieveInputFromSymbol} from './nodes_to_input';
@@ -124,6 +126,15 @@ export class KnownInputs
124126
if (!this.knownInputIds.has(input.key)) {
125127
throw new Error(`Input cannot be marked as incompatible because it's not registered.`);
126128
}
129+
130+
const inputInfo = this.knownInputIds.get(input.key)!;
131+
const existingIncompatibility = inputInfo.container.getInputMemberIncompatibility(input);
132+
133+
// Ensure an existing more significant incompatibility is not overridden.
134+
if (existingIncompatibility !== null && isInputMemberIncompatibility(existingIncompatibility)) {
135+
incompatibility = pickInputIncompatibility(existingIncompatibility, incompatibility);
136+
}
137+
127138
this.knownInputIds
128139
.get(input.key)!
129140
.container.memberIncompatibility.set(input.key, incompatibility);

‎packages/core/schematics/migrations/signal-migration/test/golden.txt‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1111,8 +1111,7 @@ import {Input} from '@angular/core';
11111111

11121112
class MyComp {
11131113
// TODO: Skipped for migration because:
1114-
// Class of this input is manually instantiated. This is discouraged and prevents
1115-
// migration
1114+
// A jasmine `spyOn` call spies on this input. This breaks with signal inputs.
11161115
@Input() myInput = () => {};
11171116
}
11181117

0 commit comments

Comments
 (0)