Skip to content

Commit 04169e1

Browse files
dylhunnatscott
dylhunn
authored and
atscott
committed
refactor(language-service): Prepare to support blocks in the langauge service (#52038)
Two key refactors to enable deeper language service support for blocks: (1) We now generate accurate source spans for the various block types. Additionally, all the top-level source spans for a block are now *inclusive* of all the connected or descending blocks. This helps the language service visit connected blocks. (2) The language service's template visitor was previously skipping over the AST nodes corresponding to several block types. We are now careful to visit all such nodes. PR Close #52038
1 parent 5464bea commit 04169e1

File tree

6 files changed

+334
-30
lines changed

6 files changed

+334
-30
lines changed

‎packages/compiler/src/render3/r3_ast.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,8 @@ export class DeferredBlock implements Node {
210210
public children: Node[], triggers: DeferredBlockTriggers,
211211
prefetchTriggers: DeferredBlockTriggers, public placeholder: DeferredBlockPlaceholder|null,
212212
public loading: DeferredBlockLoading|null, public error: DeferredBlockError|null,
213-
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
214-
public endSourceSpan: ParseSourceSpan|null) {
213+
public sourceSpan: ParseSourceSpan, public mainBlockSpan: ParseSourceSpan,
214+
public startSourceSpan: ParseSourceSpan, public endSourceSpan: ParseSourceSpan|null) {
215215
this.triggers = triggers;
216216
this.prefetchTriggers = prefetchTriggers;
217217
// We cache the keys since we know that they won't change and we
@@ -228,16 +228,14 @@ export class DeferredBlock implements Node {
228228
this.visitTriggers(this.definedTriggers, this.triggers, visitor);
229229
this.visitTriggers(this.definedPrefetchTriggers, this.prefetchTriggers, visitor);
230230
visitAll(visitor, this.children);
231-
this.placeholder && visitor.visitDeferredBlockPlaceholder(this.placeholder);
232-
this.loading && visitor.visitDeferredBlockLoading(this.loading);
233-
this.error && visitor.visitDeferredBlockError(this.error);
231+
const remainingBlocks =
232+
[this.placeholder, this.loading, this.error].filter(x => x !== null) as Array<Node>;
233+
visitAll(visitor, remainingBlocks);
234234
}
235235

236236
private visitTriggers(
237237
keys: (keyof DeferredBlockTriggers)[], triggers: DeferredBlockTriggers, visitor: Visitor) {
238-
for (const key of keys) {
239-
visitor.visitDeferredTrigger(triggers[key]!);
240-
}
238+
visitAll(visitor, keys.map(k => triggers[k]!));
241239
}
242240
}
243241

@@ -272,7 +270,8 @@ export class ForLoopBlock implements Node {
272270
public item: Variable, public expression: ASTWithSource, public trackBy: ASTWithSource,
273271
public contextVariables: ForLoopBlockContext, public children: Node[],
274272
public empty: ForLoopBlockEmpty|null, public sourceSpan: ParseSourceSpan,
275-
public startSourceSpan: ParseSourceSpan, public endSourceSpan: ParseSourceSpan|null) {}
273+
public mainBlockSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
274+
public endSourceSpan: ParseSourceSpan|null) {}
276275

