From 1ea04ffc056c506c724db14d40f75408b8ee226f Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 14 Jan 2020 18:54:40 -0800 Subject: [PATCH] feat(language-service): support multiple symbol definitions (#34782) In Angular, symbol can have multiple definitions (e.g. a two-way binding). This commit adds support for for multiple definitions for a queried location in a template. PR Close #34782 --- packages/language-service/src/definitions.ts | 50 ++-- packages/language-service/src/expressions.ts | 8 +- packages/language-service/src/hover.ts | 5 +- .../language-service/src/locate_symbol.ts | 236 ++++++++++-------- .../language-service/test/definitions_spec.ts | 54 ++-- 5 files changed, 199 insertions(+), 154 deletions(-) diff --git a/packages/language-service/src/definitions.ts b/packages/language-service/src/definitions.ts index 1d12c7df89..a3f5f1fbf3 100644 --- a/packages/language-service/src/definitions.ts +++ b/packages/language-service/src/definitions.ts @@ -9,9 +9,9 @@ import * as path from 'path'; import * as ts from 'typescript'; // used as value and is provided at runtime import {AstResult} from './common'; -import {locateSymbol} from './locate_symbol'; +import {locateSymbols} from './locate_symbol'; import {getPropertyAssignmentFromValue, isClassDecoratorProperty} from './template'; -import {Span, TemplateSource} from './types'; +import {Span} from './types'; import {findTightestNode} from './utils'; /** @@ -34,30 +34,38 @@ function ngSpanToTsTextSpan(span: Span): ts.TextSpan { */ export function getDefinitionAndBoundSpan( info: AstResult, position: number): ts.DefinitionInfoAndBoundSpan|undefined { - const symbolInfo = locateSymbol(info, position); - if (!symbolInfo) { + const symbols = locateSymbols(info, position); + if (!symbols.length) { return; } - const textSpan = ngSpanToTsTextSpan(symbolInfo.span); - const {symbol} = symbolInfo; - const {container, definition: locations} = symbol; - if (!locations || !locations.length) { + + const textSpan = ngSpanToTsTextSpan(symbols[0].span); + const definitions: ts.DefinitionInfo[] = []; + for (const symbolInfo of symbols) { + const {symbol} = symbolInfo; + // symbol.definition is really the locations of the symbol. There could be // more than one. No meaningful info could be provided without any location. - return {textSpan}; + const {kind, name, container, definition: locations} = symbol; + if (!locations || !locations.length) { + continue; + } + + const containerKind = + container ? container.kind as ts.ScriptElementKind : ts.ScriptElementKind.unknown; + const containerName = container ? container.name : ''; + definitions.push(...locations.map((location) => { + return { + kind: kind as ts.ScriptElementKind, + name, + containerKind, + containerName, + textSpan: ngSpanToTsTextSpan(location.span), + fileName: location.fileName, + }; + })); } - const containerKind = container ? container.kind : ts.ScriptElementKind.unknown; - const containerName = container ? container.name : ''; - const definitions = locations.map((location) => { - return { - kind: symbol.kind as ts.ScriptElementKind, - name: symbol.name, - containerKind: containerKind as ts.ScriptElementKind, - containerName: containerName, - textSpan: ngSpanToTsTextSpan(location.span), - fileName: location.fileName, - }; - }); + return { definitions, textSpan, }; diff --git a/packages/language-service/src/expressions.ts b/packages/language-service/src/expressions.ts index 26be536414..9d49448f93 100644 --- a/packages/language-service/src/expressions.ts +++ b/packages/language-service/src/expressions.ts @@ -148,8 +148,14 @@ export function getExpressionSymbol( }, visitPropertyWrite(ast) { const receiverType = getType(ast.receiver); + const {start} = ast.span; symbol = receiverType && receiverType.members().get(ast.name); - span = ast.span; + // A PropertyWrite span includes both the LHS (name) and the RHS (value) of the write. In this + // visit, only the name is relevant. + // prop=$event + // ^^^^ name + // ^^^^^^ value; visited separately as a nested AST + span = {start, end: start + ast.name.length}; }, visitQuote(ast) {}, visitSafeMethodCall(ast) { diff --git a/packages/language-service/src/hover.ts b/packages/language-service/src/hover.ts index 9234377069..9167c60433 100644 --- a/packages/language-service/src/hover.ts +++ b/packages/language-service/src/hover.ts @@ -10,7 +10,7 @@ import {CompileSummaryKind, StaticSymbol} from '@angular/compiler'; import * as ts from 'typescript'; import {AstResult} from './common'; -import {locateSymbol} from './locate_symbol'; +import {locateSymbols} from './locate_symbol'; import * as ng from './types'; import {TypeScriptServiceHost} from './typescript_host'; import {findTightestNode} from './utils'; @@ -32,10 +32,11 @@ const SYMBOL_INTERFACE = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.inter */ export function getHover(info: AstResult, position: number, host: Readonly): ts.QuickInfo|undefined { - const symbolInfo = locateSymbol(info, position); + const symbolInfo = locateSymbols(info, position)[0]; if (!symbolInfo) { return; } + const {symbol, span, compileTypeSummary} = symbolInfo; const textSpan = {start: span.start, length: span.end - span.start}; diff --git a/packages/language-service/src/locate_symbol.ts b/packages/language-service/src/locate_symbol.ts index 7849a611b1..1671991687 100644 --- a/packages/language-service/src/locate_symbol.ts +++ b/packages/language-service/src/locate_symbol.ts @@ -12,7 +12,7 @@ import {AstResult} from './common'; import {getExpressionScope} from './expression_diagnostics'; import {getExpressionSymbol} from './expressions'; import {Definition, DirectiveKind, Span, Symbol} from './types'; -import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getPathToNodeAtPosition, inSpan, offsetSpan, spanOf} from './utils'; +import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getPathToNodeAtPosition, inSpan, isNarrower, offsetSpan, spanOf} from './utils'; export interface SymbolInfo { symbol: Symbol; @@ -21,122 +21,144 @@ export interface SymbolInfo { } /** - * Traverse the template AST and locate the Symbol at the specified `position`. - * @param info Ast and Template Source - * @param position location to look for + * Traverses a template AST and locates symbol(s) at a specified position. + * @param info template AST information set + * @param position location to locate symbols at */ -export function locateSymbol(info: AstResult, position: number): SymbolInfo|undefined { +export function locateSymbols(info: AstResult, position: number): SymbolInfo[] { const templatePosition = position - info.template.span.start; + // TODO: update `findTemplateAstAt` to use absolute positions. const path = findTemplateAstAt(info.templateAst, templatePosition); - let compileTypeSummary: CompileTypeSummary|undefined = undefined; - if (path.tail) { - let symbol: Symbol|undefined = undefined; - let span: Span|undefined = undefined; - const attributeValueSymbol = (ast: AST, inEvent: boolean = false): boolean => { - const attribute = findAttribute(info, position); - if (attribute) { - if (inSpan(templatePosition, spanOf(attribute.valueSpan))) { - const dinfo = diagnosticInfoFromTemplateInfo(info); - const scope = getExpressionScope(dinfo, path); - if (attribute.valueSpan) { - const result = getExpressionSymbol(scope, ast, templatePosition, info.template.query); - if (result) { - symbol = result.symbol; - const expressionOffset = attribute.valueSpan.start.offset; - span = offsetSpan(result.span, expressionOffset); - } - } - return true; - } - } - return false; - }; - path.tail.visit( - { - visitNgContent(ast) {}, - visitEmbeddedTemplate(ast) {}, - visitElement(ast) { - const component = ast.directives.find(d => d.directive.isComponent); - if (component) { - compileTypeSummary = component.directive; - symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); - symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.COMPONENT); - span = spanOf(ast); - } else { - // Find a directive that matches the element name - const directive = ast.directives.find( - d => d.directive.selector != null && d.directive.selector.indexOf(ast.name) >= 0); - if (directive) { - compileTypeSummary = directive.directive; - symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); - symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); - span = spanOf(ast); - } - } - }, - visitReference(ast) { - symbol = ast.value && info.template.query.getTypeSymbol(tokenReference(ast.value)); - span = spanOf(ast); - }, - visitVariable(ast) {}, - visitEvent(ast) { - if (!attributeValueSymbol(ast.handler, /* inEvent */ true)) { - symbol = findOutputBinding(info, path, ast); - symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.EVENT); - span = spanOf(ast); - } - }, - visitElementProperty(ast) { attributeValueSymbol(ast.value); }, - visitAttr(ast) { - const element = path.head; - if (!element || !(element instanceof ElementAst)) return; - // Create a mapping of all directives applied to the element from their selectors. - const matcher = new SelectorMatcher(); - for (const dir of element.directives) { - if (!dir.directive.selector) continue; - matcher.addSelectables(CssSelector.parse(dir.directive.selector), dir); - } + if (!path.tail) return []; - // See if this attribute matches the selector of any directive on the element. - const attributeSelector = `[${ast.name}=${ast.value}]`; - const parsedAttribute = CssSelector.parse(attributeSelector); - if (!parsedAttribute.length) return; - matcher.match(parsedAttribute[0], (_, directive) => { - symbol = info.template.query.getTypeSymbol(directive.directive.type.reference); - symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); - span = spanOf(ast); - }); - }, - visitBoundText(ast) { - const expressionPosition = templatePosition - ast.sourceSpan.start.offset; - if (inSpan(expressionPosition, ast.value.span)) { - const dinfo = diagnosticInfoFromTemplateInfo(info); - const scope = getExpressionScope(dinfo, path); - const result = - getExpressionSymbol(scope, ast.value, templatePosition, info.template.query); - if (result) { - symbol = result.symbol; - span = offsetSpan(result.span, ast.sourceSpan.start.offset); - } - } - }, - visitText(ast) {}, - visitDirective(ast) { - compileTypeSummary = ast.directive; + const narrowest = spanOf(path.tail); + const toVisit: TemplateAst[] = []; + for (let node: TemplateAst|undefined = path.tail; + node && isNarrower(spanOf(node.sourceSpan), narrowest); node = path.parentOf(node)) { + toVisit.push(node); + } + + return toVisit.map(ast => locateSymbol(ast, path, info)) + .filter((sym): sym is SymbolInfo => sym !== undefined); +} + +/** + * Visits a template node and locates the symbol in that node at a path position. + * @param ast template AST node to visit + * @param path non-empty set of narrowing AST nodes at a position + * @param info template AST information set + */ +function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): SymbolInfo| + undefined { + const templatePosition = path.position; + const position = templatePosition + info.template.span.start; + let compileTypeSummary: CompileTypeSummary|undefined = undefined; + let symbol: Symbol|undefined; + let span: Span|undefined; + const attributeValueSymbol = (ast: AST): boolean => { + const attribute = findAttribute(info, position); + if (attribute) { + if (inSpan(templatePosition, spanOf(attribute.valueSpan))) { + const dinfo = diagnosticInfoFromTemplateInfo(info); + const scope = getExpressionScope(dinfo, path); + if (attribute.valueSpan) { + const result = getExpressionSymbol(scope, ast, templatePosition, info.template.query); + if (result) { + symbol = result.symbol; + const expressionOffset = attribute.valueSpan.start.offset; + span = offsetSpan(result.span, expressionOffset); + } + } + return true; + } + } + return false; + }; + ast.visit( + { + visitNgContent(ast) {}, + visitEmbeddedTemplate(ast) {}, + visitElement(ast) { + const component = ast.directives.find(d => d.directive.isComponent); + if (component) { + compileTypeSummary = component.directive; symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.COMPONENT); span = spanOf(ast); - }, - visitDirectiveProperty(ast) { - if (!attributeValueSymbol(ast.value)) { - symbol = findInputBinding(info, templatePosition, ast); + } else { + // Find a directive that matches the element name + const directive = ast.directives.find( + d => d.directive.selector != null && d.directive.selector.indexOf(ast.name) >= 0); + if (directive) { + compileTypeSummary = directive.directive; + symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); span = spanOf(ast); } } }, - null); - if (symbol && span) { - return {symbol, span: offsetSpan(span, info.template.span.start), compileTypeSummary}; - } + visitReference(ast) { + symbol = ast.value && info.template.query.getTypeSymbol(tokenReference(ast.value)); + span = spanOf(ast); + }, + visitVariable(ast) {}, + visitEvent(ast) { + if (!attributeValueSymbol(ast.handler)) { + symbol = findOutputBinding(info, path, ast); + symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.EVENT); + span = spanOf(ast); + } + }, + visitElementProperty(ast) { attributeValueSymbol(ast.value); }, + visitAttr(ast) { + const element = path.head; + if (!element || !(element instanceof ElementAst)) return; + // Create a mapping of all directives applied to the element from their selectors. + const matcher = new SelectorMatcher(); + for (const dir of element.directives) { + if (!dir.directive.selector) continue; + matcher.addSelectables(CssSelector.parse(dir.directive.selector), dir); + } + + // See if this attribute matches the selector of any directive on the element. + const attributeSelector = `[${ast.name}=${ast.value}]`; + const parsedAttribute = CssSelector.parse(attributeSelector); + if (!parsedAttribute.length) return; + matcher.match(parsedAttribute[0], (_, directive) => { + symbol = info.template.query.getTypeSymbol(directive.directive.type.reference); + symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); + span = spanOf(ast); + }); + }, + visitBoundText(ast) { + const expressionPosition = templatePosition - ast.sourceSpan.start.offset; + if (inSpan(expressionPosition, ast.value.span)) { + const dinfo = diagnosticInfoFromTemplateInfo(info); + const scope = getExpressionScope(dinfo, path); + const result = + getExpressionSymbol(scope, ast.value, templatePosition, info.template.query); + if (result) { + symbol = result.symbol; + span = offsetSpan(result.span, ast.sourceSpan.start.offset); + } + } + }, + visitText(ast) {}, + visitDirective(ast) { + compileTypeSummary = ast.directive; + symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + span = spanOf(ast); + }, + visitDirectiveProperty(ast) { + if (!attributeValueSymbol(ast.value)) { + symbol = findInputBinding(info, templatePosition, ast); + span = spanOf(ast); + } + } + }, + null); + if (symbol && span) { + return {symbol, span: offsetSpan(span, info.template.span.start), compileTypeSummary}; } } diff --git a/packages/language-service/test/definitions_spec.ts b/packages/language-service/test/definitions_spec.ts index 73c06117a7..25bc24a60a 100644 --- a/packages/language-service/test/definitions_spec.ts +++ b/packages/language-service/test/definitions_spec.ts @@ -14,6 +14,7 @@ import {TypeScriptServiceHost} from '../src/typescript_host'; import {MockTypescriptHost} from './test_utils'; const TEST_TEMPLATE = '/app/test.ng'; +const PARSING_CASES = '/app/parsing-cases.ts'; describe('definitions', () => { const mockHost = new MockTypescriptHost(['/app/main.ts']); @@ -49,28 +50,29 @@ describe('definitions', () => { }); it('should be able to find a field in a attribute reference', () => { - const fileName = mockHost.addCode(` - @Component({ - template: '' - }) - export class MyComponent { - «ᐱnameᐱ: string;» - }`); + mockHost.override(TEST_TEMPLATE, ``); - const marker = mockHost.getReferenceMarkerFor(fileName, 'name'); - const result = ngService.getDefinitionAndBoundSpan(fileName, marker.start); + const marker = mockHost.getReferenceMarkerFor(TEST_TEMPLATE, 'title'); + const result = ngService.getDefinitionAndBoundSpan(TEST_TEMPLATE, marker.start); expect(result).toBeDefined(); const {textSpan, definitions} = result !; expect(textSpan).toEqual(marker); expect(definitions).toBeDefined(); - expect(definitions !.length).toBe(1); - const def = definitions ![0]; - expect(def.fileName).toBe(fileName); - expect(def.name).toBe('name'); - expect(def.kind).toBe('property'); - expect(def.textSpan).toEqual(mockHost.getDefinitionMarkerFor(fileName, 'name')); + // There are exactly two, indentical definitions here, corresponding to the "name" on the + // property and event bindings of the two-way binding. The two-way binding is effectively + // syntactic sugar for `[ngModel]="name" (ngModel)="name=$event"`. + expect(definitions !.length).toBe(2); + for (const def of definitions !) { + expect(def.fileName).toBe(PARSING_CASES); + expect(def.name).toBe('title'); + expect(def.kind).toBe('property'); + + const fileContent = mockHost.readFile(def.fileName); + expect(fileContent !.substring(def.textSpan.start, def.textSpan.start + def.textSpan.length)) + .toEqual(`title = 'Some title';`); + } }); it('should be able to find a method from a call', () => { @@ -304,18 +306,24 @@ describe('definitions', () => { const boundedText = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'my'); expect(textSpan).toEqual(boundedText); - // There should be exactly 1 definition expect(definitions).toBeDefined(); - expect(definitions !.length).toBe(1); - const def = definitions ![0]; + expect(definitions !.length).toBe(2); + const [def1, def2] = definitions !; const refFileName = '/app/parsing-cases.ts'; - expect(def.fileName).toBe(refFileName); - expect(def.name).toBe('model'); - expect(def.kind).toBe('property'); - const content = mockHost.readFile(refFileName) !; - expect(content.substring(def.textSpan.start, def.textSpan.start + def.textSpan.length)) + expect(def1.fileName).toBe(refFileName); + expect(def1.name).toBe('model'); + expect(def1.kind).toBe('property'); + let content = mockHost.readFile(refFileName) !; + expect(content.substring(def1.textSpan.start, def1.textSpan.start + def1.textSpan.length)) .toEqual(`@Input() model: string = 'model';`); + + expect(def2.fileName).toBe(refFileName); + expect(def2.name).toBe('modelChange'); + expect(def2.kind).toBe('event'); + content = mockHost.readFile(refFileName) !; + expect(content.substring(def2.textSpan.start, def2.textSpan.start + def2.textSpan.length)) + .toEqual(`@Output() modelChange: EventEmitter = new EventEmitter();`); }); it('should be able to find a template from a url', () => {