Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b144126

Browse filesBrowse files
Ryan Russellkirjs
Ryan Russell
authored and
kirjs
committed
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
Copy full SHA for b144126

File tree

Expand file treeCollapse file tree

4 files changed

+355
-51
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+355
-51
lines changed
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/inject-migration/analysis.ts
+21Lines 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. */
Collapse file

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

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/inject-migration/internal.ts
+95-2Lines 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() &&
Collapse file

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

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/inject-migration/migration.ts
+74-24Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.