277276
visit<Result>(visitor: Visitor<Result>): Result {
278277
return visitor.visitForLoopBlock(this);
@@ -282,7 +281,7 @@ export class ForLoopBlock implements Node {
282281
export class ForLoopBlockEmpty implements Node {
283282
constructor(
284283
public children: Node[], public sourceSpan: ParseSourceSpan,
285-
public startSourceSpan: ParseSourceSpan) {}
284+
public startSourceSpan: ParseSourceSpan, public endSourceSpan: ParseSourceSpan|null) {}
286285

287286
visit<Result>(visitor: Visitor<Result>): Result {
288287
return visitor.visitForLoopBlockEmpty(this);
@@ -302,7 +301,8 @@ export class IfBlock implements Node {
302301
export class IfBlockBranch implements Node {
303302
constructor(
304303
public expression: AST|null, public children: Node[], public expressionAlias: Variable|null,
305-
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan) {}
304+
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
305+
public endSourceSpan: ParseSourceSpan|null) {}
306306

307307
visit<Result>(visitor: Visitor<Result>): Result {
308308
return visitor.visitIfBlockBranch(this);

‎packages/compiler/src/render3/r3_control_flow.ts‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export function createIfBlock(
6464
if (mainBlockParams !== null) {
6565
branches.push(new t.IfBlockBranch(
6666
mainBlockParams.expression, html.visitAll(visitor, ast.children, ast.children),
67-
mainBlockParams.expressionAlias, ast.sourceSpan, ast.startSourceSpan));
67+
mainBlockParams.expressionAlias, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan));
6868
}
6969

7070
// Assumes that the structure is valid since we validated it above.
@@ -77,16 +77,28 @@ export function createIfBlock(
7777
if (params !== null) {
7878
branches.push(new t.IfBlockBranch(
7979
params.expression, children, params.expressionAlias, block.sourceSpan,
80-
block.startSourceSpan));
80+
block.startSourceSpan, block.endSourceSpan));
8181
}
8282
} else if (block.name === 'else') {
83-
branches.push(
84-
new t.IfBlockBranch(null, children, null, block.sourceSpan, block.startSourceSpan));
83+
branches.push(new t.IfBlockBranch(
84+
null, children, null, block.sourceSpan, block.startSourceSpan, block.endSourceSpan));
8585
}
8686
}
8787

88+
// The outer IfBlock should have a span that encapsulates all branches.
89+
const ifBlockStartSourceSpan =
90+
branches.length > 0 ? branches[0].startSourceSpan : ast.startSourceSpan;
91+
const ifBlockEndSourceSpan =
92+
branches.length > 0 ? branches[branches.length - 1].endSourceSpan : ast.endSourceSpan;
93+
94+
let wholeSourceSpan = ast.sourceSpan;
95+
const lastBranch = branches[branches.length - 1];
96+
if (lastBranch !== undefined) {
97+
wholeSourceSpan = new ParseSourceSpan(ifBlockStartSourceSpan.start, lastBranch.sourceSpan.end);
98+
}
99+
88100
return {
89-
node: new t.IfBlock(branches, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan),
101+
node: new t.IfBlock(branches, wholeSourceSpan, ast.startSourceSpan, ifBlockEndSourceSpan),
90102
errors,
91103
};
92104
}
@@ -109,21 +121,29 @@ export function createForLoop(
109121
} else {
110122
empty = new t.ForLoopBlockEmpty(
111123
html.visitAll(visitor, block.children, block.children), block.sourceSpan,
112-
block.startSourceSpan);
124+
block.startSourceSpan, block.endSourceSpan);
113125
}
114126
} else {
115127
errors.push(new ParseError(block.sourceSpan, `Unrecognized @for loop block "${block.name}"`));
116128
}
117129
}
118130

131+
119132
if (params !== null) {
120133
if (params.trackBy === null) {
134+
// TODO: We should not fail here, and instead try to produce some AST for the language
135+
// service.
121136
errors.push(new ParseError(ast.sourceSpan, '@for loop must have a "track" expression'));
122137
} else {
138+
// The `for` block has a main span that includes the `empty` branch. For only the span of the
139+
// main `for` body, use `mainSourceSpan`.
140+
const endSpan = empty?.endSourceSpan ?? ast.endSourceSpan;
141+
const sourceSpan =
142+
new ParseSourceSpan(ast.sourceSpan.start, endSpan?.end ?? ast.sourceSpan.end);
123143
node = new t.ForLoopBlock(
124144
params.itemName, params.expression, params.trackBy, params.context,
125-
html.visitAll(visitor, ast.children, ast.children), empty, ast.sourceSpan,
126-
ast.startSourceSpan, ast.endSourceSpan);
145+
html.visitAll(visitor, ast.children, ast.children), empty, sourceSpan, ast.sourceSpan,
146+
ast.startSourceSpan, endSpan);
127147
}
128148
}
129149

@@ -231,8 +251,12 @@ function parseForLoopParameters(
231251
// Fill out any variables that haven't been defined explicitly.
232252
for (const variableName of ALLOWED_FOR_LOOP_LET_VARIABLES) {
233253
if (!result.context.hasOwnProperty(variableName)) {
234-
result.context[variableName] =
235-
new t.Variable(variableName, variableName, block.startSourceSpan, block.startSourceSpan);
254+
// Give ambiently-available context variables empty spans at the end of the start of the `for`
255+
// block, since they are not explicitly defined.
256+
const emptySpanAfterForBlockStart =
257+
new ParseSourceSpan(block.startSourceSpan.end, block.startSourceSpan.end);
258+
result.context[variableName] = new t.Variable(
259+
variableName, variableName, emptySpanAfterForBlockStart, emptySpanAfterForBlockStart);
236260
}
237261
}
238262

‎packages/compiler/src/render3/r3_deferred_blocks.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import * as html from '../ml_parser/ast';
10-
import {ParseError} from '../parse_util';
10+
import {ParseError, ParseSourceSpan} from '../parse_util';
1111
import {BindingParser} from '../template_parser/binding_parser';
1212

1313
import * as t from './r3_ast';
@@ -47,9 +47,23 @@ export function createDeferredBlock(
4747
const {placeholder, loading, error} = parseConnectedBlocks(connectedBlocks, errors, visitor);
4848
const {triggers, prefetchTriggers} =
4949
parsePrimaryTriggers(ast.parameters, bindingParser, errors, placeholder);
50+
51+
// The `defer` block has a main span encompassing all of the connected branches as well. For the
52+
// span of only the first "main" branch, use `mainSourceSpan`.
53+
let lastEndSourceSpan = ast.endSourceSpan;
54+
let endOfLastSourceSpan = ast.sourceSpan.end;
55+
if (connectedBlocks.length > 0) {
56+
const lastConnectedBlock = connectedBlocks[connectedBlocks.length - 1];
57+
lastEndSourceSpan = lastConnectedBlock.endSourceSpan;
58+
endOfLastSourceSpan = lastConnectedBlock.sourceSpan.end;
59+
}
60+
61+
const mainDeferredSourceSpan = new ParseSourceSpan(ast.sourceSpan.start, endOfLastSourceSpan);
62+
5063
const node = new t.DeferredBlock(
5164
html.visitAll(visitor, ast.children, ast.children), triggers, prefetchTriggers, placeholder,
52-
loading, error, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan);
65+
loading, error, mainDeferredSourceSpan, ast.sourceSpan, ast.startSourceSpan,
66+
lastEndSourceSpan);
5367

5468
return {node, errors};
5569
}

