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` (https://github.com/angular/material2/pull/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).
This commit is contained in:
Paul Gschwendtner
2018-09-06 01:31:24 +02:00
committed by Jeremy Elbourn
parent 86be1dcb19
commit e124418d07
9 changed files with 323 additions and 188 deletions
@@ -0,0 +1,29 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
/**
* List of class names for which the constructor signature has been changed. The new constructor
* signature types don't need to be stored here because the signature will be determined
* automatically through type checking.
*/
export const constructorChecks = [
// https://github.com/angular/material2/pull/9190
'NativeDateAdapter',
// https://github.com/angular/material2/pull/10319
'MatAutocomplete',
// https://github.com/angular/material2/pull/10344
'MatTooltip',
// https://github.com/angular/material2/pull/10389
'MatIconRegistry',
// https://github.com/angular/material2/pull/9775
'MatCalendar',
];
@@ -20,38 +20,6 @@ export interface MaterialMethodCallData {
export const methodCallChecks: VersionChanges<MaterialMethodCallData> = {
[TargetVersion.V6]: [
{
pr: 'https://github.com/angular/material2/pull/9190',
changes: [
{
className: 'NativeDateAdapter',
method: 'constructor',
invalidArgCounts: [
{
count: 1,
message: '"g{{platform}}" is now required as a second argument'
}
]
}
]
},
{
pr: 'https://github.com/angular/material2/pull/10319',
changes: [
{
className: 'MatAutocomplete',
method: 'constructor',
invalidArgCounts: [
{
count: 2,
message: '"g{{defaults}}" is now required as a third argument'
}
]
}
]
},
{
pr: 'https://github.com/angular/material2/pull/10325',
changes: [
@@ -66,59 +34,6 @@ export const methodCallChecks: VersionChanges<MaterialMethodCallData> = {
]
}
]
},
{
pr: 'https://github.com/angular/material2/pull/10344',
changes: [
{
className: 'MatTooltip',
method: 'constructor',
invalidArgCounts: [
{
count: 11,
message: '"g{{_defaultOptions}}" is now required as a twelfth argument'
}
]
}
]
},
{
pr: 'https://github.com/angular/material2/pull/10389',
changes: [
{
className: 'MatIconRegistry',
method: 'constructor',
invalidArgCounts: [
{
count: 2,
message: '"g{{document}}" is now required as a third argument'
}
]
}
]
},
{
pr: 'https://github.com/angular/material2/pull/9775',
changes: [
{
className: 'MatCalendar',
method: 'constructor',
invalidArgCounts: [
{
count: 6,
message: '"r{{_elementRef}}" and "r{{_ngZone}}" arguments have been removed'
},
{
count: 7,
message: '"r{{_elementRef}}", "r{{_ngZone}}", and "r{{_dir}}" arguments have been ' +
'removed'
}
]
}
]
}
]
};
@@ -0,0 +1,81 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {bold, green} from 'chalk';
import {ProgramAwareRuleWalker, RuleFailure, Rules} from 'tslint';
import * as ts from 'typescript';
import {constructorChecks} from '../../material/data/constructor-checks';
/**
* Rule that visits every TypeScript new expression or super call and checks if the parameter
* type signature is invalid and needs to be updated manually.
*/
export class Rule extends Rules.TypedRule {
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
return this.applyWithWalker(new Walker(sourceFile, this.getOptions(), program));
}
}
export class Walker extends ProgramAwareRuleWalker {
visitNewExpression(node: ts.NewExpression) {
this.checkExpressionSignature(node);
super.visitNewExpression(node);
}
visitCallExpression(node: ts.CallExpression) {
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
this.checkExpressionSignature(node);
}
return super.visitCallExpression(node);
}
private getParameterTypesFromSignature(signature: ts.Signature): ts.Type[] {
return signature.getParameters()
.map(param => param.declarations[0] as ts.ParameterDeclaration)
.map(node => node.type)
.map(node => this.getTypeChecker().getTypeFromTypeNode(node));
}
private checkExpressionSignature(node: ts.CallExpression | ts.NewExpression) {
const classType = this.getTypeChecker().getTypeAtLocation(node.expression);
const className = classType.symbol && classType.symbol.name;
const isNewExpression = ts.isNewExpression(node);
// TODO(devversion): Consider handling pass-through classes better.
// TODO(devversion): e.g. `export class CustomCalendar extends MatCalendar {}`
if (!classType || !constructorChecks.includes(className)) {
return;
}
const callExpressionSignature = node.arguments
.map(argument => this.getTypeChecker().getTypeAtLocation(argument));
const classSignatures = classType.getConstructSignatures()
.map(signature => this.getParameterTypesFromSignature(signature));
// TODO(devversion): we should check if the type is assignable to the signature
// TODO(devversion): blocked on https://github.com/Microsoft/TypeScript/issues/9879
const doesMatchSignature = classSignatures.some(signature => {
return signature.every((type, index) => callExpressionSignature[index] === type) &&
signature.length === callExpressionSignature.length;
});
if (!doesMatchSignature) {
const expressionName = isNewExpression ? `new ${className}` : 'super';
const signatures = classSignatures
.map(signature => signature.map(t => this.getTypeChecker().typeToString(t)))
.map(signature => `${expressionName}(${signature.join(', ')})`)
.join(' or ');
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
`an invalid signature. Please manually update the ${bold(expressionName)} expression to ` +
`match the new signature${classSignatures.length > 1 ? 's' : ''}: ${green(signatures)}`);
}
}
}
@@ -14,8 +14,8 @@ import {methodCallChecks} from '../../material/data/method-call-checks';
import {getChangesForTarget} from '../../material/transform-change-data';
/**
* Rule that visits every TypeScript call expression or TypeScript new expression and checks
* if the argument count is invalid and needs to be *manually* updated.
* Rule that visits every TypeScript method call expression and checks if the argument count
* is invalid and needs to be *manually* updated.
*/
export class Rule extends Rules.TypedRule {
applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
@@ -28,24 +28,7 @@ export class Walker extends ProgramAwareRuleWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], methodCallChecks);
visitNewExpression(expression: ts.NewExpression) {
const classType = this.getTypeChecker().getTypeAtLocation(expression);
if (classType && classType.symbol) {
this.checkConstructor(expression, classType.symbol.name);
}
}
visitCallExpression(node: ts.CallExpression) {
if (node.expression.kind === ts.SyntaxKind.SuperKeyword) {
const superClassType = this.getTypeChecker().getTypeAtLocation(node.expression);
const superClassName = superClassType.symbol && superClassType.symbol.name;
if (superClassName) {
this.checkConstructor(node, superClassName);
}
}
if (ts.isPropertyAccessExpression(node.expression)) {
this._checkPropertyAccessMethodCall(node);
}
@@ -79,18 +62,4 @@ export class Walker extends ProgramAwareRuleWalker {
this.addFailureAtNode(node, `Found call to "${bold(hostTypeName + '.' + methodName)}" ` +
`with ${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
}
private checkConstructor(node: ts.NewExpression | ts.CallExpression, className: string) {
const argumentsLength = node.arguments ? node.arguments.length : 0;
const failure = this.data
.filter(data => data.method === 'constructor' && data.className === className)
.map(data => data.invalidArgCounts.find(f => f.count === argumentsLength))[0];
if (!failure) {
return;
}
this.addFailureAtNode(node, `Found "${bold(className)}" constructed with ` +
`${bold(`${failure.count}`)} arguments. Message: ${color(failure.message)}`);
}
}
@@ -1,77 +1,48 @@
import {getSystemPath, normalize, virtualFs} from '@angular-devkit/core';
import {getSystemPath, normalize} from '@angular-devkit/core';
import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
import {readFileSync} from 'fs';
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
import {createTestApp, migrationCollection} from '../../test-setup/test-app';
import {createTestApp} from '../../test-setup/test-app';
describe('test cases', () => {
/** Module name suffix for data files of the `jasmine_node_test` Bazel rule. */
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
// Module name suffix for data files of the `jasmine_node_test` Bazel rule.
const bazelModuleSuffix = 'angular_material/src/lib/schematics/update/test-cases';
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
export function readFileContent(filePath: string): string {
return readFileSync(filePath, 'utf8');
}
/**
* Name of test cases that will be used to verify that update schematics properly update
* a developers application.
*/
const testCases = [
'v5/attribute-selectors',
];
/**
* Resolves the original file path of the specified file that has been to the `data` of the
* jasmine_node_test Bazel rule.
*
* Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
* that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
* resolve function and maps the module paths to the original file location.
*/
export function resolveBazelDataFile(filePath: string) {
return require.resolve(`${bazelModuleSuffix}/${filePath}`);
}
// Iterates through every test case directory and generates a jasmine test block that will
// verify that the update schematics properly update the test input to the expected output.
testCases.forEach(testCaseName => {
/**
* Creates a test app schematic tree that includes the specified test case as TypeScript
* entry point file. Also writes the app tree to a real file system location in order to be
* able to test the tslint fix rules.
*/
export function createTestAppWithTestCase(testCaseInputPath: string) {
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
const appTree = createTestApp();
// Adding the test case files to the data of the `jasmine_node_test` Bazel rule does not mean
// that the files are being copied over to the Bazel bin output. Bazel just patches the NodeJS
// resolve function and maps the module paths to the original file location. Since we
// need to load the content of those test cases, we need to resolve the original file path.
const inputPath = require.resolve(`${bazelModuleSuffix}/${testCaseName}_input.ts`);
const expectedOutputPath = require
.resolve(`${bazelModuleSuffix}/${testCaseName}_expected_output.ts`);
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
it(`should apply update schematics to test case: ${testCaseName}`, () => {
const runner = new SchematicTestRunner('schematics', migrationCollection);
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
expect(readFileContent('projects/material/src/main.ts'))
.toBe(readFileContent(expectedOutputPath));
});
});
// Since the TSLint fix task expects all files to be present on the real file system, we
// map every file in the app tree to a temporary location on the file system.
appTree.files.map(f => normalize(f)).forEach(f => {
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
});
/** Reads the UTF8 content of the specified file. Normalizes the path and ensures that */
function readFileContent(filePath: string): string {
return readFileSync(filePath, 'utf8');
}
/**
* Creates a test app schematic tree that includes the specified test case as TypeScript
* entry point file. Also writes the app tree to a real file system location in order to be
* able to test the tslint fix rules.
*/
function createTestAppWithTestCase(testCaseInputPath: string) {
const tempFileSystemHost = new TempScopedNodeJsSyncHost();
const appTree = createTestApp();
appTree.overwrite('/projects/material/src/main.ts', readFileContent(testCaseInputPath));
// Since the TSLint fix task expects all files to be present on the real file system, we
// map every file in the app tree to a temporary location on the file system.
appTree.files.map(f => normalize(f)).forEach(f => {
tempFileSystemHost.sync.write(f, virtualFs.stringToFileBuffer(appTree.readContent(f)));
});
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
process.chdir(getSystemPath(tempFileSystemHost.root));
return appTree;
}
});
// Switch to the new temporary directory because otherwise TSLint cannot read the files.
process.chdir(getSystemPath(tempFileSystemHost.root));
return appTree;
}
@@ -0,0 +1,44 @@
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
import {migrationCollection} from '../../test-setup/test-app';
import {createTestAppWithTestCase, readFileContent, resolveBazelDataFile} from './index.spec';
describe('v5 test cases', () => {
/**
* Name of test cases that will be used to verify that update schematics properly update
* a developers application.
*/
const testCases = [
'v5/attribute-selectors',
'v5/class-names',
'v5/css-names',
'v5/element-selectors',
'v5/input-names',
'v5/output-names',
'v5/property-names',
];
// Iterates through every test case directory and generates a jasmine test block that will
// verify that the update schematics properly update the test input to the expected output.
testCases.forEach(testCaseName => {
const inputPath = resolveBazelDataFile(`${testCaseName}_input.ts`);
const expectedOutputPath = resolveBazelDataFile(`${testCaseName}_expected_output.ts`);
it(`should apply update schematics to test case: ${testCaseName}`, () => {
const runner = new SchematicTestRunner('schematics', migrationCollection);
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
expect(readFileContent('projects/material/src/main.ts'))
.toBe(readFileContent(expectedOutputPath));
});
});
});
});
@@ -0,0 +1,36 @@
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
import {runPostScheduledTasks} from '../../../../test-setup/post-scheduled-tasks';
import {migrationCollection} from '../../../../test-setup/test-app';
import {createTestAppWithTestCase, resolveBazelDataFile} from '../../index.spec';
describe('v5 constructor checks', () => {
it('should properly report invalid constructor expression signatures', async () => {
const inputPath = resolveBazelDataFile(`v5/checks/constructor-checks_input.ts`);
const runner = new SchematicTestRunner('schematics', migrationCollection);
runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath));
let output = '';
runner.logger.subscribe(entry => output += entry.message);
await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
expect(output).toMatch(/Found "NativeDateAdapter".*super.*: super\(string, Platform\)/);
expect(output).toMatch(/Found "NativeDateAdapter".*: new \w+\(string, Platform\)/);
expect(output).toMatch(/Found "MatAutocomplete".*super.*: super\(any, any, string\[]\)/);
expect(output).toMatch(/Found "MatAutocomplete".*: new \w+\(any, any, string\[]\)/);
expect(output).toMatch(/Found "MatTooltip".*super.*: super\((any, ){10}{ opt1: string; }\)/);
expect(output).toMatch(/Found "MatTooltip".*: new \w+\((any, ){10}{ opt1: string; }\)/);
expect(output).toMatch(/Found "MatIconRegistry".*super.*: super\(any, any, Document\)/);
expect(output).toMatch(/Found "MatIconRegistry".*: new \w+\(any, any, Document\)/);
expect(output).toMatch(/Found "MatCalendar".*super.*: super\(any, any, any, any\)/);
expect(output).toMatch(/Found "MatCalendar".*: new \w+\(any, any, any, any\)/);
});
});
@@ -0,0 +1,89 @@
/*
* Fake definitions because the property name rules can only determine the host type
* properly by using type checking.
*/
class Platform {
IOS: boolean;
}
interface Document {}
class NativeDateAdapter {
constructor(_locale: string, _platform: Platform) {}
}
class MatAutocomplete {
constructor(_changeDetector: any, _elementRef: any, _defaults: string[]) {}
}
class MatTooltip {
constructor(
private _overlay: any,
private _elementRef: any,
private _scrollDispatcher: any,
private _viewContainerRef: any,
private _ngZone: any,
private _platform: any,
private _ariaDescriber: any,
private _focusMonitor: any,
private _scrollStrategy: any,
private _dir: any,
private _defaultOptions: {opt1: string}) {}
}
class MatIconRegistry {
constructor(_httpClient: any, _sanitizer: any, _document: Document) {}
}
class MatCalendar {
constructor(_intl: any, _adapter: any, _formats: any, _changeDetector: any) {}
}
/* Actual test case using the previously defined definitions. */
class A extends NativeDateAdapter {
constructor() {
super('hardCodedLocale');
}
}
const _A = new NativeDateAdapter('myLocale');
class B extends MatAutocomplete {
constructor(cd: any, elementRef: any) {
super(cd, elementRef);
}
}
const _B = new MatAutocomplete({}, {});
class C extends MatTooltip {
constructor(a: any, b: any, c: any, d: any, e: any, f: any, g: any, h: any, i: any, j: any) {
super(a, b, c, d, e, f, g, h, i, j);
}
}
const _C = new MatTooltip({}, {}, {}, {}, {}, {}, {}, {}, {}, {});
class D extends MatIconRegistry {
constructor(httpClient: any, sanitizer: any) {
super(httpClient, sanitizer);
}
}
const _D = new MatIconRegistry({}, {});
class E extends MatCalendar {
constructor(elementRef: any,
intl: any,
zone: any,
adapter: any,
formats: any,
cd: any,
dir: any) {
super(elementRef, intl, zone, adapter, formats, cd, dir);
}
}
const _E = new MatCalendar({}, {}, {}, {}, {}, {}, {});
+5 -4
View File
@@ -41,16 +41,17 @@ const upgradeRules = [
'property-names-access',
'property-names-misc',
// Method call checks
'method-calls-check',
// Class inheritance
'class-inheritance-check',
'class-inheritance-misc',
// signature checks
'constructor-signature-check',
'method-calls-check',
// Additional misc rules.
'check-import-misc',
'check-template-misc'
'check-template-misc',
];
/** List of absolute paths that refer to directories that contain the upgrade rules. */