Skip to content

Commit

Permalink
refactor(language-service): Prepare to support blocks in the langauge…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
dylhunn authored and atscott committed Oct 5, 2023
1 parent 5464bea commit 04169e1
Show file tree
Hide file tree
Showing 6 changed files with 334 additions and 30 deletions.
22 changes: 11 additions & 11 deletions packages/compiler/src/render3/r3_ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ export class DeferredBlock implements Node {
public children: Node[], triggers: DeferredBlockTriggers,
prefetchTriggers: DeferredBlockTriggers, public placeholder: DeferredBlockPlaceholder|null,
public loading: DeferredBlockLoading|null, public error: DeferredBlockError|null,
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
public endSourceSpan: ParseSourceSpan|null) {
public sourceSpan: ParseSourceSpan, public mainBlockSpan: ParseSourceSpan,
public startSourceSpan: ParseSourceSpan, public endSourceSpan: ParseSourceSpan|null) {
this.triggers = triggers;
this.prefetchTriggers = prefetchTriggers;
// We cache the keys since we know that they won't change and we
Expand All @@ -228,16 +228,14 @@ export class DeferredBlock implements Node {
this.visitTriggers(this.definedTriggers, this.triggers, visitor);
this.visitTriggers(this.definedPrefetchTriggers, this.prefetchTriggers, visitor);
visitAll(visitor, this.children);
this.placeholder && visitor.visitDeferredBlockPlaceholder(this.placeholder);
this.loading && visitor.visitDeferredBlockLoading(this.loading);
this.error && visitor.visitDeferredBlockError(this.error);
const remainingBlocks =
[this.placeholder, this.loading, this.error].filter(x => x !== null) as Array<Node>;
visitAll(visitor, remainingBlocks);
}

private visitTriggers(
keys: (keyof DeferredBlockTriggers)[], triggers: DeferredBlockTriggers, visitor: Visitor) {
for (const key of keys) {
visitor.visitDeferredTrigger(triggers[key]!);
}
visitAll(visitor, keys.map(k => triggers[k]!));
}
}

Expand Down Expand Up @@ -272,7 +270,8 @@ export class ForLoopBlock implements Node {
public item: Variable, public expression: ASTWithSource, public trackBy: ASTWithSource,
public contextVariables: ForLoopBlockContext, public children: Node[],
public empty: ForLoopBlockEmpty|null, public sourceSpan: ParseSourceSpan,
public startSourceSpan: ParseSourceSpan, public endSourceSpan: ParseSourceSpan|null) {}
public mainBlockSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
public endSourceSpan: ParseSourceSpan|null) {}

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

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

visit<Result>(visitor: Visitor<Result>): Result {
return visitor.visitIfBlockBranch(this);
Expand Down
44 changes: 34 additions & 10 deletions packages/compiler/src/render3/r3_control_flow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function createIfBlock(
if (mainBlockParams !== null) {
branches.push(new t.IfBlockBranch(
mainBlockParams.expression, html.visitAll(visitor, ast.children, ast.children),
mainBlockParams.expressionAlias, ast.sourceSpan, ast.startSourceSpan));
mainBlockParams.expressionAlias, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan));
}

// Assumes that the structure is valid since we validated it above.
Expand All @@ -77,16 +77,28 @@ export function createIfBlock(
if (params !== null) {
branches.push(new t.IfBlockBranch(
params.expression, children, params.expressionAlias, block.sourceSpan,
block.startSourceSpan));
block.startSourceSpan, block.endSourceSpan));
}
} else if (block.name === 'else') {
branches.push(
new t.IfBlockBranch(null, children, null, block.sourceSpan, block.startSourceSpan));
branches.push(new t.IfBlockBranch(
null, children, null, block.sourceSpan, block.startSourceSpan, block.endSourceSpan));
}
}

// The outer IfBlock should have a span that encapsulates all branches.
const ifBlockStartSourceSpan =
branches.length > 0 ? branches[0].startSourceSpan : ast.startSourceSpan;
const ifBlockEndSourceSpan =
branches.length > 0 ? branches[branches.length - 1].endSourceSpan : ast.endSourceSpan;

let wholeSourceSpan = ast.sourceSpan;
const lastBranch = branches[branches.length - 1];
if (lastBranch !== undefined) {
wholeSourceSpan = new ParseSourceSpan(ifBlockStartSourceSpan.start, lastBranch.sourceSpan.end);
}

return {
node: new t.IfBlock(branches, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan),
node: new t.IfBlock(branches, wholeSourceSpan, ast.startSourceSpan, ifBlockEndSourceSpan),
errors,
};
}
Expand All @@ -109,21 +121,29 @@ export function createForLoop(
} else {
empty = new t.ForLoopBlockEmpty(
html.visitAll(visitor, block.children, block.children), block.sourceSpan,
block.startSourceSpan);
block.startSourceSpan, block.endSourceSpan);
}
} else {
errors.push(new ParseError(block.sourceSpan, `Unrecognized @for loop block "${block.name}"`));
}
}