‎packages/compiler/test/render3/r3_ast_spans_spec.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -623,7 +623,7 @@ describe('R3 AST source spans', () => {
623623
expectFromHtml(html).toEqual([
624624
[
625625
'DeferredBlock',
626-
'@defer (when isVisible() && foo; on hover(button), timer(10s), idle, immediate, interaction(button), viewport(container); prefetch on immediate; prefetch when isDataLoaded()) {<calendar-cmp [date]="current"/>}',
626+
'@defer (when isVisible() && foo; on hover(button), timer(10s), idle, immediate, interaction(button), viewport(container); prefetch on immediate; prefetch when isDataLoaded()) {<calendar-cmp [date]="current"/>}@loading (minimum 1s; after 100ms) {Loading...}@placeholder (minimum 500) {Placeholder content!}@error {Loading failed :(}',
627627
'@defer (when isVisible() && foo; on hover(button), timer(10s), idle, immediate, interaction(button), viewport(container); prefetch on immediate; prefetch when isDataLoaded()) {',
628628
'}'
629629
],
@@ -691,7 +691,8 @@ describe('R3 AST source spans', () => {
691691

692692
expectFromHtml(html).toEqual([
693693
[
694-
'ForLoopBlock', '@for (item of items.foo.bar; track item.id) {<h1>{{ item }}</h1>}',
694+
'ForLoopBlock',
695+
'@for (item of items.foo.bar; track item.id) {<h1>{{ item }}</h1>}@empty {There were no items in the list.}',
695696
'@for (item of items.foo.bar; track item.id) {', '}'
696697
],
697698
['Element', '<h1>{{ item }}</h1>', '<h1>', '</h1>'],
@@ -710,8 +711,9 @@ describe('R3 AST source spans', () => {
710711

711712
expectFromHtml(html).toEqual([
712713
[
713-
'IfBlock', '@if (cond.expr; as foo) {Main case was true!}', '@if (cond.expr; as foo) {',
714-
'}'
714+
'IfBlock',
715+
'@if (cond.expr; as foo) {Main case was true!}@else if (other.expr) {Extra case was true!}@else {False case!}',
716+
'@if (cond.expr; as foo) {', '}'
715717
],
716718
[
717719
'IfBlockBranch', '@if (cond.expr; as foo) {Main case was true!}',

‎packages/language-service/src/template_target.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -507,11 +507,12 @@ class TemplateTargetVisitor implements t.Visitor {
507507
}
508508

509509
visitForLoopBlock(block: t.ForLoopBlock) {
510-
block.item.visit(this);
510+
this.visit(block.item);
511511
this.visitAll(Object.values(block.contextVariables));
512512
this.visitBinding(block.expression);
513+
this.visitBinding(block.trackBy);
513514
this.visitAll(block.children);
514-
block.empty?.visit(this);
515+
block.empty && this.visit(block.empty);
515516
}
516517

517518
visitForLoopBlockEmpty(block: t.ForLoopBlockEmpty) {
@@ -524,7 +525,7 @@ class TemplateTargetVisitor implements t.Visitor {
524525

525526
visitIfBlockBranch(block: t.IfBlockBranch) {
526527
block.expression && this.visitBinding(block.expression);
527-
block.expressionAlias?.visit(this);
528+
block.expressionAlias && this.visit(block.expressionAlias);
528529
this.visitAll(block.children);
529530
}
530531

0 commit comments

Comments
 (0)