refactor(migrations): improve temporary variable generation for input migration (angular#57292)

This changes the migration so that we don't generate `_1` suffixes when a temporary variable wouldn't conflict with any variables in the lexical scope. In addition, if we discover conflicts, we try alternative suffixes that seem more natural and follow style guides. E.g. `Value`, `Val` or `Input`. PR Close angular#57292

Author: devversion

packages/core/schematics/migrations/signal-migration/src/flow_analysis/flow_containers.ts

@@ -23,7 +23,7 @@ export function isControlFlowBoundary(node: ts.Node): boolean {
 
 /** Determines the current flow container of a given node. */
 export function getControlFlowContainer(node: ts.Node): ts.Node {
- return findAncestor(node.parent, (node) => isControlFlowBoundary(node))!;
+ return ts.findAncestor(node.parent, (node) => isControlFlowBoundary(node))!;
 }
 
 /** Checks whether the given node refers to an IIFE declaration. */
@@ -44,18 +44,3 @@ function getImmediatelyInvokedFunctionExpression(func: ts.Node): ts.CallExpressi
  }
  return undefined;
 }
-
-/** Traverses up the node chain until the callback is true, returning the node. */
-function findAncestor(
- node: ts.Node | undefined,
- callback: (element: ts.Node) => boolean,
-): ts.Node | undefined {
- while (node) {
- const result = callback(node);
- if (result) {
- return node;
- }
- node = node.parent;
- }
- return undefined;
-}

packages/core/schematics/migrations/signal-migration/src/passes/5_migrate_ts_references.ts

@@ -14,7 +14,7 @@ import {InputUniqueKey} from '../utils/input_id';
 import {isTsInputReference} from '../utils/input_reference';
 import {traverseAccess} from '../utils/traverse_access';
 import {KnownInputs} from '../input_detection/known_inputs';
-import {createGenerateUniqueIdentifierHelper} from '../../../../../../compiler-cli/src/ngtsc/translator/src/import_manager/check_unique_identifier_name';
+import {UniqueNamesGenerator} from '../utils/unique_names';
 
 /**
  * Phase that migrates TypeScript input references to be signal compatible.
@@ -46,9 +46,9 @@ export function pass5__migrateTypeScriptReferences(
  checker: ts.TypeChecker,
  knownInputs: KnownInputs,
 ) {
- const generateUniqueIdentifier = createGenerateUniqueIdentifierHelper();
  const tsReferences = new Map<InputUniqueKey, {accesses: ts.Identifier[]}>();
  const seenIdentifiers = new WeakSet<ts.Identifier>();
+ const nameGenerator = new UniqueNamesGenerator();
 
  for (const reference of result.references) {
  // This pass only deals with TS references.
@@ -130,8 +130,7 @@ export function pass5__migrateTypeScriptReferences(
  const leadingSpace = ts.getLineAndCharacterOfPosition(sf, previous.getStart());
 
  const replaceNode = traverseAccess(originalNode);
- const uniqueFieldName = generateUniqueIdentifier(sf, originalNode.text);
- const fieldName = uniqueFieldName === null ? originalNode.text : uniqueFieldName.text;
+ const fieldName = nameGenerator.generate(originalNode.text, previous);
 
  idToSharedField.set(id, fieldName);
 

packages/core/schematics/migrations/signal-migration/src/utils/is_descendant_of.ts

@@ -0,0 +1,18 @@
+/**
+ * @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 ts from 'typescript';
+
+/** Whether the given node is a descendant of the given ancestor. */
+export function isNodeDescendantOf(node: ts.Node, ancestor: ts.Node | undefined): boolean {
+ while (node) {
+ if (node === ancestor) return true;
+ node = node.parent;
+ }
+ return false;
+}

packages/core/schematics/migrations/signal-migration/src/utils/is_identifier_free_in_scope.ts