if (params !== null) {
if (params.trackBy === null) {
// TODO: We should not fail here, and instead try to produce some AST for the language
// service.
errors.push(new ParseError(ast.sourceSpan, '@for loop must have a "track" expression'));
} else {
// The `for` block has a main span that includes the `empty` branch. For only the span of the
// main `for` body, use `mainSourceSpan`.
const endSpan = empty?.endSourceSpan ?? ast.endSourceSpan;
const sourceSpan =
new ParseSourceSpan(ast.sourceSpan.start, endSpan?.end ?? ast.sourceSpan.end);
node = new t.ForLoopBlock(
params.itemName, params.expression, params.trackBy, params.context,
html.visitAll(visitor, ast.children, ast.children), empty, ast.sourceSpan,
ast.startSourceSpan, ast.endSourceSpan);
html.visitAll(visitor, ast.children, ast.children), empty, sourceSpan, ast.sourceSpan,
ast.startSourceSpan, endSpan);
}
}

Expand Down Expand Up @@ -231,8 +251,12 @@ function parseForLoopParameters(
// Fill out any variables that haven't been defined explicitly.
for (const variableName of ALLOWED_FOR_LOOP_LET_VARIABLES) {
if (!result.context.hasOwnProperty(variableName)) {
result.context[variableName] =
new t.Variable(variableName, variableName, block.startSourceSpan, block.startSourceSpan);
// Give ambiently-available context variables empty spans at the end of the start of the `for`
// block, since they are not explicitly defined.
const emptySpanAfterForBlockStart =
new ParseSourceSpan(block.startSourceSpan.end, block.startSourceSpan.end);
result.context[variableName] = new t.Variable(
variableName, variableName, emptySpanAfterForBlockStart, emptySpanAfterForBlockStart);
}
}

Expand Down
18 changes: 16 additions & 2 deletions packages/compiler/src/render3/r3_deferred_blocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import * as html from '../ml_parser/ast';
import {ParseError} from '../parse_util';
import {ParseError, ParseSourceSpan} from '../parse_util';
import {BindingParser} from '../template_parser/binding_parser';

import * as t from './r3_ast';
Expand Down Expand Up @@ -47,9 +47,23 @@ export function createDeferredBlock(
const {placeholder, loading, error} = parseConnectedBlocks(connectedBlocks, errors, visitor);
const {triggers, prefetchTriggers} =
parsePrimaryTriggers(ast.parameters, bindingParser, errors, placeholder);

// The `defer` block has a main span encompassing all of the connected branches as well. For the
// span of only the first "main" branch, use `mainSourceSpan`.
let lastEndSourceSpan = ast.endSourceSpan;
let endOfLastSourceSpan = ast.sourceSpan.end;
if (connectedBlocks.length > 0) {
const lastConnectedBlock = connectedBlocks[connectedBlocks.length - 1];
lastEndSourceSpan = lastConnectedBlock.endSourceSpan;
endOfLastSourceSpan = lastConnectedBlock.sourceSpan.end;
}

const mainDeferredSourceSpan = new ParseSourceSpan(ast.sourceSpan.start, endOfLastSourceSpan);

const node = new t.DeferredBlock(
html.visitAll(visitor, ast.children, ast.children), triggers, prefetchTriggers, placeholder,
loading, error, ast.sourceSpan, ast.startSourceSpan, ast.endSourceSpan);
loading, error, mainDeferredSourceSpan, ast.sourceSpan, ast.startSourceSpan,
lastEndSourceSpan);

return {node, errors};
}
Expand Down
10 changes: 6 additions & 4 deletions packages/compiler/test/render3/r3_ast_spans_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ describe('R3 AST source spans', () => {
expectFromHtml(html).toEqual([
[
'DeferredBlock',
'@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"/>}',
'@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 :(}',
'@defer (when isVisible() && foo; on hover(button), timer(10s), idle, immediate, interaction(button), viewport(container); prefetch on immediate; prefetch when isDataLoaded()) {',
'}'
],
Expand Down Expand Up @@ -691,7 +691,8 @@ describe('R3 AST source spans', () => {

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

expectFromHtml(html).toEqual([
[
'IfBlock', '@if (cond.expr; as foo) {Main case was true!}', '@if (cond.expr; as foo) {',
'}'
'IfBlock',
'@if (cond.expr; as foo) {Main case was true!}@else if (other.expr) {Extra case was true!}@else {False case!}',
'@if (cond.expr; as foo) {', '}'
],
[
'IfBlockBranch', '@if (cond.expr; as foo) {Main case was true!}',
Expand Down
7 changes: 4 additions & 3 deletions packages/language-service/src/template_target.ts
Original file line number Diff line number Diff line change
Expand Up @@ -507,11 +507,12 @@ class TemplateTargetVisitor implements t.Visitor {
}

visitForLoopBlock(block: t.ForLoopBlock) {
block.item.visit(this);
this.visit(block.item);
this.visitAll(Object.values(block.contextVariables));
this.visitBinding(block.expression);
this.visitBinding(block.trackBy);
this.visitAll(block.children);
block.empty?.visit(this);
block.empty && this.visit(block.empty);
}

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

visitIfBlockBranch(block: t.IfBlockBranch) {
block.expression && this.visitBinding(block.expression);
block.expressionAlias?.visit(this);
block.expressionAlias && this.visit(block.expressionAlias);
this.visitAll(block.children);
}

Expand Down
Loading

0 comments on commit 04169e1

Please sign in to comment.