Skip to content

Commit 22e8f53

Browse files
devversiondevversion
committed
refactor(ng-update): better check for updated constructor signatures
Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can *way* better report invalid signatures for constructor changes like for the `MatCalendar` (#9775). Just relying on the length of arguments means that *order* is being ignored. This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).
1 parent cfb4f58 commit 22e8f53

File tree

9 files changed

+329
-194
lines changed

9 files changed

+329
-194
lines changed

‎src/lib/schematics/update/material/data/constructor-checks.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
/**
10+
* List of class names for which the constructor signature has been changed. The new constructor
11+
* signature types don't need to be stored here because the signature will be determined
12+
* dynamically.
13+
*/
14+
export const constructorChecks = [
15+
// https://github.com/angular/material2/pull/9190
16+
'NativeDateAdapter',
17+
18+
// https://github.com/angular/material2/pull/10319
19+
'MatAutocomplete',
20+
21+
// https://github.com/angular/material2/pull/10344
22+
'MatTooltip',
23+
24+
// https://github.com/angular/material2/pull/10389
25+
'MatIconRegistry',
26+
27+
// https://github.com/angular/material2/pull/9775
28+
'MatCalendar',
29+
];

‎src/lib/schematics/update/material/data/method-call-checks.ts‎

Lines changed: 0 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -20,38 +20,6 @@ export interface MaterialMethodCallData {
2020

2121
export const methodCallChecks: VersionChanges<MaterialMethodCallData> = {
2222
[TargetVersion.V6]: [
23-
{
24-
pr: 'https://github.com/angular/material2/pull/9190',
25-
changes: [
26-
{
27-
className: 'NativeDateAdapter',
28-
method: 'constructor',
29-
invalidArgCounts: [
30-
{
31-
count: 1,
32-
message: '"g{{platform}}" is now required as a second argument'
33-
}
34-
]
35-
}
36-
]
37-
},
38-
39-
{
40-
pr: 'https://github.com/angular/material2/pull/10319',
41-
changes: [
42-
{
43-
className: 'MatAutocomplete',
44-
method: 'constructor',
45-
invalidArgCounts: [
46-
{
47-
count: 2,
48-
message: '"g{{defaults}}" is now required as a third argument'
49-
}
50-
]
51-
}
52-
]
53-
},
54-
5523
{
5624
pr: 'https://github.com/angular/material2/pull/10325',
5725
changes: [
@@ -66,59 +34,6 @@ export const methodCallChecks: VersionChanges<MaterialMethodCallData> = {
6634
]
6735
}
6836
]
69-
},
70-
71-
{
72-
pr: 'https://github.com/angular/material2/pull/10344',
73-
changes: [
74-
{
75-
className: 'MatTooltip',
76-
method: 'constructor',
77-
invalidArgCounts: [
78-
{
79-
count: 11,
80-
message: '"g{{_defaultOptions}}" is now required as a twelfth argument'
81-
}
82-
]
83-
}
84-
]
85-
},
86-
87-
{
88-
pr: 'https://github.com/angular/material2/pull/10389',
89-
changes: [
90-
{
91-
className: 'MatIconRegistry',
92-
method: 'constructor',
93-
invalidArgCounts: [
94-
{
95-
count: 2,
96-
message: '"g{{document}}" is now required as a third argument'
97-
}
98-
]
99-
}
100-
]
101-
},
102-
103-
{
104-
pr: 'https://github.com/angular/material2/pull/9775',
105-
changes: [
106-
{
107-
className: 'MatCalendar',
108-
method: 'constructor',
109-
invalidArgCounts: [
110-
{
111-
count: 6,
112-
message: '"r{{_elementRef}}" and "r{{_ngZone}}" arguments have been removed'
113-
},
114-
{
115-
count: 7,
116-
message: '"r{{_elementRef}}", "r{{_ngZone}}", and "r{{_dir}}" arguments have been ' +
117-
'removed'
118-
}
119-
]
120-
}
121-
]
12237
}
12338
]
12439
};

‎src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {bold, green} from 'chalk';
10+
import {ProgramAwareRuleWalker, RuleFailure, Rules} from 'tslint';
11+
import * as ts from 'typescript';
12+
import {constructorChecks} from '../../material/data/constructor-checks';
13+
14+
/**
15+
* Rule that visits every TypeScript new expression or super call and checks if the parameter
16+
* type signature is invalid and needs to be updated manually.
17+
*/
18+
export class Rule extends Rules.TypedRule {
19+
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
20+
return this.applyWithWalker(new Walker(sourceFile, this.getOptions(), program));
21+
}
22+
}
23+
24+
export class Walker extends ProgramAwareRuleWalker {
25+
26+
visitNewExpression(node: ts.NewExpression) {
27+
this.checkExpressionSignature(node);
28+
super.visitNewExpression(node);
29+
}
30+
31+
visitCallExpression(node: ts.CallExpression) {
32+
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
33+
this.checkExpressionSignature(node);
34+
}
35+
36+
return super.visitCallExpression(node);
37+
}
38+
39+
private getParameterTypesFromSignature(signature: ts.Signature): ts.Type[] {
40+
return signature.getParameters()
41+
.map(param => param.declarations[0] as ts.ParameterDeclaration)
42+
.map(node => node.type)
43+
.map(node => this.getTypeChecker().getTypeFromTypeNode(node));
44+
}
45+
46+
private checkExpressionSignature(node: ts.CallExpression | ts.NewExpression) {
47+
const classType = this.getTypeChecker().getTypeAtLocation(node.expression);
48+
const className = classType.symbol && classType.symbol.name;
49+
const isNewExpression = ts.isNewExpression(node);
50+
51+
// TODO(devversion): Consider handling pass-through classes better.
52+
// TODO(devversion): e.g. `export class CustomCalendar extends MatCalendar {}`
53+
if (!classType || !constructorChecks.includes(className)) {
54+
return;
55+
}
56+
57+
const callExpressionSignature = node.arguments
58+
.map(argument => this.getTypeChecker().getTypeAtLocation(argument));
59+
const classSignatures = classType.getConstructSignatures()
60+
.map(signature => this.getParameterTypesFromSignature(signature));
61+
62+
// TODO(devversion): we should check if the type is assignable to the signature
63+
// TODO(devversion): blocked on https://github.com/Microsoft/TypeScript/issues/9879
64+
const doesMatchSignature = classSignatures.some(signature => {
65+
return signature.every((type, index) => callExpressionSignature[index] === type) &&
66+
signature.length === callExpressionSignature.length;
67+
});
68+
69+
if (!doesMatchSignature) {
70+
const expressionName = isNewExpression ? `new ${className}` : 'super';
71+
const signatures = classSignatures
72+
.map(signature => signature.map(t => this.getTypeChecker().typeToString(t)))
73+
.map(signature => `${expressionName}(${signature.join(', ')})`)
74+
.join(' or ');
75+
76+
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
77+
`an invalid signature. Please manually update the ${bold(expressionName)} expression to ` +
78+
`match the new signature${classSignatures.length > 1 ? 's' : ''}: ${green(signatures)}`);
79+
}
80+
}
81+
}