@@ -0,0 +1,122 @@
+/**
+ * @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 assert from 'assert';
+import ts from 'typescript';
+import {isNodeDescendantOf} from './is_descendant_of';
+
+// typescript/stable/src/compiler/types.ts;l=967;rcl=651008033
+export interface LocalsContainer extends ts.Node {
+ locals?: Map<string, ts.Symbol>;
+ nextContainer?: LocalsContainer;
+}
+
+/**
+ * Gets whether the given identifier name is free for use in the
+ * given location, avoiding shadowed variable names.
+ *
+ */
+export function isIdentifierFreeInScope(
+ name: string,
+ location: ts.Node,
+): null | {container: LocalsContainer} {
+ const startContainer = findClosestParentLocalsContainer(location);
+ assert(startContainer !== undefined, 'Expecting a locals container.');
+
+ // Traverse up and check for potential collisions.
+ let container: LocalsContainer | undefined = startContainer;
+ let firstNextContainer: LocalsContainer | undefined = undefined;
+
+ while (container !== undefined) {
+ if (!isIdentifierFreeInContainer(name, container)) {
+ return null;
+ }
+ if (firstNextContainer === undefined && container.nextContainer !== undefined) {
+ firstNextContainer = container.nextContainer;
+ }
+ container = findClosestParentLocalsContainer(container.parent);
+ }
+
+ // Check descendent local containers to avoid shadowing variables.
+ // Note that this is not strictly needed, but it's helping avoid
+ // some lint errors, like TSLint's no shadowed variables.
+ container = firstNextContainer;
+ while (container && isNodeDescendantOf(container, startContainer)) {
+ if (!isIdentifierFreeInContainer(name, container)) {
+ return null;
+ }
+ container = container.nextContainer;
+ }
+
+ return {container: startContainer};
+}
+
+/** Finds the closest parent locals container. */
+function findClosestParentLocalsContainer(node: ts.Node): LocalsContainer | undefined {
+ return ts.findAncestor(node, isLocalsContainer);
+}
+
+/** Whether the given identifier is free in the given locals container. */
+function isIdentifierFreeInContainer(name: string, container: LocalsContainer): boolean {
+ if (container.locals === undefined || !container.locals.has(name)) {
+ return true;
+ }
+
+ // We consider alias symbols as locals conservatively.
+ // Note: This check is similar to the check by the TypeScript emitter.
+ // typescript/stable/src/compiler/emitter.ts;l=5436;rcl=651008033
+ const local = container.locals.get(name)!;
+ return !(
+ local.flags &
+ (ts.SymbolFlags.Value | ts.SymbolFlags.ExportValue | ts.SymbolFlags.Alias)
+ );
+}
+
+/**
+ * Whether the given node can contain local variables.
+ *
+ * Note: This is similar to TypeScript's `canHaveLocals` internal helper.
+ * typescript/stable/src/compiler/utilitiesPublic.ts;l=2265;rcl=651008033
+ */
+function isLocalsContainer(node: ts.Node): node is LocalsContainer {
+ switch (node.kind) {
+ case ts.SyntaxKind.ArrowFunction:
+ case ts.SyntaxKind.Block:
+ case ts.SyntaxKind.CallSignature:
+ case ts.SyntaxKind.CaseBlock:
+ case ts.SyntaxKind.CatchClause:
+ case ts.SyntaxKind.ClassStaticBlockDeclaration:
+ case ts.SyntaxKind.ConditionalType:
+ case ts.SyntaxKind.Constructor:
+ case ts.SyntaxKind.ConstructorType:
+ case ts.SyntaxKind.ConstructSignature:
+ case ts.SyntaxKind.ForStatement:
+ case ts.SyntaxKind.ForInStatement:
+ case ts.SyntaxKind.ForOfStatement:
+ case ts.SyntaxKind.FunctionDeclaration:
+ case ts.SyntaxKind.FunctionExpression:
+ case ts.SyntaxKind.FunctionType:
+ case ts.SyntaxKind.GetAccessor:
+ case ts.SyntaxKind.IndexSignature:
+ case ts.SyntaxKind.JSDocCallbackTag:
+ case ts.SyntaxKind.JSDocEnumTag:
+ case ts.SyntaxKind.JSDocFunctionType:
+ case ts.SyntaxKind.JSDocSignature:
+ case ts.SyntaxKind.JSDocTypedefTag:
+ case ts.SyntaxKind.MappedType:
+ case ts.SyntaxKind.MethodDeclaration:
+ case ts.SyntaxKind.MethodSignature:
+ case ts.SyntaxKind.ModuleDeclaration:
+ case ts.SyntaxKind.SetAccessor:
+ case ts.SyntaxKind.SourceFile:
+ case ts.SyntaxKind.TypeAliasDeclaration:
+ return true;
+ default:
+ return false;
+ }
+}

packages/core/schematics/migrations/signal-migration/src/utils/unique_names.ts

@@ -0,0 +1,58 @@
+/**
+ * @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 ts from 'typescript';
+import {isIdentifierFreeInScope} from './is_identifier_free_in_scope';
+
+/** List of potential suffixes to avoid conflicts. */
+const fallbackSuffixes = ['Value', 'Val', 'Input'];
+
+/**
+ * Helper that can generate unique identifier names at a
+ * given location.
+ *
+ * Used for generating unique names to extract input reads
+ * to support narrowing.
+ */
+export class UniqueNamesGenerator {
+ generate(base: string, location: ts.Node): string {
+ const checkNameAndClaimIfAvailable = (name: string): boolean => {
+ const freeInfo = isIdentifierFreeInScope(name, location);
+ if (freeInfo === null) {
+ return false;
+ }
+
+ // Claim the locals to avoid conflicts with future generations.
+ freeInfo.container.locals?.set(name, null! as ts.Symbol);
+ return true;
+ };
+
+ // Check the base name. Ideally, we'd use this one.
+ if (checkNameAndClaimIfAvailable(base)) {
+ return base;
+ }
+
+ // Try any of the possible suffixes.
+ for (const suffix of fallbackSuffixes) {
+ const name = `${base}${suffix}`;
+ if (checkNameAndClaimIfAvailable(name)) {
+ return name;
+ }
+ }
+
+ // Worst case, suffix the base name with a unique number until
+ // we find an available name.
+ let name = null;
+ let counter = 1;
+ do {
+ name = `${base}_${counter++}`;
+ } while (!checkNameAndClaimIfAvailable(name));
+
+ return name;
+ }
+}

packages/core/schematics/migrations/signal-migration/test/golden-test/identifier_collisions.ts

@@ -0,0 +1,51 @@
+/**
+ * @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
+ */
+
+// tslint:disable
+
+import {Component, Input} from '@angular/core';
+
+const complex = 'some global variable';
+
+@Component({template: ''})
+class MyComp {
+ @Input() name: string | null = null;
+ @Input() complex: string | null = null;
+
+ valid() {
+ if (this.name) {
+ this.name.charAt(0);
+ }
+ }
+
+ // Input read cannot be stored in a variable: `name`.
+ simpleLocalCollision() {
+ const name = 'some other name';
+ if (this.name) {
+ this.name.charAt(0);
+ }
+ }
+
+ // `this.complex` should conflict with the file-level `complex` variable,
+ // and result in a suffix variable.
+ complexParentCollision() {
+ if (this.complex) {
+ this.complex.charAt(0);
+ }
+ }
+
+ nestedShadowing() {
+ if (this.name) {
+ this.name.charAt(0);
+ }
+
+ function nested() {
+ const name = '';
+ }
+ }
+}