diff --git a/src/lib/schematics/update/material/data/constructor-checks.ts b/src/lib/schematics/update/material/data/constructor-checks.ts new file mode 100644 index 000000000000..ac2f70e745bb --- /dev/null +++ b/src/lib/schematics/update/material/data/constructor-checks.ts @@ -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', +]; diff --git a/src/lib/schematics/update/material/data/method-call-checks.ts b/src/lib/schematics/update/material/data/method-call-checks.ts index d7b9a8aa885e..1c1b1e70d181 100644 --- a/src/lib/schematics/update/material/data/method-call-checks.ts +++ b/src/lib/schematics/update/material/data/method-call-checks.ts @@ -20,38 +20,6 @@ export interface MaterialMethodCallData { export const methodCallChecks: VersionChanges = { [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 = { ] } ] - }, - - { - 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' - } - ] - } - ] } ] }; diff --git a/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts b/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts new file mode 100644 index 000000000000..4ef1a8348cb9 --- /dev/null +++ b/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts @@ -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)}`); + } + } +} diff --git a/src/lib/schematics/update/rules/method-calls/methodCallsCheckRule.ts b/src/lib/schematics/update/rules/signature-check/methodCallsCheckRule.ts similarity index 60% rename from src/lib/schematics/update/rules/method-calls/methodCallsCheckRule.ts rename to src/lib/schematics/update/rules/signature-check/methodCallsCheckRule.ts index abe0c3b2d6e0..75dc92a9dd5e 100644 --- a/src/lib/schematics/update/rules/method-calls/methodCallsCheckRule.ts +++ b/src/lib/schematics/update/rules/signature-check/methodCallsCheckRule.ts @@ -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)}`); - } } diff --git a/src/lib/schematics/update/test-cases/index.spec.ts b/src/lib/schematics/update/test-cases/index.spec.ts index 7e75496077b1..1ac26af6cee2 100644 --- a/src/lib/schematics/update/test-cases/index.spec.ts +++ b/src/lib/schematics/update/test-cases/index.spec.ts @@ -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'; - -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'; - - /** - * Name of test cases that will be used to verify that update schematics properly update - * a developers application. - */ - const testCases = [ - 'v5/attribute-selectors', - ]; - - // 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 => { - - // 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`); - - 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)); - }); - }); +import {createTestApp} from '../../test-setup/test-app'; + +/** 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'); +} + +/** + * 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}`); +} + +/** + * 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(); + + 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))); }); - /** 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; +} diff --git a/src/lib/schematics/update/test-cases/v5-test-cases.spec.ts b/src/lib/schematics/update/test-cases/v5-test-cases.spec.ts new file mode 100644 index 000000000000..26c1a0becbf8 --- /dev/null +++ b/src/lib/schematics/update/test-cases/v5-test-cases.spec.ts @@ -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)); + }); + }); + }); +}); + + diff --git a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts b/src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts new file mode 100644 index 000000000000..26ea4e939322 --- /dev/null +++ b/src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts @@ -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\)/); + }); +}); + + diff --git a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts b/src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts new file mode 100644 index 000000000000..b58c2bb9e75a --- /dev/null +++ b/src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts @@ -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({}, {}, {}, {}, {}, {}, {}); diff --git a/src/lib/schematics/update/tslint-update.ts b/src/lib/schematics/update/tslint-update.ts index fd52b26ea463..d25883a98990 100644 --- a/src/lib/schematics/update/tslint-update.ts +++ b/src/lib/schematics/update/tslint-update.ts @@ -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. */