From 9d0fe9d26a805d2e2bffbbf4a434ff90fe950b50 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 8 Aug 2024 15:41:06 +0000 Subject: [PATCH] refactor(migrations): detect if an input is not narrowed and can be migrated (#57308) By default, we don't migrate inputs if they are part of e.g. `@if` for now. That is because we don't have the template narrowing feature available yet. To improve impact of the migration until we have the narrowing, we add some additional checks that allow us to migrate instances of inputs that are part of e.g. `@if` but are actually not used inside (and hence are guaranteed to be _not_ narrowed). PR Close #57308 --- .../template_reference_visitor.ts | 168 ++++++++++-------- .../references/identify_host_references.ts | 5 +- .../identify_template_references.ts | 5 +- .../test/golden-test/template_ng_if.ts | 10 ++ .../signal-migration/test/golden.txt | 12 +- 5 files changed, 119 insertions(+), 81 deletions(-) diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/template_reference_visitor.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/template_reference_visitor.ts index 80161bb5b9e..7e38d1a334c 100644 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/template_reference_visitor.ts +++ b/packages/core/schematics/migrations/signal-migration/src/input_detection/template_reference_visitor.ts @@ -38,7 +38,7 @@ import { import {KnownInputs} from './known_inputs'; import {attemptRetrieveInputFromSymbol} from './nodes_to_input'; import {MigrationHost} from '../migration_host'; -import {InputDescriptor} from '../utils/input_id'; +import {InputDescriptor, InputUniqueKey} from '../utils/input_id'; import {InputIncompatibilityReason} from './incompatibility'; import {BoundAttribute, BoundEvent} from '../../../../../../compiler/src/render3/r3_ast'; @@ -51,8 +51,8 @@ export interface TmplInputExpressionReference { targetInput: InputDescriptor; read: PropertyRead; context: ExprContext; - isInsideNarrowingExpression: boolean; isObjectShorthandExpression: boolean; + isLikelyNarrowed: boolean; } /** @@ -67,12 +67,15 @@ export class TemplateReferenceVisitor extends TmplAstRecursiveVisitor { /** * Whether we are currently descending into HTML AST nodes - * where bound attributes should be considered "narrowed". + * where all bound attributes are considered potentially narrowing. * - * TODO: Remove with: https://github.com/angular/angular/pull/55456. + * Keeps track of all referenced inputs in such attribute expressions. */ - private isAttributeInsideNarrowingExpression = false; + private templateAttributeReferencedInputs: TmplInputExpressionReference[] | null = + null; + private expressionVisitor: TemplateExpressionReferenceVisitor; + private seenInputsCount = new Map(); constructor( host: MigrationHost, @@ -88,16 +91,56 @@ export class TemplateReferenceVisitor extends TmplAstRecursiveVisitor { templateTypeChecker, componentClass, knownInputs, - this.result, ); } + private checkExpressionForReferencedInputs(activeNode: TmplAstNode, expressionNode: AST) { + const referencedInputs = this.expressionVisitor.checkTemplateExpression( + activeNode, + expressionNode, + ); + // Add all references to the overall visitor result. + this.result.push(...referencedInputs); + + // Count usages of seen input references. We'll use this to make decisions + // based on whether inputs are potentially narrowed or not. + for (const input of referencedInputs) { + this.seenInputsCount.set( + input.targetInput.key, + (this.seenInputsCount.get(input.targetInput.key) ?? 0) + 1, + ); + } + + return referencedInputs; + } + + private descendAndCheckForNarrowedSimilarReferences( + potentiallyNarrowedInputs: TmplInputExpressionReference[], + descend: () => void, + ) { + const inputs = potentiallyNarrowedInputs.map((i) => ({ + ref: i, + key: i.targetInput.key, + pastCount: this.seenInputsCount.get(i.targetInput.key) ?? 0, + })); + + descend(); + + for (const input of inputs) { + // Input was referenced inside a narrowable spot, and is used in child nodes. + // This is a sign for the input to be narrowed. Mark it as such. + if ((this.seenInputsCount.get(input.key) ?? 0) > input.pastCount) { + input.ref.isLikelyNarrowed = true; + } + } + } + override visitTemplate(template: TmplAstTemplate): void { // Note: We assume all bound expressions for templates may be subject // to TCB narrowing. This is relevant for now until we support narrowing // of signal calls in templates. // TODO: Remove with: https://github.com/angular/angular/pull/55456. - this.isAttributeInsideNarrowingExpression = true; + this.templateAttributeReferencedInputs = []; tmplAstVisitAll(this, template.attributes); tmplAstVisitAll(this, template.templateAttrs); @@ -110,98 +153,77 @@ export class TemplateReferenceVisitor extends TmplAstRecursiveVisitor { tmplAstVisitAll(this, template.outputs); } - this.isAttributeInsideNarrowingExpression = false; + const referencedInputs = this.templateAttributeReferencedInputs; + this.templateAttributeReferencedInputs = null; - tmplAstVisitAll(this, template.children); - tmplAstVisitAll(this, template.references); - tmplAstVisitAll(this, template.variables); + this.descendAndCheckForNarrowedSimilarReferences(referencedInputs, () => { + tmplAstVisitAll(this, template.children); + tmplAstVisitAll(this, template.references); + tmplAstVisitAll(this, template.variables); + }); } override visitIfBlockBranch(block: TmplAstIfBlockBranch): void { if (block.expression) { - this.expressionVisitor.checkTemplateExpression( - block, - /* isInsideNarrowingExpression */ true, - block.expression, - ); + const referencedInputs = this.checkExpressionForReferencedInputs(block, block.expression); + this.descendAndCheckForNarrowedSimilarReferences(referencedInputs, () => { + super.visitIfBlockBranch(block); + }); + } else { + super.visitIfBlockBranch(block); } - super.visitIfBlockBranch(block); } override visitForLoopBlock(block: TmplAstForLoopBlock): void { - this.expressionVisitor.checkTemplateExpression( - block, - /* isInsideNarrowingExpression */ false, - block.expression, - ); - this.expressionVisitor.checkTemplateExpression( - block, - /* isInsideNarrowingExpression */ false, - block.trackBy, - ); + this.checkExpressionForReferencedInputs(block, block.expression); + this.checkExpressionForReferencedInputs(block, block.trackBy); super.visitForLoopBlock(block); } override visitSwitchBlock(block: TmplAstSwitchBlock): void { - this.expressionVisitor.checkTemplateExpression( - block, - /* isInsideNarrowingExpression */ true, - block.expression, - ); - super.visitSwitchBlock(block); + const referencedInputs = this.checkExpressionForReferencedInputs(block, block.expression); + this.descendAndCheckForNarrowedSimilarReferences(referencedInputs, () => { + super.visitSwitchBlock(block); + }); } override visitSwitchBlockCase(block: TmplAstSwitchBlockCase): void { if (block.expression) { - this.expressionVisitor.checkTemplateExpression( - block, - /* isInsideNarrowingExpression */ true, - block.expression, - ); + const referencedInputs = this.checkExpressionForReferencedInputs(block, block.expression); + this.descendAndCheckForNarrowedSimilarReferences(referencedInputs, () => { + super.visitSwitchBlockCase(block); + }); + } else { + super.visitSwitchBlockCase(block); } - super.visitSwitchBlockCase(block); } override visitDeferredBlock(deferred: TmplAstDeferredBlock): void { if (deferred.triggers.when) { - this.expressionVisitor.checkTemplateExpression( - deferred, - /* isInsideNarrowingExpression */ false, - deferred.triggers.when.value, - ); + this.checkExpressionForReferencedInputs(deferred, deferred.triggers.when.value); } if (deferred.prefetchTriggers.when) { - this.expressionVisitor.checkTemplateExpression( - deferred, - /* isInsideNarrowingExpression */ false, - deferred.prefetchTriggers.when.value, - ); + this.checkExpressionForReferencedInputs(deferred, deferred.prefetchTriggers.when.value); } super.visitDeferredBlock(deferred); } override visitBoundText(text: TmplAstBoundText): void { - this.expressionVisitor.checkTemplateExpression( - text, - /* isInsideNarrowingExpression */ false, - text.value, - ); + this.checkExpressionForReferencedInputs(text, text.value); } override visitBoundEvent(attribute: TmplAstBoundEvent): void { - this.expressionVisitor.checkTemplateExpression( - attribute, - /* isInsideNarrowingExpression */ false, - attribute.handler, - ); + this.checkExpressionForReferencedInputs(attribute, attribute.handler); } override visitBoundAttribute(attribute: TmplAstBoundAttribute): void { - this.expressionVisitor.checkTemplateExpression( - attribute, - /* isInsideNarrowingExpression */ this.isAttributeInsideNarrowingExpression, - attribute.value, - ); + const referencedInputs = this.checkExpressionForReferencedInputs(attribute, attribute.value); + + // Attributes inside templates are potentially "narrowed" and hence we + // keep track of all referenced inputs to see if they actually are. + if (this.templateAttributeReferencedInputs !== null) { + this.templateAttributeReferencedInputs.push(...referencedInputs); + } } } @@ -214,7 +236,8 @@ export class TemplateReferenceVisitor extends TmplAstRecursiveVisitor { */ export class TemplateExpressionReferenceVisitor extends RecursiveAstVisitor { private activeTmplAstNode: ExprContext | null = null; - private isInsideNarrowingExpression = false; + private detectedInputReferences: TmplInputExpressionReference[] = []; + private isInsideObjectShorthandExpression = false; constructor( @@ -223,7 +246,6 @@ export class TemplateExpressionReferenceVisitor extends RecursiveAs private templateTypeChecker: TemplateTypeChecker | null, private componentClass: ts.ClassDeclaration, private knownInputs: KnownInputs, - private result: TmplInputExpressionReference[], ) { super(); } @@ -231,13 +253,13 @@ export class TemplateExpressionReferenceVisitor extends RecursiveAs /** Checks the given AST expression. */ checkTemplateExpression( activeNode: ExprContext, - isInsideNarrowingExpression: boolean, expressionNode: AST, - ) { + ): TmplInputExpressionReference[] { + this.detectedInputReferences = []; this.activeTmplAstNode = activeNode; - this.isInsideNarrowingExpression = isInsideNarrowingExpression; expressionNode.visit(this); + return this.detectedInputReferences; } // Keep track when we are inside an object shorthand expression. This is @@ -316,12 +338,12 @@ export class TemplateExpressionReferenceVisitor extends RecursiveAs return null; } - this.result.push({ + this.detectedInputReferences.push({ target: targetInput.descriptor.node, targetInput: targetInput.descriptor, read: ast, context: this.activeTmplAstNode!, - isInsideNarrowingExpression: this.isInsideNarrowingExpression, + isLikelyNarrowed: false, isObjectShorthandExpression: this.isInsideObjectShorthandExpression, }); @@ -365,12 +387,12 @@ export class TemplateExpressionReferenceVisitor extends RecursiveAs return null; } - this.result.push({ + this.detectedInputReferences.push({ target: matchingTarget.descriptor.node, targetInput: matchingTarget.descriptor, read: ast, context: this.activeTmplAstNode!, - isInsideNarrowingExpression: this.isInsideNarrowingExpression, + isLikelyNarrowed: false, isObjectShorthandExpression: this.isInsideObjectShorthandExpression, }); return matchingTarget.descriptor; diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_host_references.ts b/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_host_references.ts index 955814afd77..f7e9a38838d 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_host_references.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_host_references.ts @@ -95,13 +95,12 @@ export function identifyHostBindingReferences( } const hostMap = reflectObjectLiteral(hostField); const expressionResult: TmplInputExpressionReference[] = []; - const expressionVisitor = new TemplateExpressionReferenceVisitor( + const expressionVisitor = new TemplateExpressionReferenceVisitor( host, checker, null, node, knownDecoratorInputs, - expressionResult, ); for (const [rawName, expression] of hostMap.entries()) { @@ -158,7 +157,7 @@ export function identifyHostBindingReferences( } if (parsed != null) { - expressionVisitor.checkTemplateExpression(expression, false, parsed); + expressionResult.push(...expressionVisitor.checkTemplateExpression(expression, parsed)); } } diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_template_references.ts b/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_template_references.ts index 7bdfbb4d453..e9a50c44348 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_template_references.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/references/identify_template_references.ts @@ -55,10 +55,7 @@ export function identifyTemplateReferences( // TODO: Remove this when we support signal narrowing in templates. // https://github.com/angular/angular/pull/55456. - if ( - process.env['MIGRATE_NARROWED_NARROWED_IN_TEMPLATES'] !== '1' && - res.isInsideNarrowingExpression - ) { + if (process.env['MIGRATE_NARROWED_NARROWED_IN_TEMPLATES'] !== '1' && res.isLikelyNarrowed) { knownInputs.markInputAsIncompatible(res.targetInput, { reason: InputIncompatibilityReason.NarrowedInTemplateButNotSupportedYetTODO, context: null, diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/template_ng_if.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/template_ng_if.ts index a6c6e733a0b..366458f0bf8 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden-test/template_ng_if.ts +++ b/packages/core/schematics/migrations/signal-migration/test/golden-test/template_ng_if.ts @@ -15,10 +15,20 @@ import {Component, Input} from '@angular/core';
{{third}}
+ +
+ {{notTheInput}} +
+ + @if (fifth) { + {{notTheInput}} + } `, }) export class MyComp { @Input() first = true; @Input() second = false; @Input() third = true; + @Input() fourth = true; + @Input() fifth = true; } diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index 48b08927005..abc4bc09b82 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -848,7 +848,7 @@ spyOn(new MyComp(), 'myInput').and.returnValue(); // tslint:disable -import {Component, Input} from '@angular/core'; +import {Component, Input, input} from '@angular/core'; @Component({ template: ` @@ -863,12 +863,22 @@ import {Component, Input} from '@angular/core';
{{third}}
+ +
+ {{notTheInput}} +
+ + @if (fifth()) { + {{notTheInput}} + } `, }) export class MyComp { @Input() first = true; @Input() second = false; @Input() third = true; + fourth = input(true); + fifth = input(true); } @@@@@@ template_object_shorthand.ts @@@@@@