From aeec223db1236db236ab56a4a283cf2ca410cae3 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 26 Sep 2020 21:35:25 +0300 Subject: [PATCH] feat(forms): add migration for AbstractControl.parent accesses (#39009) As of #32671, the type of `AbstractControl.parent` can be null which can cause compilation errors in existing apps. These changes add a migration that will append non-null assertions to existing unsafe accesses. ```` // Before console.log(control.parent.value); // After console.log(control.parent!.value); ``` The migration also tries its best to avoid cases where the non-null assertions aren't necessary (e.g. if the `parent` was null checked already). PR Close #39009 --- packages/core/schematics/BUILD.bazel | 1 + packages/core/schematics/migrations.json | 5 + .../abstract-control-parent/BUILD.bazel | 18 ++ .../abstract-control-parent/README.md | 34 +++ .../abstract-control-parent/index.ts | 57 ++++ .../abstract-control-parent/util.ts | 155 ++++++++++ packages/core/schematics/test/BUILD.bazel | 1 + .../abstract_control_parent_migration_spec.ts | 289 ++++++++++++++++++ 8 files changed, 560 insertions(+) create mode 100644 packages/core/schematics/migrations/abstract-control-parent/BUILD.bazel create mode 100644 packages/core/schematics/migrations/abstract-control-parent/README.md create mode 100644 packages/core/schematics/migrations/abstract-control-parent/index.ts create mode 100644 packages/core/schematics/migrations/abstract-control-parent/util.ts create mode 100644 packages/core/schematics/test/abstract_control_parent_migration_spec.ts diff --git a/packages/core/schematics/BUILD.bazel b/packages/core/schematics/BUILD.bazel index 375fcd5920..9d665c5763 100644 --- a/packages/core/schematics/BUILD.bazel +++ b/packages/core/schematics/BUILD.bazel @@ -10,6 +10,7 @@ pkg_npm( srcs = ["migrations.json"], visibility = ["//packages/core:__pkg__"], deps = [ + "//packages/core/schematics/migrations/abstract-control-parent", "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/module-with-providers", diff --git a/packages/core/schematics/migrations.json b/packages/core/schematics/migrations.json index b4d86667f6..4e5191af2a 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -54,6 +54,11 @@ "version": "11.0.0-beta", "description": "The default value for `relativeLinkResolution` is changing from 'legacy' to 'corrected'.\nThis migration updates `RouterModule` configurations that use the default value to \nnow specifically use 'legacy' to prevent breakages when updating.", "factory": "./migrations/relative-link-resolution/index" + }, + "migration-v11-abstract-control-parent": { + "version": "11.0.0-beta", + "description": "In Angular version 11, the type of `AbstractControl.parent` can be `null` to reflect the runtime value more accurately. This migration automatically adds non-null assertions to existing accesses of the `parent` property on types like `FormControl`, `FormArray` and `FormGroup`.", + "factory": "./migrations/abstract-control-parent/index" } } } diff --git a/packages/core/schematics/migrations/abstract-control-parent/BUILD.bazel b/packages/core/schematics/migrations/abstract-control-parent/BUILD.bazel new file mode 100644 index 0000000000..c78e69f55f --- /dev/null +++ b/packages/core/schematics/migrations/abstract-control-parent/BUILD.bazel @@ -0,0 +1,18 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "abstract-control-parent", + srcs = glob(["**/*.ts"]), + tsconfig = "//packages/core/schematics:tsconfig.json", + visibility = [ + "//packages/core/schematics:__pkg__", + "//packages/core/schematics/migrations/google3:__pkg__", + "//packages/core/schematics/test:__pkg__", + ], + deps = [ + "//packages/core/schematics/utils", + "@npm//@angular-devkit/schematics", + "@npm//@types/node", + "@npm//typescript", + ], +) diff --git a/packages/core/schematics/migrations/abstract-control-parent/README.md b/packages/core/schematics/migrations/abstract-control-parent/README.md new file mode 100644 index 0000000000..b40ca8deda --- /dev/null +++ b/packages/core/schematics/migrations/abstract-control-parent/README.md @@ -0,0 +1,34 @@ +## `AbstractControl.parent` migration + +As of Angular v11, the type of `AbstractControl.parent` can be null. This migration automatically +identifies usages and adds non-null assertions. + +#### Before +```ts +import { Component } from '@angular/core'; +import { FormControl } from '@angular/forms'; + +@Component() +export class MyComponent { + private _control = new FormControl(); + + getParentValue() { + return this._control.parent.value; // <- Compilation error in v11. + } +} +``` + +#### After +```ts +import { Component } from '@angular/core'; +import { FormControl } from '@angular/forms'; + +@Component() +export class MyComponent { + private _control = new FormControl(); + + getParentValue() { + return this._control.parent!.value; // <- Non-null assertion added during the migration. + } +} +``` diff --git a/packages/core/schematics/migrations/abstract-control-parent/index.ts b/packages/core/schematics/migrations/abstract-control-parent/index.ts new file mode 100644 index 0000000000..9aea59128b --- /dev/null +++ b/packages/core/schematics/migrations/abstract-control-parent/index.ts @@ -0,0 +1,57 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Rule, SchematicsException, Tree} from '@angular-devkit/schematics'; +import {relative} from 'path'; + +import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; +import {createMigrationProgram} from '../../utils/typescript/compiler_host'; +import {findParentAccesses} from './util'; + + +/** Migration that marks accesses of `AbstractControl.parent` as non-null. */ +export default function(): Rule { + return (tree: Tree) => { + const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const basePath = process.cwd(); + const allPaths = [...buildPaths, ...testPaths]; + + if (!allPaths.length) { + throw new SchematicsException( + 'Could not find any tsconfig file. Cannot migrate AbstractControl.parent accesses.'); + } + + for (const tsconfigPath of allPaths) { + runNativeAbstractControlParentMigration(tree, tsconfigPath, basePath); + } + }; +} + +function runNativeAbstractControlParentMigration( + tree: Tree, tsconfigPath: string, basePath: string) { + const {program} = createMigrationProgram(tree, tsconfigPath, basePath); + const typeChecker = program.getTypeChecker(); + const sourceFiles = program.getSourceFiles().filter( + f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f)); + + sourceFiles.forEach(sourceFile => { + // We sort the nodes based on their position in the file and we offset the positions by one + // for each non-null assertion that we've added. We have to do it this way, rather than + // creating and printing a new AST node like in other migrations, because property access + // expressions can be nested (e.g. `control.parent.parent.value`), but the node positions + // aren't being updated as we're inserting new code. If we were to go through the AST, + // we'd have to update the `SourceFile` and start over after each operation. + findParentAccesses(typeChecker, sourceFile) + .sort((a, b) => a.getStart() - b.getStart()) + .forEach((node, index) => { + const update = tree.beginUpdate(relative(basePath, sourceFile.fileName)); + update.insertRight(node.getStart() + node.getWidth() + index, '!'); + tree.commitUpdate(update); + }); + }); +} diff --git a/packages/core/schematics/migrations/abstract-control-parent/util.ts b/packages/core/schematics/migrations/abstract-control-parent/util.ts new file mode 100644 index 0000000000..f5e47c516f --- /dev/null +++ b/packages/core/schematics/migrations/abstract-control-parent/util.ts @@ -0,0 +1,155 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {normalize} from 'path'; +import * as ts from 'typescript'; + +/** Names of symbols from `@angular/forms` whose `parent` accesses have to be migrated. */ +const abstractControlSymbols = new Set([ + 'AbstractControl', + 'FormArray', + 'FormControl', + 'FormGroup', +]); + +/** + * Finds the `PropertyAccessExpression`-s that are accessing the `parent` property in + * such a way that may result in a compilation error after the v11 type changes. + */ +export function findParentAccesses( + typeChecker: ts.TypeChecker, sourceFile: ts.SourceFile): ts.PropertyAccessExpression[] { + const results: ts.PropertyAccessExpression[] = []; + + sourceFile.forEachChild(function walk(node: ts.Node) { + if (ts.isPropertyAccessExpression(node) && node.name.text === 'parent' && !isNullCheck(node) && + !isSafeAccess(node) && results.indexOf(node) === -1 && + isAbstractControlReference(typeChecker, node) && isNullableType(typeChecker, node)) { + results.unshift(node); + } + + node.forEachChild(walk); + }); + + return results; +} + +/** Checks whether a node's type is nullable (`null`, `undefined` or `void`). */ +function isNullableType(typeChecker: ts.TypeChecker, node: ts.Node) { + // Skip expressions in the form of `foo.bar!.baz` since the `TypeChecker` seems + // to identify them as null, even though the user indicated that it won't be. + if (node.parent && ts.isNonNullExpression(node.parent)) { + return false; + } + + const type = typeChecker.getTypeAtLocation(node); + const typeNode = typeChecker.typeToTypeNode(type, undefined, ts.NodeBuilderFlags.None); + let hasSeenNullableType = false; + + // Trace the type of the node back to a type node, walk + // through all of its sub-nodes and look for nullable tyes. + if (typeNode) { + (function walk(current: ts.Node) { + if (current.kind === ts.SyntaxKind.NullKeyword || + current.kind === ts.SyntaxKind.UndefinedKeyword || + current.kind === ts.SyntaxKind.VoidKeyword) { + hasSeenNullableType = true; + // Note that we don't descend into type literals, because it may cause + // us to mis-identify the root type as nullable, because it has a nullable + // property (e.g. `{ foo: string | null }`). + } else if (!hasSeenNullableType && !ts.isTypeLiteralNode(current)) { + current.forEachChild(walk); + } + })(typeNode); + } + + return hasSeenNullableType; +} + +/** + * Checks whether a particular node is part of a null check. E.g. given: + * `control.parent ? control.parent.value : null` the null check would be `control.parent`. + */ +function isNullCheck(node: ts.PropertyAccessExpression): boolean { + if (!node.parent) { + return false; + } + + // `control.parent && control.parent.value` where `node` is `control.parent`. + if (ts.isBinaryExpression(node.parent) && node.parent.left === node) { + return true; + } + + // `control.parent && control.parent.parent && control.parent.parent.value` + // where `node` is `control.parent`. + if (node.parent.parent && ts.isBinaryExpression(node.parent.parent) && + node.parent.parent.left === node.parent) { + return true; + } + + // `if (control.parent) {...}` where `node` is `control.parent`. + if (ts.isIfStatement(node.parent) && node.parent.expression === node) { + return true; + } + + // `control.parent ? control.parent.value : null` where `node` is `control.parent`. + if (ts.isConditionalExpression(node.parent) && node.parent.condition === node) { + return true; + } + + return false; +} + +/** Checks whether a property access is safe (e.g. `foo.parent?.value`). */ +function isSafeAccess(node: ts.PropertyAccessExpression): boolean { + return node.parent != null && ts.isPropertyAccessExpression(node.parent) && + node.parent.expression === node && node.parent.questionDotToken != null; +} + +/** Checks whether a property access is on an `AbstractControl` coming from `@angular/forms`. */ +function isAbstractControlReference( + typeChecker: ts.TypeChecker, node: ts.PropertyAccessExpression): boolean { + let current: ts.Expression = node; + const formsPattern = /node_modules\/?.*\/@angular\/forms/; + // Walks up the property access chain and tries to find a symbol tied to a `SourceFile`. + // If such a node is found, we check whether the type is one of the `AbstractControl` symbols + // and whether it comes from the `@angular/forms` directory in the `node_modules`. + while (ts.isPropertyAccessExpression(current)) { + const type = typeChecker.getTypeAtLocation(current.expression); + const symbol = type.getSymbol(); + if (symbol && type) { + const sourceFile = symbol.valueDeclaration?.getSourceFile(); + return sourceFile != null && + formsPattern.test(normalize(sourceFile.fileName).replace(/\\/g, '/')) && + hasAbstractControlType(typeChecker, type); + } + current = current.expression; + } + return false; +} + +/** + * Walks through the sub-types of a type, looking for a type that + * has the same name as one of the `AbstractControl` types. + */ +function hasAbstractControlType(typeChecker: ts.TypeChecker, type: ts.Type): boolean { + const typeNode = typeChecker.typeToTypeNode(type, undefined, ts.NodeBuilderFlags.None); + let hasMatch = false; + if (typeNode) { + (function walk(current: ts.Node) { + if (ts.isIdentifier(current) && abstractControlSymbols.has(current.text)) { + hasMatch = true; + // Note that we don't descend into type literals, because it may cause + // us to mis-identify the root type as nullable, because it has a nullable + // property (e.g. `{ foo: FormControl }`). + } else if (!hasMatch && !ts.isTypeLiteralNode(current)) { + current.forEachChild(walk); + } + })(typeNode); + } + return hasMatch; +} diff --git a/packages/core/schematics/test/BUILD.bazel b/packages/core/schematics/test/BUILD.bazel index fc8cdb52e2..a8b958bdd7 100644 --- a/packages/core/schematics/test/BUILD.bazel +++ b/packages/core/schematics/test/BUILD.bazel @@ -8,6 +8,7 @@ ts_library( "//packages/core/schematics:migrations.json", ], deps = [ + "//packages/core/schematics/migrations/abstract-control-parent", "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/missing-injectable", "//packages/core/schematics/migrations/module-with-providers", diff --git a/packages/core/schematics/test/abstract_control_parent_migration_spec.ts b/packages/core/schematics/test/abstract_control_parent_migration_spec.ts new file mode 100644 index 0000000000..cdf396734d --- /dev/null +++ b/packages/core/schematics/test/abstract_control_parent_migration_spec.ts @@ -0,0 +1,289 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {getSystemPath, normalize, virtualFs} from '@angular-devkit/core'; +import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing'; +import {HostTree} from '@angular-devkit/schematics'; +import {SchematicTestRunner, UnitTestTree} from '@angular-devkit/schematics/testing'; +import * as shx from 'shelljs'; + +describe('AbstractControl.parent migration', () => { + let runner: SchematicTestRunner; + let host: TempScopedNodeJsSyncHost; + let tree: UnitTestTree; + let tmpDirPath: string; + let previousWorkingDir: string; + + beforeEach(() => { + runner = new SchematicTestRunner('test', require.resolve('../migrations.json')); + host = new TempScopedNodeJsSyncHost(); + tree = new UnitTestTree(new HostTree(host)); + + writeFile('/tsconfig.json', JSON.stringify({ + compilerOptions: {lib: ['es2015'], strictNullChecks: true}, + })); + writeFile('/angular.json', JSON.stringify({ + projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} + })); + // 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 abstract class AbstractControl { + get dirty(): boolean; + get disabled(): boolean; + get parent(): FormGroup | FormArray | null; + } + + export declare class FormArray extends AbstractControl { + getRawValue(): any[]; + } + + export declare class FormControl extends AbstractControl { + setValue(value: any): void; + } + + export declare class FormGroup extends AbstractControl { + getRawValue(): any; + } + `); + + // Fake non-Angular package to make sure that we don't migrate packages we don't own. + writeFile('/node_modules/@not-angular/forms/index.d.ts', ` + export declare abstract class AbstractControl { + get dirty(): boolean; + get disabled(): boolean; + get parent(): FormGroup | FormArray | null; + } + + export declare class FormControl extends AbstractControl { + setValue(value: any): void; + } + `); + + previousWorkingDir = shx.pwd(); + tmpDirPath = getSystemPath(host.root); + + // Switch into the temporary directory path. This allows us to run + // the schematic against our custom unit test tree. + shx.cd(tmpDirPath); + }); + + afterEach(() => { + shx.cd(previousWorkingDir); + shx.rm('-r', tmpDirPath); + }); + + it('should add non-null assertions to accesses of AbstractControl.parent', async () => { + writeFile('/index.ts', ` + import {AbstractControl} from '@angular/forms'; + + class App { + private _control: AbstractControl; + + getParentValue() { + return this._control.parent.value; + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return this._control.parent!.value;`); + }); + + it('should add non-null assertions to accesses of FormArray.parent', async () => { + writeFile('/index.ts', ` + import {FormArray} from '@angular/forms'; + + class App { + getParentValueOf(control: FormArray) { + return control.parent.value; + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return control.parent!.value;`); + }); + + it('should add non-null assertions to accesses of FormControl.parent', async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + class App { + getBlankControlParentValue() { + return this._getControl().parent.value; + } + + private _getControl() { + return new FormControl(); + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return this._getControl().parent!.value;`); + }); + + it('should add non-null assertions to accesses of FormGroup.parent', async () => { + writeFile('/index.ts', ` + import {FormGroup} from '@angular/forms'; + + class App { + getGlobalGroupParentValue() { + const parent = (window.foo as FormGroup).parent; + return parent.value; + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')) + .toContain(`const parent = (window.foo as FormGroup).parent!;`); + }); + + it('should add non-null assertions to nested accesses of `AbstractControl.parent`', async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + class App { + private _control = new FormControl(); + + getGreatGrandParentValue() { + return this._control.parent.parent.parent.value; + } + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')) + .toContain(`return this._control.parent!.parent!.parent!.value;`); + }); + + it('should not add non-null assertions if the `parent` has been null checked in an if statement', + async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getParentValue(control: FormControl) { + if (control.parent) { + return control.parent.value; + } + + return null; + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`if (control.parent) {`); + expect(content).toContain(`return control.parent.value;`); + }); + + it('should not add non-null assertions if the `parent` has been null checked in an else if statement', + async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getParentValue(foo: boolean, control: FormControl) { + if (foo) { + return foo; + } else if (control.parent) { + return control.parent.value; + } + + return null; + } + `); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toContain(`} else if (control.parent) {`); + expect(content).toContain(`return control.parent.value;`); + }); + + it('should not add non-null assertions if the `parent` has been null checked in a ternary expression', + async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getParentValue(control: FormControl) { + return control.parent ? control.parent.value : null; + } + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`return control.parent ? control.parent.value : null;`); + }); + + it('should not add non-null assertions if a nested `parent` has been null checked', async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getGreatGrandParentValue(control: FormControl) { + return control.parent && control.parent.parent && control.parent.parent.parent && control.parent.parent.parent.value; + } + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain( + `return control.parent && control.parent.parent && control.parent.parent.parent && control.parent.parent.parent.value;`); + }); + + it('should not add non-null assertions if there is one already', async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getParentValue(control: FormControl) { + return control.parent!.value; + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return control.parent!.value;`); + }); + + it('should not add non-null assertions if there is a safe access', async () => { + writeFile('/index.ts', ` + import {FormControl} from '@angular/forms'; + + function getParentValue(control: FormControl) { + return control.parent?.value; + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return control.parent?.value;`); + }); + + it('should not add non-null assertions if the symbol does not come from @angular/forms', + async () => { + writeFile('/index.ts', ` + import {FormControl} from '@not-angular/forms'; + + function getParentValue(control: FormControl) { + return control.parent.value; + } + `); + + await runMigration(); + expect(tree.readContent('/index.ts')).toContain(`return control.parent.value;`); + }); + + function writeFile(filePath: string, contents: string) { + host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); + } + + function runMigration() { + return runner.runSchematicAsync('migration-v11-abstract-control-parent', {}, tree).toPromise(); + } +});