diff --git a/packages/core/schematics/migrations/typed-forms/util.ts b/packages/core/schematics/migrations/typed-forms/util.ts index bf3d99777fd..23400490d8f 100644 --- a/packages/core/schematics/migrations/typed-forms/util.ts +++ b/packages/core/schematics/migrations/typed-forms/util.ts @@ -9,14 +9,14 @@ import ts from 'typescript'; import {getImportSpecifier} from '../../utils/typescript/imports'; -import {isReferenceToImport} from '../../utils/typescript/symbol'; export const classes = new Set(['FormArray', 'FormBuilder', 'FormControl', 'FormGroup']); +export const formControl = 'FormControl'; export const untypedPrefix = 'Untyped'; export const forms = '@angular/forms'; export interface MigratableNode { - node: ts.Expression; + node: ts.Node; importName: string; } @@ -50,7 +50,7 @@ export function migrateFile( } } - // For each imported control class, insert the corresponding uptyped import. + // For each imported control class, migrate to the corresponding uptyped import. for (const imp of imports) { const untypedClass = getUntypedVersionOfImportOrName(imp.getText()); if (untypedClass === null) { @@ -64,7 +64,7 @@ export function migrateFile( // class is already present. If present, immediately continue. continue; } - rewrite(imp.getEnd(), 0, `, ${untypedClass}`); + rewrite(imp.getStart(), imp.getWidth(), untypedClass); } } @@ -89,18 +89,52 @@ function getUntypedVersionOfImportOrName(name: string): string|null { function getUsages( sourceFile: ts.SourceFile, typeChecker: ts.TypeChecker, - importSpecifier: ts.ImportSpecifier|null): MigratableNode[] { - if (importSpecifier === null) return []; + importSpecifier: ts.ImportSpecifier): MigratableNode[] { const usages: MigratableNode[] = []; const visitNode = (node: ts.Node) => { - // Look for a `new` expression with no type arguments which references an import we care about: - // `new FormControl()` - if (ts.isNewExpression(node) && !node.typeArguments && - isReferenceToImport(typeChecker, node.expression, importSpecifier)) { - usages.push({node: node.expression, importName: importSpecifier.getText()}); + if (ts.isImportSpecifier(node)) { + // Skip this node and all of its children; imports are a special case. + return; + } + if (ts.isIdentifier(node) && isUsageOfFormsImport(typeChecker, node, importSpecifier)) { + usages.push({node, importName: importSpecifier.getText()}); } ts.forEachChild(node, visitNode); }; ts.forEachChild(sourceFile, visitNode); return usages; } + +function isUsageOfFormsImport( + typeChecker: ts.TypeChecker, node: ts.Identifier, + importSpecifier: ts.ImportSpecifier): boolean { + const symbol = typeChecker.getSymbolAtLocation(node); + + // We check symbol.declarations because we actually care about the name at the declaration site, + // not the usage site. These could be different in the case of overriden constructors. + if (!symbol || symbol.declarations === undefined || !symbol.declarations.length) return false; + + const decl = symbol.declarations[0]; + if (!ts.isImportSpecifier(decl)) return false; + + // As per `typescript/imports.ts`, we must walk up the tree to find the enclosing import + // declaration. For reasons specific to the TS AST, this is always 3 levels up from an import + // specifier node. + const importDecl = decl.parent.parent.parent; + if (!ts.isStringLiteral(importDecl.moduleSpecifier)) return false; + + const importName = typeChecker.getTypeAtLocation(importSpecifier)?.getSymbol()?.getName(); + if (!importName) return false; + + // Handles aliased imports: e.g. "import {Component as myComp} from ..."; + const declName = decl.propertyName ? decl.propertyName.text : decl.name.text; + + if (importName === declName) return true; + + // In the case of FormControl's overridden exported constructor, the value name and declaration + // name are not exactly the same. For our purposes, it's enough to check whether the latter is a + // substring of the former. + if (declName === formControl && importName.includes(declName)) return true; + + return false; +} diff --git a/packages/core/schematics/test/google3/typed_forms_spec.ts b/packages/core/schematics/test/google3/typed_forms_spec.ts index b568b0e5209..d79a896fcc1 100644 --- a/packages/core/schematics/test/google3/typed_forms_spec.ts +++ b/packages/core/schematics/test/google3/typed_forms_spec.ts @@ -66,43 +66,26 @@ describe('Google3 typedForms TSLint rule', () => { return readFileSync(join(tmpDir, fileName), 'utf8'); } - it('should migrate a complete example', () => { + // This is just a sanity check for the TSLint configuration; see test/typed_forms_spec.ts for the + // full test suite. + it('should migrate a simple example', () => { writeFile('/index.ts', ` import { Component } from '@angular/core'; import { AbstractControl, FormArray, FormBuilder, FormControl as FC, FormGroup, UntypedFormGroup } from '@angular/forms'; @Component({template: ''}) export class MyComponent { - private _control = new FC(42); - private _group = new FormGroup({}); + private _control: FC = new FC(42); + private _group: FormGroup = new FormGroup({}); private _array = new FormArray([]); - private _ungroup = new FormGroup({}); - - private fb = new FormBuilder(); - - build() { - const c = this.fb.control(42); - const g = this.fb.group({one: this.fb.control('')}); - const a = this.fb.array([42]); - const fc2 = new FC(0); - } } `); const linter = runTSLint(true); const cases = [ - // All the imports should be paired with an new untyped version, - // except UntypedFormGroup (which is already present). - `import { AbstractControl, FormArray, UntypedFormArray, FormBuilder, UntypedFormBuilder, FormControl as FC, UntypedFormControl, FormGroup, UntypedFormGroup } from '@angular/forms';`, - // Existing constructor calls should be rewritten, in various positions, including qualified - // imports. - `private _control = new UntypedFormControl(42);`, - `private _group = new UntypedFormGroup({});`, + `private _control: UntypedFormControl = new UntypedFormControl(42);`, + `private _group: UntypedFormGroup = new UntypedFormGroup({});`, `private _array = new UntypedFormArray([]);`, - `private fb = new UntypedFormBuilder();`, - `const fc2 = new UntypedFormControl(0);`, - // Except UntypedFormGroup, which is already migrated. - `private _ungroup = new UntypedFormGroup({});`, ]; cases.forEach(t => expect(getFile(`/index.ts`)).toContain(t)); }); diff --git a/packages/core/schematics/test/typed_forms_spec.ts b/packages/core/schematics/test/typed_forms_spec.ts index 4b9a2239c61..bd54b37738d 100644 --- a/packages/core/schematics/test/typed_forms_spec.ts +++ b/packages/core/schematics/test/typed_forms_spec.ts @@ -38,15 +38,42 @@ describe('Typed Forms migration', () => { // We need to declare the Angular symbols we're testing for, otherwise type checking won't work. writeFile('/node_modules/@angular/forms/index.d.ts', ` - export declare class FormControl {} - export declare class FormGroup {} - export declare class FormArray {} - export declare class AbstractControl {} - export declare class FormBuilder {} - export declare class UntypedFormControl {} - export declare class UntypedFormGroup {} - export declare class UntypedFormArray {} - export declare class UntypedFormBuilder {} + export interface FormControl {} + type FormControlInterface = FormControl; + export interface ɵFormControlCtor { + new(value?: any): FormControl; + } + export const FormControl: ɵFormControlCtor = + (class FormControl implements FormControlInterface { + constructor(value?: any) {} + }); + export declare class FormGroup { + constructor(controls?: any) + } + export declare class FormArray { + constructor(controls?: any) + } + export declare class AbstractControl {} + export declare class FormBuilder { + control(v: any): void; + group(v: any): void; + array(v: any): void; + } + export declare class UntypedFormControl { + constructor(value?: any) + } + export declare class UntypedFormGroup { + constructor(controls?: any) + } + export declare class UntypedFormArray { + constructor(controls?: any) + } + export declare class UntypedFormBuilder { + control(v: any): void; + group(v: any): void; + array(v: any): void; + } + export declare class Form {} `); previousWorkingDir = shx.pwd(); @@ -69,58 +96,94 @@ describe('Typed Forms migration', () => { @Component({template: ''}) export class MyComponent { - private _control = new FC(42); + private _control: FC = new FC(42); private _group = new FormGroup({}); - private _array = new FormArray([]); - private _ungroup = new FormGroup({}); + private _array: FormArray = new FormArray([]); + private _ungroup: UntypedFormGroup = new UntypedFormGroup({}); + private FormC: Form = new Form(); + + private nested = new FormGroup({a: new FC(1)}); + private nested2 = new FormGroup<{a: FormGroup<{b: FC}>}>({a: new FormGroup<{b: FC}>(new FC(1))}); private fb = new FormBuilder(); + private fb2!: FormBuilder; private someSet = new Set([1]); + private FCSet = new Set(new FC(1)); - build() { + foo(fc: FC) {} + + bar(baz: T) {} + baz(T: FormGroup&string) {} + + build(fg: FormGroup) { + let tg: UntypedFormGroup; const c = this.fb.control(42); const g = this.fb.group({one: this.fb.control('')}); const a = this.fb.array([42]); const fc2 = new FC(0); } } + + class TypedFormGroup extends FormGroup {} + let a!: TypedFormGroup; + + class ormGroup extends FormGroup {} `); await runMigration(); + // There are a huge number of positions in which identifiers can show up. This tests an + // assortment of them, but is not exhaustive. const cases = [ - // All the imports should be paired with an new untyped version, - // except UntypedFormGroup (which is already present). - `import { AbstractControl, FormArray, UntypedFormArray, FormBuilder, UntypedFormBuilder, FormControl as FC, UntypedFormControl, FormGroup, UntypedFormGroup } from '@angular/forms';`, - // Existing constructor calls should be rewritten, in various positions, including qualified - // imports. - `private _control = new UntypedFormControl(42);`, + // Imports, excluding already migrated imports + `import { AbstractControl, UntypedFormArray, UntypedFormBuilder, UntypedFormControl, FormGroup, UntypedFormGroup } from '@angular/forms';`, + // Constructor calls, in various positions and qualifications + `private _control: UntypedFormControl = new UntypedFormControl(42);`, `private _group = new UntypedFormGroup({});`, - `private _array = new UntypedFormArray([]);`, + `private _array: UntypedFormArray = new UntypedFormArray([]);`, `private fb = new UntypedFormBuilder();`, `const fc2 = new UntypedFormControl(0);`, - // Except UntypedFormGroup, which is already migrated. - `private _ungroup = new UntypedFormGroup({});`, + // Declarations + `let tg: UntypedFormGroup;`, + `private fb2!: UntypedFormBuilder;`, + // Function parameters + `foo(fc: UntypedFormControl) {}`, + `build(fg: UntypedFormGroup) {`, + // Generic arguments + `private FCSet = new Set(new UntypedFormControl(1));`, + // Generic functions + `bar(baz: T) {}`, + // Intersection types + `baz(T: UntypedFormGroup&string) {}`, + // Nested types + `private nested = new UntypedFormGroup({a: new UntypedFormControl(1)});`, + `private nested2 = new UntypedFormGroup<{a: UntypedFormGroup<{b: UntypedFormControl}>}>({a: new UntypedFormGroup<{b: UntypedFormControl}>(new UntypedFormControl(1))});`, + // Skip UntypedFormGroup, which is already migrated (idempotent migration) + `private _ungroup: UntypedFormGroup = new UntypedFormGroup({});`, + // Form class should not be changed. + `private FormC: Form = new Form();`, // Unrelated constructors should not be changed. `private someSet = new Set([1]);`, + // Unrelated classes with similar names should not be changed. + `private someSet = new Set([1]);`, + `class TypedFormGroup extends UntypedFormGroup {}`, + `let a!: TypedFormGroup;`, + `class ormGroup extends UntypedFormGroup {}`, ]; cases.forEach(t => expect(tree.readContent('/index.ts')).toContain(t)); }); it('skip adding imports that would be unused', async () => { writeFile('/index.ts', ` - import { Component } from '@angular/core'; - import { FormControl, FormGroup } from '@angular/forms'; + import {Component} from '@angular/core'; + import {FormControl, FormGroup} from '@angular/forms'; - @Component({template: ''}) - export class MyComponent { - private _group!: FormGroup; - private _control = new FormControl(42); - } - `); + @Component({template: ''}) export class MyComponent { + private fc: FormControl; + } `); await runMigration(); const cases = [ - // Because FormGroup is never directly constructed, the import should not be added. - `import { FormControl, UntypedFormControl, FormGroup } from '@angular/forms';`, + // Because FormGroup is never used, the import should not be updated. + `import {UntypedFormControl, FormGroup} from '@angular/forms';`, ]; cases.forEach(t => expect(tree.readContent('/index.ts')).toContain(t)); });