From 3e97a1ea433974c8241e8e90a59b98b05a59e3fb Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 15 Jan 2021 15:57:24 -0800 Subject: [PATCH] fix(language-service): fix go to definition for template variables and references (#40455) 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 https://github.com/angular/vscode-ng-language-service/issues/1054 PR Close #40455 --- packages/language-service/ivy/definitions.ts | 27 ++++++++++------ packages/language-service/ivy/references.ts | 23 +++----------- .../ivy/test/definitions_spec.ts | 22 +++++++++++++ .../language-service/ivy/test/test_utils.ts | 2 +- packages/language-service/ivy/utils.ts | 31 ++++++++++++++++++- 5 files changed, 75 insertions(+), 30 deletions(-) diff --git a/packages/language-service/ivy/definitions.ts b/packages/language-service/ivy/definitions.ts index b748f78eec..e78068bcf7 100644 --- 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 index 4e71360764..114385e5fd 100644 --- 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'; interface FilePosition { fileName: string; @@ -376,27 +376,14 @@ export class ReferencesAndRenameBuilder { // 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(shimDocumentSpan.fileName), - positionInShimFile: shimDocumentSpan.textSpan.start, - }); + const mapping = getTemplateLocationFromShimLocation( + templateTypeChecker, absoluteFrom(shimDocumentSpan.fileName), + shimDocumentSpan.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; if (requiredNodeText !== undefined && span.toString() !== requiredNodeText) { return null; } diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 52f1f82983..5b0cd19aaa 100644 --- 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, humanizeDocumentSpanLike} 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', ' {{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 index ccc0408862..4ddb57d80a 100644 --- a/packages/language-service/ivy/test/test_utils.ts +++ b/packages/language-service/ivy/test/test_utils.ts @@ -60,7 +60,7 @@ export function humanizeDocumentSpanLike( env.host.readFile(item.fileName)) ?? ''; if (!fileContents) { - throw new Error('Could not read file ${entry.fileName}'); + throw new Error(`Could not read file ${item.fileName}`); } return { ...item, diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts index 07c4c15c28..a7ab8156b8 100644 --- a/packages/language-service/ivy/utils.ts +++ b/packages/language-service/ivy/utils.ts @@ -7,9 +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 {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'; @@ -346,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}; +} \ No newline at end of file