Skip to content

Commit eddf5da

Browse files
atscottthePunderWoman
atscott
authored and
thePunderWoman
committed
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 angular/vscode-ng-language-service#1988 PR Close #53980
1 parent 47e6e84 commit eddf5da

12 files changed

Lines changed: 429 additions & 89 deletions

‎packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ export enum ExpressionIdentifier {
4444
DIRECTIVE = 'DIR',
4545
COMPONENT_COMPLETION = 'COMPCOMP',
4646
EVENT_PARAMETER = 'EP',
47+
VARIABLE_AS_EXPRESSION = 'VAE',
4748
}
4849

4950
/** Tags the node with the given expression identifier. */

‎packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ export class SymbolBuilder {
111111
const elementSourceSpan = element.startSourceSpan ?? element.sourceSpan;
112112
const tcbSourceFile = this.typeCheckBlock.getSourceFile();
113113
// directives could be either:
114-
// - var _t1: TestDir /*T:D*/ = (null!);
114+
// - var _t1: TestDir /*T:D*/ = null! as TestDir;
115115
// - var _t1 /*T:D*/ = _ctor1({});
116116
const isDirectiveDeclaration = (node: ts.Node): node is ts.TypeNode|ts.Identifier =>
117117
(ts.isTypeNode(node) || ts.isIdentifier(node)) && ts.isVariableDeclaration(node.parent) &&
@@ -429,19 +429,25 @@ export class SymbolBuilder {
429429
private getSymbolOfVariable(variable: TmplAstVariable): VariableSymbol|null {
430430
const node = findFirstMatchingNode(
431431
this.typeCheckBlock, {withSpan: variable.sourceSpan, filter: ts.isVariableDeclaration});
432-
if (node === null || node.initializer === undefined) {
432+
if (node === null) {
433433
return null;
434434
}
435435

436-
const expressionSymbol = this.getSymbolOfTsNode(node.initializer);
437-
if (expressionSymbol === null) {
436+
let nodeValueSymbol: TsNodeSymbolInfo|null = null;
437+
if (ts.isForOfStatement(node.parent.parent)) {
438+
nodeValueSymbol = this.getSymbolOfTsNode(node);
439+
} else if (node.initializer !== undefined) {
440+
nodeValueSymbol = this.getSymbolOfTsNode(node.initializer);
441+
}
442+
443+
if (nodeValueSymbol === null) {
438444
return null;
439445
}
440446

441447
return {
442-
tsType: expressionSymbol.tsType,
443-
tsSymbol: expressionSymbol.tsSymbol,
444-
initializerLocation: expressionSymbol.tcbLocation,
448+
tsType: nodeValueSymbol.tsType,
449+
tsSymbol: nodeValueSymbol.tsSymbol,
450+
initializerLocation: nodeValueSymbol.tcbLocation,
445451
kind: SymbolKind.Variable,
446452
declaration: variable,
447453
localVarLocation: {

‎packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@
88

99
import ts from 'typescript';
1010

11+
import {addExpressionIdentifier, ExpressionIdentifier} from './comments';
12+
import {wrapForTypeChecker} from './diagnostics';
13+
14+
1115

1216
/**
1317
* 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 {
7781
* Unlike with `tsCreateVariable`, the type of the variable is explicitly specified.
7882
*/
7983
export function tsDeclareVariable(id: ts.Identifier, type: ts.TypeNode): ts.VariableStatement {
80-
let initializer: ts.Expression = ts.factory.createNonNullExpression(ts.factory.createNull());
81-
8284
// When we create a variable like `var _t1: boolean = null!`, TypeScript actually infers `_t1`
83-
// to be `never`, instead of a `boolean`. To work around it, we cast the value to boolean again
84-
// in the initializer, e.g. `var _t1: boolean = null! as boolean;`.
85-
if (type.kind === ts.SyntaxKind.BooleanKeyword) {
86-
initializer = ts.factory.createAsExpression(
87-
initializer, ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword));
88-
}
85+
// to be `never`, instead of a `boolean`. To work around it, we cast the value
86+
// in the initializer, e.g. `var _t1 = null! as boolean;`.
87+
addExpressionIdentifier(type, ExpressionIdentifier.VARIABLE_AS_EXPRESSION);
88+
const initializer: ts.Expression = ts.factory.createAsExpression(
89+
ts.factory.createNonNullExpression(ts.factory.createNull()), type);
8990

9091
const decl = ts.factory.createVariableDeclaration(
9192
/* name */ id,
9293
/* exclamationToken */ undefined,
93-
/* type */ type,
94+
/* type */ undefined,
9495
/* initializer */ initializer);
9596
return ts.factory.createVariableStatement(
9697
/* modifiers */ undefined,

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,8 +465,8 @@ abstract class TcbDirectiveTypeOpBase extends TcbOp {
465465
}
466466

467467
const id = this.tcb.allocateId();
468-
addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE);
469-
addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan);
468+
addExpressionIdentifier(id, ExpressionIdentifier.DIRECTIVE);
469+
addParseSpanInfo(id, this.node.startSourceSpan || this.node.sourceSpan);
470470
this.scope.addStatement(tsDeclareVariable(id, type));
471471
return id;
472472
}
@@ -1332,7 +1332,9 @@ class TcbBlockVariableOp extends TcbOp {
13321332
override execute(): ts.Identifier {
13331333
const id = this.tcb.allocateId();
13341334
addParseSpanInfo(id, this.variable.keySpan);
1335-
this.scope.addStatement(tsCreateVariable(id, this.initializer));
1335+
const variable = tsCreateVariable(id, wrapForTypeChecker(this.initializer));
1336+
addParseSpanInfo(variable.declarationList.declarations[0], this.variable.sourceSpan);
1337+
this.scope.addStatement(variable);
13361338
return id;
13371339
}
13381340
}
@@ -1355,7 +1357,9 @@ class TcbBlockImplicitVariableOp extends TcbOp {
13551357
override execute(): ts.Identifier {
13561358
const id = this.tcb.allocateId();
13571359
addParseSpanInfo(id, this.variable.keySpan);
1358-
this.scope.addStatement(tsDeclareVariable(id, this.type));
1360+
const variable = tsDeclareVariable(id, this.type);
1361+
addParseSpanInfo(variable.declarationList.declarations[0], this.variable.sourceSpan);
1362+
this.scope.addStatement(variable);
13591363
return id;
13601364
}
13611365
}
@@ -1610,6 +1614,7 @@ class TcbForOfOp extends TcbOp {
16101614
}
16111615
const initializer = ts.factory.createVariableDeclarationList(
16121616
[ts.factory.createVariableDeclaration(initializerId)], ts.NodeFlags.Const);
1617+
addParseSpanInfo(initializer, this.block.item.keySpan);
16131618
// It's common to have a for loop over a nullable value (e.g. produced by the `async` pipe).
16141619
// Add a non-null expression to allow such values to be assigned.
16151620
const expression = ts.factory.createNonNullExpression(

‎packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ describe('type check blocks diagnostics', () => {
173173
pipeName: 'test',
174174
}];
175175
const block = tcbWithSpans(TEMPLATE, PIPES);
176-
expect(block).toContain('var _pipe1: i0.TestPipe = null!');
176+
expect(block).toContain('var _pipe1 = null! as i0.TestPipe');
177177
expect(block).toContain(
178178
'(_pipe1.transform /*7,11*/(((this).a /*3,4*/) /*3,4*/, ((this).b /*12,13*/) /*12,13*/) /*3,13*/);');
179179
});
@@ -215,6 +215,37 @@ describe('type check blocks diagnostics', () => {
215215
.toContain('_t1.inputA /*9,15*/ = ("" /*18,20*/) /*8,21*/;');
216216
});
217217
});
218+
219+
describe('control flow', () => {
220+
it('@for', () => {
221+
const template = `@for (user of users; track user; let i = $index) { {{i}} }`;
222+
// user variable
223+
expect(tcbWithSpans(template, [])).toContain('const _t1 /*6,10*/');
224+
expect(tcbWithSpans(template, [])).toContain('(this).users /*14,19*/');
225+
// index variable
226+
expect(tcbWithSpans(template, []))
227+
.toContain('var _t2 /*37,38*/ = null! as number /*T:VAE*/ /*37,47*/');
228+
// track expression
229+
expect(tcbWithSpans(template, [])).toContain('_t1 /*27,31*/;');
230+
});
231+
232+
it('@for with comma separated variables', () => {
233+
const template =
234+
`@for (x of y; track x; let i = $index, odd = $odd,e_v_e_n=$even) { {{i + odd + e_v_e_n}} }`;
235+
expect(tcbWithSpans(template, []))
236+
.toContain('var _t2 /*27,28*/ = null! as number /*T:VAE*/ /*27,37*/');
237+
expect(tcbWithSpans(template, []))
238+
.toContain('var _t3 /*39,42*/ = null! as boolean /*T:VAE*/ /*39,54*/');
239+
expect(tcbWithSpans(template, []))
240+
.toContain('var _t4 /*55,62*/ = null! as boolean /*T:VAE*/ /*55,68*/');
241+
});
242+
243+
it('@if', () => {
244+
const template = `@if (x; as alias) { {{alias}} }`;
245+
expect(tcbWithSpans(template, [])).toContain('var _t1 /*11,16*/');
246+
expect(tcbWithSpans(template, [])).toContain('(this).x /*5,6*/');
247+
});
248+
});
218249
});
219250
});
220251

0 commit comments

Comments
 (0)