Skip to content

Commit e124418

Browse files
devversionjelbourn
authored andcommittedSep 5, 2018
refactor(ng-update): better check for updated constructor signatures (#12970)
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 86be1dc commit e124418

File tree

9 files changed

+329
-194
lines changed

9 files changed

+329
-194
lines changed
 
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+
* automatically through type checking.
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+
];

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

-85
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
};
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+
}

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

+2-33
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
}

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

+43-72
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+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
2+
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
3+
import {migrationCollection} from '../../test-setup/test-app';
4+
import {createTestAppWithTestCase, readFileContent, resolveBazelDataFile} from './index.spec';
5+
6+
describe('v5 test cases', () => {
7+
8+
/**
9+
* Name of test cases that will be used to verify that update schematics properly update
10+
* a developers application.
11+
*/
12+
const testCases = [
13+
'v5/attribute-selectors',
14+
'v5/class-names',
15+
'v5/css-names',
16+
'v5/element-selectors',
17+
'v5/input-names',
18+
'v5/output-names',
19+
'v5/property-names',
20+
];
21+
22+
// Iterates through every test case directory and generates a jasmine test block that will
23+
// verify that the update schematics properly update the test input to the expected output.
24+
testCases.forEach(testCaseName => {
25+
const inputPath = resolveBazelDataFile(`${testCaseName}_input.ts`);
26+
const expectedOutputPath = resolveBazelDataFile(`${testCaseName}_expected_output.ts`);
27+
28+
it(`should apply update schematics to test case: ${testCaseName}`, () => {
29+
const runner = new SchematicTestRunner('schematics', migrationCollection);
30+
31+
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
32+
33+
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
34+
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
35+
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
36+
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
37+
expect(readFileContent('projects/material/src/main.ts'))
38+
.toBe(readFileContent(expectedOutputPath));
39+
});
40+
});
41+
});
42+
});
43+
44+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
2+
import {runPostScheduledTasks} from '../../../../test-setup/post-scheduled-tasks';
3+
import {migrationCollection} from '../../../../test-setup/test-app';
4+
import {createTestAppWithTestCase, resolveBazelDataFile} from '../../index.spec';
5+
6+
describe('v5 constructor checks', () => {
7+
8+
it('should properly report invalid constructor expression signatures', async () => {
9+
const inputPath = resolveBazelDataFile(`v5/checks/constructor-checks_input.ts`);
10+
const runner = new SchematicTestRunner('schematics', migrationCollection);
11+
12+
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
13+
14+
let output = '';
15+
runner.logger.subscribe(entry => output += entry.message);
16+
17+
await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
18+
19+
expect(output).toMatch(/Found "NativeDateAdapter".*super.*: super\(string, Platform\)/);
20+
expect(output).toMatch(/Found "NativeDateAdapter".*: new \w+\(string, Platform\)/);
21+
22+
expect(output).toMatch(/Found "MatAutocomplete".*super.*: super\(any, any, string\[]\)/);
23+
expect(output).toMatch(/Found "MatAutocomplete".*: new \w+\(any, any, string\[]\)/);
24+
25+
expect(output).toMatch(/Found "MatTooltip".*super.*: super\((any, ){10}{ opt1: string; }\)/);
26+
expect(output).toMatch(/Found "MatTooltip".*: new \w+\((any, ){10}{ opt1: string; }\)/);
27+
28+
expect(output).toMatch(/Found "MatIconRegistry".*super.*: super\(any, any, Document\)/);
29+
expect(output).toMatch(/Found "MatIconRegistry".*: new \w+\(any, any, Document\)/);
30+
31+
expect(output).toMatch(/Found "MatCalendar".*super.*: super\(any, any, any, any\)/);
32+
expect(output).toMatch(/Found "MatCalendar".*: new \w+\(any, any, any, any\)/);
33+
});
34+
});
35+
36+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Fake definitions because the property name rules can only determine the host type
3+
* properly by using type checking.
4+
*/
5+
6+
class Platform {
7+
IOS: boolean;
8+
}
9+
10+
interface Document {}
11+
12+
class NativeDateAdapter {
13+
constructor(_locale: string, _platform: Platform) {}
14+
}
15+
16+
class MatAutocomplete {
17+
constructor(_changeDetector: any, _elementRef: any, _defaults: string[]) {}
18+
}
19+
20+
class MatTooltip {
21+
constructor(
22+
private _overlay: any,
23+
private _elementRef: any,
24+
private _scrollDispatcher: any,
25+
private _viewContainerRef: any,
26+
private _ngZone: any,
27+
private _platform: any,
28+
private _ariaDescriber: any,
29+
private _focusMonitor: any,
30+
private _scrollStrategy: any,
31+
private _dir: any,
32+
private _defaultOptions: {opt1: string}) {}
33+
}
34+
35+
class MatIconRegistry {
36+
constructor(_httpClient: any, _sanitizer: any, _document: Document) {}
37+
}
38+
39+
class MatCalendar {
40+
constructor(_intl: any, _adapter: any, _formats: any, _changeDetector: any) {}
41+
}
42+
43+
/* Actual test case using the previously defined definitions. */
44+
45+
class A extends NativeDateAdapter {
46+
constructor() {
47+
super('hardCodedLocale');
48+
}
49+
}
50+
51+
const _A = new NativeDateAdapter('myLocale');
52+
53+
class B extends MatAutocomplete {
54+
constructor(cd: any, elementRef: any) {
55+
super(cd, elementRef);
56+
}
57+
}
58+
59+
const _B = new MatAutocomplete({}, {});
60+
61+
class C extends MatTooltip {
62+
constructor(a: any, b: any, c: any, d: any, e: any, f: any, g: any, h: any, i: any, j: any) {
63+
super(a, b, c, d, e, f, g, h, i, j);
64+
}
65+
}
66+
67+
const _C = new MatTooltip({}, {}, {}, {}, {}, {}, {}, {}, {}, {});
68+
69+
class D extends MatIconRegistry {
70+
constructor(httpClient: any, sanitizer: any) {
71+
super(httpClient, sanitizer);
72+
}
73+
}
74+
75+
const _D = new MatIconRegistry({}, {});
76+
77+
class E extends MatCalendar {
78+
constructor(elementRef: any,
79+
intl: any,
80+
zone: any,
81+
adapter: any,
82+
formats: any,
83+
cd: any,
84+
dir: any) {
85+
super(elementRef, intl, zone, adapter, formats, cd, dir);
86+
}
87+
}
88+
89+
const _E = new MatCalendar({}, {}, {}, {}, {}, {}, {});

Diff for: ‎src/lib/schematics/update/tslint-update.ts

+5-4
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,17 @@ const upgradeRules = [
4141
'property-names-access',
4242
'property-names-misc',
4343

44-
// Method call checks
45-
'method-calls-check',
46-
4744
// Class inheritance
4845
'class-inheritance-check',
4946
'class-inheritance-misc',
5047

48+
// signature checks
49+
'constructor-signature-check',
50+
'method-calls-check',
51+
5152
// Additional misc rules.
5253
'check-import-misc',
53-
'check-template-misc'
54+
'check-template-misc',
5455
];
5556

5657
/** List of absolute paths that refer to directories that contain the upgrade rules. */

0 commit comments

Comments
 (0)
Please sign in to comment.