refactor(schematics): constructor checks should respect assignability

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<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));
-
-    // 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;
+}

src/lib/schematics/update/test-cases/v5/checks/constructor-checks.spec.ts → 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\(\)/);
   });
 });
 

src/lib/schematics/update/test-cases/v5/checks/constructor-checks_input.ts → 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');