src/lib/schematics/update/rules/method-calls/methodCallsCheckRule.ts renamed to src/lib/schematics/update/rules/signature-check/methodCallsCheckRule.ts

Lines changed: 2 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ import {methodCallChecks} from '../../material/data/method-call-checks';
1414
import {getChangesForTarget} from '../../material/transform-change-data';
1515

1616
/**
17-
* Rule that visits every TypeScript call expression or TypeScript new expression and checks
18-
* if the argument count is invalid and needs to be *manually* updated.
17+
* Rule that visits every TypeScript method call expression and checks if the argument count
18+
* is invalid and needs to be *manually* updated.
1919
*/
2020
export class Rule extends Rules.TypedRule {
2121
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
@@ -28,24 +28,7 @@ export class Walker extends ProgramAwareRuleWalker {
2828
/** Change data that upgrades to the specified target version. */
2929
data = getChangesForTarget(this.getOptions()[0], methodCallChecks);
3030

31-
visitNewExpression(expression: ts.NewExpression) {
32-
const classType = this.getTypeChecker().getTypeAtLocation(expression);
33-
34-
if (classType && classType.symbol) {
35-
this.checkConstructor(expression, classType.symbol.name);
36-
}
37-
}
38-
3931
visitCallExpression(node: ts.CallExpression) {
40-
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
41-
const superClassType = this.getTypeChecker().getTypeAtLocation(node.expression);
42-
const superClassName = superClassType.symbol && superClassType.symbol.name;
43-
44-
if (superClassName) {
45-
this.checkConstructor(node, superClassName);
46-
}
47-
}
48-
4932
if (ts.isPropertyAccessExpression(node.expression)) {
5033
this._checkPropertyAccessMethodCall(node);
5134
}
@@ -79,18 +62,4 @@ export class Walker extends ProgramAwareRuleWalker {
7962
this.addFailureAtNode(node, `Found call to "${bold(hostTypeName + '.' + methodName)}" ` +
8063
`with ${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
8164
}
82-
83-
private checkConstructor(node: ts.NewExpression | ts.CallExpression, className: string) {
84-
const argumentsLength = node.arguments ? node.arguments.length : 0;
85-
const failure = this.data
86-
.filter(data => data.method === 'constructor' && data.className === className)
87-
.map(data => data.invalidArgCounts.find(f => f.count === argumentsLength))[0];
88-
89-
if (!failure) {
90-
return;
91-
}
92-
93-
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
94-
`${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
95-
}
9665
}

‎src/lib/schematics/update/test-cases/index.spec.ts‎

Lines changed: 43 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,77 +1,48 @@
1-
import {getSystemPath, normalize, virtualFs} from '@angular-devkit/core';
1+
import {getSystemPath, normalize} from '@angular-devkit/core';
22
import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
3-
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
3+
import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
44
import {readFileSync} from 'fs';
5-
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
6-
import {createTestApp, migrationCollection} from '../../test-setup/test-app';
7-
8-
describe('test cases', () => {
9-
10-
// Module name suffix for data files of the `jasmine_node_test` Bazel rule.
11-
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
12-
13-
/**
14-
* Name of test cases that will be used to verify that update schematics properly update
15-
* a developers application.
16-
*/
17-
const testCases = [
18-
'v5/attribute-selectors',
19-
];
20-
21-
// Iterates through every test case directory and generates a jasmine test block that will
22-
// verify that the update schematics properly update the test input to the expected output.
23-
testCases.forEach(testCaseName => {
24-
25-
// Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
26-
// that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
27-
// resolve function and maps the module paths to the original file location. Since we
28-
// need to load the content of those test cases, we need to resolve the original file path.
29-
const inputPath = require.resolve(`${bazelModuleSuffix}/${testCaseName}_input.ts`);
30-
const expectedOutputPath = require
31-
.resolve(`${bazelModuleSuffix}/${testCaseName}_expected_output.ts`);
32-
33-
it(`should apply update schematics to test case: ${testCaseName}`, () => {
34-
const runner = new SchematicTestRunner('schematics', migrationCollection);
35-
36-
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
37-
38-
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
39-
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
40-
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
41-
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
42-
expect(readFileContent('projects/material/src/main.ts'))
43-
.toBe(readFileContent(expectedOutputPath));
44-
});
45-
});
5+
import {createTestApp} from '../../test-setup/test-app';
6+
7+
/** Module name suffix for data files of the `jasmine_node_test` Bazel rule. */
8+
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
9+
10+
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
11+
export function readFileContent(filePath: string): string {
12+
return readFileSync(filePath, 'utf8');
13+
}
14+
15+
/**
16+
* Resolves the original file path of the specified file that has been to the `data` of the
17+
* jasmine_node_test Bazel rule.
18+
*
19+
* Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
20+
* that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
21+
* resolve function and maps the module paths to the original file location.
22+
*/
23+
export function resolveBazelDataFile(filePath: string) {
24+
return require.resolve(`${bazelModuleSuffix}/${filePath}`);
25+
}
26+
27+
/**
28+
* Creates a test app schematic tree that includes the specified test case as TypeScript
29+
* entry point file. Also writes the app tree to a real file system location in order to be
30+
* able to test the tslint fix rules.
31+
*/
32+
export function createTestAppWithTestCase(testCaseInputPath: string) {
33+
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
34+
const appTree = createTestApp();
35+
36+
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
37+
38+
// Since the TSLint fix task expects all files to be present on the real file system, we
39+
// map every file in the app tree to a temporary location on the file system.
40+
appTree.files.map(f => normalize(f)).forEach(f => {
41+
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
4642
});
4743

48-
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
49-
function readFileContent(filePath: string): string {
50-
return readFileSync(filePath, 'utf8');
51-
}
52-
53-
/**
54-
* Creates a test app schematic tree that includes the specified test case as TypeScript
55-
* entry point file. Also writes the app tree to a real file system location in order to be
56-
* able to test the tslint fix rules.
57-
*/
58-
function createTestAppWithTestCase(testCaseInputPath: string) {
59-
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
60-
const appTree = createTestApp();
61-
62-
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
63-
64-
// Since the TSLint fix task expects all files to be present on the real file system, we
65-
// map every file in the app tree to a temporary location on the file system.
66-
appTree.files.map(f => normalize(f)).forEach(f => {
67-
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
68-
});
69-
70-
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
71-
process.chdir(getSystemPath(tempFileSystemHost.root));
72-
73-
return appTree;
74-
}
75-
});
76-
44+
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
45+
process.chdir(getSystemPath(tempFileSystemHost.root));
7746

47+
return appTree;
48+
}

0 commit comments

Comments
 (0)