feat(core): add migration for invalid two-way bindings (#54630)

As a part of #54154, an old parser behavior came up where two-way bindings were parsed by appending `= $event` to the event side. This was problematic, because it allowed some non-writable expressions to be passed into two-way bindings.

These changes introduce a migration that will change the two-way bindings into two separate input/output bindings that represent the old behavior so that in a future version we can throw a parser error for the invalid expressions.

```ts
// Before
@component({
  template: `<input [(ngModel)]="a && b"/>`
})
export class MyComp {}

// After
@component({
  template: `<input [ngModel]="a && b" (ngModelChange)="a && (b = $event)"/>`
})
export class MyComp {}
```

PR Close #54630

Author: crisbeto

packages/core/schematics/BUILD.bazel

@@ -20,6 +20,7 @@ pkg_npm(
     deps = [
         "//packages/core/schematics/migrations/block-template-entities:bundle",
         "//packages/core/schematics/migrations/compiler-options:bundle",
+        "//packages/core/schematics/migrations/invalid-two-way-bindings:bundle",
         "//packages/core/schematics/migrations/transfer-state:bundle",
         "//packages/core/schematics/ng-generate/control-flow-migration:bundle",
         "//packages/core/schematics/ng-generate/standalone-migration:bundle",

packages/core/schematics/migrations.json

@@ -14,6 +14,11 @@
       "version": "17.0.0",
       "description": "Updates `TransferState`, `makeStateKey`, `StateKey` imports from `@angular/platform-browser` to `@angular/core`.",
       "factory": "./migrations/transfer-state/bundle"
+    },
+    "invalid-two-way-bindings": {
+      "version": "17.3.0",
+      "description": "Updates two-way bindings that have an invalid expression to use the longform expression instead.",
+      "factory": "./migrations/invalid-two-way-bindings/bundle"
     }
   }
 }

packages/core/schematics/migrations/invalid-two-way-bindings/BUILD.bazel

@@ -0,0 +1,34 @@
+load("//tools:defaults.bzl", "esbuild", "ts_library")
+
+package(
+    default_visibility = [
+        "//packages/core/schematics:__pkg__",
+        "//packages/core/schematics/migrations/google3:__pkg__",
+        "//packages/core/schematics/test:__pkg__",
+    ],
+)
+
+ts_library(
+    name = "invalid-two-way-bindings",
+    srcs = glob(["**/*.ts"]),
+    tsconfig = "//packages/core/schematics:tsconfig.json",
+    deps = [
+        "//packages/compiler",
+        "//packages/core/schematics/utils",
+        "@npm//@angular-devkit/schematics",
+        "@npm//@types/node",
+        "@npm//typescript",
+    ],
+)
+
+esbuild(
+    name = "bundle",
+    entry_point = ":index.ts",
+    external = [
+        "@angular-devkit/*",
+        "typescript",
+    ],
+    format = "cjs",
+    platform = "node",
+    deps = [":invalid-two-way-bindings"],
+)

packages/core/schematics/migrations/invalid-two-way-bindings/README.md

@@ -0,0 +1,38 @@
+## Invalid two-way bindings migration
+
+Due to a quirk in the template parser, Angular previously allowed some unassignable expressions
+to be passed into two-way bindings which may produce incorrect results. This migration will
+replace the invalid two-way bindings with their input/output pair while preserving the original
+behavior. Note that the migrated expression may not be the original intent of the code as it was
+written, but they match what the Angular runtime would've executed.
+
+The invalid bindings will become errors in a future version of Angular.
+
+Some examples of invalid expressions include:
+* Binary expressions like `[(ngModel)]="a || b"`. Previously Angular would append `= $event` to
+the right-hand-side of the expression (e.g. `(ngModelChange)="a || (b = $event)"`).
+* Unary expressions like `[(ngModel)]="!a"` which Angular would wrap in a parentheses and execute
+(e.g. `(ngModelChange)="!(a = $event)"`).
+* Conditional expressions like `[(ngModel)]="a ? b : c"` where Angular would add `= $event` to
+the false case, e.g. `(ngModelChange)="a ? b : c = $event"`.
+
+#### Before
+```ts
+import {Component} from '@angular/core';
+
+@Component({
+  template: `<input [(ngModel)]="a && b"/>`
+})
+export class MyComp {}
+```
+
+
+#### After
+```ts
+import {Component} from '@angular/core';
+
+@Component({
+  template: `<input [ngModel]="a && b" (ngModelChange)="a && (b = $event)"/>`
+})
+export class MyComp {}
+```

packages/core/schematics/migrations/invalid-two-way-bindings/analysis.ts

