refactor: fix v8 material imports update rule (#15594)

* Fixes that the V8 `updateAngularMaterialImportsRule` does currently not work (due to incorrect resolution of the value declaration source file)
* Fixes that aliased imports are transformed into non-alias imports (e.g. `import {a as b} from ..` turns into `import {a} from ...`)
* Fixes that inline comments within the module specifiers are omitted.
* Adds a test-case that ensures that the rule properly works
* Slightly refactors the test-case setup so that we can modify the test-case temporary dirctory before running the fixers.

src/cdk/schematics/ng-update/test-cases/misc/method-call-checks.spec.ts

@@ -1,12 +1,14 @@
 import {migrationCollection} from '../index.spec';
-import {runTestCases} from '../../../testing';
+import {createTestCaseSetup} from '../../../testing';
 
 describe('v6 method call checks', () => {
 
   it('should properly report invalid method calls', async () => {
-    const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
+    const {runFixers, removeTempDir} = createTestCaseSetup('migration-v6', migrationCollection,
       [require.resolve('./method-call-checks_input.ts')]);
 
+    const {logOutput} = await runFixers();
+
     expect(logOutput)
       .toMatch(/:15.*Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);
     expect(logOutput)

src/cdk/schematics/testing/test-case-setup.ts

@@ -12,7 +12,7 @@ import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
 import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
 import {mkdirpSync, readFileSync, writeFileSync, removeSync} from 'fs-extra';
 import {sync as globSync} from 'glob';
-import {dirname, join, basename, relative, sep} from 'path';
+import {dirname, join, extname, basename, relative, sep} from 'path';
 import {createTestApp, runPostScheduledTasks} from '../testing';
 
 /** Suffix that indicates whether a given file is a test case input. */
@@ -44,7 +44,7 @@ export function createFileSystemTestApp(runner: SchematicTestRunner) {
   return {appTree, tempPath, removeTempDir: () => removeSync(tempPath)};
 }
 
-export async function runTestCases(migrationName: string, collectionPath: string,
+export function createTestCaseSetup(migrationName: string, collectionPath: string,
                                    inputFiles: string[]) {
 
   const runner = new SchematicTestRunner('schematics', collectionPath);
@@ -58,27 +58,31 @@ export async function runTestCases(migrationName: string, collectionPath: string
   // Write each test-case input to the file-system. This is necessary because otherwise
   // TSLint won't be able to pick up the test cases.
   inputFiles.forEach(inputFilePath => {
-    const inputTestName = basename(inputFilePath);
+    const inputTestName = basename(inputFilePath, extname(inputFilePath));
     const tempInputPath = join(tempPath, `projects/cdk-testing/src/test-cases/${inputTestName}.ts`);
 
     mkdirpSync(dirname(tempInputPath));
     writeFileSync(tempInputPath, readFileContent(inputFilePath));
   });
 
-  runner.runSchematic(migrationName, {}, appTree);
+  const runFixers = async function() {
+    runner.runSchematic(migrationName, {}, appTree);
 
-  // Switch to the new temporary directory because otherwise TSLint cannot read the files.
-  process.chdir(tempPath);
+    // Switch to the new temporary directory because otherwise TSLint cannot read the files.
+    process.chdir(tempPath);
 
-  // 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.
-  await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
+    // 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.
+    await runPostScheduledTasks(runner, 'tslint-fix').toPromise();
 
-  // Switch back to the initial working directory.
-  process.chdir(initialWorkingDir);
+    // Switch back to the initial working directory.
+    process.chdir(initialWorkingDir);
 
-  return {tempPath, logOutput, removeTempDir};
+    return {logOutput};
+  };
+
+  return {tempPath, removeTempDir, runFixers};
 }
 
 /**
@@ -144,8 +148,10 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
   let cleanupTestApp: () => void;
 
   beforeAll(async () => {
-    const {tempPath, removeTempDir} =
-      await runTestCases(`migration-${versionName}`, collectionFile, inputFiles);
+    const {tempPath, runFixers, removeTempDir} =
+      createTestCaseSetup(`migration-${versionName}`, collectionFile, inputFiles);
+
+    await runFixers();
 
     testCasesOutputPath = join(tempPath, 'projects/cdk-testing/src/test-cases/');
     cleanupTestApp = removeTempDir;
@@ -156,7 +162,7 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
   // Iterates through every test case directory and generates a jasmine test block that will
   // verify that the update schematics properly updated the test input to the expected output.
   inputFiles.forEach(inputFile => {
-    const inputTestName = basename(inputFile);
+    const inputTestName = basename(inputFile, extname(inputFile));
 
     it(`should apply update schematics to test case: ${inputTestName}`, () => {
       expect(readFileContent(join(testCasesOutputPath, `${inputTestName}.ts`)))

src/lib/schematics/BUILD.bazel

@@ -55,8 +55,10 @@ ts_library(
     "//src/cdk/schematics/testing",
     "@npm//@schematics/angular",
     "@npm//@angular-devkit/schematics",
-    "@npm//@types/jasmine",
+    "@npm//@types/fs-extra",
     "@npm//@types/node",
+    "@npm//@types/jasmine",
+    "@npm//fs-extra",
   ],
   tsconfig = ":tsconfig.json",
   testonly = True,

src/lib/schematics/ng-update/test-cases/misc/constructor-checks.spec.ts

@@ -1,11 +1,13 @@
 import {migrationCollection} from '../index.spec';
-import {runTestCases} from '@angular/cdk/schematics/testing';
+import {createTestCaseSetup} from '@angular/cdk/schematics/testing';
 
 describe('constructor checks', () => {
 
   it('should properly report invalid constructor expression signatures', async () => {
-    const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
-      [require.resolve('./constructor-checks_input.ts')]);
+    const {removeTempDir, runFixers} = createTestCaseSetup('migration-v6',
+      migrationCollection, [require.resolve('./constructor-checks_input.ts')]);
+
+    const {logOutput} = await runFixers();
 
     expect(logOutput).toMatch(/:22.*Found "NativeDateAdapter"/,
       'Expected the constructor checks to report if an argument is not assignable.');

src/lib/schematics/ng-update/test-cases/misc/import-checks.spec.ts

@@ -1,11 +1,13 @@
-import {runTestCases} from '@angular/cdk/schematics/testing';
+import {createTestCaseSetup} from '@angular/cdk/schematics/testing';
 import {migrationCollection} from '../index.spec';
 
 describe('v6 import misc checks', () => {
 
   it('should report imports for deleted animation constants', async () => {
-    const {logOutput, removeTempDir} = await runTestCases('migration-v6', migrationCollection,
-      [require.resolve('./import-checks_input.ts')]);
+    const {removeTempDir, runFixers} = createTestCaseSetup('migration-v6',
+      migrationCollection, [require.resolve('./import-checks_input.ts')]);
+
+    const {logOutput} = await runFixers();
 
     expect(logOutput).toMatch(/Found deprecated symbol "SHOW_ANIMATION"/);
     expect(logOutput).toMatch(/Found deprecated symbol "HIDE_ANIMATION"/);

src/lib/schematics/ng-update/test-cases/misc/material-imports.spec.ts

@@ -0,0 +1,42 @@
+import {createTestCaseSetup, readFileContent} from '@angular/cdk/schematics/testing';
+import {migrationCollection} from '../index.spec';
+import {writeFileSync, mkdirpSync} from 'fs-extra';
+import {join} from 'path';
+
+describe('v8 material imports', () => {
+
+  function writeSecondaryEntryPoint(materialPath: string, name: string) {
+    const entryPointPath = join(materialPath, name);
+    mkdirpSync(entryPointPath);
+    writeFileSync(join(entryPointPath, 'index.d.ts'), `export const ${name} = '';`);
+  }
+
+  it('should report imports for deleted animation constants', async () => {
+    const {runFixers, removeTempDir, tempPath} = createTestCaseSetup(
+      'migration-v8', migrationCollection, [require.resolve('./material-imports_input.ts')]);
+    const materialPath = join(tempPath, 'node_modules/@angular/material');
+
+    mkdirpSync(materialPath);
+    writeFileSync(join(materialPath, 'index.d.ts'), `
+      export * from './a';
+      export * from './b';
+      export * from './c';
+    `);
+
+    writeSecondaryEntryPoint(materialPath, 'a');
+    writeSecondaryEntryPoint(materialPath, 'b');
+    writeSecondaryEntryPoint(materialPath, 'c');
+
+    await runFixers();
+
+    const outputPath = join(tempPath,
+      'projects/cdk-testing/src/test-cases/material-imports_input.ts');
+
+    expect(readFileContent(outputPath)).toBe(
+      readFileContent(require.resolve('./material-imports_expected_output.ts')));
+
+    removeTempDir();
+  });
+});
+
+

src/lib/schematics/ng-update/test-cases/misc/material-imports_expected_output.ts

@@ -0,0 +1,12 @@
+import { c as c2 } from "@angular/material/c";
+import { a } from "@angular/material/a";
+import { b } from "@angular/material/b";
+import { c } from "@angular/material/c";
+import { a as a2 } from "@angular/material/a";
+
+// unsorted
+import { a } from "@angular/material/a";
+import { b } from "@angular/material/b";
+import { c } from "@angular/material/c";
+
+import { /* comment */ a as a3 } from "@angular/material/a";

src/lib/schematics/ng-update/test-cases/misc/material-imports_input.ts

@@ -0,0 +1,8 @@
+import {c as c2} from '@angular/material';
+import {a, b, c} from '@angular/material';
+import {a as a2} from '@angular/material';
+
+// unsorted
+import {c, b, a} from '@angular/material';
+
+import {/* comment */ a as a3} from '@angular/material';

src/lib/schematics/ng-update/upgrade-rules/package-imports-v8/updateAngularMaterialImportsRule.ts

@@ -11,7 +11,7 @@ import * as ts from 'typescript';
 import {materialModuleSpecifier} from '../../../ng-update/typescript/module-specifiers';
 
 /**
- * Regex for testing file paths against to determinte if the file is from the
+ * Regex for testing file paths against to determine if the file is from the
  * Angular Material library.
  */
 const ANGULAR_MATERIAL_FILEPATH_REGEX = new RegExp(`${materialModuleSpecifier}/(.*?)/`);
@@ -51,17 +51,20 @@ export class Rule extends Lint.Rules.TypedRule {
 
 /**
  * A walker to walk a given source file to check for imports from the
- * @angular/material module.
+ * "@angular/material" module.
  */
 function walk(ctx: Lint.WalkContext<boolean>, checker: ts.TypeChecker): void {
   // The source file to walk.
   const sf = ctx.sourceFile;
+  const printer = ts.createPrinter();
   const cb = (declaration: ts.Node) => {
     // Only look at import declarations.
-    if (!ts.isImportDeclaration(declaration)) {
+    if (!ts.isImportDeclaration(declaration) ||
+        !ts.isStringLiteralLike(declaration.moduleSpecifier)) {
       return;
     }
-    const importLocation = declaration.moduleSpecifier.getText(sf);
+
+    const importLocation = declaration.moduleSpecifier.text;
     // If the import module is not @angular/material, skip check.
     if (importLocation !== materialModuleSpecifier) {
       return;
@@ -85,86 +88,95 @@ function walk(ctx: Lint.WalkContext<boolean>, checker: ts.TypeChecker): void {
       return ctx.addFailureAtNode(declaration, Rule.NO_IMPORT_NAMED_SYMBOLS_FAILURE_STR);
     }
 
-    // Map of submodule locations to arrays of imported symbols.
-    const importMap = new Map<string, Set<string>>();
+    // Map which consists of secondary entry-points and import specifiers which are used
+    // within the current import declaration.
+    const importMap = new Map<string, ts.ImportSpecifier[]>();
 
     // Determine the subpackage each symbol in the namedBinding comes from.
     for (const element of declaration.importClause.namedBindings.elements) {
-      // Confirm the named import is a symbol that can be looked up.
+      // Ensure the import specifier name is statically analyzable.
       if (!ts.isIdentifier(element.name)) {
-        return ctx.addFailureAtNode(
-            element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
+        return ctx.addFailureAtNode(element, element.getText() + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
       }
-      // Get the type for the named binding element.
-      const type = checker.getTypeAtLocation(element.name);
-      // Get the original symbol where it is declared upstream.
-      const symbol = type.getSymbol();
+
+      // Get the symbol for the named binding element. Note that we cannot determine the
+      // value declaration based on the type of the element as types are not necessarily
+      // specific to a given secondary entry-point (e.g. exports with the type of "string")
+      // would resolve to the module types provided by TypeScript itself.
+      const symbol = getDeclarationSymbolOfNode(element.name, checker);
 
       // If the symbol can't be found, add failure saying the symbol
       // can't be found.
       if (!symbol || !symbol.valueDeclaration) {
-        return ctx.addFailureAtNode(
-            element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
-      }
-
-      // If the symbol has no declarations, add failure saying the symbol can't
-      // be found.
-      if (!symbol.declarations || !symbol.declarations.length) {
-        return ctx.addFailureAtNode(
-            element, element.getFullText(sf) + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
+        return ctx.addFailureAtNode(element, element.getText() + Rule.SYMBOL_NOT_FOUND_FAILURE_STR);
       }
 
       // The filename for the source file of the node that contains the
-      // first declaration of the symbol.  All symbol declarations must be
+      // first declaration of the symbol. All symbol declarations must be
       // part of a defining node, so parent can be asserted to be defined.
-      const sourceFile = symbol.valueDeclaration.getSourceFile().fileName;
+      const sourceFile: string = symbol.valueDeclaration.getSourceFile().fileName;
+
       // File the module the symbol belongs to from a regex match of the
-      // filename. This will always match since only @angular/material symbols
-      // are being looked at.
-      const [, moduleName] = sourceFile.match(ANGULAR_MATERIAL_FILEPATH_REGEX) || [] as undefined[];
-      if (!moduleName) {
+      // filename. This will always match since only "@angular/material"
+      // elements are analyzed.
+      const matches = sourceFile.match(ANGULAR_MATERIAL_FILEPATH_REGEX);
+      if (!matches) {
         return ctx.addFailureAtNode(
-            element, element.getFullText(sf) + Rule.SYMBOL_FILE_NOT_FOUND_FAILURE_STR);
+            element, element.getText() + Rule.SYMBOL_FILE_NOT_FOUND_FAILURE_STR);
       }
-      // The module name where the symbol is defined e.g. card, dialog.  The
+      const [, moduleName] = matches;
+
+      // The module name where the symbol is defined e.g. card, dialog. The
       // first capture group is contains the module name.
       if (importMap.has(moduleName)) {
-        importMap.get(moduleName)!.add(symbol.getName());
+        importMap.get(moduleName)!.push(element);
       } else {
-        importMap.set(moduleName, new Set([symbol.getName()]));
+        importMap.set(moduleName, [element]);
       }
     }
-    const fix = buildSecondaryImportStatements(importMap);
 
-    // Without a fix to provide, show error message only.
-    if (!fix) {
+    // Transforms the import declaration into multiple import declarations that import
+    // the given symbols from the individual secondary entry-points. For example:
+    // import {MatCardModule, MatCardTitle} from '@angular/material/card';
+    // import {MatRadioModule} from '@angular/material/radio';
+    const newImportStatements =
+        Array.from(importMap.entries())
+            .sort()
+            .map(([name, elements]) => {
+              const newImport = ts.createImportDeclaration(
+                  undefined, undefined,
+                  ts.createImportClause(undefined, ts.createNamedImports(elements)),
+                  ts.createStringLiteral(`${materialModuleSpecifier}/${name}`));
+              return printer.printNode(ts.EmitHint.Unspecified, newImport, sf);
+            })
+            .join('\n');
+
+    // Without any import statements that were generated, we can assume that this was an empty
+    // import declaration. We still want to add a failure in order to make developers aware that
+    // importing from "@angular/material" is deprecated.
+    if (!newImportStatements) {
       return ctx.addFailureAtNode(declaration.moduleSpecifier, Rule.ONLY_SUBPACKAGE_FAILURE_STR);
     }
-    // Mark the lint failure at the module specifier, providing a
-    // recommended fix.
+
+    // Mark the lint failure at the module specifier, providing the replacement that switches
+    // the import to individual secondary entry-point imports.
     ctx.addFailureAtNode(
         declaration.moduleSpecifier, Rule.ONLY_SUBPACKAGE_FAILURE_STR,
-        new Lint.Replacement(declaration.getStart(sf), declaration.getWidth(), fix));
+        new Lint.Replacement(declaration.getStart(), declaration.getWidth(), newImportStatements));
   };
+
   sf.statements.forEach(cb);
 }
 
-/**
- * Builds the recommended fix from a map of the imported symbols found in the
- * import declaration.  Imports declarations are sorted by module, then by
- * symbol within each import declaration.
- *
- * Example of the format:
- *
- * import {MatCardModule, MatCardTitle} from '@angular/material/card';
- * import {MatRadioModule} from '@angular/material/radio';
- */
-function buildSecondaryImportStatements(importMap: Map<string, Set<string>>): string {
-  return Array.from(importMap.entries())
-      .sort((a, b) => a[0] > b[0] ? 1 : -1)
-      .map(entry => {
-        const imports = Array.from(entry[1]).sort((a, b) => a > b ? 1 : -1).join(', ');
-        return `import {${imports}} from '@angular/material/${entry[0]}';\n`;
-      })
-      .join('');
+/** Gets the symbol that contains the value declaration of the given node. */
+function getDeclarationSymbolOfNode(node: ts.Node, checker: ts.TypeChecker) {
+  const symbol = checker.getSymbolAtLocation(node);
+
+  // Symbols can be aliases of the declaration symbol. e.g. in named import specifiers.
+  // We need to resolve the aliased symbol back to the declaration symbol.
+  // tslint:disable-next-line:no-bitwise
+  if (symbol && (symbol.flags & ts.SymbolFlags.Alias) !== 0) {
+    return checker.getAliasedSymbol(symbol);
+  }
+  return symbol;
 }