From eddf5dae5eb9e1aa3ca4c276c2cb2b897b73a9e0 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 17 Jan 2024 17:00:06 -0800 Subject: [PATCH] fix(compiler): Update type check block to fix control flow source mappings (#53980) The source mappings and types for various pieces of the control flow were not quite right and prevented the language service from providing accurate information. fixes https://github.com/angular/vscode-ng-language-service/issues/1988 PR Close #53980 --- .../src/ngtsc/typecheck/src/comments.ts | 1 + .../typecheck/src/template_symbol_builder.ts | 20 ++- .../src/ngtsc/typecheck/src/ts_util.ts | 19 +- .../ngtsc/typecheck/src/type_check_block.ts | 13 +- .../typecheck/test/span_comments_spec.ts | 33 +++- .../typecheck/test/type_check_block_spec.ts | 110 ++++++------ ...ecker__get_symbol_of_template_node_spec.ts | 170 +++++++++++++++++- .../compiler/src/render3/r3_control_flow.ts | 57 +++++- .../test/render3/r3_ast_spans_spec.ts | 20 ++- .../src/references_and_rename_utils.ts | 14 ++ .../language-service/test/quick_info_spec.ts | 25 +++ .../test/references_and_rename_spec.ts | 36 ++++ 12 files changed, 429 insertions(+), 89 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts index fd9117ac8ce9b..a28dd0d8bb2eb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts @@ -44,6 +44,7 @@ export enum ExpressionIdentifier { DIRECTIVE = 'DIR', COMPONENT_COMPLETION = 'COMPCOMP', EVENT_PARAMETER = 'EP', + VARIABLE_AS_EXPRESSION = 'VAE', } /** Tags the node with the given expression identifier. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index 0776c1843bc4d..8c7ceb58e4833 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -111,7 +111,7 @@ export class SymbolBuilder { const elementSourceSpan = element.startSourceSpan ?? element.sourceSpan; const tcbSourceFile = this.typeCheckBlock.getSourceFile(); // directives could be either: - // - var _t1: TestDir /*T:D*/ = (null!); + // - var _t1: TestDir /*T:D*/ = null! as TestDir; // - var _t1 /*T:D*/ = _ctor1({}); const isDirectiveDeclaration = (node: ts.Node): node is ts.TypeNode|ts.Identifier => (ts.isTypeNode(node) || ts.isIdentifier(node)) && ts.isVariableDeclaration(node.parent) && @@ -429,19 +429,25 @@ export class SymbolBuilder { private getSymbolOfVariable(variable: TmplAstVariable): VariableSymbol|null { const node = findFirstMatchingNode( this.typeCheckBlock, {withSpan: variable.sourceSpan, filter: ts.isVariableDeclaration}); - if (node === null || node.initializer === undefined) { + if (node === null) { return null; } - const expressionSymbol = this.getSymbolOfTsNode(node.initializer); - if (expressionSymbol === null) { + let nodeValueSymbol: TsNodeSymbolInfo|null = null; + if (ts.isForOfStatement(node.parent.parent)) { + nodeValueSymbol = this.getSymbolOfTsNode(node); + } else if (node.initializer !== undefined) { + nodeValueSymbol = this.getSymbolOfTsNode(node.initializer); + } + + if (nodeValueSymbol === null) { return null; } return { - tsType: expressionSymbol.tsType, - tsSymbol: expressionSymbol.tsSymbol, - initializerLocation: expressionSymbol.tcbLocation, + tsType: nodeValueSymbol.tsType, + tsSymbol: nodeValueSymbol.tsSymbol, + initializerLocation: nodeValueSymbol.tcbLocation, kind: SymbolKind.Variable, declaration: variable, localVarLocation: { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts index 7b496f0f529b6..eae3512b129ef 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts @@ -8,6 +8,10 @@ import ts from 'typescript'; +import {addExpressionIdentifier, ExpressionIdentifier} from './comments'; +import {wrapForTypeChecker} from './diagnostics'; + + /** * A `Set` of `ts.SyntaxKind`s of `ts.Expression` which are safe to wrap in a `ts.AsExpression` @@ -77,20 +81,17 @@ export function tsCreateElement(tagName: string): ts.Expression { * Unlike with `tsCreateVariable`, the type of the variable is explicitly specified. */ export function tsDeclareVariable(id: ts.Identifier, type: ts.TypeNode): ts.VariableStatement { - let initializer: ts.Expression = ts.factory.createNonNullExpression(ts.factory.createNull()); - // When we create a variable like `var _t1: boolean = null!`, TypeScript actually infers `_t1` - // to be `never`, instead of a `boolean`. To work around it, we cast the value to boolean again - // in the initializer, e.g. `var _t1: boolean = null! as boolean;`. - if (type.kind === ts.SyntaxKind.BooleanKeyword) { - initializer = ts.factory.createAsExpression( - initializer, ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword)); - } + // to be `never`, instead of a `boolean`. To work around it, we cast the value + // in the initializer, e.g. `var _t1 = null! as boolean;`. + addExpressionIdentifier(type, ExpressionIdentifier.VARIABLE_AS_EXPRESSION); + const initializer: ts.Expression = ts.factory.createAsExpression( + ts.factory.createNonNullExpression(ts.factory.createNull()), type); const decl = ts.factory.createVariableDeclaration( /* name */ id, /* exclamationToken */ undefined, - /* type */ type, + /* type */ undefined, /* initializer */ initializer); return ts.factory.createVariableStatement( /* modifiers */ undefined, 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 390c88f230846..97b451c5d25d3 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 @@ -465,8 +465,8 @@ abstract class TcbDirectiveTypeOpBase extends TcbOp { } const id = this.tcb.allocateId(); - addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE); - addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan); + addExpressionIdentifier(id, ExpressionIdentifier.DIRECTIVE); + addParseSpanInfo(id, this.node.startSourceSpan || this.node.sourceSpan); this.scope.addStatement(tsDeclareVariable(id, type)); return id; } @@ -1332,7 +1332,9 @@ class TcbBlockVariableOp extends TcbOp { override execute(): ts.Identifier { const id = this.tcb.allocateId(); addParseSpanInfo(id, this.variable.keySpan); - this.scope.addStatement(tsCreateVariable(id, this.initializer)); + const variable = tsCreateVariable(id, wrapForTypeChecker(this.initializer)); + addParseSpanInfo(variable.declarationList.declarations[0], this.variable.sourceSpan); + this.scope.addStatement(variable); return id; } } @@ -1355,7 +1357,9 @@ class TcbBlockImplicitVariableOp extends TcbOp { override execute(): ts.Identifier { const id = this.tcb.allocateId(); addParseSpanInfo(id, this.variable.keySpan); - this.scope.addStatement(tsDeclareVariable(id, this.type)); + const variable = tsDeclareVariable(id, this.type); + addParseSpanInfo(variable.declarationList.declarations[0], this.variable.sourceSpan); + this.scope.addStatement(variable); return id; } } @@ -1610,6 +1614,7 @@ class TcbForOfOp extends TcbOp { } const initializer = ts.factory.createVariableDeclarationList( [ts.factory.createVariableDeclaration(initializerId)], ts.NodeFlags.Const); + addParseSpanInfo(initializer, this.block.item.keySpan); // 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( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts index 22a914f14f350..84b99065da44e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts @@ -173,7 +173,7 @@ describe('type check blocks diagnostics', () => { pipeName: 'test', }]; const block = tcbWithSpans(TEMPLATE, PIPES); - expect(block).toContain('var _pipe1: i0.TestPipe = null!'); + expect(block).toContain('var _pipe1 = null! as i0.TestPipe'); expect(block).toContain( '(_pipe1.transform /*7,11*/(((this).a /*3,4*/) /*3,4*/, ((this).b /*12,13*/) /*12,13*/) /*3,13*/);'); }); @@ -215,6 +215,37 @@ describe('type check blocks diagnostics', () => { .toContain('_t1.inputA /*9,15*/ = ("" /*18,20*/) /*8,21*/;'); }); }); + + describe('control flow', () => { + it('@for', () => { + const template = `@for (user of users; track user; let i = $index) { {{i}} }`; + // user variable + expect(tcbWithSpans(template, [])).toContain('const _t1 /*6,10*/'); + expect(tcbWithSpans(template, [])).toContain('(this).users /*14,19*/'); + // index variable + expect(tcbWithSpans(template, [])) + .toContain('var _t2 /*37,38*/ = null! as number /*T:VAE*/ /*37,47*/'); + // track expression + expect(tcbWithSpans(template, [])).toContain('_t1 /*27,31*/;'); + }); + + it('@for with comma separated variables', () => { + const template = + `@for (x of y; track x; let i = $index, odd = $odd,e_v_e_n=$even) { {{i + odd + e_v_e_n}} }`; + expect(tcbWithSpans(template, [])) + .toContain('var _t2 /*27,28*/ = null! as number /*T:VAE*/ /*27,37*/'); + expect(tcbWithSpans(template, [])) + .toContain('var _t3 /*39,42*/ = null! as boolean /*T:VAE*/ /*39,54*/'); + expect(tcbWithSpans(template, [])) + .toContain('var _t4 /*55,62*/ = null! as boolean /*T:VAE*/ /*55,68*/'); + }); + + it('@if', () => { + const template = `@if (x; as alias) { {{alias}} }`; + expect(tcbWithSpans(template, [])).toContain('var _t1 /*11,16*/'); + expect(tcbWithSpans(template, [])).toContain('(this).x /*5,6*/'); + }); + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 27f40c70b9326..54bf998d26da0 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -67,7 +67,7 @@ describe('type check blocks', () => { selector: '[dir]', inputs: {inputA: 'inputA'}, }]; - expect(tcb(TEMPLATE, DIRECTIVES)).toContain('_t1: i0.DirA = null!; _t1.inputA = ("value");'); + expect(tcb(TEMPLATE, DIRECTIVES)).toContain('_t1 = null! as i0.DirA; _t1.inputA = ("value");'); }); it('should handle multiple bindings to the same property', () => { @@ -275,7 +275,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: typeof i0.Dir.ngAcceptInputType_fieldA = null!; ' + + 'var _t1 = null! as typeof i0.Dir.ngAcceptInputType_fieldA; ' + '_t1 = (((this).foo));'); }); }); @@ -335,11 +335,11 @@ describe('type check blocks', () => { }, ]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.HasInput = null!'); + expect(block).toContain('var _t1 = null! as i0.HasInput'); expect(block).toContain('_t1.input = (((this).value));'); - expect(block).toContain('var _t2: i0.HasOutput = null!'); + expect(block).toContain('var _t2 = null! as i0.HasOutput'); expect(block).toContain('_t2["output"]'); - expect(block).toContain('var _t4: i0.HasReference = null!'); + expect(block).toContain('var _t4 = null! as i0.HasReference'); expect(block).toContain('var _t3 = _t4;'); expect(block).toContain('(_t3).a'); expect(block).not.toContain('NoBindings'); @@ -369,7 +369,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t2: i0.Dir = null!; ' + + 'var _t2 = null! as i0.Dir; ' + 'var _t1 = _t2; ' + '"" + (((_t1).value));'); }); @@ -397,7 +397,7 @@ describe('type check blocks', () => { inputs: {'color': 'color', 'strong': 'strong', 'enabled': 'enabled'}, }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).not.toContain('var _t1: Dir = null!;'); + expect(block).not.toContain('var _t1 = null! as Dir;'); expect(block).not.toContain('"color"'); expect(block).not.toContain('"strong"'); expect(block).not.toContain('"enabled"'); @@ -417,7 +417,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t2: i0.Dir = null!; ' + + 'var _t2 = null! as i0.Dir; ' + 'var _t1 = _t2; ' + '_t2.input = (_t1);'); }); @@ -445,9 +445,9 @@ describe('type check blocks', () => { ]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t2: i0.DirB = null!; ' + + 'var _t2 = null! as i0.DirB; ' + 'var _t1 = _t2; ' + - 'var _t3: i0.DirA = null!; ' + + 'var _t3 = null! as i0.DirA; ' + '_t3.inputA = (_t1); ' + 'var _t4 = _t3; ' + '_t2.inputA = (_t4);'); @@ -465,7 +465,7 @@ describe('type check blocks', () => { undeclaredInputFields: ['fieldA'] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).not.toContain('var _t1: Dir = null!;'); + expect(block).not.toContain('var _t1 = null! as Dir;'); expect(block).toContain('(((this).foo)); '); }); @@ -482,8 +482,8 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: i0.Dir = null!; ' + - 'var _t2: (typeof _t1)["fieldA"] = null!; ' + + 'var _t1 = null! as i0.Dir; ' + + 'var _t2 = null! as (typeof _t1)["fieldA"]; ' + '_t2 = (((this).foo)); '); }); @@ -501,7 +501,7 @@ describe('type check blocks', () => { }]; const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - 'var _t1: i0.Dir = null!; ' + + 'var _t1 = null! as i0.Dir; ' + '_t1["some-input.xs"] = (((this).foo)); '); }); @@ -518,7 +518,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: i0.Dir = null!; ' + + 'var _t1 = null! as i0.Dir; ' + '_t1.field2 = _t1.field1 = (((this).foo));'); }); @@ -537,8 +537,8 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: typeof i0.Dir.ngAcceptInputType_field1 = null!; ' + - 'var _t2: i0.Dir = null!; ' + + 'var _t1 = null! as typeof i0.Dir.ngAcceptInputType_field1; ' + + 'var _t2 = null! as i0.Dir; ' + '_t2.field2 = _t1 = (((this).foo));'); }); @@ -557,7 +557,7 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: i0.Dir = null!; ' + + 'var _t1 = null! as i0.Dir; ' + '_t1.field2 = (((this).foo));'); }); @@ -573,9 +573,9 @@ describe('type check blocks', () => { coercedInputFields: ['fieldA'], }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).not.toContain('var _t1: Dir = null!;'); + expect(block).not.toContain('var _t1 = null! as Dir;'); expect(block).toContain( - 'var _t1: typeof i0.Dir.ngAcceptInputType_fieldA = null!; ' + + 'var _t1 = null! as typeof i0.Dir.ngAcceptInputType_fieldA; ' + '_t1 = (((this).foo));'); }); @@ -592,9 +592,9 @@ describe('type check blocks', () => { undeclaredInputFields: ['fieldA'], }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).not.toContain('var _t1: Dir = null!;'); + expect(block).not.toContain('var _t1 = null! as Dir;'); expect(block).toContain( - 'var _t1: typeof i0.Dir.ngAcceptInputType_fieldA = null!; ' + + 'var _t1 = null! as typeof i0.Dir.ngAcceptInputType_fieldA; ' + '_t1 = (((this).foo));'); }); @@ -626,7 +626,7 @@ describe('type check blocks', () => { const block = tcb(TEMPLATE, DIRECTIVES); expect(block).toContain( - 'var _t1: boolean | string = null!; ' + + 'var _t1 = null! as boolean | string; ' + '_t1 = (((this).expr));'); }); @@ -1020,13 +1020,13 @@ describe('type check blocks', () => { it('should check types of pipes when enabled', () => { const block = tcb(TEMPLATE, PIPES); - expect(block).toContain('var _pipe1: i0.TestPipe = null!;'); + expect(block).toContain('var _pipe1 = null! as i0.TestPipe;'); expect(block).toContain('(_pipe1.transform(((this).a), ((this).b), ((this).c)));'); }); it('should not check types of pipes when disabled', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfPipes: false}; const block = tcb(TEMPLATE, PIPES, DISABLED_CONFIG); - expect(block).toContain('var _pipe1: i0.TestPipe = null!;'); + expect(block).toContain('var _pipe1 = null! as i0.TestPipe;'); expect(block).toContain('((_pipe1.transform as any)(((this).a), ((this).b), ((this).c))'); }); }); @@ -1110,7 +1110,7 @@ describe('type check blocks', () => { TypeCheckingConfig = {...BASE_CONFIG, honorAccessModifiersForInputBindings: true}; const block = tcb(TEMPLATE, DIRECTIVES, enableChecks); expect(block).toContain( - 'var _t1: i0.Dir = null!; ' + + 'var _t1 = null! as i0.Dir; ' + '_t1["some-input.xs"] = (((this).foo)); '); }); @@ -1128,7 +1128,7 @@ describe('type check blocks', () => { TypeCheckingConfig = {...BASE_CONFIG, honorAccessModifiersForInputBindings: true}; const block = tcb(TEMPLATE, DIRECTIVES, enableChecks); expect(block).toContain( - 'var _t1: i0.Dir = null!; ' + + 'var _t1 = null! as i0.Dir; ' + '_t1.fieldA = (((this).foo)); '); }); }); @@ -1160,7 +1160,7 @@ describe('type check blocks', () => { const renderedTcb = tcb(template, declarations, {useInlineTypeConstructors: false}); - expect(renderedTcb).toContain(`var _t1: i0.Dir = null!;`); + expect(renderedTcb).toContain(`var _t1 = null! as i0.Dir;`); expect(renderedTcb).toContain(`_t1.inputA = (((this).foo));`); expect(renderedTcb).toContain(`_t1.inputB = (((this).bar));`); }); @@ -1186,7 +1186,7 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.HostDir = null!'); + expect(block).toContain('var _t1 = null! as i0.HostDir'); expect(block).toContain('_t1.hostInput = (1)'); expect(block).toContain('_t1["hostOutput"].subscribe'); }); @@ -1211,7 +1211,7 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.HostDir = null!'); + expect(block).toContain('var _t1 = null! as i0.HostDir'); expect(block).toContain('_t1.hostInput = (1)'); expect(block).toContain('_t1["hostOutput"].subscribe'); }); @@ -1242,7 +1242,7 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.MultiLevelHostDir = null!;'); + expect(block).toContain('var _t1 = null! as i0.MultiLevelHostDir;'); expect(block).toContain('_t1.multiLevelHostInput = (1)'); }); @@ -1274,8 +1274,8 @@ describe('type check blocks', () => { ] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t2: i0.HostA = null!;'); - expect(block).toContain('var _t4: i0.HostB = null!;'); + expect(block).toContain('var _t2 = null! as i0.HostA;'); + expect(block).toContain('var _t4 = null! as i0.HostB;'); expect(block).toContain('(((_t1).propA)) + (((_t3).propB))'); }); @@ -1298,8 +1298,8 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.HostDir = null!'); - expect(block).toContain('var _t2: i0.DirA = null!;'); + expect(block).toContain('var _t1 = null! as i0.HostDir'); + expect(block).toContain('var _t2 = null! as i0.DirA;'); expect(block).toContain('_t1.input = (1)'); expect(block).toContain('_t2.input = (1)'); }); @@ -1326,7 +1326,7 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).not.toContain('var _t1: i0.HostDir = null!'); + expect(block).not.toContain('var _t1 = null! i0.HostDir'); expect(block).not.toContain('_t1.hostInput = (1)'); expect(block).not.toContain('_t1["hostOutput"].subscribe'); expect(block).toContain('_t1.addEventListener("hostOutput"'); @@ -1353,7 +1353,7 @@ describe('type check blocks', () => { }] }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: i0.HostDir = null!'); + expect(block).toContain('var _t1 = null! as i0.HostDir'); expect(block).toContain('_t1.hostInput = (1)'); expect(block).toContain('_t1["hostOutput"].subscribe'); }); @@ -1451,7 +1451,7 @@ describe('type check blocks', () => { }`; expect(tcb(TEMPLATE)) - .toContain('var _t1 = (((this).expr)) === (1); if (_t1) { "" + (_t1); } }'); + .toContain('var _t1 = ((((this).expr)) === (1)); if (_t1) { "" + (_t1); } } }'); }); it('should generate a switch block', () => { @@ -1529,7 +1529,7 @@ describe('type check blocks', () => { expect(tcb(TEMPLATE)) .toContain( - 'var _t1: any = null!; { var _t2 = (_t1.exp); _t2(); ' + + 'var _t1 = null! as any; { var _t2 = (_t1.exp); _t2(); ' + 'if ((_t2()) === "one") { "" + ((this).one()); } ' + 'else if ((_t2()) === "two") { "" + ((this).two()); } ' + 'else { "" + ((this).default()); } }'); @@ -1565,12 +1565,12 @@ describe('type check blocks', () => { const result = tcb(TEMPLATE); expect(result).toContain('for (const _t1 of ((this).items)!) {'); - expect(result).toContain('var _t2: number = null!;'); - expect(result).toContain('var _t3: boolean = null! as boolean;'); - expect(result).toContain('var _t4: boolean = null! as boolean;'); - expect(result).toContain('var _t5: boolean = null! as boolean;'); - expect(result).toContain('var _t6: boolean = null! as boolean;'); - expect(result).toContain('var _t7: number = null!;'); + expect(result).toContain('var _t2 = null! as number;'); + expect(result).toContain('var _t3 = null! as boolean;'); + expect(result).toContain('var _t4 = null! as boolean;'); + expect(result).toContain('var _t5 = null! as boolean;'); + expect(result).toContain('var _t6 = null! as boolean;'); + expect(result).toContain('var _t7 = null! as number;'); expect(result).toContain('"" + (_t2) + (_t3) + (_t4) + (_t5) + (_t6) + (_t7)'); }); @@ -1583,12 +1583,12 @@ describe('type check blocks', () => { const result = tcb(TEMPLATE); expect(result).toContain('for (const _t1 of ((this).items)!) {'); - expect(result).toContain('var _t2: number = null!;'); - expect(result).toContain('var _t3: boolean = null! as boolean;'); - expect(result).toContain('var _t4: boolean = null! as boolean;'); - expect(result).toContain('var _t5: boolean = null! as boolean;'); - expect(result).toContain('var _t6: boolean = null! as boolean;'); - expect(result).toContain('var _t7: number = null!;'); + expect(result).toContain('var _t2 = null! as number;'); + expect(result).toContain('var _t3 = null! as boolean;'); + expect(result).toContain('var _t4 = null! as boolean;'); + expect(result).toContain('var _t5 = null! as boolean;'); + expect(result).toContain('var _t6 = null! as boolean;'); + expect(result).toContain('var _t7 = null! as number;'); expect(result).toContain('"" + (_t2) + (_t3) + (_t4) + (_t5) + (_t6) + (_t7)'); }); @@ -1599,7 +1599,7 @@ describe('type check blocks', () => { const result = tcb(TEMPLATE); expect(result).toContain('for (const _t1 of ((this).items)!) {'); - expect(result).toContain('var _t2: number = null!;'); + expect(result).toContain('var _t2 = null! as number;'); expect(result).toContain('"" + (((this).$index)) + (_t2)'); }); @@ -1615,15 +1615,15 @@ describe('type check blocks', () => { `; const result = tcb(TEMPLATE); - expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2: number = null!;'); + expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2 = null! as number;'); expect(result).toContain('"" + (_t1) + (_t2)'); - expect(result).toContain('for (const _t3 of ((_t1).items)!) { var _t4: number = null!;'); + expect(result).toContain('for (const _t3 of ((_t1).items)!) { var _t4 = null! as number;'); expect(result).toContain('"" + (_t1) + (_t2) + (_t3) + (_t4)'); }); it('should generate the tracking expression of a for loop', () => { const result = tcb(`@for (item of items; track trackingFn($index, item, prop)) {}`); - expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2: number = null!;'); + expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2 = null! as number;'); expect(result).toContain('(this).trackingFn(_t2, _t1, ((this).prop));'); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index 60893aff92d7b..59a8b9941cdce 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {ASTWithSource, Binary, BindingPipe, Conditional, Interpolation, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate} from '@angular/compiler'; -import {AST, LiteralArray, LiteralMap} from '@angular/compiler/src/compiler'; +import {ASTWithSource, Binary, BindingPipe, Conditional, Interpolation, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstForLoopBlock, TmplAstNode, TmplAstReference, TmplAstTemplate} from '@angular/compiler'; +import {AST, LiteralArray, LiteralMap, TmplAstIfBlock} from '@angular/compiler/src/compiler'; import ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError} from '../../file_system'; @@ -339,6 +339,172 @@ runInEachFileSystem(() => { expect((indexSymbol).declaration).toEqual(templateNode.variables[1]); } }); + + describe('control flow @if block', () => { + let templateTypeChecker: TemplateTypeChecker; + let cmp: ClassDeclaration; + let ifBlockNode: TmplAstIfBlock; + let program: ts.Program; + + beforeEach(() => { + const fileName = absoluteFrom('/main.ts'); + const templateString = ` + @if (user; as userAlias) { + {{userAlias.name}} {{userAlias.streetNumber}} + }`; + const testValues = setup([ + { + fileName, + templates: {'Cmp': templateString}, + source: ` + export interface User { + name: string; + streetNumber: number; + } + export class Cmp { user?: User; } + `, + }, + ]); + templateTypeChecker = testValues.templateTypeChecker; + program = testValues.program; + const sf = getSourceFileOrError(testValues.program, fileName); + cmp = getClass(sf, 'Cmp'); + ifBlockNode = templateTypeChecker.getTemplate(cmp)![0] as unknown as TmplAstIfBlock; + }); + + + it('should retrieve a symbol for the loop expression', () => { + const symbol = + templateTypeChecker.getSymbolOfNode(ifBlockNode.branches[0].expression!, cmp)!; + assertExpressionSymbol(symbol); + expectUserSymbol(symbol); + }); + + it('should retrieve a symbol for the track expression', () => { + const symbol = + templateTypeChecker.getSymbolOfNode(ifBlockNode.branches[0].expressionAlias!, cmp)!; + assertVariableSymbol(symbol); + expectUserSymbol(symbol); + }); + + function expectUserSymbol(userSymbol: VariableSymbol|ExpressionSymbol) { + expect(userSymbol.tsSymbol!.escapedName).toContain('user'); + expect(program.getTypeChecker().typeToString(userSymbol.tsType!)) + .toEqual('User | undefined'); + } + }); + + describe('control flow @for block', () => { + let templateTypeChecker: TemplateTypeChecker; + let cmp: ClassDeclaration; + let forLoopNode: TmplAstForLoopBlock; + let program: ts.Program; + + beforeEach(() => { + const fileName = absoluteFrom('/main.ts'); + const dirFile = absoluteFrom('/dir.ts'); + const templateString = ` + @for (user of users; let i = $index; track user) { +
+ {{user.name}} {{user.streetNumber}} +
+
+ }`; + const testValues = setup([ + { + fileName, + templates: {'Cmp': templateString}, + source: ` + export interface User { + name: string; + streetNumber: number; + } + export class Cmp { users: User[]; } + `, + }, + { + fileName: dirFile, + source: `export class TestDir {name:string}`, + templates: {}, + }, + ]); + templateTypeChecker = testValues.templateTypeChecker; + program = testValues.program; + const sf = getSourceFileOrError(testValues.program, fileName); + cmp = getClass(sf, 'Cmp'); + forLoopNode = templateTypeChecker.getTemplate(cmp)![0] as unknown as TmplAstForLoopBlock; + }); + + + it('should retrieve a symbol for the loop expression', () => { + const symbol = templateTypeChecker.getSymbolOfNode(forLoopNode.expression.ast, cmp)!; + assertExpressionSymbol(symbol); + expect(program.getTypeChecker().symbolToString(symbol.tsSymbol!)).toEqual('users'); + expect(program.getTypeChecker().typeToString(symbol.tsType)).toEqual('Array'); + }); + + it('should retrieve a symbol for the track expression', () => { + const userSymbol = templateTypeChecker.getSymbolOfNode(forLoopNode.trackBy.ast, cmp)!; + expectUserSymbol(userSymbol); + }); + + it('should retrieve a symbol for property reads of the loop variable', () => { + const boundText = + (forLoopNode.children[0] as TmplAstElement).children[0] as TmplAstBoundText; + const interpolation = (boundText.value as ASTWithSource).ast as Interpolation; + const namePropRead = interpolation.expressions[0] as PropertyRead; + const streetNumberPropRead = interpolation.expressions[1] as PropertyRead; + + const nameSymbol = templateTypeChecker.getSymbolOfNode(namePropRead, cmp)!; + assertExpressionSymbol(nameSymbol); + expect(program.getTypeChecker().symbolToString(nameSymbol.tsSymbol!)).toEqual('name'); + expect(program.getTypeChecker().typeToString(nameSymbol.tsType)).toEqual('string'); + + const streetSymbol = templateTypeChecker.getSymbolOfNode(streetNumberPropRead, cmp)!; + assertExpressionSymbol(streetSymbol); + expect(program.getTypeChecker().symbolToString(streetSymbol.tsSymbol!)) + .toEqual('streetNumber'); + expect(program.getTypeChecker().typeToString(streetSymbol.tsType)).toEqual('number'); + + const userSymbol = templateTypeChecker.getSymbolOfNode(namePropRead.receiver, cmp)!; + expectUserSymbol(userSymbol); + }); + + it('finds symbols for loop variable', () => { + const userVar = forLoopNode.item; + const userSymbol = templateTypeChecker.getSymbolOfNode(userVar, cmp)!; + expectUserSymbol(userSymbol); + }); + + it('finds symbols for $index variable', () => { + const iVar = forLoopNode.contextVariables.$index; + const iSymbol = templateTypeChecker.getSymbolOfNode(iVar, cmp)!; + expectIndexSymbol(iSymbol); + }); + + it('finds symbol when using the index in the body', () => { + const innerElementNodes = + onlyAstElements((forLoopNode.children[0] as TmplAstElement).children); + const indexSymbol = + templateTypeChecker.getSymbolOfNode(innerElementNodes[0].inputs[0].value, cmp)!; + expectIndexSymbol(indexSymbol); + }); + + function expectUserSymbol(userSymbol: Symbol) { + assertVariableSymbol(userSymbol); + expect(userSymbol.tsSymbol!.escapedName).toContain('User'); + expect(program.getTypeChecker().typeToString(userSymbol.tsType!)).toEqual('User'); + expect((userSymbol).declaration).toEqual(forLoopNode.item); + } + + function expectIndexSymbol(indexSymbol: Symbol) { + assertVariableSymbol(indexSymbol); + expect(indexSymbol.tsSymbol) + .toBeNull(); // implicit variable doesn't have a TS definition location + expect(program.getTypeChecker().typeToString(indexSymbol.tsType!)).toEqual('number'); + expect((indexSymbol).declaration).toEqual(forLoopNode.contextVariables.$index); + } + }); }); describe('for expressions', () => { diff --git a/packages/compiler/src/render3/r3_control_flow.ts b/packages/compiler/src/render3/r3_control_flow.ts index 20425c578d362..b18a681190416 100644 --- a/packages/compiler/src/render3/r3_control_flow.ts +++ b/packages/compiler/src/render3/r3_control_flow.ts @@ -20,7 +20,7 @@ const FOR_LOOP_EXPRESSION_PATTERN = /^\s*([0-9A-Za-z_$]*)\s+of\s+([\S\s]*)/; const FOR_LOOP_TRACK_PATTERN = /^track\s+([\S\s]*)/; /** Pattern for the `as` expression in a conditional block. */ -const CONDITIONAL_ALIAS_PATTERN = /^as\s+(.*)/; +const CONDITIONAL_ALIAS_PATTERN = /^(as\s)+(.*)/; /** Pattern used to identify an `else if` block. */ const ELSE_IF_PATTERN = /^else[^\S\r\n]+if/; @@ -28,6 +28,12 @@ const ELSE_IF_PATTERN = /^else[^\S\r\n]+if/; /** Pattern used to identify a `let` parameter. */ const FOR_LOOP_LET_PATTERN = /^let\s+([\S\s]*)/; +/** + * Pattern to group a string into leading whitespace, non whitespace, and trailing whitespace. + * Useful for getting the variable name span when a span can contain leading and trailing space. + */ +const CHARACTERS_IN_SURROUNDING_WHITESPACE_PATTERN = /(\s*)(\S+)(\s*)/; + /** Names of variables that are allowed to be used in the `let` expression of a `for` loop. */ const ALLOWED_FOR_LOOP_LET_VARIABLES = new Set(['$index', '$first', '$last', '$even', '$odd', '$count']); @@ -217,9 +223,15 @@ function parseForLoopParameters( } const [, itemName, rawExpression] = match; + // `expressionParam.expression` contains the variable declaration and the expression of the + // for...of statement, i.e. 'user of users' The variable of a ForOfStatement is _only_ the "const + // user" part and does not include "of x". + const variableName = expressionParam.expression.split(' ')[0]; + const variableSpan = new ParseSourceSpan( + expressionParam.sourceSpan.start, + expressionParam.sourceSpan.start.moveBy(variableName.length)); const result = { - itemName: new t.Variable( - itemName, '$implicit', expressionParam.sourceSpan, expressionParam.sourceSpan), + itemName: new t.Variable(itemName, '$implicit', variableSpan, variableSpan), trackBy: null as {expression: ASTWithSource, keywordSpan: ParseSourceSpan} | null, expression: parseBlockParameterToBinding(expressionParam, bindingParser, rawExpression), context: {} as t.ForLoopBlockContext, @@ -229,7 +241,10 @@ function parseForLoopParameters( const letMatch = param.expression.match(FOR_LOOP_LET_PATTERN); if (letMatch !== null) { - parseLetParameter(param.sourceSpan, letMatch[1], param.sourceSpan, result.context, errors); + const variablesSpan = new ParseSourceSpan( + param.sourceSpan.start.moveBy(letMatch[0].length - letMatch[1].length), + param.sourceSpan.end); + parseLetParameter(param.sourceSpan, letMatch[1], variablesSpan, result.context, errors); continue; } @@ -272,7 +287,7 @@ function parseLetParameter( sourceSpan: ParseSourceSpan, expression: string, span: ParseSourceSpan, context: t.ForLoopBlockContext, errors: ParseError[]): void { const parts = expression.split(','); - + let startSpan = span.start; for (const part of parts) { const expressionParts = part.split('='); const name = expressionParts.length === 2 ? expressionParts[0].trim() : ''; @@ -292,8 +307,32 @@ function parseLetParameter( errors.push( new ParseError(sourceSpan, `Duplicate "let" parameter variable "${variableName}"`)); } else { - context[variableName] = new t.Variable(name, variableName, span, span); + const [, keyLeadingWhitespace, keyName] = + expressionParts[0].match(CHARACTERS_IN_SURROUNDING_WHITESPACE_PATTERN) ?? []; + const keySpan = keyLeadingWhitespace !== undefined && expressionParts.length === 2 ? + new ParseSourceSpan( + /* strip leading spaces */ + startSpan.moveBy(keyLeadingWhitespace.length), + /* advance to end of the variable name */ + startSpan.moveBy(keyLeadingWhitespace.length + keyName.length)) : + span; + + let valueSpan: ParseSourceSpan|undefined = undefined; + if (expressionParts.length === 2) { + const [, valueLeadingWhitespace, implicit] = + expressionParts[1].match(CHARACTERS_IN_SURROUNDING_WHITESPACE_PATTERN) ?? []; + valueSpan = valueLeadingWhitespace !== undefined ? + new ParseSourceSpan( + startSpan.moveBy(expressionParts[0].length + 1 + valueLeadingWhitespace.length), + startSpan.moveBy( + expressionParts[0].length + 1 + valueLeadingWhitespace.length + + implicit.length)) : + undefined; + } + const sourceSpan = new ParseSourceSpan(keySpan.start, valueSpan?.end ?? keySpan.end); + context[variableName] = new t.Variable(name, variableName, sourceSpan, keySpan, valueSpan); } + startSpan = startSpan.moveBy(part.length + 1 /* add 1 to move past the comma */); } } @@ -423,8 +462,10 @@ function parseConditionalBlockParameters( errors.push( new ParseError(param.sourceSpan, 'Conditional can only have one "as" expression')); } else { - const name = aliasMatch[1].trim(); - expressionAlias = new t.Variable(name, name, param.sourceSpan, param.sourceSpan); + const name = aliasMatch[2].trim(); + const variableStart = param.sourceSpan.start.moveBy(aliasMatch[1].length); + const variableSpan = new ParseSourceSpan(variableStart, variableStart.moveBy(name.length)); + expressionAlias = new t.Variable(name, name, variableSpan, variableSpan); } } diff --git a/packages/compiler/test/render3/r3_ast_spans_spec.ts b/packages/compiler/test/render3/r3_ast_spans_spec.ts index 33b7abb782d9a..a7badf063aaa4 100644 --- a/packages/compiler/test/render3/r3_ast_spans_spec.ts +++ b/packages/compiler/test/render3/r3_ast_spans_spec.ts @@ -132,6 +132,8 @@ class R3AstSourceSpans implements t.Visitor { 'ForLoopBlock', humanizeSpan(block.sourceSpan), humanizeSpan(block.startSourceSpan), humanizeSpan(block.endSourceSpan) ]); + this.visitVariable(block.item); + this.visitAll([Object.values(block.contextVariables)]); this.visitAll([block.children]); block.empty?.visit(this); } @@ -153,6 +155,9 @@ class R3AstSourceSpans implements t.Visitor { visitIfBlockBranch(block: t.IfBlockBranch): void { this.result.push( ['IfBlockBranch', humanizeSpan(block.sourceSpan), humanizeSpan(block.startSourceSpan)]); + if (block.expressionAlias) { + this.visitVariable(block.expressionAlias); + } this.visitAll([block.children]); } @@ -686,15 +691,23 @@ describe('R3 AST source spans', () => { describe('for loop blocks', () => { it('is correct for loop blocks', () => { - const html = `@for (item of items.foo.bar; track item.id) {

{{ item }}

}` + + const html = + `@for (item of items.foo.bar; track item.id; let i = $index, _o_d_d_ = $odd) {

{{ item }}

}` + `@empty {There were no items in the list.}`; expectFromHtml(html).toEqual([ [ 'ForLoopBlock', - '@for (item of items.foo.bar; track item.id) {

{{ item }}

}@empty {There were no items in the list.}', - '@for (item of items.foo.bar; track item.id) {', '}' + '@for (item of items.foo.bar; track item.id; let i = $index, _o_d_d_ = $odd) {

{{ item }}

}@empty {There were no items in the list.}', + '@for (item of items.foo.bar; track item.id; let i = $index, _o_d_d_ = $odd) {', '}' ], + ['Variable', 'item', 'item', ''], + ['Variable', 'i = $index', 'i', '$index'], + ['Variable', '_o_d_d_ = $odd', '_o_d_d_', '$odd'], + ['Variable', '', '', ''], + ['Variable', '', '', ''], + ['Variable', '', '', ''], + ['Variable', '', '', ''], ['Element', '

{{ item }}

', '

', '

'], ['BoundText', '{{ item }}'], ['ForLoopBlockEmpty', '@empty {There were no items in the list.}', '@empty {'], @@ -719,6 +732,7 @@ describe('R3 AST source spans', () => { 'IfBlockBranch', '@if (cond.expr; as foo) {Main case was true!}', '@if (cond.expr; as foo) {' ], + ['Variable', 'foo', 'foo', ''], ['Text', 'Main case was true!'], [ 'IfBlockBranch', '@else if (other.expr) {Extra case was true!}', '@else if (other.expr) {' diff --git a/packages/language-service/src/references_and_rename_utils.ts b/packages/language-service/src/references_and_rename_utils.ts index 96c6646a70afa..126ba98809afd 100644 --- a/packages/language-service/src/references_and_rename_utils.ts +++ b/packages/language-service/src/references_and_rename_utils.ts @@ -218,6 +218,20 @@ export function convertToTemplateDocumentSpan( // want to return references to the parameter in the template itself. return null; } + // Variables in the typecheck block are generated with the type on the right hand + // side: `var _t1 = null! as i1.DirA`. Finding references of DirA will return the type + // assertion and we need to map it back to the variable identifier _t1. + if (hasExpressionIdentifier(sf, tcbNode, ExpressionIdentifier.VARIABLE_AS_EXPRESSION)) { + let newNode = tcbNode; + while (!ts.isVariableDeclaration(newNode)) { + newNode = newNode.parent; + } + newNode = newNode.name; + shimDocumentSpan.textSpan = { + start: newNode.getStart(), + length: newNode.getEnd() - newNode.getStart(), + }; + } // TODO(atscott): Determine how to consistently resolve paths. i.e. with the project // serverHost or LSParseConfigHost in the adapter. We should have a better defined way to // normalize paths. diff --git a/packages/language-service/test/quick_info_spec.ts b/packages/language-service/test/quick_info_spec.ts index 22575d6e29489..7c63fddf2ab76 100644 --- a/packages/language-service/test/quick_info_spec.ts +++ b/packages/language-service/test/quick_info_spec.ts @@ -662,6 +662,31 @@ describe('quick info', () => { expectedDisplayString: '(keyword) track' }); }); + + it('implicit variable assignment', () => { + expectQuickInfo({ + templateOverride: `@for (name of constNames; track $index; let od¦d = $odd) {}`, + expectedSpanText: 'odd', + expectedDisplayString: '(variable) odd: boolean' + }); + }); + + it('implicit variable assignment in comma separated list', () => { + expectQuickInfo({ + templateOverride: + `@for (name of constNames; track index; let odd = $odd, ind¦ex = $index) {}`, + expectedSpanText: 'index', + expectedDisplayString: '(variable) index: number' + }); + }); + + it('if block alias variable', () => { + expectQuickInfo({ + templateOverride: `@if (constNames; as al¦iasName) {}`, + expectedSpanText: 'aliasName', + expectedDisplayString: '(variable) aliasName: [{ readonly name: "name"; }]' + }); + }); }); it('should work for object literal with shorthand property declarations', () => { diff --git a/packages/language-service/test/references_and_rename_spec.ts b/packages/language-service/test/references_and_rename_spec.ts index c0294fed949a0..5e0018dbfecc8 100644 --- a/packages/language-service/test/references_and_rename_spec.ts +++ b/packages/language-service/test/references_and_rename_spec.ts @@ -1507,6 +1507,42 @@ describe('find references and rename locations', () => { }); }); + describe('control flow', () => { + it('can find references and rename alias variable from @if block', () => { + const app = ` + import {Component} from '@angular/core'; + + @Component({ + template: '@if (x; as aliasX) { {{aliasX}} {{aliasX + "second"}} }', + standalone: true + }) + export class AppCmp { + x?: string; + } + `; + env = LanguageServiceTestEnv.setup(); + const project = env.addProject('test', {'app.ts': app}); + const file = project.openFile('app.ts'); + file.moveCursorToText('{{alia¦sX}}'); + + const refs = getReferencesAtPosition(file)!; + expect(refs.length).toBe(3); + assertTextSpans(refs, ['aliasX']); + assertFileNames(refs, ['app.ts']); + + const renameLocations = getRenameLocationsAtPosition(file)!; + // TODO(atscott): Aliases cannot be renamed because the type check block creates a local + // variable and uses it in the `if` statement without a source map. When the rename operation + // cannot map a type check block location back to a template location, it bails on the rename + // because it does not want to provide incomplete rename results: + // var _t1 /*104,110*/ = (((this).x /*98,99*/) /*98,99*/) /*104,110*/; + // if (_t1) { // <------------- this causes renaming to bail + // "" + (_t1 /*116,122*/) + ((_t1 /*127,133*/) + ("second" /*136,144*/) /*127,144*/); + // } + expect(renameLocations).toBeUndefined(); + }); + }); + describe('get rename info', () => { it('indicates inability to rename when cursor is outside template and in a string literal', () => {