From 61071cc71595f3dc28b9707f17863fcac686bd67 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 9 Sep 2018 15:24:10 +0200 Subject: [PATCH] refactor(schematics): constructor checks should respect assignability Reworks the constructor signature check rule to take advantage of the TypeScript pre-emit diagnostics. This is necessary because there is no public API to check the relation of two types (e.g. checking if the super-call arguments are assignable to the actual constructor parameter signature) This now means that the constructor checks respect type assignability (e.g. it won't throw incorrectly if a developer does something like: ```ts class ExtendedPlatform extends Platform {} new NativeDateAdapter(a, new ExtendedPlatform(...))` ``` --- .../constructorSignatureCheckRule.ts | 130 +++++++++++------- .../constructor-checks.spec.ts | 15 +- .../constructor-checks_input.ts | 12 ++ 3 files changed, 105 insertions(+), 52 deletions(-) rename src/lib/schematics/update/test-cases/{v5/checks => misc}/constructor-checks.spec.ts (70%) rename src/lib/schematics/update/test-cases/{v5/checks => misc}/constructor-checks_input.ts (88%) diff --git a/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts b/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts index 27ca5267668f..1c830b8b2a8a 100644 --- a/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts +++ b/src/lib/schematics/update/rules/signature-check/constructorSignatureCheckRule.ts @@ -7,78 +7,112 @@ */ import {bold, green} from 'chalk'; -import {ProgramAwareRuleWalker, RuleFailure, Rules} from 'tslint'; +import {RuleFailure, Rules, WalkContext} from 'tslint'; import * as ts from 'typescript'; import {constructorChecks} from '../../material/data/constructor-checks'; +/** + * List of diagnostic codes that refer to pre-emit diagnostics which indicate invalid + * new expression or super call signatures. See the list of diagnostics here: + * + * https://github.com/Microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json + */ +const signatureErrorDiagnostics = [ + // Type not assignable error diagnostic. + 2345, + // Constructor argument length invalid diagnostics + 2554, 2555, 2556, 2557, +]; + /** * 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)); + return this.applyWithFunction(sourceFile, visitSourceFile, null, program); } } -export class Walker extends ProgramAwareRuleWalker { +/** + * Function that will be called for each source file of the upgrade project. In order to properly + * determine invalid constructor signatures, we take advantage of the pre-emit diagnostics from + * TypeScript. + * + * By using the diagnostics we can properly respect type assignability because otherwise we + * would need to rely on type equality checking which is too strict. + * See related issue: https://github.com/Microsoft/TypeScript/issues/9879 + */ +function visitSourceFile(context: WalkContext, program: ts.Program) { + const sourceFile = context.sourceFile; + const diagnostics = ts.getPreEmitDiagnostics(program, sourceFile) + .filter(diagnostic => signatureErrorDiagnostics.includes(diagnostic.code)) + .filter(diagnostic => diagnostic.start !== undefined); - visitNewExpression(node: ts.NewExpression) { - this.checkExpressionSignature(node); - super.visitNewExpression(node); - } + for (const diagnostic of diagnostics) { + const node = findConstructorNode(diagnostic, sourceFile); - visitCallExpression(node: ts.CallExpression) { - if (node.expression.kind === ts.SyntaxKind.SuperKeyword) { - this.checkExpressionSignature(node); + if (!node) { + return; } - 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) - // TODO(devversion): handle non resolvable constructor types - .map(typeNode => this.getTypeChecker().getTypeFromTypeNode(typeNode!)); - } - - private checkExpressionSignature(node: ts.CallExpression | ts.NewExpression) { - const classType = this.getTypeChecker().getTypeAtLocation(node.expression); + const classType = program.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) || !node.arguments) { + if (!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 => { - // TODO(devversion): better handling if signature item type is unresolved but assignable - // to everything. - 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)}`); - } + .map(signature => getParameterTypesFromSignature(signature, program)); + + const expressionName = isNewExpression ? `new ${className}` : 'super'; + const signatures = classSignatures + .map(signature => signature.map(t => program.getTypeChecker().typeToString(t))) + .map(signature => `${expressionName}(${signature.join(', ')})`) + .join(' or '); + + context.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)}`); } } + +/** Resolves the type for each parameter in the specified signature. */ +function getParameterTypesFromSignature(signature: ts.Signature, program: ts.Program): ts.Type[] { + return signature.getParameters() + .map(param => param.declarations[0] as ts.ParameterDeclaration) + .map(node => node.type) + .map(typeNode => program.getTypeChecker().getTypeFromTypeNode(typeNode!)); +} + +/** + * Walks through each node of a source file in order to find a new-expression node or super-call + * expression node that is captured by the specified diagnostic. + */ +function findConstructorNode(diagnostic: ts.Diagnostic, sourceFile: ts.SourceFile): + ts.CallExpression | ts.NewExpression | null { + + let resolvedNode: ts.Node | null = null; + + const _visitNode = (node: ts.Node) => { + // Check whether the current node contains the diagnostic. If the node contains the diagnostic, + // walk deeper in order to find all constructor expression nodes. + if (node.getStart() <= diagnostic.start! && node.getEnd() >= diagnostic.start!) { + + if (ts.isNewExpression(node) || + (ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.SuperKeyword)) { + resolvedNode = node; + } + + ts.forEachChild(node, _visitNode); + } + }; + + ts.forEachChild(sourceFile, _visitNode); + + return resolvedNode; +} diff --git a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts b/src/lib/schematics/update/test-cases/misc/constructor-checks.spec.ts similarity index 70% rename from src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts rename to src/lib/schematics/update/test-cases/misc/constructor-checks.spec.ts index 26ea4e939322..9cd1cdb7df60 100644 --- a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts +++ b/src/lib/schematics/update/test-cases/misc/constructor-checks.spec.ts @@ -1,12 +1,12 @@ 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'; +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 inputPath = resolveBazelDataFile(`misc/constructor-checks_input.ts`); const runner = new SchematicTestRunner('schematics', migrationCollection); runner.runSchematic('migration-01', {}, createTestAppWithTestCase(inputPath)); @@ -30,6 +30,13 @@ describe('v5 constructor checks', () => { expect(output).toMatch(/Found "MatCalendar".*super.*: super\(any, any, any, any\)/); expect(output).toMatch(/Found "MatCalendar".*: new \w+\(any, any, any, any\)/); + + expect(output).toMatch(/\[97.*Found "NativeDateAdapter"/, + 'Expected the constructor checks to report if an argument is not assignable.'); + expect(output).not.toMatch(/\[99.*Found "NativeDateAdapter".*/, + 'Expected the constructor to not report if an argument is assignable.'); + + expect(output).not.toMatch(/Found "NonMaterialClass".*: new NonMaterialClass\(\)/); }); }); diff --git a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts b/src/lib/schematics/update/test-cases/misc/constructor-checks_input.ts similarity index 88% rename from src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts rename to src/lib/schematics/update/test-cases/misc/constructor-checks_input.ts index b58c2bb9e75a..2e089f9957bd 100644 --- a/src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts +++ b/src/lib/schematics/update/test-cases/misc/constructor-checks_input.ts @@ -40,6 +40,12 @@ class MatCalendar { constructor(_intl: any, _adapter: any, _formats: any, _changeDetector: any) {} } +class NonMaterialClass {} + +class DelegateClass { + constructor(_adapter: NativeDateAdapter) {} +} + /* Actual test case using the previously defined definitions. */ class A extends NativeDateAdapter { @@ -87,3 +93,9 @@ class E extends MatCalendar { } const _E = new MatCalendar({}, {}, {}, {}, {}, {}, {}); + +const _F = new NativeDateAdapter('b', 'invalid-argument'); + +const _G = new NativeDateAdapter('a', {IOS: true}); + +const _H = new NonMaterialClass('invalid-argument');