From 12cb39c1a415d912233b4ef77685b13d6d7c762a Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 22 Dec 2020 09:12:26 -0800 Subject: [PATCH] fix(language-service): shorthand syntax with variables (#40239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes an issue in the ivy native language service that caused the logic that finds a target node given a template position to throw away the results. This happened because the source span of a variable node in the shorthand structural directive syntax (i.e. `*ngIf=`) included the entire binding. The result was that we would add the variable node to the path and then later detect that the cursor was outside the key and value spans and throw away the whole result. In general, we do this because we do not want to show information when the cursor is between a key/value (`inputA=¦"123"`). However, when using the shorthand syntax, we run into the situation where we can match an `AttributeBinding` as well as the vaariable in `*ngIf="som¦eValue as myLocalVar"`. This commit updates the visitor to retain enough information in the visit path to throw away invalid targets but keep valid ones if there were multiple results on a `t.Element` or `t.Template`. PR Close #40239 --- .../language-service/ivy/template_target.ts | 51 ++++++++++++------- .../ivy/test/legacy/template_target_spec.ts | 9 ++++ packages/language-service/ivy/utils.ts | 12 ++++- 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/packages/language-service/ivy/template_target.ts b/packages/language-service/ivy/template_target.ts index 85860c5b0f..eff933a3d4 100644 --- a/packages/language-service/ivy/template_target.ts +++ b/packages/language-service/ivy/template_target.ts @@ -6,11 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ -import {TmplAstBoundEvent} from '@angular/compiler'; +import {ParseSpan, TmplAstBoundEvent} from '@angular/compiler'; import * as e from '@angular/compiler/src/expression_parser/ast'; // e for expression AST import * as t from '@angular/compiler/src/render3/r3_ast'; // t for template AST -import {isTemplateNode, isTemplateNodeWithKeyAndValue, isWithin} from './utils'; +import {isTemplateNode, isTemplateNodeWithKeyAndValue, isWithin, isWithinKeyValue} from './utils'; /** * Contextual information for a target position within the template. @@ -105,6 +105,12 @@ export interface AttributeInValueContext { node: t.TextAttribute|t.BoundAttribute|t.BoundEvent; } +/** + * This special marker is added to the path when the cursor is within the sourceSpan but not the key + * or value span of a node with key/value spans. + */ +const OUTSIDE_K_V_MARKER = new e.AST(new ParseSpan(-1, -1), new e.AbsoluteSourceSpan(-1, -1)); + /** * Return the template AST node or expression AST node that most accurately * represents the node at the specified cursor `position`. @@ -119,20 +125,6 @@ export function getTargetAtPosition(template: t.Node[], position: number): Templ } const candidate = path[path.length - 1]; - if (isTemplateNodeWithKeyAndValue(candidate)) { - let {keySpan, valueSpan} = candidate; - if (valueSpan === undefined && candidate instanceof TmplAstBoundEvent) { - valueSpan = candidate.handlerSpan; - } - const isWithinKeyValue = - isWithin(position, keySpan) || (valueSpan && isWithin(position, valueSpan)); - if (!isWithinKeyValue) { - // If cursor is within source span but not within key span or value span, - // do not return the node. - return null; - } - } - // Walk up the result nodes to find the nearest `t.Template` which contains the targeted node. let context: t.Template|null = null; for (let i = path.length - 2; i >= 0; i--) { @@ -212,7 +204,22 @@ class TemplateTargetVisitor implements t.Visitor { static visitTemplate(template: t.Node[], position: number): Array { const visitor = new TemplateTargetVisitor(position); visitor.visitAll(template); - return visitor.path; + const {path} = visitor; + + const strictPath = path.filter(v => v !== OUTSIDE_K_V_MARKER); + const candidate = strictPath[strictPath.length - 1]; + const matchedASourceSpanButNotAKvSpan = path.some(v => v === OUTSIDE_K_V_MARKER); + if (matchedASourceSpanButNotAKvSpan && + (candidate instanceof t.Template || candidate instanceof t.Element)) { + // Template nodes with key and value spans are always defined on a `t.Template` or + // `t.Element`. If we found a node on a template with a `sourceSpan` that includes the cursor, + // it is possible that we are outside the k/v spans (i.e. in-between them). If this is the + // case and we do not have any other candidate matches on the `t.Element` or `t.Template`, we + // want to return no results. Otherwise, the `t.Element`/`t.Template` result is incorrect for + // that cursor position. + return []; + } + return strictPath; } // Position must be absolute in the source file. @@ -229,7 +236,15 @@ class TemplateTargetVisitor implements t.Visitor { return; } const {start, end} = getSpanIncludingEndTag(node); - if (isWithin(this.position, {start, end})) { + if (!isWithin(this.position, {start, end})) { + return; + } + + if (isTemplateNodeWithKeyAndValue(node) && !isWithinKeyValue(this.position, node)) { + // If cursor is within source span but not within key span or value span, + // do not return the node. + this.path.push(OUTSIDE_K_V_MARKER); + } else { this.path.push(node); node.visit(this); } diff --git a/packages/language-service/ivy/test/legacy/template_target_spec.ts b/packages/language-service/ivy/test/legacy/template_target_spec.ts index 9fe85fcbb0..047cf508c3 100644 --- a/packages/language-service/ivy/test/legacy/template_target_spec.ts +++ b/packages/language-service/ivy/test/legacy/template_target_spec.ts @@ -598,6 +598,15 @@ describe('findNodeAtPosition for microsyntax expression', () => { expect(node).toBeInstanceOf(e.PropertyRead); }); + it('should locate property read next to variable in structural directive syntax', () => { + const {errors, nodes, position} = parse(`
`); + expect(errors).toBe(null); + const {nodeInContext} = getTargetAtPosition(nodes, position)!; + const {node} = nodeInContext; + expect(isExpressionNode(node!)).toBe(true); + expect(node).toBeInstanceOf(e.PropertyRead); + }); + it('should locate text attribute', () => { const {errors, nodes, position} = parse(`
`); // ngFor is a text attribute because the desugared form is diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts index 28327990a3..7c67840c3c 100644 --- a/packages/language-service/ivy/utils.ts +++ b/packages/language-service/ivy/utils.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteSourceSpan, CssSelector, ParseSourceSpan, SelectorMatcher} from '@angular/compiler'; +import {AbsoluteSourceSpan, CssSelector, ParseSourceSpan, SelectorMatcher, TmplAstBoundEvent} from '@angular/compiler'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; import {isExternalResource} from '@angular/compiler-cli/src/ngtsc/metadata'; import {DeclarationNode} from '@angular/compiler-cli/src/ngtsc/reflection'; @@ -54,6 +54,16 @@ export function isTemplateNodeWithKeyAndValue(node: t.Node|e.AST): node is NodeW return isTemplateNode(node) && node.hasOwnProperty('keySpan'); } +export function isWithinKeyValue(position: number, node: NodeWithKeyAndValue): boolean { + let {keySpan, valueSpan} = node; + if (valueSpan === undefined && node instanceof TmplAstBoundEvent) { + valueSpan = node.handlerSpan; + } + const isWithinKeyValue = + isWithin(position, keySpan) || !!(valueSpan && isWithin(position, valueSpan)); + return isWithinKeyValue; +} + export function isTemplateNode(node: t.Node|e.AST): node is t.Node { // Template node implements the Node interface so we cannot use instanceof. return node.sourceSpan instanceof ParseSourceSpan;