From 22132639e26d4579e2aec63c191893aa5b72b4c7 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 8 Oct 2024 20:11:19 +0000 Subject: [PATCH] refactor(migrations): fix unique name generation not marking generated identifiers (#58126) The unique name generator did not properly work to avoid collisions with previously generated unique names. This commit fixes this and also improves type safety of the logic. PR Close #58126 --- .../src/utils/is_identifier_free_in_scope.ts | 11 ++++--- .../src/utils/unique_names.ts | 5 +-- .../test/golden-test/temporary_variables.ts | 23 +++++++++++++ .../signal-migration/test/golden.txt | 33 +++++++++++++++++-- .../test/golden_best_effort.txt | 33 +++++++++++++++++-- 5 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 packages/core/schematics/migrations/signal-migration/test/golden-test/temporary_variables.ts diff --git a/packages/core/schematics/migrations/signal-migration/src/utils/is_identifier_free_in_scope.ts b/packages/core/schematics/migrations/signal-migration/src/utils/is_identifier_free_in_scope.ts index 334142153d6..ea5ed640c5a 100644 --- a/packages/core/schematics/migrations/signal-migration/src/utils/is_identifier_free_in_scope.ts +++ b/packages/core/schematics/migrations/signal-migration/src/utils/is_identifier_free_in_scope.ts @@ -10,9 +10,12 @@ import assert from 'assert'; import ts from 'typescript'; import {isNodeDescendantOf} from './is_descendant_of'; +/** Symbol that can be used to mark a variable as reserved, synthetically. */ +export const ReservedMarker = Symbol(); + // typescript/stable/src/compiler/types.ts;l=967;rcl=651008033 export interface LocalsContainer extends ts.Node { - locals?: Map; + locals?: Map; nextContainer?: LocalsContainer; } @@ -71,9 +74,9 @@ function isIdentifierFreeInContainer(name: string, container: LocalsContainer): // 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) + return ( + local !== ReservedMarker && + !(local.flags & (ts.SymbolFlags.Value | ts.SymbolFlags.ExportValue | ts.SymbolFlags.Alias)) ); } diff --git a/packages/core/schematics/migrations/signal-migration/src/utils/unique_names.ts b/packages/core/schematics/migrations/signal-migration/src/utils/unique_names.ts index 7b0bb988c0f..2277083931d 100644 --- a/packages/core/schematics/migrations/signal-migration/src/utils/unique_names.ts +++ b/packages/core/schematics/migrations/signal-migration/src/utils/unique_names.ts @@ -7,7 +7,7 @@ */ import ts from 'typescript'; -import {isIdentifierFreeInScope} from './is_identifier_free_in_scope'; +import {isIdentifierFreeInScope, ReservedMarker} from './is_identifier_free_in_scope'; /** * Helper that can generate unique identifier names at a @@ -27,7 +27,8 @@ export class UniqueNamesGenerator { } // Claim the locals to avoid conflicts with future generations. - freeInfo.container.locals?.set(name, null! as ts.Symbol); + freeInfo.container.locals ??= new Map(); + freeInfo.container.locals.set(name, ReservedMarker); return true; }; diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/temporary_variables.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/temporary_variables.ts new file mode 100644 index 00000000000..ddf0a63707f --- /dev/null +++ b/packages/core/schematics/migrations/signal-migration/test/golden-test/temporary_variables.ts @@ -0,0 +1,23 @@ +// tslint:disable + +import {Directive, Input} from '@angular/core'; + +export class OtherCmp { + @Input() name = false; +} + +@Directive() +export class MyComp { + @Input() name = ''; + other: OtherCmp = null!; + + click() { + if (this.name) { + console.error(this.name); + } + + if (this.other.name) { + console.error(this.other.name); + } + } +} diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index f2fd5803be7..0afbbfb0a92 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -602,12 +602,12 @@ export class AppComponent { if (isAudi(car)) { console.log(car.__audi); } - const narrowableMultipleTimes = ctx.narrowableMultipleTimes(); - if (!isCar(narrowableMultipleTimes!) || !isAudi(narrowableMultipleTimes)) { + const narrowableMultipleTimesValue = ctx.narrowableMultipleTimes(); + if (!isCar(narrowableMultipleTimesValue!) || !isAudi(narrowableMultipleTimesValue)) { return; } - narrowableMultipleTimes.__audi; + narrowableMultipleTimesValue.__audi; } // iife @@ -1249,6 +1249,33 @@ class TwoWayBinding { @Input() inputC = false; readonly inputD = input(false); } +@@@@@@ temporary_variables.ts @@@@@@ + +// tslint:disable + +import {Directive, input} from '@angular/core'; + +export class OtherCmp { + readonly name = input(false); +} + +@Directive() +export class MyComp { + readonly name = input(''); + other: OtherCmp = null!; + + click() { + const name = this.name(); + if (name) { + console.error(name); + } + + const nameValue = this.other.name(); + if (nameValue) { + console.error(nameValue); + } + } +} @@@@@@ transform_functions.ts @@@@@@ // tslint:disable diff --git a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt index 29f26627cea..61d8945a34d 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt @@ -576,12 +576,12 @@ export class AppComponent { if (isAudi(car)) { console.log(car.__audi); } - const narrowableMultipleTimes = ctx.narrowableMultipleTimes(); - if (!isCar(narrowableMultipleTimes!) || !isAudi(narrowableMultipleTimes)) { + const narrowableMultipleTimesValue = ctx.narrowableMultipleTimes(); + if (!isCar(narrowableMultipleTimesValue!) || !isAudi(narrowableMultipleTimesValue)) { return; } - narrowableMultipleTimes.__audi; + narrowableMultipleTimesValue.__audi; } // iife @@ -1201,6 +1201,33 @@ class TwoWayBinding { readonly inputC = input(false); readonly inputD = input(false); } +@@@@@@ temporary_variables.ts @@@@@@ + +// tslint:disable + +import {Directive, input} from '@angular/core'; + +export class OtherCmp { + readonly name = input(false); +} + +@Directive() +export class MyComp { + readonly name = input(''); + other: OtherCmp = null!; + + click() { + const name = this.name(); + if (name) { + console.error(name); + } + + const nameValue = this.other.name(); + if (nameValue) { + console.error(nameValue); + } + } +} @@@@@@ transform_functions.ts @@@@@@ // tslint:disable