fix(language-service): fix go to definition for template variables an…

…d references (#40455) (#40491) The current "go to definition" is broken for template variables and references when a template is overridden. This is because we get the file url from the source span, which uses the overridden name 'override.html'. Instead, we can retrieve the template file from the compiler in the same manner that is done for references. Another way to fix this would have been to use the real template file path when overriding a template, but this was the more straightforward fix since the strategy was already used in find references and rename locations. fixes angular/vscode-ng-language-service#1054 PR Close #40455 PR Close #40491
angular · Jan 19, 2021 · 3e134e4 · 3e134e4
1 parent 2c5ad5c
commit 3e134e4
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 34 deletions.
diff --git a/packages/language-service/ivy/definitions.ts b/packages/language-service/ivy/definitions.ts
@@ -14,7 +14,7 @@ import * as ts from 'typescript';
 
 import {getTargetAtPosition, TargetNodeKind} from './template_target';
 import {findTightestNode, getParentClassDeclaration} from './ts_utils';
-import {flatMap, getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, getTextSpanOfNode, isDollarEvent, isTypeScriptFile, TemplateInfo, toTextSpan} from './utils';
+import {flatMap, getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, getTemplateLocationFromShimLocation, getTextSpanOfNode, isDollarEvent, isTypeScriptFile, TemplateInfo, toTextSpan} from './utils';
 
 interface DefinitionMeta {
   node: AST|TmplAstNode;
@@ -100,15 +100,22 @@ export class DefinitionBuilder {
       case SymbolKind.Reference: {
         const definitions: ts.DefinitionInfo[] = [];
         if (symbol.declaration !== node) {
-          definitions.push({
-            name: symbol.declaration.name,
-            containerName: '',
-            containerKind: ts.ScriptElementKind.unknown,
-            kind: ts.ScriptElementKind.variableElement,
-            textSpan: getTextSpanOfNode(symbol.declaration),
-            contextSpan: toTextSpan(symbol.declaration.sourceSpan),
-            fileName: symbol.declaration.sourceSpan.start.file.url,
-          });
+          const shimLocation = symbol.kind === SymbolKind.Variable ? symbol.localVarLocation :
+                                                                     symbol.referenceVarLocation;
+          const mapping = getTemplateLocationFromShimLocation(
+              this.compiler.getTemplateTypeChecker(), shimLocation.shimPath,
+              shimLocation.positionInShimFile);
+          if (mapping !== null) {
+            definitions.push({
+              name: symbol.declaration.name,
+              containerName: '',
+              containerKind: ts.ScriptElementKind.unknown,
+              kind: ts.ScriptElementKind.variableElement,
+              textSpan: getTextSpanOfNode(symbol.declaration),
+              contextSpan: toTextSpan(symbol.declaration.sourceSpan),
+              fileName: mapping.templateUrl,
+            });
+          }
         }
         if (symbol.kind === SymbolKind.Variable) {
           definitions.push(

diff --git a/packages/language-service/ivy/references.ts b/packages/language-service/ivy/references.ts
@@ -14,7 +14,7 @@ import * as ts from 'typescript';
 
 import {getTargetAtPosition, TargetNodeKind} from './template_target';
 import {findTightestNode} from './ts_utils';
-import {getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, isWithin, TemplateInfo, toTextSpan} from './utils';
+import {getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, getTemplateLocationFromShimLocation, isWithin, TemplateInfo, toTextSpan} from './utils';
 
 export class ReferenceBuilder {
   private readonly ttc = this.compiler.getTemplateTypeChecker();
@@ -184,30 +184,17 @@ export class ReferenceBuilder {
       // return references to the parameter in the template itself.
       return null;
     }
-
-    // TODO(atscott): Determine how to consistently resolve paths. i.e. with the project serverHost
-    // or LSParseConfigHost in the adapter. We should have a better defined way to normalize paths.
-    const mapping = templateTypeChecker.getTemplateMappingAtShimLocation({
-      shimPath: absoluteFrom(shimReferenceEntry.fileName),
-      positionInShimFile: shimReferenceEntry.textSpan.start,
-    });
+    // TODO(atscott): Determine how to consistently resolve paths. i.e. with the project
+    // serverHost or LSParseConfigHost in the adapter. We should have a better defined way to
+    // normalize paths.
+    const mapping = getTemplateLocationFromShimLocation(
+        templateTypeChecker, absoluteFrom(shimReferenceEntry.fileName),
+        shimReferenceEntry.textSpan.start);
     if (mapping === null) {
       return null;
     }
-    const {templateSourceMapping, span} = mapping;
-
-    let templateUrl: AbsoluteFsPath;
-    if (templateSourceMapping.type === 'direct') {
-      templateUrl = absoluteFromSourceFile(templateSourceMapping.node.getSourceFile());
-    } else if (templateSourceMapping.type === 'external') {
-      templateUrl = absoluteFrom(templateSourceMapping.templateUrl);
-    } else {
-      // This includes indirect mappings, which are difficult to map directly to the code location.
-      // Diagnostics similarly return a synthetic template string for this case rather than a real
-      // location.
-      return null;
-    }
 
+    const {span, templateUrl} = mapping;
     return {
       ...shimReferenceEntry,
       fileName: templateUrl,

diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts
@@ -13,6 +13,28 @@ import {extractCursorInfo, LanguageServiceTestEnvironment} from './env';
 import {assertFileNames, createModuleWithDeclarations, humanizeDefinitionInfo} from './test_utils';
 
 describe('definitions', () => {
+  it('gets definition for template reference in overridden template', () => {
+    initMockFileSystem('Native');
+    const templateFile = {contents: '', name: absoluteFrom('/app.html')};
+    const appFile = {
+      name: absoluteFrom('/app.ts'),
+      contents: `
+        import {Component} from '@angular/core';
+
+        @Component({templateUrl: '/app.html'})
+        export class AppCmp {}
+      `,
+    };
+
+    const env = createModuleWithDeclarations([appFile], [templateFile]);
+    const {cursor} = env.overrideTemplateWithCursor(
+        absoluteFrom('/app.ts'), 'AppCmp', '<input #myInput /> {{myIn¦put.value}}');
+    env.expectNoSourceDiagnostics();
+    const {definitions} = env.ngLS.getDefinitionAndBoundSpan(absoluteFrom('/app.html'), cursor)!;
+    expect(definitions![0].name).toEqual('myInput');
+    assertFileNames(Array.from(definitions!), ['app.html']);
+  });
+
   it('returns the pipe class as definition when checkTypeOfPipes is false', () => {
     initMockFileSystem('Native');
     const {cursor, text} = extractCursorInfo('{{"1/1/2020" | dat¦e}}');

diff --git a/packages/language-service/ivy/test/test_utils.ts b/packages/language-service/ivy/test/test_utils.ts
@@ -66,7 +66,9 @@ export function humanizeDefinitionInfo(
   const contents = (overrides.get(def.fileName) !== undefined ? overrides.get(def.fileName) :
                                                                 host.readFile(def.fileName)) ??
       '';
-
+  if (!contents) {
+    throw new Error(`Could not read file ${def.fileName}`);
+  }
   return {
     fileName: def.fileName,
     textSpan: contents.substr(def.textSpan.start, def.textSpan.start + def.textSpan.length),

diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts
@@ -7,10 +7,10 @@
  */
 import {AbsoluteSourceSpan, CssSelector, ParseSourceSpan, SelectorMatcher, TmplAstBoundEvent} from '@angular/compiler';
 import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
+import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
 import {isExternalResource} from '@angular/compiler-cli/src/ngtsc/metadata';
 import {DeclarationNode} from '@angular/compiler-cli/src/ngtsc/reflection';
-import {DirectiveSymbol} from '@angular/compiler-cli/src/ngtsc/typecheck/api';
-import {Diagnostic as ngDiagnostic, isNgDiagnostic} from '@angular/compiler-cli/src/transformers/api';
+import {DirectiveSymbol, TemplateTypeChecker} from '@angular/compiler-cli/src/ngtsc/typecheck/api';
 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 * as ts from 'typescript';
@@ -347,3 +347,31 @@ export function isWithin(position: number, span: AbsoluteSourceSpan|ParseSourceS
   // like ¦start and end¦ where ¦ is the cursor.
   return start <= position && position <= end;
 }
+
+/**
+ * For a given location in a shim file, retrieves the corresponding file url for the template and
+ * the span in the template.
+ */
+export function getTemplateLocationFromShimLocation(
+    templateTypeChecker: TemplateTypeChecker, shimPath: AbsoluteFsPath,
+    positionInShimFile: number): {templateUrl: AbsoluteFsPath, span: ParseSourceSpan}|null {
+  const mapping =
+      templateTypeChecker.getTemplateMappingAtShimLocation({shimPath, positionInShimFile});
+  if (mapping === null) {
+    return null;
+  }
+  const {templateSourceMapping, span} = mapping;
+
+  let templateUrl: AbsoluteFsPath;
+  if (templateSourceMapping.type === 'direct') {
+    templateUrl = absoluteFromSourceFile(templateSourceMapping.node.getSourceFile());
+  } else if (templateSourceMapping.type === 'external') {
+    templateUrl = absoluteFrom(templateSourceMapping.templateUrl);
+  } else {
+    // This includes indirect mappings, which are difficult to map directly to the code
+    // location. Diagnostics similarly return a synthetic template string for this case rather
+    // than a real location.
+    return null;
+  }
+  return {templateUrl, span};
+}