refactor(language-service): Allow "go to definition" for directives i…

…n Ivy (#39228)

For directives/components, it would be generally more appropriate for
"go to type definition" to be the function which navigates to the class
definition. However, for a better user experience, we should do this
for "go to definition" as well.

PR Close #39228

packages/language-service/ivy/definitions.ts

@@ -8,7 +8,7 @@
 
 import {AST, TmplAstNode, TmplAstTextAttribute} from '@angular/compiler';
 import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
-import {ShimLocation, Symbol, SymbolKind} from '@angular/compiler-cli/src/ngtsc/typecheck/api';
+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';
@@ -52,17 +52,12 @@ export class DefinitionBuilder {
       case SymbolKind.Element:
       case SymbolKind.Template:
       case SymbolKind.DomBinding:
-        // `Template` and `Element` types should not return anything because their "definitions" are
-        // the template locations themselves. Instead, `getTypeDefinitionAtPosition` should return
-        // the directive class / native element interface. `Directive` would have similar reasoning,
-        // though the `TemplateTypeChecker` only returns it as a list on `DomBinding`, `Element`, or
-        // `Template` so it's really only here for switch case completeness (it wouldn't ever appear
-        // here).
-        //
-        // `DomBinding` also does not return anything because the value assignment is internal to
-        // the TCB. Again, `getTypeDefinitionAtPosition` could return a possible directive the
-        // attribute binds to or the property in the native interface.
-        return [];
+        // Though it is generally more appropriate for the above symbol definitions to be
+        // associated with "type definitions" since the location in the template is the
+        // actual definition location, the better user experience would be to allow
+        // 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);
@@ -110,14 +105,32 @@ export class DefinitionBuilder {
     }
 
     const {symbol, node} = definitionMeta;
+    switch (symbol.kind) {
+      case SymbolKind.Directive:
+      case SymbolKind.DomBinding:
+      case SymbolKind.Element:
+      case SymbolKind.Template:
+        return this.getTypeDefinitionsForTemplateInstance(symbol, node);
+      case SymbolKind.Output:
+      case SymbolKind.Input:
+        return this.getTypeDefinitionsForSymbols(...symbol.bindings);
+      case SymbolKind.Reference:
+      case SymbolKind.Expression:
+      case SymbolKind.Variable:
+        return this.getTypeDefinitionsForSymbols(symbol);
+    }
+  }
+
+  private getTypeDefinitionsForTemplateInstance(
+      symbol: TemplateSymbol|ElementSymbol|DomBindingSymbol|DirectiveSymbol,
+      node: AST|TmplAstNode): ts.DefinitionInfo[] {
     switch (symbol.kind) {
       case SymbolKind.Template: {
         const matches = getDirectiveMatchesForElementTag(symbol.templateNode, symbol.directives);
         return this.getTypeDefinitionsForSymbols(...matches);
       }
       case SymbolKind.Element: {
-        const matches = getDirectiveMatchesForAttribute(
-            symbol.templateNode.name, symbol.templateNode, symbol.directives);
+        const matches = getDirectiveMatchesForElementTag(symbol.templateNode, symbol.directives);
         // If one of the directive matches is a component, we should not include the native element
         // in the results because it is replaced by the component.
         return Array.from(matches).some(dir => dir.isComponent) ?
@@ -132,13 +145,7 @@ export class DefinitionBuilder {
             node.name, symbol.host.templateNode, symbol.host.directives);
         return this.getTypeDefinitionsForSymbols(...dirs);
       }
-      case SymbolKind.Output:
-      case SymbolKind.Input:
-        return this.getTypeDefinitionsForSymbols(...symbol.bindings);
-      case SymbolKind.Reference:
       case SymbolKind.Directive:
-      case SymbolKind.Expression:
-      case SymbolKind.Variable:
         return this.getTypeDefinitionsForSymbols(symbol);
     }
   }

packages/language-service/ivy/test/definitions_spec.ts

@@ -22,12 +22,15 @@ describe('definitions', () => {
   });
 
   describe('elements', () => {
-    it('should return nothing for native elements', () => {
-      const {position} = service.overwriteInlineTemplate(APP_COMPONENT, `<butt¦on></button>`);
-      const definitionAndBoundSpan = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position);
-      // The "definition" is this location itself so we should return nothing.
-      // getTypeDefinitionAtPosition would return the HTMLButtonElement interface.
-      expect(definitionAndBoundSpan!.definitions).toEqual([]);
+    it('should work for native elements', () => {
+      const defs = getDefinitionsAndAssertBoundSpan({
+        templateOverride: `<butt¦on></button>`,
+        expectedSpanText: `<button></button>`,
+      });
+      expect(defs.length).toEqual(2);
+      expect(defs[0].fileName).toContain('lib.dom.d.ts');
+      expect(defs[0].contextSpan).toContain('interface HTMLButtonElement extends HTMLElement');
+      expect(defs[1].contextSpan).toContain('declare var HTMLButtonElement');
     });
   });
 
@@ -42,11 +45,13 @@ describe('definitions', () => {
 
   describe('directives', () => {
     it('should work for directives', () => {
-      const definitions = getDefinitionsAndAssertBoundSpan({
+      const defs = getDefinitionsAndAssertBoundSpan({
         templateOverride: `<div string-model¦></div>`,
         expectedSpanText: 'string-model',
       });
-      expect(definitions).toEqual([]);
+      expect(defs.length).toEqual(1);
+      expect(defs[0].contextSpan).toContain('@Directive');
+      expect(defs[0].contextSpan).toContain('export class StringModel');
     });
 
     it('should work for components', () => {
@@ -58,17 +63,25 @@ describe('definitions', () => {
         templateOverride,
         expectedSpanText: templateOverride.replace('¦', '').trim(),
       });
-      expect(definitions).toEqual([]);
+      expect(definitions.length).toEqual(1);
+
+      expect(definitions.length).toEqual(1);
+      expect(definitions[0].textSpan).toEqual('TestComponent');
+      expect(definitions[0].contextSpan).toContain('@Component');
     });
 
-    it('should not return anything for structural directives where the key does not map to a binding',
-       () => {
-         const definitions = getDefinitionsAndAssertBoundSpan({
-           templateOverride: `<div *¦ngFor="let item of heroes"></div>`,
-           expectedSpanText: 'ngFor',
-         });
-         expect(definitions).toEqual([]);
-       });
+    it('should work for structural directives', () => {
+      const definitions = getDefinitionsAndAssertBoundSpan({
+        templateOverride: `<div *¦ngFor="let item of heroes"></div>`,
+        expectedSpanText: 'ngFor',
+      });
+      expect(definitions.length).toEqual(1);
+      expect(definitions[0].fileName).toContain('ng_for_of.d.ts');
+      expect(definitions[0].textSpan).toEqual('NgForOf');
+      expect(definitions[0].contextSpan)
+          .toContain(
+              'export declare class NgForOf<T, U extends NgIterable<T> = NgIterable<T>> implements DoCheck');
+    });
 
     it('should return binding for structural directive where key maps to a binding', () => {
       const definitions = getDefinitionsAndAssertBoundSpan({
@@ -83,11 +96,18 @@ describe('definitions', () => {
     });
 
     it('should work for directives with compound selectors', () => {
-      const definitions = getDefinitionsAndAssertBoundSpan({
-        templateOverride: `<ng-template ngF¦or let-item [ngForOf]="items">{{item}}</ng-template>`,
-        expectedSpanText: 'ngFor',
+      let defs = getDefinitionsAndAssertBoundSpan({
+        templateOverride: `<button com¦pound custom-button></button>`,
+        expectedSpanText: 'compound',
       });
-      expect(definitions).toEqual([]);
+      expect(defs.length).toEqual(1);
+      expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective');
+      defs = getDefinitionsAndAssertBoundSpan({
+        templateOverride: `<button compound cu¦stom-button></button>`,
+        expectedSpanText: 'custom-button',
+      });
+      expect(defs.length).toEqual(1);
+      expect(defs[0].contextSpan).toContain('export class CompoundCustomButtonDirective');
     });
   });