Skip to content

Commit b144126

Browse files
Ryan Russellkirjs
Ryan Russell
authored andcommitted
fix(core): inject migration: replace param with this. (#60713)
The inject tool inserts `const foo = this.foo` if code in the constructor referenced the constructor parameter `foo`. If `foo` is a readonly property, we can instead replace `foo` with `this.foo`. This allows more properties to be moved out of the constructor with combineMemberInitializers. For now, it only touches initializers, not all of the code in the constructor. PR Close #60713
1 parent 77c6041 commit b144126

File tree

4 files changed

+355
-51
lines changed

4 files changed

+355
-51
lines changed

packages/core/schematics/ng-generate/inject-migration/analysis.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,27 @@ export interface MigrationOptions {
4040
* ```
4141
*/
4242
_internalCombineMemberInitializers?: boolean;
43+
44+
/**
45+
* Internal-only option that determines whether the migration should
46+
* replace constructor parameter references with `this.param` property
47+
* references. Only applies to references to readonly properties in
48+
* initializers.
49+
*
50+
* ```
51+
* // Before
52+
* private foo;
53+
*
54+
* constructor(readonly service: Service) {
55+
* this.foo = service.getFoo();
56+
* }
57+
*
58+
* // After
59+
* readonly service = inject(Service);
60+
* private foo = this.service.getFoo();
61+
* ```
62+
*/
63+
_internalReplaceParameterReferencesInInitializers?: boolean;
4364
}
4465

4566
/** Names of decorators that enable DI on a class declaration. */

packages/core/schematics/ng-generate/inject-migration/internal.ts

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,12 @@
77
*/
88

99
import ts from 'typescript';
10-
import {isAccessedViaThis, isInlineFunction, parameterDeclaresProperty} from './analysis';
10+
import {
11+
isAccessedViaThis,
12+
isInlineFunction,
13+
MigrationOptions,
14+
parameterDeclaresProperty,
15+
} from './analysis';
1116

1217
/** Property that is a candidate to be combined. */
1318
interface CombineCandidate {
@@ -40,6 +45,7 @@ export function findUninitializedPropertiesToCombine(
4045
node: ts.ClassDeclaration,
4146
constructor: ts.ConstructorDeclaration,
4247
localTypeChecker: ts.TypeChecker,
48+
options: MigrationOptions,
4349
): {
4450
toCombine: CombineCandidate[];
4551
toHoist: ts.PropertyDeclaration[];
@@ -67,11 +73,15 @@ export function findUninitializedPropertiesToCombine(
6773
return null;
6874
}
6975

76+
const inlinableParameters = options._internalReplaceParameterReferencesInInitializers
77+
? findInlinableParameterReferences(constructor, localTypeChecker)
78+
: new Set<ts.Declaration>();
79+
7080
for (const [name, decl] of membersToDeclarations.entries()) {
7181
if (memberInitializers.has(name)) {
7282
const initializer = memberInitializers.get(name)!;
7383

74-
if (!hasLocalReferences(initializer, constructor, localTypeChecker)) {
84+
if (!hasLocalReferences(initializer, constructor, inlinableParameters, localTypeChecker)) {
7585
toCombine ??= [];
7686
toCombine.push({declaration: membersToDeclarations.get(name)!, initializer});
7787
}
@@ -230,6 +240,87 @@ function getMemberInitializers(constructor: ts.ConstructorDeclaration) {
230240
return memberInitializers;
231241
}
232242

243+
/**
244+
* Checks if the node is an identifier that references a property from the given
245+
* list. Returns the property if it is.
246+
*/
247+
function getIdentifierReferencingProperty(
248+
node: ts.Node,
249+
localTypeChecker: ts.TypeChecker,
250+
propertyNames: Set<string>,
251+
properties: Set<ts.Declaration>,
252+
): ts.ParameterDeclaration | undefined {
253+
if (!ts.isIdentifier(node) || !propertyNames.has(node.text)) {
254+
return undefined;
255+
}
256+
const declarations = localTypeChecker.getSymbolAtLocation(node)?.declarations;
257+
if (!declarations) {
258+
return undefined;
259+
}
260+
261+
for (const decl of declarations) {
262+
if (properties.has(decl)) {
263+
return decl as ts.ParameterDeclaration;
264+
}
265+
}
266+
return undefined;
267+
}
268+
269+
/**
270+
* Returns true if the node introduces a new `this` scope (so we can't
271+
* reference the outer this).
272+
*/
273+
function introducesNewThisScope(node: ts.Node): boolean {
274+
return (
275+
ts.isFunctionDeclaration(node) ||
276+
ts.isFunctionExpression(node) ||
277+
ts.isMethodDeclaration(node) ||
278+
ts.isClassDeclaration(node) ||
279+
ts.isClassExpression(node)
280+
);
281+
}
282+
283+
/**
284+
* Finds constructor parameter references which can be inlined as `this.prop`.
285+
* - prop must be a readonly property
286+
* - the reference can't be in a nested function where `this` might refer
287+
* to something else
288+
*/
289+
function findInlinableParameterReferences(
290+
constructorDeclaration: ts.ConstructorDeclaration,
291+
localTypeChecker: ts.TypeChecker,
292+
): Set<ts.Declaration> {
293+
const eligibleProperties = constructorDeclaration.parameters.filter(
294+
(p) =>
295+
ts.isIdentifier(p.name) && p.modifiers?.some((s) => s.kind === ts.SyntaxKind.ReadonlyKeyword),
296+
);
297+
const eligibleNames = new Set(eligibleProperties.map((p) => (p.name as ts.Identifier).text));
298+
const eligiblePropertiesSet: Set<ts.Declaration> = new Set(eligibleProperties);
299+
300+
function walk(node: ts.Node, canReferenceThis: boolean) {
301+
const property = getIdentifierReferencingProperty(
302+
node,
303+
localTypeChecker,
304+
eligibleNames,
305+
eligiblePropertiesSet,
306+
);
307+
if (property && !canReferenceThis) {
308+
// The property is referenced in a nested context where
309+
// we can't use `this`, so we can't inline it.
310+
eligiblePropertiesSet.delete(property);
311+
} else if (introducesNewThisScope(node)) {
312+
canReferenceThis = false;
313+
}
314+
315+
ts.forEachChild(node, (child) => {
316+
walk(child, canReferenceThis);
317+
});
318+
}
319+
320+
walk(constructorDeclaration, true);
321+
return eligiblePropertiesSet;
322+
}
323+
233324
/**
234325
* Determines if a node has references to local symbols defined in the constructor.
235326
* @param root Expression to check for local references.
@@ -239,6 +330,7 @@ function getMemberInitializers(constructor: ts.ConstructorDeclaration) {
239330
function hasLocalReferences(
240331
root: ts.Expression,
241332
constructor: ts.ConstructorDeclaration,
333+
allowedParameters: Set<ts.Declaration>,
242334
localTypeChecker: ts.TypeChecker,
243335
): boolean {
244336
const sourceFile = root.getSourceFile();
@@ -265,6 +357,7 @@ function hasLocalReferences(
265357
// The source file check is a bit redundant since the type checker
266358
// is local to the file, but it's inexpensive and it can prevent
267359
// bugs in the future if we decide to use a full type checker.
360+
!allowedParameters.has(decl) &&
268361
decl.getSourceFile() === sourceFile &&
269362
decl.getStart() >= constructor.getStart() &&
270363
decl.getEnd() <= constructor.getEnd() &&

packages/core/schematics/ng-generate/inject-migration/migration.ts

Lines changed: 74 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
7171
prependToClass,
7272
afterInjectCalls,
7373
memberIndentation,
74+
options,
7475
);
7576
}
7677

@@ -755,8 +756,9 @@ function applyInternalOnlyChanges(
755756
prependToClass: string[],
756757
afterInjectCalls: string[],
757758
memberIndentation: string,
759+
options: MigrationOptions,
758760
) {
759-
const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker);
761+
const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker, options);
760762

761763
if (result === null) {
762764
return;
@@ -774,36 +776,46 @@ function applyInternalOnlyChanges(
774776
result.toCombine.forEach(({declaration, initializer}) => {
775777
const initializerStatement = closestNode(initializer, ts.isStatement);
776778

779+
// Strip comments if we are just going modify the node in-place.
780+
const modifiers = preserveInitOrder
781+
? declaration.modifiers
782+
: cloneModifiers(declaration.modifiers);
783+
const name = preserveInitOrder ? declaration.name : cloneName(declaration.name);
784+
785+
const newProperty = ts.factory.createPropertyDeclaration(
786+
modifiers,
787+
name,
788+
declaration.questionToken,
789+
declaration.type,
790+
undefined,
791+
);
792+
793+
const propText = printer.printNode(
794+
ts.EmitHint.Unspecified,
795+
newProperty,
796+
declaration.getSourceFile(),
797+
);
798+
const initializerText = replaceParameterReferencesInInitializer(
799+
initializer,
800+
constructor,
801+
localTypeChecker,
802+
);
803+
const withInitializer = `${propText.slice(0, -1)} = ${initializerText};`;
804+
777805
// If the initialization order is being preserved, we have to remove the original
778806
// declaration and re-declare it. Otherwise we can do the replacement in-place.
779807
if (preserveInitOrder) {
780-
// Preserve comment in the new property since we are removing the entire node.
781-
const newProperty = ts.factory.createPropertyDeclaration(
782-
declaration.modifiers,
783-
declaration.name,
784-
declaration.questionToken,
785-
declaration.type,
786-
initializer,
787-
);
788-
789808
tracker.removeNode(declaration, true);
790809
removedMembers.add(declaration);
791-
afterInjectCalls.push(
792-
memberIndentation +
793-
printer.printNode(ts.EmitHint.Unspecified, newProperty, declaration.getSourceFile()),
794-
);
810+
afterInjectCalls.push(memberIndentation + withInitializer);
795811
} else {
796-
// Strip comments from the declaration since we are replacing just
797-
// the node, not the leading comment.
798-
const newProperty = ts.factory.createPropertyDeclaration(
799-
cloneModifiers(declaration.modifiers),
800-
cloneName(declaration.name),
801-
declaration.questionToken,
802-
declaration.type,
803-
initializer,
812+
const sourceFile = declaration.getSourceFile();
813+
tracker.replaceText(
814+
sourceFile,
815+
declaration.getStart(),
816+
declaration.getWidth(),
817+
withInitializer,
804818
);
805-
806-
tracker.replaceNode(declaration, newProperty);
807819
}
808820

809821
// This should always be defined, but null check it just in case.
@@ -826,3 +838,41 @@ function applyInternalOnlyChanges(
826838
prependToClass.push('');
827839
}
828840
}
841+
842+
function replaceParameterReferencesInInitializer(
843+
initializer: ts.Expression,
844+
constructor: ts.ConstructorDeclaration,
845+
localTypeChecker: ts.TypeChecker,
846+
): string {
847+
// 1. Collect the locations of identifier nodes that reference constructor parameters.
848+
// 2. Add `this.` to those locations.
849+
const insertLocations = [0];
850+
851+
function walk(node: ts.Node) {
852+
if (
853+
ts.isIdentifier(node) &&
854+
!(ts.isPropertyAccessExpression(node.parent) && node === node.parent.name) &&
855+
localTypeChecker
856+
.getSymbolAtLocation(node)
857+
?.declarations?.some((decl) =>
858+
constructor.parameters.includes(decl as ts.ParameterDeclaration),
859+
)
860+
) {
861+
insertLocations.push(node.getStart() - initializer.getStart());
862+
}
863+
ts.forEachChild(node, walk);
864+
}
865+
walk(initializer);
866+
867+
const initializerText = initializer.getText();
868+
insertLocations.push(initializerText.length);
869+
870+
insertLocations.sort((a, b) => a - b);
871+
872+
const result: string[] = [];
873+
for (let i = 0; i < insertLocations.length - 1; i++) {
874+
result.push(initializerText.slice(insertLocations[i], insertLocations[i + 1]));
875+
}
876+
877+
return result.join('this.');
878+
}

0 commit comments

Comments
 (0)