From cdebf751e4949048b01acc92de2517f46fcd5d37 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 4 Jul 2024 11:30:59 +0200 Subject: [PATCH] fix(compiler-cli): used before declared diagnostic not firing for control flow blocks (#56843) When we process `@if` and `@for` blocks, we create a scope around their expressions in order to encapsulate the aliases to them. The problem is that this doesn't represent the actual structure since the expression is part of the outer scope. This surfaces by not raising the "used before declared" diagnostic for `@let` declarations. These changes resolve the issue by processing the expression as a part of the parent scope. Fixes #56842. PR Close #56843 --- .../ngtsc/typecheck/src/type_check_block.ts | 18 ++-- .../test/ngtsc/template_typecheck_spec.ts | 84 ++++++++++++++++++- 2 files changed, 92 insertions(+), 10 deletions(-) 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 021bcd83cd5..d8f5084c21b 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 @@ -1647,23 +1647,23 @@ class TcbIfOp extends TcbOp { return ts.factory.createBlock(branchScope.render()); } - // We need to process the expression first so it gets its own scope that the body of the - // conditional will inherit from. We do this, because we need to declare a separate variable + // We process the expression first in the parent scope, but create a scope around the block + // that the body will inherit from. We do this, because we need to declare a separate variable // for the case where the expression has an alias _and_ because we need the processed // expression when generating the guard for the body. - const expressionScope = Scope.forNodes(this.tcb, this.scope, branch, [], null); - expressionScope.render().forEach((stmt) => this.scope.addStatement(stmt)); - this.expressionScopes.set(branch, expressionScope); + const outerScope = Scope.forNodes(this.tcb, this.scope, branch, [], null); + outerScope.render().forEach((stmt) => this.scope.addStatement(stmt)); + this.expressionScopes.set(branch, outerScope); - let expression = tcbExpression(branch.expression, this.tcb, expressionScope); + let expression = tcbExpression(branch.expression, this.tcb, this.scope); if (branch.expressionAlias !== null) { expression = ts.factory.createBinaryExpression( ts.factory.createParenthesizedExpression(expression), ts.SyntaxKind.AmpersandAmpersandToken, - expressionScope.resolve(branch.expressionAlias), + outerScope.resolve(branch.expressionAlias), ); } - const bodyScope = this.getBranchScope(expressionScope, branch, index); + const bodyScope = this.getBranchScope(outerScope, branch, index); return ts.factory.createIfStatement( expression, @@ -1889,7 +1889,7 @@ class TcbForOfOp extends TcbOp { // It's common to have a for loop over a nullable value (e.g. produced by the `async` pipe). // Add a non-null expression to allow such values to be assigned. const expression = ts.factory.createNonNullExpression( - tcbExpression(this.block.expression, this.tcb, loopScope), + tcbExpression(this.block.expression, this.tcb, this.scope), ); const trackTranslator = new TcbForLoopTrackTranslator(this.tcb, loopScope, this.block); const trackExpression = trackTranslator.translate(this.block.trackBy); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 6004cc446d2..930844c75ce 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -5818,7 +5818,7 @@ suppress ]); }); - it('should not allow usages of aliased `if` block variables inside the tracking exprssion', () => { + it('should not allow usages of aliased `if` block variables inside the tracking expression', () => { env.write( '/test.ts', ` @@ -7342,6 +7342,88 @@ suppress const diags = env.driveDiagnostics(); expect(diags.length).toBe(0); }); + + it('should report @let declaration used in the expression of a @if block before it is defined', () => { + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + @if (value) { + Hello + } + @let value = 123; + \`, + standalone: true, + }) + export class Main {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe( + `Cannot read @let declaration 'value' before it has been defined.`, + ); + }); + + it('should report @let declaration used in the expression of a @for block before it is defined', () => { + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + @for (current of value; track $index) { + {{current}} + } + + @let value = [1, 2, 3]; + \`, + standalone: true, + }) + export class Main {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe( + `Cannot read @let declaration 'value' before it has been defined.`, + ); + }); + + it('should report @let declaration used in the expression of a @switch block before it is defined', () => { + env.write( + 'test.ts', + ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + @switch (value) { + @case (123) { + Hello + } + } + + @let value = [1, 2, 3]; + \`, + standalone: true, + }) + export class Main {} + `, + ); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe( + `Cannot read @let declaration 'value' before it has been defined.`, + ); + }); }); }); });