mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
refactor(schematics): constructor checks should respect assignability (#13050)
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(...))`
```
This commit is contained in:
committed by
Jeremy Elbourn
parent
7c4fa66d53
commit
c54c89769c
@@ -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<null>, 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));
|
||||
.map(signature => getParameterTypesFromSignature(signature, program));
|
||||
|
||||
// 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;
|
||||
});
|
||||
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 ');
|
||||
|
||||
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)}`);
|
||||
}
|
||||
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;
|
||||
}
|
||||
|
||||
+11
-4
@@ -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\(\)/);
|
||||
});
|
||||
});
|
||||
|
||||
+12
@@ -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');
|
||||
Reference in New Issue
Block a user