@@ -0,0 +1,109 @@
+/**
+ * @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 {dirname, join} from 'path';
+import ts from 'typescript';
+
+/**
+ * Represents a range of text within a file. Omitting the end
+ * means that it's until the end of the file.
+ */
+type Range = [start: number, end?: number];
+
+/** Represents a file that was analyzed by the migration. */
+export class AnalyzedFile {
+  private ranges: Range[] = [];
+
+  /** Returns the ranges in the order in which they should be migrated. */
+  getSortedRanges(): Range[] {
+    return this.ranges.slice().sort(([aStart], [bStart]) => bStart - aStart);
+  }
+
+  /**
+   * Adds a text range to an `AnalyzedFile`.
+   * @param path Path of the file.
+   * @param analyzedFiles Map keeping track of all the analyzed files.
+   * @param range Range to be added.
+   */
+  static addRange(path: string, analyzedFiles: Map<string, AnalyzedFile>, range: Range): void {
+    let analysis = analyzedFiles.get(path);
+
+    if (!analysis) {
+      analysis = new AnalyzedFile();
+      analyzedFiles.set(path, analysis);
+    }
+
+    const duplicate =
+        analysis.ranges.find(current => current[0] === range[0] && current[1] === range[1]);
+
+    if (!duplicate) {
+      analysis.ranges.push(range);
+    }
+  }
+}
+
+/**
+ * Analyzes a source file to find file that need to be migrated and the text ranges within them.
+ * @param sourceFile File to be analyzed.
+ * @param analyzedFiles Map in which to store the results.
+ */
+export function analyze(sourceFile: ts.SourceFile, analyzedFiles: Map<string, AnalyzedFile>) {
+  forEachClass(sourceFile, node => {
+    // Note: we have a utility to resolve the Angular decorators from a class declaration already.
+    // We don't use it here, because it requires access to the type checker which makes it more
+    // time-consuming to run internally.
+    const decorator = ts.getDecorators(node)?.find(dec => {
+      return ts.isCallExpression(dec.expression) && ts.isIdentifier(dec.expression.expression) &&
+          dec.expression.expression.text === 'Component';
+    }) as (ts.Decorator & {expression: ts.CallExpression}) |
+        undefined;
+
+    const metadata = decorator && decorator.expression.arguments.length > 0 &&
+            ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ?
+        decorator.expression.arguments[0] :
+        null;
+
+    if (!metadata) {
+      return;
+    }
+
+    for (const prop of metadata.properties) {
+      // All the properties we care about should have static
+      // names and be initialized to a static string.
+      if (!ts.isPropertyAssignment(prop) || !ts.isStringLiteralLike(prop.initializer) ||
+          (!ts.isIdentifier(prop.name) && !ts.isStringLiteralLike(prop.name))) {
+        continue;
+      }
+
+      switch (prop.name.text) {
+        case 'template':
+          // +1/-1 to exclude the opening/closing characters from the range.
+          AnalyzedFile.addRange(
+              sourceFile.fileName, analyzedFiles,
+              [prop.initializer.getStart() + 1, prop.initializer.getEnd() - 1]);
+          break;
+
+        case 'templateUrl':
+          // Leave the end as undefined which means that the range is until the end of the file.
+          const path = join(dirname(sourceFile.fileName), prop.initializer.text);
+          AnalyzedFile.addRange(path, analyzedFiles, [0]);
+          break;
+      }
+    }
+  });
+}
+
+/** Executes a callback on each class declaration in a file. */
+function forEachClass(sourceFile: ts.SourceFile, callback: (node: ts.ClassDeclaration) => void) {
+  sourceFile.forEachChild(function walk(node) {
+    if (ts.isClassDeclaration(node)) {
+      callback(node);
+    }
+    node.forEachChild(walk);
+  });
+}

packages/core/schematics/migrations/invalid-two-way-bindings/index.ts

@@ -0,0 +1,70 @@
+/**
+ * @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 {canMigrateFile, createMigrationProgram} from '../../utils/typescript/compiler_host';
+
+import {analyze, AnalyzedFile} from './analysis';
+import {migrateTemplate} from './migration';
+
+export default function(): Rule {
+  return async (tree: Tree) => {
+    const {buildPaths, testPaths} = await getProjectTsConfigPaths(tree);
+    const basePath = process.cwd();
+    const allPaths = [...buildPaths, ...testPaths];
+
+    if (!allPaths.length) {
+      throw new SchematicsException(
+          'Could not find any tsconfig file. Cannot run the invalid two-way bindings migration.');
+    }
+
+    for (const tsconfigPath of allPaths) {
+      runInvalidTwoWayBindingsMigration(tree, tsconfigPath, basePath);
+    }
+  };
+}
+
+function runInvalidTwoWayBindingsMigration(tree: Tree, tsconfigPath: string, basePath: string) {
+  const program = createMigrationProgram(tree, tsconfigPath, basePath);
+  const sourceFiles =
+      program.getSourceFiles().filter(sourceFile => canMigrateFile(basePath, sourceFile, program));
+  const analysis = new Map<string, AnalyzedFile>();
+
+  for (const sourceFile of sourceFiles) {
+    analyze(sourceFile, analysis);
+  }
+
+  for (const [path, file] of analysis) {
+    const ranges = file.getSortedRanges();
+    const relativePath = relative(basePath, path);
+
+    // Don't interrupt the entire migration if a file can't be read.
+    if (!tree.exists(relativePath)) {
+      continue;
+    }
+
+    const content = tree.readText(relativePath);
+    const update = tree.beginUpdate(relativePath);
+
+    for (const [start, end] of ranges) {
+      const template = content.slice(start, end);
+      const length = (end ?? content.length) - start;
+      const migrated = migrateTemplate(template);
+
+      if (migrated !== null) {
+        update.remove(start, length);
+        update.insertLeft(start, migrated);
+      }
+    }
+
+    tree.commitUpdate(update);
+  }
+}