From f63c0f6e1c50deea3c6747cf10c321717d561ba7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 17 Nov 2023 20:34:40 +0100 Subject: [PATCH] Revert "fix(compiler-cli): add diagnostic for control flow that prevents content projection (#52726)" (#53012) This reverts commit b4d022e230ca141b12437949d11dc384bfe5c082. PR Close #53012 --- goldens/public-api/compiler-cli/error_code.md | 1 - .../src/ngtsc/diagnostics/src/error_code.ts | 15 - .../src/ngtsc/typecheck/src/oob.ts | 38 +-- .../ngtsc/typecheck/src/type_check_block.ts | 113 +------ .../src/ngtsc/typecheck/testing/index.ts | 1 - .../test/ngtsc/template_typecheck_spec.ts | 299 ------------------ 6 files changed, 2 insertions(+), 465 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index 24007655bce9c..ca3dd2a1c3257 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -25,7 +25,6 @@ export enum ErrorCode { // (undocumented) CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, CONFLICTING_INPUT_TRANSFORM = 2020, - CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011, // (undocumented) DECORATOR_ARG_NOT_LITERAL = 1001, // (undocumented) diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index f5ec27d15ba63..3f523a09eb087 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -286,21 +286,6 @@ export enum ErrorCode { */ INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT = 8010, - /** - * A control flow node is projected at the root of a component and is preventing its direct - * descendants from being projected, because it has more than one root node. - * - * ``` - * - * @if (expr) { - *
- * Text preventing the div from being projected - * } - * - * ``` - */ - CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011, - /** * A two way binding in a template has an incorrect syntax, * parentheses outside brackets. For example: diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 8adee446fb9b4..7bbe04cceafeb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; +import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; import ts from 'typescript'; import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics'; @@ -100,14 +100,6 @@ export interface OutOfBandDiagnosticRecorder { templateId: TemplateId, trigger: TmplAstHoverDeferredTrigger|TmplAstInteractionDeferredTrigger| TmplAstViewportDeferredTrigger): void; - - /** - * Reports cases where control flow nodes prevent content projection. - */ - controlFlowPreventingContentProjection( - templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, - slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock, - preservesWhitespaces: boolean): void; } export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { @@ -348,34 +340,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT), message)); } - - controlFlowPreventingContentProjection( - templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, - slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock, - preservesWhitespaces: boolean): void { - const blockName = controlFlowNode instanceof TmplAstIfBlockBranch ? '@if' : '@for'; - const lines = [ - `Node matches the "${slotSelector}" slot of the "${ - componentName}" component, but will not be projected into the specific slot because the surrounding ${ - blockName} has more than one node at its root. To project the node in the right slot, you can:\n`, - `1. Wrap the content of the ${blockName} block in an that matches the "${ - slotSelector}" selector.`, - `2. Split the content of the ${blockName} block across multiple ${ - blockName} blocks such that each one only has a single projectable node at its root.`, - `3. Remove all content from the ${blockName} block, except for the node being projected.` - ]; - - if (preservesWhitespaces) { - lines.push( - `Note: the host component has \`preserveWhitespaces: true\` which may ` + - `cause whitespace to affect content projection.`); - } - - this._diagnostics.push(makeTemplateDiagnostic( - templateId, this.resolver.getSourceMapping(templateId), projectionNode.startSourceSpan, - ts.DiagnosticCategory.Warning, - ngErrorCode(ErrorCode.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION), lines.join('\n'))); - } } function makeInlineDiagnostic( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index c56e0bebece80..dda37e07da0fa 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, BindingPipe, BindingType, BoundTarget, Call, createCssSelectorFromNode, CssSelector, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, SelectorMatcher, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler'; +import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler'; import ts from 'typescript'; import {Reference} from '../../imports'; @@ -905,100 +905,6 @@ class TcbDomSchemaCheckerOp extends TcbOp { } -/** - * A `TcbOp` that finds and flags control flow nodes that interfere with content projection. - * - * Context: - * `@if` and `@for` try to emulate the content projection behavior of `*ngIf` and `*ngFor` - * in order to reduce breakages when moving from one syntax to the other (see #52414), however the - * approach only works if there's only one element at the root of the control flow expression. - * This means that a stray sibling node (e.g. text) can prevent an element from being projected - * into the right slot. The purpose of the `TcbOp` is to find any places where a node at the root - * of a control flow expression *would have been projected* into a specific slot, if the control - * flow node didn't exist. - */ -class TcbControlFlowContentProjectionOp extends TcbOp { - constructor( - private tcb: Context, private element: TmplAstElement, private ngContentSelectors: string[], - private componentName: string) { - super(); - } - - override readonly optional = false; - - override execute(): null { - const controlFlowToCheck = this.findPotentialControlFlowNodes(); - - if (controlFlowToCheck.length > 0) { - const matcher = new SelectorMatcher(); - - for (const selector of this.ngContentSelectors) { - // `*` is a special selector for the catch-all slot. - if (selector !== '*') { - matcher.addSelectables(CssSelector.parse(selector), selector); - } - } - - for (const root of controlFlowToCheck) { - for (const child of root.children) { - if (child instanceof TmplAstElement || child instanceof TmplAstTemplate) { - matcher.match(createCssSelectorFromNode(child), (_, originalSelector) => { - this.tcb.oobRecorder.controlFlowPreventingContentProjection( - this.tcb.id, child, this.componentName, originalSelector, root, - this.tcb.hostPreserveWhitespaces); - }); - } - } - } - } - - return null; - } - - private findPotentialControlFlowNodes() { - const result: Array = []; - - for (const child of this.element.children) { - let eligibleNode: TmplAstForLoopBlock|TmplAstIfBlockBranch|null = null; - - // Only `@for` blocks and the first branch of `@if` blocks participate in content projection. - if (child instanceof TmplAstForLoopBlock) { - eligibleNode = child; - } else if (child instanceof TmplAstIfBlock) { - eligibleNode = child.branches[0]; // @if blocks are guaranteed to have at least one branch. - } - - // Skip nodes with less than two children since it's impossible - // for them to run into the issue that we're checking for. - if (eligibleNode === null || eligibleNode.children.length < 2) { - continue; - } - - // Count the number of root nodes while skipping empty text where relevant. - const rootNodeCount = eligibleNode.children.reduce((count, node) => { - // Normally `preserveWhitspaces` would have been accounted for during parsing, however - // in `ngtsc/annotations/component/src/resources.ts#parseExtractedTemplate` we enable - // `preserveWhitespaces` to preserve the accuracy of source maps diagnostics. This means - // that we have to account for it here since the presence of text nodes affects the - // content projection behavior. - if (!(node instanceof TmplAstText) || this.tcb.hostPreserveWhitespaces || - node.value.trim().length > 0) { - count++; - } - - return count; - }, 0); - - // Content projection can only be affected if there is more than one root node. - if (rootNodeCount > 1) { - result.push(eligibleNode); - } - } - - return result; - } -} - /** * Mapping between attributes names that don't correspond to their element property names. * Note: this mapping has to be kept in sync with the equally named mapping in the runtime. @@ -1931,7 +1837,6 @@ class Scope { if (node instanceof TmplAstElement) { const opIndex = this.opQueue.push(new TcbElementOp(this.tcb, this, node)) - 1; this.elementOpMap.set(node, opIndex); - this.appendContentProjectionCheckOp(node); this.appendDirectivesAndInputsOfNode(node); this.appendOutputsOfNode(node); this.appendChildren(node); @@ -2128,22 +2033,6 @@ class Scope { } } - private appendContentProjectionCheckOp(root: TmplAstElement): void { - const meta = - this.tcb.boundTarget.getDirectivesOfNode(root)?.find(meta => meta.isComponent) || null; - - if (meta !== null && meta.ngContentSelectors !== null && meta.ngContentSelectors.length > 0) { - const selectors = meta.ngContentSelectors; - - // We don't need to generate anything for components that don't have projection - // slots, or they only have one catch-all slot (represented by `*`). - if (selectors.length > 1 || (selectors.length === 1 && selectors[0] !== '*')) { - this.opQueue.push( - new TcbControlFlowContentProjectionOp(this.tcb, root, selectors, meta.name)); - } - } - } - private appendDeferredBlock(block: TmplAstDeferredBlock): void { this.appendDeferredTriggers(block, block.triggers); this.appendDeferredTriggers(block, block.prefetchTriggers); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts index 46ba484496311..51bf3d900a359 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts @@ -800,5 +800,4 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { missingRequiredInputs(): void {} illegalForLoopTrackAccess(): void {} inaccessibleDeferredTriggerElement(): void {} - controlFlowPreventingContentProjection(): void {} } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 799f8921992d5..e3a9291589bec 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -4923,304 +4923,5 @@ suppress ]); }); }); - - describe('control flow content projection diagnostics', () => { - it('should report when an @if block prevents an element from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @if has more than one node at its root.`); - }); - - it('should report when an @if block prevents a template from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { - - breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @if has more than one node at its root.`); - }); - - it('should report when an @for block prevents content from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @for (i of [1, 2, 3]; track i) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @for has more than one node at its root.`); - }); - - it('should report nodes that are targeting different slots but cannot be projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
-
- } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(2); - expect(diags[0]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - expect(diags[1]).toContain( - `Node matches the "[bar]" slot of the "Comp" component, but will not be projected`); - }); - - it('should report nodes that are targeting the same slot but cannot be projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(2); - expect(diags[0]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - expect(diags[1]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - }); - - it('should report when preserveWhitespaces may affect content projection', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - preserveWhitespaces: true, - template: \` - - @if (true) { -
- } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain(`Node matches the "[foo]" slot of the "Comp" component`); - expect(diags[0]).toContain( - `Note: the host component has \`preserveWhitespaces: true\` which may cause ` + - `whitespace to affect content projection.`); - }); - - it('should not report when there is only one root node', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - - it('should not report when there are comments at the root of the control flow node', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { - -
- - } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - - it('should not report when the component only has a catch-all slot', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - }); }); });