From 5bda62c51d8f5590755c04acb50b49cd8cd84bbd Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 12 Oct 2020 12:48:56 -0700 Subject: [PATCH] refactor(language-service): Return directive defs when input name is part of selector (#39243) When an input name is part of the directive selector, it would be good to return the directive as well when performing 'go to definition' or 'go to type definition'. As an example, this would allow 'go to type definition' for ngIf to take the user to the NgIf directive. 'Go to type definition' would otherwise return no results because the input is a generic type. This would also be the case for all primitive input types. PR Close #39243 --- packages/language-service/ivy/definitions.ts | 59 +++++++++++++++---- .../language-service/ivy/hybrid_visitor.ts | 23 +++++++- .../ivy/test/definitions_spec.ts | 49 ++++++++++++--- .../ivy/test/type_definitions_spec.ts | 49 ++++++++++----- packages/language-service/ivy/utils.ts | 18 ++++-- .../language-service/test/project/app/main.ts | 1 + .../test/project/app/parsing-cases.ts | 5 ++ 7 files changed, 161 insertions(+), 43 deletions(-) diff --git a/packages/language-service/ivy/definitions.ts b/packages/language-service/ivy/definitions.ts index 7de4ecc63a..4ce1e15caa 100644 --- a/packages/language-service/ivy/definitions.ts +++ b/packages/language-service/ivy/definitions.ts @@ -6,16 +6,17 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, TmplAstNode, TmplAstTextAttribute} from '@angular/compiler'; +import {AST, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute} from '@angular/compiler'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; import {DirectiveSymbol, DomBindingSymbol, ElementSymbol, ShimLocation, Symbol, SymbolKind, TemplateSymbol} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as ts from 'typescript'; -import {findNodeAtPosition} from './hybrid_visitor'; +import {getPathToNodeAtPosition} from './hybrid_visitor'; import {flatMap, getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, getTextSpanOfNode, isDollarEvent, TemplateInfo, toTextSpan} from './utils'; interface DefinitionMeta { node: AST|TmplAstNode; + path: Array; symbol: Symbol; } @@ -41,12 +42,12 @@ export class DefinitionBuilder { return undefined; } - const definitions = this.getDefinitionsForSymbol(definitionMeta.symbol, definitionMeta.node); + const definitions = this.getDefinitionsForSymbol({...definitionMeta, ...templateInfo}); return {definitions, textSpan: getTextSpanOfNode(definitionMeta.node)}; } - private getDefinitionsForSymbol(symbol: Symbol, node: TmplAstNode|AST): - readonly ts.DefinitionInfo[]|undefined { + private getDefinitionsForSymbol({symbol, node, path, component}: DefinitionMeta& + TemplateInfo): readonly ts.DefinitionInfo[]|undefined { switch (symbol.kind) { case SymbolKind.Directive: case SymbolKind.Element: @@ -58,9 +59,14 @@ export class DefinitionBuilder { // LS users to "go to definition" on an item in the template that maps to a class and be // taken to the directive or HTML class. return this.getTypeDefinitionsForTemplateInstance(symbol, node); - case SymbolKind.Input: case SymbolKind.Output: - return this.getDefinitionsForSymbols(...symbol.bindings); + case SymbolKind.Input: { + const bindingDefs = this.getDefinitionsForSymbols(...symbol.bindings); + // Also attempt to get directive matches for the input name. If there is a directive that + // has the input name as part of the selector, we want to return that as well. + const directiveDefs = this.getDirectiveTypeDefsForBindingNode(node, path, component); + return [...bindingDefs, ...directiveDefs]; + } case SymbolKind.Variable: case SymbolKind.Reference: { const definitions: ts.DefinitionInfo[] = []; @@ -112,8 +118,14 @@ export class DefinitionBuilder { case SymbolKind.Template: return this.getTypeDefinitionsForTemplateInstance(symbol, node); case SymbolKind.Output: - case SymbolKind.Input: - return this.getTypeDefinitionsForSymbols(...symbol.bindings); + case SymbolKind.Input: { + const bindingDefs = this.getTypeDefinitionsForSymbols(...symbol.bindings); + // Also attempt to get directive matches for the input name. If there is a directive that + // has the input name as part of the selector, we want to return that as well. + const directiveDefs = this.getDirectiveTypeDefsForBindingNode( + node, definitionMeta.path, templateInfo.component); + return [...bindingDefs, ...directiveDefs]; + } case SymbolKind.Reference: case SymbolKind.Expression: case SymbolKind.Variable: @@ -150,6 +162,28 @@ export class DefinitionBuilder { } } + private getDirectiveTypeDefsForBindingNode( + node: TmplAstNode|AST, pathToNode: Array, component: ts.ClassDeclaration) { + if (!(node instanceof TmplAstBoundAttribute) && !(node instanceof TmplAstTextAttribute) && + !(node instanceof TmplAstBoundEvent)) { + return []; + } + const parent = pathToNode[pathToNode.length - 2]; + if (!(parent instanceof TmplAstTemplate || parent instanceof TmplAstElement)) { + return []; + } + const templateOrElementSymbol = + this.compiler.getTemplateTypeChecker().getSymbolOfNode(parent, component); + if (templateOrElementSymbol === null || + (templateOrElementSymbol.kind !== SymbolKind.Template && + templateOrElementSymbol.kind !== SymbolKind.Element)) { + return []; + } + const dirs = + getDirectiveMatchesForAttribute(node.name, parent, templateOrElementSymbol.directives); + return this.getTypeDefinitionsForSymbols(...dirs); + } + private getTypeDefinitionsForSymbols(...symbols: HasShimLocation[]): ts.DefinitionInfo[] { return flatMap(symbols, ({shimLocation}) => { const {shimPath, positionInShimFile} = shimLocation; @@ -159,15 +193,16 @@ export class DefinitionBuilder { private getDefinitionMetaAtPosition({template, component}: TemplateInfo, position: number): DefinitionMeta|undefined { - const node = findNodeAtPosition(template, position); - if (node === undefined) { + const path = getPathToNodeAtPosition(template, position); + if (path === undefined) { return; } + const node = path[path.length - 1]; const symbol = this.compiler.getTemplateTypeChecker().getSymbolOfNode(node, component); if (symbol === null) { return; } - return {node, symbol}; + return {node, path, symbol}; } } diff --git a/packages/language-service/ivy/hybrid_visitor.ts b/packages/language-service/ivy/hybrid_visitor.ts index c8e8c3a152..391809e38d 100644 --- a/packages/language-service/ivy/hybrid_visitor.ts +++ b/packages/language-service/ivy/hybrid_visitor.ts @@ -13,12 +13,14 @@ import * as t from '@angular/compiler/src/render3/r3_ast'; // t for temp import {isTemplateNode, isTemplateNodeWithKeyAndValue} from './utils'; /** - * Return the template AST node or expression AST node that most accurately + * Return the path to the template AST node or expression AST node that most accurately * represents the node at the specified cursor `position`. + * * @param ast AST tree * @param position cursor position */ -export function findNodeAtPosition(ast: t.Node[], position: number): t.Node|e.AST|undefined { +export function getPathToNodeAtPosition(ast: t.Node[], position: number): Array| + undefined { const visitor = new R3Visitor(position); visitor.visitAll(ast); const candidate = visitor.path[visitor.path.length - 1]; @@ -35,7 +37,22 @@ export function findNodeAtPosition(ast: t.Node[], position: number): t.Node|e.AS return; } } - return candidate; + return visitor.path; +} + +/** + * Return the template AST node or expression AST node that most accurately + * represents the node at the specified cursor `position`. + * + * @param ast AST tree + * @param position cursor position + */ +export function findNodeAtPosition(ast: t.Node[], position: number): t.Node|e.AST|undefined { + const path = getPathToNodeAtPosition(ast, position); + if (!path) { + return; + } + return path[path.length - 1]; } class R3Visitor implements t.Visitor { diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 99313d2328..6e2e568bf1 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -88,11 +88,14 @@ describe('definitions', () => { templateOverride: `
`, expectedSpanText: 'ngIf', }); - expect(definitions!.length).toEqual(1); + // Because the input is also part of the selector, the directive is also returned. + expect(definitions!.length).toEqual(2); + const [inputDef, directiveDef] = definitions; - const [def] = definitions; - expect(def.textSpan).toEqual('ngIf'); - expect(def.contextSpan).toEqual('set ngIf(condition: T);'); + expect(inputDef.textSpan).toEqual('ngIf'); + expect(inputDef.contextSpan).toEqual('set ngIf(condition: T);'); + expect(directiveDef.textSpan).toEqual('NgIf'); + expect(directiveDef.contextSpan).toContain('export declare class NgIf'); }); it('should work for directives with compound selectors', () => { @@ -125,6 +128,18 @@ describe('definitions', () => { expect(def.contextSpan).toEqual(`@Input('tcName') name = 'test';`); }); + it('should work for text inputs', () => { + const definitions = getDefinitionsAndAssertBoundSpan({ + templateOverride: ``, + expectedSpanText: 'tcName="name"', + }); + expect(definitions!.length).toEqual(1); + + const [def] = definitions; + expect(def.textSpan).toEqual('name'); + expect(def.contextSpan).toEqual(`@Input('tcName') name = 'test';`); + }); + it('should work for structural directive inputs ngForTrackBy', () => { const definitions = getDefinitionsAndAssertBoundSpan({ templateOverride: `
`, @@ -145,12 +160,16 @@ describe('definitions', () => { templateOverride: `
`, expectedSpanText: 'of', }); - expect(definitions!.length).toEqual(1); + // Because the input is also part of the selector ([ngFor][ngForOf]), the directive is also + // returned. + expect(definitions!.length).toEqual(2); + const [inputDef, directiveDef] = definitions; - const [def] = definitions; - expect(def.textSpan).toEqual('ngForOf'); - expect(def.contextSpan) + expect(inputDef.textSpan).toEqual('ngForOf'); + expect(inputDef.contextSpan) .toEqual('set ngForOf(ngForOf: U & NgIterable | undefined | null);'); + expect(directiveDef.textSpan).toEqual('NgForOf'); + expect(directiveDef.contextSpan).toContain('export declare class NgForOf'); }); it('should work for two-way binding providers', () => { @@ -191,6 +210,20 @@ describe('definitions', () => { const definitionAndBoundSpan = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position); expect(definitionAndBoundSpan).toBeUndefined(); }); + + it('should return the directive when the event is part of the selector', () => { + const definitions = getDefinitionsAndAssertBoundSpan({ + templateOverride: `
`, + expectedSpanText: `(eventSelector)="title = ''"`, + }); + expect(definitions!.length).toEqual(2); + + const [inputDef, directiveDef] = definitions; + expect(inputDef.textSpan).toEqual('eventSelector'); + expect(inputDef.contextSpan).toEqual('@Output() eventSelector = new EventEmitter();'); + expect(directiveDef.textSpan).toEqual('EventSelectorDirective'); + expect(directiveDef.contextSpan).toContain('export class EventSelectorDirective'); + }); }); }); diff --git a/packages/language-service/ivy/test/type_definitions_spec.ts b/packages/language-service/ivy/test/type_definitions_spec.ts index fd394b0e0e..cffed6d2fc 100644 --- a/packages/language-service/ivy/test/type_definitions_spec.ts +++ b/packages/language-service/ivy/test/type_definitions_spec.ts @@ -14,6 +14,10 @@ import {HumanizedDefinitionInfo, humanizeDefinitionInfo} from './test_utils'; describe('type definitions', () => { const {project, service, tsLS} = setup(); const ngLS = new LanguageService(project, tsLS); + const possibleArrayDefFiles = new Set([ + 'lib.es5.d.ts', 'lib.es2015.core.d.ts', 'lib.es2015.iterable.d.ts', + 'lib.es2015.symbol.wellknown.d.ts', 'lib.es2016.array.include.d.ts' + ]); beforeEach(() => { service.reset(); @@ -121,7 +125,11 @@ describe('type definitions', () => { const definitions = getTypeDefinitionsAndAssertBoundSpan({ templateOverride: `
`, }); - expectAllArrayDefinitions(definitions); + // In addition to all the array defs, this will also return the NgForOf def because the + // input is part of the selector ([ngFor][ngForOf]). + expectAllDefinitions( + definitions, new Set(['Array', 'NgForOf']), + new Set([...possibleArrayDefFiles, 'ng_for_of.d.ts'])); }); it('should return nothing for two-way binding providers', () => { @@ -141,10 +149,25 @@ describe('type definitions', () => { }); expect(definitions!.length).toEqual(2); - const [def, xyz] = definitions; - expect(def.textSpan).toEqual('EventEmitter'); - expect(def.contextSpan).toContain('export interface EventEmitter extends Subject'); - expect(xyz.textSpan).toEqual('EventEmitter'); + const [emitterInterface, emitterConst] = definitions; + expect(emitterInterface.textSpan).toEqual('EventEmitter'); + expect(emitterInterface.contextSpan) + .toContain('export interface EventEmitter extends Subject'); + expect(emitterInterface.fileName).toContain('event_emitter.d.ts'); + expect(emitterConst.textSpan).toEqual('EventEmitter'); + expect(emitterConst.contextSpan).toContain('export declare const EventEmitter'); + expect(emitterConst.fileName).toContain('event_emitter.d.ts'); + }); + + it('should return the directive when the event is part of the selector', () => { + const definitions = getTypeDefinitionsAndAssertBoundSpan({ + templateOverride: `
`, + }); + expect(definitions!.length).toEqual(3); + + // EventEmitter tested in previous test + const directiveDef = definitions[2]; + expect(directiveDef.contextSpan).toContain('export class EventSelectorDirective'); }); }); }); @@ -264,21 +287,21 @@ describe('type definitions', () => { const definitions = getTypeDefinitionsAndAssertBoundSpan({ templateOverride: `
`, }); - expectAllArrayDefinitions(definitions); + expectAllDefinitions(definitions, new Set(['Array']), possibleArrayDefFiles); }); it('should work for uses of members in structural directives', () => { const definitions = getTypeDefinitionsAndAssertBoundSpan({ templateOverride: `
{{her¦oes2}}
`, }); - expectAllArrayDefinitions(definitions); + expectAllDefinitions(definitions, new Set(['Array']), possibleArrayDefFiles); }); it('should work for members in structural directives', () => { const definitions = getTypeDefinitionsAndAssertBoundSpan({ templateOverride: `
`, }); - expectAllArrayDefinitions(definitions); + expectAllDefinitions(definitions, new Set(['Array']), possibleArrayDefFiles); }); it('should return nothing for the $any() cast function', () => { @@ -297,14 +320,12 @@ describe('type definitions', () => { return defs!.map(d => humanizeDefinitionInfo(d, service)); } - function expectAllArrayDefinitions(definitions: HumanizedDefinitionInfo[]) { + function expectAllDefinitions( + definitions: HumanizedDefinitionInfo[], textSpans: Set, + possibleFileNames: Set) { expect(definitions!.length).toBeGreaterThan(0); const actualTextSpans = new Set(definitions.map(d => d.textSpan)); - expect(actualTextSpans).toEqual(new Set(['Array'])); - const possibleFileNames = [ - 'lib.es5.d.ts', 'lib.es2015.core.d.ts', 'lib.es2015.iterable.d.ts', - 'lib.es2015.symbol.wellknown.d.ts', 'lib.es2016.array.include.d.ts' - ]; + expect(actualTextSpans).toEqual(textSpans); for (const def of definitions) { const fileName = def.fileName.split('/').slice(-1)[0]; expect(possibleFileNames) diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts index 1e5c4107d6..8fb027c7c6 100644 --- a/packages/language-service/ivy/utils.ts +++ b/packages/language-service/ivy/utils.ts @@ -143,8 +143,12 @@ function getInlineTemplateInfoAtPosition( /** * Given an attribute node, converts it to string form. */ -function toAttributeString(attribute: t.TextAttribute|t.BoundAttribute): string { - return `[${attribute.name}=${attribute.valueSpan?.toString() ?? ''}]`; +function toAttributeString(attribute: t.TextAttribute|t.BoundAttribute|t.BoundEvent): string { + if (attribute instanceof t.BoundEvent) { + return `[${attribute.name}]`; + } else { + return `[${attribute.name}=${attribute.valueSpan?.toString() ?? ''}]`; + } } function getNodeName(node: t.Template|t.Element): string { @@ -154,8 +158,10 @@ function getNodeName(node: t.Template|t.Element): string { /** * Given a template or element node, returns all attributes on the node. */ -function getAttributes(node: t.Template|t.Element): Array { - const attributes: Array = [...node.attributes, ...node.inputs]; +function getAttributes(node: t.Template| + t.Element): Array { + const attributes: Array = + [...node.attributes, ...node.inputs, ...node.outputs]; if (node instanceof t.Template) { attributes.push(...node.templateAttrs); } @@ -216,8 +222,8 @@ export function getDirectiveMatchesForAttribute( const allDirectiveMatches = getDirectiveMatchesForSelector(directives, getNodeName(hostNode) + allAttrs.join('')); const attrsExcludingName = attributes.filter(a => a.name !== name).map(toAttributeString); - const matchesWithoutAttr = - getDirectiveMatchesForSelector(directives, attrsExcludingName.join('')); + const matchesWithoutAttr = getDirectiveMatchesForSelector( + directives, getNodeName(hostNode) + attrsExcludingName.join('')); return difference(allDirectiveMatches, matchesWithoutAttr); } diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 331a79cd3b..4860cc3930 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -25,6 +25,7 @@ import * as ParsingCases from './parsing-cases'; ParsingCases.TestPipe, ParsingCases.WithContextDirective, ParsingCases.CompoundCustomButtonDirective, + ParsingCases.EventSelectorDirective, ] }) export class AppModule { diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 74004c02c6..5f9bec3e7c 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -75,6 +75,11 @@ export class CompoundCustomButtonDirective { @Input() config?: {color?: string}; } +@Directive({selector: '[eventSelector]'}) +export class EventSelectorDirective { + @Output() eventSelector = new EventEmitter(); +} + @Pipe({ name: 'prefixPipe', })